[Contracts] Overflowing bounded DeletionQueue allows DoS against co…

…ntract termination (#13702) * [Contracts review] Overflowing bounded `DeletionQueue` allows DoS against contract termination * wip * wip * wip * wip * wip * fix doc * wip * PR review * unbreak tests * fixes * update budget computation * PR comment: use BlockWeights::get().max_block * PR comment: Update queue_trie_for_deletion signature * PR comment: update deletion budget docstring * PR comment: impl Default with derive(DefaultNoBound) * PR comment: Remove DeletedContract * PR comment Add ring_buffer test * remove missed comment * misc comments * contracts: add sr25519_recover * Revert "contracts: add sr25519_recover" This reverts commit d4600e0. * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts * PR comments update print_schedule * Update frame/contracts/src/benchmarking/mod.rs * Update frame/contracts/src/storage.rs * Update frame/contracts/src/storage.rs * rm temporary fixes * fix extra ; * Update frame/contracts/src/storage.rs Co-authored-by: juangirini <[email protected]> * Update frame/contracts/src/storage.rs Co-authored-by: Alexander Theißen <[email protected]> * Update frame/contracts/src/lib.rs Co-authored-by: Alexander Theißen <[email protected]> * Update frame/contracts/src/lib.rs Co-authored-by: Alexander Theißen <[email protected]> * Support stable rust for compiling the runtime (#13580) * Support stable rust for compiling the runtime This pull request brings support for compiling the runtime with stable Rust. This requires at least rust 1.68.0 to work on stable. The code is written in a way that it is backwards compatible and should automatically work when someone compiles with 1.68.0+ stable. * We always support nightlies! * 🤦 * Sort by version * Review feedback * Review feedback * Fix version parsing * Apply suggestions from code review Co-authored-by: Koute <[email protected]> --------- Co-authored-by: Koute <[email protected]> * github PR commit fixes * Revert "Support stable rust for compiling the runtime (#13580)" This reverts commit 0b985aa. * Restore DeletionQueueMap * fix namings * PR comment * move comments * Update frame/contracts/src/storage.rs * Update frame/contracts/src/storage.rs * fixes --------- Co-authored-by: command-bot <> Co-authored-by: juangirini <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Koute <[email protected]>
paritytech · gpestana · Mar 27, 2023 · Mar 28, 2023 · Mar 28, 2023 · Mar 30, 2023
commit 7756bb18b6f1f17064019215e81fe08fe6f8e0ec
diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -1198,13 +1198,6 @@ impl pallet_tips::Config for Runtime {
 parameter_types! {
 	pub const DepositPerItem: Balance = deposit(1, 0);
 	pub const DepositPerByte: Balance = deposit(0, 1);
-	pub const DeletionQueueDepth: u32 = 128;
-	// The lazy deletion runs inside on_initialize.
-	pub DeletionWeightLimit: Weight = RuntimeBlockWeights::get()
-		.per_class
-		.get(DispatchClass::Normal)
-		.max_total
-		.unwrap_or(RuntimeBlockWeights::get().max_block);
 	pub Schedule: pallet_contracts::Schedule<Runtime> = Default::default();
 }
 
@@ -1227,8 +1220,6 @@ impl pallet_contracts::Config for Runtime {
 	type WeightPrice = pallet_transaction_payment::Pallet<Self>;
 	type WeightInfo = pallet_contracts::weights::SubstrateWeight<Self>;
 	type ChainExtension = ();
-	type DeletionQueueDepth = DeletionQueueDepth;
-	type DeletionWeightLimit = DeletionWeightLimit;
 	type Schedule = Schedule;
 	type AddressGenerator = pallet_contracts::DefaultAddressGenerator;
 	type MaxCodeLen = ConstU32<{ 123 * 1024 }>;

diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs
@@ -214,19 +214,7 @@ benchmarks! {
 	on_initialize_per_trie_key {
 		let k in 0..1024;
 		let instance = Contract::<T>::with_storage(WasmModule::dummy(), k, T::Schedule::get().limits.payload_len)?;
-		instance.info()?.queue_trie_for_deletion()?;
-	}: {
-		ContractInfo::<T>::process_deletion_queue_batch(Weight::MAX)
-	}
-
-	#[pov_mode = Measured]
-	on_initialize_per_queue_item {
-		let q in 0..1024.min(T::DeletionQueueDepth::get());
-		for i in 0 .. q {
-			let instance = Contract::<T>::with_index(i, WasmModule::dummy(), vec![])?;
-			instance.info()?.queue_trie_for_deletion()?;
-			ContractInfoOf::<T>::remove(instance.account_id);
-		}
+		instance.info()?.queue_trie_for_deletion();
 	}: {
 		ContractInfo::<T>::process_deletion_queue_batch(Weight::MAX)
 	}
@@ -3020,16 +3008,12 @@ benchmarks! {
 	print_schedule {
 		#[cfg(feature = "std")]
 		{
-			let weight_limit = T::DeletionWeightLimit::get();
-			let max_queue_depth = T::DeletionQueueDepth::get() as usize;
-			let empty_queue_throughput = ContractInfo::<T>::deletion_budget(0, weight_limit);
-			let full_queue_throughput = ContractInfo::<T>::deletion_budget(max_queue_depth, weight_limit);
+			let max_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
+			let (weight_per_key, key_budget) = ContractInfo::<T>::deletion_budget(max_weight);
 			println!("{:#?}", Schedule::<T>::default());
 			println!("###############################################");
-			println!("Lazy deletion weight per key: {}", empty_queue_throughput.0);
-			println!("Lazy deletion throughput per block (empty queue, full queue): {}, {}",
-				empty_queue_throughput.1, full_queue_throughput.1,
-			);
+			println!("Lazy deletion weight per key: {weight_per_key}");
+			println!("Lazy deletion throughput per block: {key_budget}");
 		}
 		#[cfg(not(feature = "std"))]
 		Err("Run this bench with a native runtime in order to see the schedule.")?;

diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs
@@ -1204,7 +1204,7 @@ where
 			T::Currency::reducible_balance(&frame.account_id, Expendable, Polite),
 			ExistenceRequirement::AllowDeath,
 		)?;
-		info.queue_trie_for_deletion()?;
+		info.queue_trie_for_deletion();
 		ContractInfoOf::<T>::remove(&frame.account_id);
 		E::remove_user(info.code_hash);
 		Contracts::<T>::deposit_event(

diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
@@ -102,7 +102,7 @@ mod tests;
 use crate::{
 	exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, Stack as ExecStack},
 	gas::GasMeter,
-	storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract},
+	storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager},
 	wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate},
 	weights::WeightInfo,
 };
@@ -245,33 +245,6 @@ pub mod pallet {
 		/// memory usage of your runtime.
 		type CallStack: Array<Item = Frame<Self>>;
 
-		/// The maximum number of contracts that can be pending for deletion.
-		///
-		/// When a contract is deleted by calling `seal_terminate` it becomes inaccessible
-		/// immediately, but the deletion of the storage items it has accumulated is performed
-		/// later. The contract is put into the deletion queue. This defines how many
-		/// contracts can be queued up at the same time. If that limit is reached `seal_terminate`
-		/// will fail. The action must be retried in a later block in that case.
-		///
-		/// The reasons for limiting the queue depth are:
-		///
-		/// 1. The queue is in storage in order to be persistent between blocks. We want to limit
-		/// 	the amount of storage that can be consumed.
-		/// 2. The queue is stored in a vector and needs to be decoded as a whole when reading
-		///		it at the end of each block. Longer queues take more weight to decode and hence
-		///		limit the amount of items that can be deleted per block.
-		#[pallet::constant]
-		type DeletionQueueDepth: Get<u32>;
-
-		/// The maximum amount of weight that can be consumed per block for lazy trie removal.
-		///
-		/// The amount of weight that is dedicated per block to work on the deletion queue. Larger
-		/// values allow more trie keys to be deleted in each block but reduce the amount of
-		/// weight that is left for transactions. See [`Self::DeletionQueueDepth`] for more
-		/// information about the deletion queue.
-		#[pallet::constant]
-		type DeletionWeightLimit: Get<Weight>;
-
 		/// The amount of balance a caller has to pay for each byte of storage.
 		///
 		/// # Note
@@ -329,25 +302,6 @@ pub mod pallet {
 				.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
 		}
 
-		fn on_initialize(_block: T::BlockNumber) -> Weight {
-			// We want to process the deletion_queue in the on_idle hook. Only in the case
-			// that the queue length has reached its maximal depth, we process it here.
-			let max_len = T::DeletionQueueDepth::get() as usize;
-			let queue_len = <DeletionQueue<T>>::decode_len().unwrap_or(0);
-			if queue_len >= max_len {
-				// We do not want to go above the block limit and rather avoid lazy deletion
-				// in that case. This should only happen on runtime upgrades.
-				let weight_limit = T::BlockWeights::get()
-					.max_block
-					.saturating_sub(System::<T>::block_weight().total())
-					.min(T::DeletionWeightLimit::get());
-				ContractInfo::<T>::process_deletion_queue_batch(weight_limit)
-					.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
-			} else {
-				T::WeightInfo::on_process_deletion_queue_batch()
-			}
-		}
-
 		fn integrity_test() {
 			// Total runtime memory is expected to have 128Mb upper limit
 			const MAX_RUNTIME_MEM: u32 = 1024 * 1024 * 128;
@@ -860,12 +814,6 @@ pub mod pallet {
 		/// in this error. Note that this usually  shouldn't happen as deploying such contracts
 		/// is rejected.
 		NoChainExtension,
-		/// Removal of a contract failed because the deletion queue is full.
-		///
-		/// This can happen when calling `seal_terminate`.
-		/// The queue is filled by deleting contracts and emptied by a fixed amount each block.
-		/// Trying again during another block is the only way to resolve this issue.
-		DeletionQueueFull,
 		/// A contract with the same AccountId already exists.
 		DuplicateContract,
 		/// A contract self destructed in its constructor.
@@ -949,10 +897,15 @@ pub mod pallet {
 	/// Evicted contracts that await child trie deletion.
 	///
 	/// Child trie deletion is a heavy operation depending on the amount of storage items
-	/// stored in said trie. Therefore this operation is performed lazily in `on_initialize`.
+	/// stored in said trie. Therefore this operation is performed lazily in `on_idle`.
+	#[pallet::storage]
+	pub(crate) type DeletionQueue<T: Config> = StorageMap<_, Twox64Concat, u32, TrieId>;
+
+	/// A pair of monotonic counters used to track the latest contract marked for deletion
+	/// and the latest deleted contract in queue.
 	#[pallet::storage]
-	pub(crate) type DeletionQueue<T: Config> =
-		StorageValue<_, BoundedVec<DeletedContract, T::DeletionQueueDepth>, ValueQuery>;
+	pub(crate) type DeletionQueueCounter<T: Config> =
+		StorageValue<_, DeletionQueueManager<T>, ValueQuery>;
 }
 
 /// Context of a contract invocation.