Contracts: runtime_call and storage_deposit (paritytech#13990)

* wip * add comments * fix comment * comments * comments * PR comment * field orders * Update frame/contracts/src/tests.rs * Update frame/contracts/fixtures/call_runtime_and_call.wat Co-authored-by: Sasha Gryaznov <[email protected]> * Apply suggestions from code review Co-authored-by: Sasha Gryaznov <[email protected]> * Apply suggestions from code review Co-authored-by: Sasha Gryaznov <[email protected]> * Update frame/contracts/src/tests.rs Co-authored-by: Sasha Gryaznov <[email protected]> * Validate fees of failed call * Update frame/contracts/src/tests.rs * Update frame/contracts/src/tests.rs * Update frame/contracts/src/tests.rs * bubble up refund error * rename fixture file --------- Co-authored-by: Sasha Gryaznov <[email protected]> Co-authored-by: parity-processbot <>
nathanwhit · Jul 19, 2023 · 4d80008 · 4d80008
1 parent 9d3eba6
commit 4d80008
Show file tree

Hide file tree

Showing 6 changed files with 189 additions and 67 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml
@@ -59,6 +59,7 @@ pallet-balances = { version = "4.0.0-dev", path = "../balances" }
 pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" }
 pallet-insecure-randomness-collective-flip = { version = "4.0.0-dev", path = "../insecure-randomness-collective-flip" }
 pallet-utility = { version = "4.0.0-dev", path = "../utility" }
+pallet-proxy = { version = "4.0.0-dev", path = "../proxy" }
 sp-keystore = { version = "0.13.0", path = "../../primitives/keystore" }
 
 [features]

diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat
@@ -0,0 +1,56 @@
+(module
+	(import "seal0" "call_runtime" (func $call_runtime (param i32 i32) (result i32)))
+	(import "seal1" "seal_call" (func $seal_call (param i32 i32 i64 i32 i32 i32 i32 i32) (result i32)))
+	(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
+	(import "seal0" "seal_return" (func $seal_return (param i32 i32 i32)))
+	(import "env" "memory" (memory 1 1))
+
+	(func $assert (param i32)
+		(block $ok
+			(br_if $ok (get_local 0))
+			(unreachable)
+		)
+	)
+
+	(func (export "call")
+	    ;; Store available input size at offset 0.
+        (i32.store (i32.const 0) (i32.const 512))
+
+		;; read input data
+        (call $seal_input (i32.const 4) (i32.const 0))
+
+		;; Input data layout.
+		;; [0..4) - size of the call
+		;; [4..8) - how many bytes to add to storage
+		;; [8..40) - address of the callee
+		;; [40..n) - encoded runtime call
+
+		;; Invoke call_runtime with the encoded call passed to this contract.
+		(call $assert (i32.eqz
+			(call $call_runtime
+				(i32.const 40)                ;; Pointer where the call is stored
+				(i32.sub
+					(i32.load (i32.const 0))  ;; Size of the call
+					(i32.const 36)            ;; Subtract size of the subcall-related part: 4 bytes for storage length to add + 32 bytes of the callee address 
+				)
+			)
+		))
+
+		;; call passed contract
+		(call $assert (i32.eqz
+			(call $seal_call
+				(i32.const 0) ;; No flags
+				(i32.const 8)  ;; Pointer to "callee" address.
+				(i64.const 0)  ;; How much gas to devote for the execution. 0 = all.
+				(i32.const 512) ;; Pointer to the buffer with value to transfer
+				(i32.const 4) ;; Pointer to input data buffer address
+				(i32.const 4)  ;; Length of input data buffer
+				(i32.const 4294967295) ;; u32 max value is the sentinel value: do not copy output
+				(i32.const 0) ;; Length is ignored in this case
+			)
+		))
+	)
+
+	(func (export "deploy"))
+)
+
diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs
@@ -1044,7 +1044,15 @@ impl<T: Config> Invokable<T> for CallInput<T> {
 			debug_message,
 			*determinism,
 		);
-		InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(&origin), result }
+
+		match storage_meter.try_into_deposit(&origin) {
+			Ok(storage_deposit) => InternalOutput { gas_meter, storage_deposit, result },
+			Err(err) => InternalOutput {
+				gas_meter,
+				storage_deposit: Default::default(),
+				result: Err(err.into()),
+			},
+		}
 	}
 }
 
@@ -1105,8 +1113,9 @@ impl<T: Config> Invokable<T> for InstantiateInput<T> {
 				&salt,
 				debug_message,
 			);
+
 			storage_deposit = storage_meter
-				.into_deposit(&origin)
+				.try_into_deposit(&origin)?
 				.saturating_add(&StorageDeposit::Charge(extra_deposit));
 			result
 		};

diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs
@@ -19,7 +19,7 @@
 
 use crate::{
 	storage::{ContractInfo, DepositAccount},
-	BalanceOf, Config, Error, Inspect, Pallet, System, LOG_TARGET,
+	BalanceOf, Config, Error, Inspect, Pallet, System,
 };
 use codec::Encode;
 use frame_support::{
@@ -85,7 +85,7 @@ pub trait Ext<T: Config> {
 		deposit_account: &DepositAccount<T>,
 		amount: &DepositOf<T>,
 		terminated: bool,
-	);
+	) -> Result<(), DispatchError>;
 }
 
 /// This [`Ext`] is used for actual on-chain execution when balance needs to be charged.
@@ -366,14 +366,14 @@ where
 	///
 	/// This drops the root meter in order to make sure it is only called when the whole
 	/// execution did finish.
-	pub fn into_deposit(self, origin: &T::AccountId) -> DepositOf<T> {
+	pub fn try_into_deposit(self, origin: &T::AccountId) -> Result<DepositOf<T>, DispatchError> {
 		for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) {
-			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
+			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
 		}
 		for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) {
-			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
+			E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
 		}
-		self.total_deposit
+		Ok(self.total_deposit)
 	}
 }
 
@@ -428,7 +428,8 @@ where
 			info.deposit_account(),
 			&deposit.saturating_sub(&Deposit::Charge(ed)),
 			false,
-		);
+		)?;
+
 		System::<T>::inc_consumers(info.deposit_account())?;
 
 		// We also need to make sure that the contract's account itself exists.
@@ -514,71 +515,27 @@ impl<T: Config> Ext<T> for ReservingExt {
 		deposit_account: &DepositAccount<T>,
 		amount: &DepositOf<T>,
 		terminated: bool,
-	) {
-		// There is nothing we can do when this fails as this constitutes a bug in the runtime.
-		// We need to settle for emitting an error log in this case.
-		//
-		// # Note
-		//
-		// This is infallible because it is called in a part of the execution where we cannot
-		// simply roll back. It might make sense to do some refactoring to move the deposit
-		// collection to the fallible part of execution.
+	) -> Result<(), DispatchError> {
 		match amount {
-			Deposit::Charge(amount) => {
-				// This will never fail because a deposit account is required to exist
-				// at all times. The pallet enforces this invariant by holding a consumer reference
-				// on the deposit account as long as the contract exists.
-				//
-				// The sender always has enough balance because we checked that it had enough
-				// balance when instantiating the storage meter. There is no way for the sender
-				// which is a plain account to send away this balance in the meantime.
-				let result = T::Currency::transfer(
-					origin,
-					deposit_account,
-					*amount,
-					ExistenceRequirement::KeepAlive,
-				);
-				if let Err(err) = result {
-					log::error!(
-						target: LOG_TARGET,
-						"Failed to transfer storage deposit {:?} from origin {:?} to deposit account {:?}: {:?}",
-						amount, origin, deposit_account, err,
-					);
-					if cfg!(debug_assertions) {
-						panic!("Unable to collect storage deposit. This is a bug.");
-					}
-				}
-			},
-			// The receiver always exists because the initial value transfer from the
-			// origin to the contract has a keep alive existence requirement. When taking a deposit
-			// we make sure to leave at least the ed in the free balance.
-			//
-			// The sender always has enough balance because we track it in the `ContractInfo` and
-			// never send more back than we have. No one has access to the deposit account. Hence no
-			// other interaction with this account takes place.
+			Deposit::Charge(amount) => T::Currency::transfer(
+				origin,
+				deposit_account,
+				*amount,
+				ExistenceRequirement::KeepAlive,
+			),
 			Deposit::Refund(amount) => {
 				if terminated {
 					System::<T>::dec_consumers(&deposit_account);
 				}
-				let result = T::Currency::transfer(
+				T::Currency::transfer(
 					deposit_account,
 					origin,
 					*amount,
 					// We can safely use `AllowDeath` because our own consumer prevents an removal.
 					ExistenceRequirement::AllowDeath,
-				);
-				if matches!(result, Err(_)) {
-					log::error!(
-						target: LOG_TARGET,
-						"Failed to refund storage deposit {:?} from deposit account {:?} to origin {:?}: {:?}",
-						amount, deposit_account, origin, result,
-					);
-					if cfg!(debug_assertions) {
-						panic!("Unable to refund storage deposit. This is a bug.");
-					}
-				}
+				)
 			},
-		};
+		}
 	}
 }
 
@@ -651,7 +608,7 @@ mod tests {
 			contract: &DepositAccount<Test>,
 			amount: &DepositOf<Test>,
 			terminated: bool,
-		) {
+		) -> Result<(), DispatchError> {
 			TestExtTestValue::mutate(|ext| {
 				ext.charges.push(Charge {
 					origin: origin.clone(),
@@ -660,6 +617,7 @@ mod tests {
 					terminated,
 				})
 			});
+			Ok(())
 		}
 	}
 
