pallet_revive: Enforce storage deposit limit on plain transfer

prdoc/pr_9416.prdoc

@@ -0,0 +1,10 @@
+title: 'pallet_revive: Enforce storage deposit limit on plain transfer'
+doc:
+- audience: Runtime Dev
+  description: |-
+    The existential deposit to create a new account is part of the storage deposit. Hence if the storage deposit limit is too low to create a new account we fail the transaction. However, this limit was not enforced for plain transfers. The reason is that we only enforce the limit at the end of each frame. But for plain transfers (transferring to a non contract) there is no frame.
+
+    This PR fixes the situation by enforcing the limit when transferring the existential deposit in order to create a new account.
+crates:
+- name: pallet-revive
+  bump: patch

substrate/frame/revive/src/exec.rs

@@ -1129,7 +1129,7 @@ where
 				let origin = &self.origin.account_id()?;
 
 				let ed = <Contracts<T>>::min_balance();
-				frame.nested_storage.record_charge(&StorageDeposit::Charge(ed));
+				frame.nested_storage.record_charge(&StorageDeposit::Charge(ed))?;
 				if self.skip_transfer {
 					T::Currency::set_balance(account_id, ed);
 				} else {
@@ -1379,41 +1379,36 @@ where
 		to: &T::AccountId,
 		value: U256,
 		storage_meter: &mut storage::meter::GenericMeter<T, S>,
-	) -> ExecResult {
+	) -> DispatchResult {
 		fn transfer_with_dust<T: Config>(
 			from: &AccountIdOf<T>,
 			to: &AccountIdOf<T>,
 			value: BalanceWithDust<BalanceOf<T>>,
-		) -> Result<(), ExecError> {
+		) -> DispatchResult {
 			let (value, dust) = value.deconstruct();
 
 			fn transfer_balance<T: Config>(
 				from: &AccountIdOf<T>,
 				to: &AccountIdOf<T>,
 				value: BalanceOf<T>,
-			) -> Result<(), ExecError> {
+			) -> DispatchResult {
 				T::Currency::transfer(from, to, value, Preservation::Preserve)
 				.map_err(|err| {
 					log::debug!(target: crate::LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}");
-					ExecError::from(Error::<T>::TransferFailed)
+					Error::<T>::TransferFailed
 				})?;
-				return Ok(())
+				Ok(())
 			}
 
 			fn transfer_dust<T: Config>(
 				from: &mut AccountInfo<T>,
 				to: &mut AccountInfo<T>,
 				dust: u32,
-			) -> Result<(), ExecError> {
-				from.dust = from
-					.dust
-					.checked_sub(dust)
-					.ok_or_else(|| ExecError::from(Error::<T>::TransferFailed))?;
-				to.dust = to
-					.dust
-					.checked_add(dust)
-					.ok_or_else(|| ExecError::from(Error::<T>::TransferFailed))?;
-				Ok::<(), ExecError>(())
+			) -> DispatchResult {
+				from.dust =
+					from.dust.checked_sub(dust).ok_or_else(|| Error::<T>::TransferFailed)?;
+				to.dust = to.dust.checked_add(dust).ok_or_else(|| Error::<T>::TransferFailed)?;
+				Ok(())
 			}
 
 			if dust.is_zero() {
@@ -1438,24 +1433,20 @@ where
 				)
 				.map_err(|err| {
 					log::debug!(target: crate::LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}");
-					ExecError::from(Error::<T>::TransferFailed)
+					Error::<T>::TransferFailed
 				})?;
 
-				from_info.dust = from_info
-					.dust
-					.checked_add(plank)
-					.ok_or_else(|| ExecError::from(Error::<T>::TransferFailed))?;
+				from_info.dust =
+					from_info.dust.checked_add(plank).ok_or_else(|| Error::<T>::TransferFailed)?;
 			}
 
 			transfer_balance::<T>(from, to, value)?;
 			transfer_dust::<T>(&mut from_info, &mut to_info, dust)?;
 
 			if to_info.dust.saturating_add(dust) >= plank {
 				T::Currency::mint_into(to, 1u32.into())?;
-				to_info.dust = to_info
-					.dust
-					.checked_sub(plank)
-					.ok_or_else(|| ExecError::from(Error::<T>::TransferFailed))?;
+				to_info.dust =
+					to_info.dust.checked_sub(plank).ok_or_else(|| Error::<T>::TransferFailed)?;
 			}
 
 			AccountInfoOf::<T>::set(&from_addr, Some(from_info));
@@ -1466,26 +1457,25 @@ where
 
 		let value = BalanceWithDust::<BalanceOf<T>>::from_value::<T>(value)?;
 		if value.is_zero() {
-			return Ok(Default::default());
+			return Ok(());
 		}
 
 		if <System<T>>::account_exists(to) {
-			return transfer_with_dust::<T>(from, to, value).map(|_| Default::default())
+			return transfer_with_dust::<T>(from, to, value)
 		}
 
 		let origin = origin.account_id()?;
 		let ed = <T as Config>::Currency::minimum_balance();
-		with_transaction(|| -> TransactionOutcome<ExecResult> {
-			match T::Currency::transfer(origin, to, ed, Preservation::Preserve)
-				.map_err(|_| Error::<T>::StorageDepositNotEnoughFunds.into())
+		with_transaction(|| -> TransactionOutcome<DispatchResult> {
+			match storage_meter
+				.record_charge(&StorageDeposit::Charge(ed))
+				.and_then(|_| {
+					T::Currency::transfer(origin, to, ed, Preservation::Preserve)
+						.map_err(|_| Error::<T>::StorageDepositNotEnoughFunds.into())
+				})
 				.and_then(|_| transfer_with_dust::<T>(from, to, value))
 			{
-				Ok(_) => {
-					// ed is taken from the transaction signer so it should be
-					// limited by the storage deposit
-					storage_meter.record_charge(&StorageDeposit::Charge(ed));
-					TransactionOutcome::Commit(Ok(Default::default()))
-				},
+				Ok(_) => TransactionOutcome::Commit(Ok(())),
 				Err(err) => TransactionOutcome::Rollback(Err(err)),
 			}
 		})
@@ -1507,6 +1497,8 @@ where
 			Origin::Root => return Err(DispatchError::RootNotAllowed.into()),
 		};
 		Self::transfer(origin, from, to, value, storage_meter)
+			.map(|_| Default::default())
+			.map_err(Into::into)
 	}
 
 	/// Reference to the current (top) frame.

substrate/frame/revive/src/lib.rs

@@ -1093,11 +1093,7 @@ where
 
 		let try_call = || {
 			let origin = Origin::from_runtime_origin(origin)?;
-			let mut storage_meter = match storage_deposit_limit {
-				DepositLimit::Balance(limit) => StorageMeter::new(limit),
-				DepositLimit::UnsafeOnlyForDryRun =>
-					StorageMeter::new_unchecked(BalanceOf::<T>::max_value()),
-			};
+			let mut storage_meter = StorageMeter::new(storage_deposit_limit.limit());
 			let result = ExecStack::<T, ContractBlob<T>>::run_call(
 				origin.clone(),
 				dest,
@@ -1152,11 +1148,7 @@ where
 		let mut gas_meter = GasMeter::new(gas_limit);
 		let mut storage_deposit = Default::default();
 		let unchecked_deposit_limit = storage_deposit_limit.is_unchecked();
-		let mut storage_deposit_limit = match storage_deposit_limit {
-			DepositLimit::Balance(limit) => limit,
-			DepositLimit::UnsafeOnlyForDryRun => BalanceOf::<T>::max_value(),
-		};
-
+		let mut storage_deposit_limit = storage_deposit_limit.limit();
 		let try_instantiate = || {
 			let instantiate_account = T::InstantiateOrigin::ensure_origin(origin.clone())?;
 
@@ -1177,12 +1169,7 @@ where
 					(ContractBlob::from_storage(code_hash, &mut gas_meter)?, Default::default()),
 			};
 			let instantiate_origin = Origin::from_account_id(instantiate_account.clone());
-			let mut storage_meter = if unchecked_deposit_limit {
-				StorageMeter::new_unchecked(storage_deposit_limit)
-			} else {
-				StorageMeter::new(storage_deposit_limit)
-			};
-
+			let mut storage_meter = StorageMeter::new(storage_deposit_limit);
 			let result = ExecStack::<T, ContractBlob<T>>::run_instantiate(
 				instantiate_account,
 				executable,

substrate/frame/revive/src/primitives.rs

@@ -23,6 +23,7 @@ use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::weights::Weight;
 use pallet_revive_uapi::ReturnFlags;
 use scale_info::TypeInfo;
+use sp_arithmetic::traits::Bounded;
 use sp_core::Get;
 use sp_runtime::{
 	traits::{One, Saturating, Zero},
@@ -53,6 +54,15 @@ impl<T> From<T> for DepositLimit<T> {
 	}
 }
 
+impl<T: Bounded + Copy> DepositLimit<T> {
+	pub fn limit(&self) -> T {
+		match self {
+			Self::UnsafeOnlyForDryRun => T::max_value(),
+			Self::Balance(limit) => *limit,
+		}
+	}
+}
+
 /// Result type of a `bare_call` or `bare_instantiate` call as well as `ContractsApi::call` and
 /// `ContractsApi::instantiate`.
 ///

substrate/frame/revive/src/storage/meter.rs

@@ -33,7 +33,7 @@ use frame_support::{
 };
 use sp_runtime::{
 	traits::{Saturating, Zero},
-	DispatchError, FixedPointNumber, FixedU128,
+	DispatchError, DispatchResult, FixedPointNumber, FixedU128,
 };
 
 /// Deposit that uses the native fungible's balance type.
@@ -104,6 +104,10 @@ pub struct RawMeter<T: Config, E, S: State + Default + Debug> {
 	/// We only have one charge per contract hence the size of this vector is
 	/// limited by the maximum call depth.
 	charges: Vec<Charge<T>>,
+	/// True if this is the root meter.
+	///
+	/// Sometimes we cannot know at compile time.
+	is_root: bool,
 	/// Type parameter only used in impls.
 	_phantom: PhantomData<(E, S)>,
 }
@@ -308,8 +312,18 @@ where
 	///
 	/// This will not perform a charge. It just records it to reflect it in the
 	/// total amount of storage required for a transaction.
-	pub fn record_charge(&mut self, amount: &DepositOf<T>) {
-		self.total_deposit = self.total_deposit.saturating_add(&amount);
+	pub fn record_charge(&mut self, amount: &DepositOf<T>) -> DispatchResult {
+		let total_deposit = self.total_deposit.saturating_add(&amount);
+
+		// Limits are enforced at the end of each frame. But plain balance transfers
+		// do not sapwn a frame. This is specifically to enforce the limit for those.
+		if self.is_root && total_deposit.charge_or_zero() > self.limit {
+			log::debug!( target: LOG_TARGET, "Storage deposit limit exhausted: {:?} > {:?}", amount, self.limit);
+			return Err(<Error<T>>::StorageDepositLimitExhausted.into())
+		}
+
+		self.total_deposit = total_deposit;
+		Ok(())
 	}
 
 	/// The amount of balance that is still available from the original `limit`.
@@ -338,12 +352,7 @@ where
 	/// If the limit larger then what the origin can afford we will just fail
 	/// when collecting the deposits in `try_into_deposit`.
 	pub fn new(limit: BalanceOf<T>) -> Self {
-		Self { limit, ..Default::default() }
-	}
-
-	/// Create new storage meter without checking the limit.
-	pub fn new_unchecked(limit: BalanceOf<T>) -> Self {
-		return Self { limit, ..Default::default() }
+		Self { limit, is_root: true, ..Default::default() }
 	}
 
 	/// The total amount of deposit that should change hands as result of the execution
@@ -401,7 +410,8 @@ impl<T: Config, E: Ext<T>> RawMeter<T, E, Nested> {
 	/// If this functions is used the amount of the charge has to be stored by the caller somewhere
 	/// alese in order to be able to refund it.
 	pub fn charge_deposit(&mut self, contract: T::AccountId, amount: DepositOf<T>) {
-		self.record_charge(&amount);
+		// will not fail in a nested meter
+		self.record_charge(&amount).ok();
 		self.charges.push(Charge { contract, amount, state: ContractState::Alive });
 	}
 

substrate/frame/revive/src/tests.rs

@@ -32,7 +32,7 @@ use crate::{
 	weights::WeightInfo,
 	AccountId32Mapper, AccountInfo, AccountInfoOf, BalanceOf, BalanceWithDust, BumpNonce, Code,
 	CodeInfoOf, Config, ContractInfo, DeletionQueueCounter, DepositLimit, Error, EthTransactError,
-	HoldReason, Origin, Pallet, PristineCode, H160,
+	HoldReason, Origin, Pallet, PristineCode, StorageDeposit, H160,
 };
 use assert_matches::assert_matches;
 use codec::Encode;
@@ -603,6 +603,41 @@ fn contract_call_transfer_with_dust_works() {
 	});
 }
 
+#[test]
+fn deposit_limit_enforced_on_plain_transfer() {
+	ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+		let _ = <Test as Config>::Currency::set_balance(&BOB, 1_000_000);
+
+		// sending balance to a new account should fail when the limit is lower than the ed
+		let result = builder::bare_call(CHARLIE_ADDR)
+			.native_value(1)
+			.storage_deposit_limit(190.into())
+			.build();
+		assert_err!(result.result, <Error<Test>>::StorageDepositLimitExhausted);
+		assert_eq!(result.storage_deposit, StorageDeposit::Charge(0));
+		assert_eq!(test_utils::get_balance(&CHARLIE), 0);
+
+		// works when the account is prefunded
+		let result = builder::bare_call(BOB_ADDR)
+			.native_value(1)
+			.storage_deposit_limit(0.into())
+			.build();
+		assert_ok!(result.result);
+		assert_eq!(result.storage_deposit, StorageDeposit::Charge(0));
+		assert_eq!(test_utils::get_balance(&BOB), 1_000_001);
+
+		// also works allowing enough deposit
+		let result = builder::bare_call(CHARLIE_ADDR)
+			.native_value(1)
+			.storage_deposit_limit(200.into())
+			.build();
+		assert_ok!(result.result);
+		assert_eq!(result.storage_deposit, StorageDeposit::Charge(200));
+		assert_eq!(test_utils::get_balance(&CHARLIE), 201);
+	});
+}
+
 #[test]
 fn instantiate_and_call_and_deposit_event() {
 	let (binary, code_hash) = compile_module("event_and_return_on_deploy").unwrap();