@@ -757,7 +715,7 @@ mod tests {
 		nested0.enforce_limit(Some(&mut nested0_info)).unwrap();
 		meter.absorb(nested0, DepositAccount(BOB), Some(&mut nested0_info));
 
-		meter.into_deposit(&ALICE);
+		meter.try_into_deposit(&ALICE).unwrap();
 
 		assert_eq!(nested0_info.extra_deposit(), 112);
 		assert_eq!(nested1_info.extra_deposit(), 110);
@@ -817,7 +775,7 @@ mod tests {
 		nested0.absorb(nested1, DepositAccount(CHARLIE), None);
 
 		meter.absorb(nested0, DepositAccount(BOB), None);
-		meter.into_deposit(&ALICE);
+		meter.try_into_deposit(&ALICE).unwrap();
 
 		assert_eq!(
 			TestExtTestValue::get(),

diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs
@@ -33,7 +33,7 @@ use crate::{
 use assert_matches::assert_matches;
 use codec::Encode;
 use frame_support::{
-	assert_err, assert_err_ignore_postinfo, assert_noop, assert_ok,
+	assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok,
 	dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo},
 	parameter_types,
 	storage::child,
@@ -70,6 +70,7 @@ frame_support::construct_runtime!(
 		Randomness: pallet_insecure_randomness_collective_flip::{Pallet, Storage},
 		Utility: pallet_utility::{Pallet, Call, Storage, Event},
 		Contracts: pallet_contracts::{Pallet, Call, Storage, Event<T>},
+		Proxy: pallet_proxy::{Pallet, Call, Storage, Event<T>},
 	}
 );
 
@@ -337,6 +338,22 @@ impl pallet_utility::Config for Test {
 	type PalletsOrigin = OriginCaller;
 	type WeightInfo = ();
 }
+
+impl pallet_proxy::Config for Test {
+	type RuntimeEvent = RuntimeEvent;
+	type RuntimeCall = RuntimeCall;
+	type Currency = Balances;
+	type ProxyType = ();
+	type ProxyDepositBase = ConstU64<1>;
+	type ProxyDepositFactor = ConstU64<1>;
+	type MaxProxies = ConstU32<32>;
+	type WeightInfo = ();
+	type MaxPending = ConstU32<32>;
+	type CallHasher = BlakeTwo256;
+	type AnnouncementDepositBase = ConstU64<1>;
+	type AnnouncementDepositFactor = ConstU64<1>;
+}
+
 parameter_types! {
 	pub MySchedule: Schedule<Test> = {
 		let mut schedule = <Schedule<Test>>::default();
@@ -2983,6 +3000,86 @@ fn sr25519_verify() {
 	})
 }
 
+#[test]
+fn failed_deposit_charge_should_roll_back_call() {
+	let (wasm_caller, _) = compile_module::<Test>("call_runtime_and_call").unwrap();
+	let (wasm_callee, _) = compile_module::<Test>("store_call").unwrap();
+	const ED: u64 = 200;
+
+	let execute = || {
+		ExtBuilder::default().existential_deposit(ED).build().execute_with(|| {
+			let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+
+			// Instantiate both contracts.
+			let addr_caller = Contracts::bare_instantiate(
+				ALICE,
+				0,
+				GAS_LIMIT,
+				None,
+				Code::Upload(wasm_caller.clone()),
+				vec![],
+				vec![],
+				false,
+			)
+			.result
+			.unwrap()
+			.account_id;
+			let addr_callee = Contracts::bare_instantiate(
+				ALICE,
+				0,
+				GAS_LIMIT,
+				None,
+				Code::Upload(wasm_callee.clone()),
+				vec![],
+				vec![],
+				false,
+			)
+			.result
+			.unwrap()
+			.account_id;
+
+			// Give caller proxy access to Alice.
+			assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0));
+
+			// Create a Proxy call that will attempt to transfer away Alice's balance.
+			let transfer_call =
+				Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death {
+					dest: CHARLIE,
+					value: pallet_balances::Pallet::<Test>::free_balance(&ALICE) - 2 * ED,
+				}));
+
+			// Wrap the transfer call in a proxy call.
+			let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy {
+				real: ALICE,
+				force_proxy_type: Some(()),
+				call: transfer_call,
+			});
+
+			let data = (
+				(ED - DepositPerItem::get()) as u32, // storage length
+				addr_callee,
+				transfer_proxy_call,
+			);
+
+			<Pallet<Test>>::call(
+				RuntimeOrigin::signed(ALICE),
+				addr_caller.clone(),
+				0,
+				GAS_LIMIT,
+				None,
+				data.encode(),
+			)
+		})
+	};
+
+	// With a low enough deposit per byte, the call should succeed.
+	let result = execute().unwrap();
+
+	// Bump the deposit per byte to a high value to trigger a FundsUnavailable error.
+	DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = ED);
+	assert_err_with_weight!(execute(), TokenError::FundsUnavailable, result.actual_weight);
+}
+
 #[test]
 fn upload_code_works() {
 	let (wasm, code_hash) = compile_module::<Test>("dummy").unwrap();