contracts: Add new seal_call that offers new features (paritytech#8909)

* Add new `seal_call` that offers new features

* Fix doc typo

Co-authored-by: Michael Müller <michi@parity.io>

* Fix doc typos

Co-authored-by: Michael Müller <michi@parity.io>

* Fix comment on assert

* Update CHANGELOG.md

Co-authored-by: Michael Müller <michi@parity.io>

Author: 2 people

Cargo.lock

@@ -20,6 +20,9 @@ In other words: Upgrading this pallet will not break pre-existing contracts.
 
 ### Added
 
+- New **unstable** version of `seal_call` that offers more features.
+[#8909](https://github.com/paritytech/substrate/pull/8909)
+
 - New **unstable** `seal_rent_params` and `seal_rent_status` contract callable function.
 [#8231](https://github.com/paritytech/substrate/pull/8231)
 [#8780](https://github.com/paritytech/substrate/pull/8780)

frame/contracts/CHANGELOG.md

@@ -13,6 +13,7 @@ readme = "README.md"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+bitflags = "1.0"
 codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] }
 log = { version = "0.4", default-features = false }
 pwasm-utils = { version = "0.18", default-features = false }

frame/contracts/Cargo.toml

@@ -167,6 +167,7 @@ pub trait Ext: sealing::Sealed {
 		to: AccountIdOf<Self::T>,
 		value: BalanceOf<Self::T>,
 		input_data: Vec<u8>,
+		allows_reentry: bool,
 	) -> Result<(ExecReturnValue, u32), (ExecError, u32)>;
 
 	/// Instantiate a contract from the given code.
@@ -457,6 +458,8 @@ pub struct Frame<T: Config> {
 	entry_point: ExportedFunction,
 	/// The gas meter capped to the supplied gas limit.
 	nested_meter: GasMeter<T>,
+	/// If `false` the contract enabled its defense against reentrance attacks.
+	allows_reentry: bool,
 }
 
 /// Parameter passed in when creating a new `Frame`.
@@ -731,6 +734,7 @@ where
 			entry_point,
 			nested_meter: gas_meter.nested(gas_limit)
 				.map_err(|e| (e.into(), executable.code_len()))?,
+			allows_reentry: true,
 		};
 
 		Ok((frame, executable))
@@ -1014,6 +1018,11 @@ where
 		self.frames().skip(1).any(|f| &f.account_id == account_id)
 	}
 
+	/// Returns whether the specified contract allows to be reentered right now.
+	fn allows_reentry(&self, id: &AccountIdOf<T>) -> bool {
+		!self.frames().any(|f| &f.account_id == id && !f.allows_reentry)
+	}
+
 	/// Increments the cached account id and returns the value to be used for the trie_id.
 	fn next_trie_seed(&mut self) -> u64 {
 		let next = if let Some(current) = self.account_counter {
@@ -1045,25 +1054,44 @@ where
 		to: T::AccountId,
 		value: BalanceOf<T>,
 		input_data: Vec<u8>,
+		allows_reentry: bool,
 	) -> Result<(ExecReturnValue, u32), (ExecError, u32)> {
-		// We ignore instantiate frames in our search for a cached contract.
-		// Otherwise it would be possible to recursively call a contract from its own
-		// constructor: We disallow calling not fully constructed contracts.
-		let cached_info = self
-			.frames()
-			.find(|f| f.entry_point == ExportedFunction::Call && f.account_id == to)
-			.and_then(|f| {
-				match &f.contract_info {
-					CachedContract::Cached(contract) => Some(contract.clone()),
-					_ => None,
-				}
-			});
-		let executable = self.push_frame(
-			FrameArgs::Call{dest: to, cached_info},
-			value,
-			gas_limit
-		)?;
-		self.run(executable, input_data)
+		// Before pushing the new frame: Protect the caller contract against reentrancy attacks.
+		// It is important to do this before calling `allows_reentry` so that a direct recursion
+		// is caught by it.
+		self.top_frame_mut().allows_reentry = allows_reentry;
+
+		let try_call = || {
+			if !self.allows_reentry(&to) {
+				return Err((<Error<T>>::ReentranceDenied.into(), 0));
+			}
+			// We ignore instantiate frames in our search for a cached contract.
+			// Otherwise it would be possible to recursively call a contract from its own
+			// constructor: We disallow calling not fully constructed contracts.
+			let cached_info = self
+				.frames()
+				.find(|f| f.entry_point == ExportedFunction::Call && f.account_id == to)
+				.and_then(|f| {
+					match &f.contract_info {
+						CachedContract::Cached(contract) => Some(contract.clone()),
+						_ => None,
+					}
+				});
+			let executable = self.push_frame(
+				FrameArgs::Call{dest: to, cached_info},
+				value,
+				gas_limit
+			)?;
+			self.run(executable, input_data)
+		};
+
+		// We need to make sure to reset `allows_reentry` even on failure.
+		let result = try_call();
+
+		// Protection is on a per call basis.
+		self.top_frame_mut().allows_reentry = true;
+
+		result
 	}
 
 	fn instantiate(
@@ -1097,7 +1125,7 @@ where
 		beneficiary: &AccountIdOf<Self::T>,
 	) -> Result<u32, (DispatchError, u32)> {
 		if self.is_recursive() {
-			return Err((Error::<T>::ReentranceDenied.into(), 0));
+			return Err((Error::<T>::TerminatedWhileReentrant.into(), 0));
 		}
 		let frame = self.top_frame_mut();
 		let info = frame.terminate();
@@ -1125,7 +1153,7 @@ where
 		delta: Vec<StorageKey>,
 	) -> Result<(u32, u32), (DispatchError, u32, u32)> {
 		if self.is_recursive() {
-			return Err((Error::<T>::ReentranceDenied.into(), 0, 0));
+			return Err((Error::<T>::TerminatedWhileReentrant.into(), 0, 0));
 		}
 		let origin_contract = self.top_frame_mut().contract_info().clone();
 		let result = Rent::<T, E>::restore_to(
@@ -1308,12 +1336,14 @@ mod tests {
 		exec::ExportedFunction::*,
 		Error, Weight,
 	};
+	use codec::{Encode, Decode};
 	use sp_core::Bytes;
 	use sp_runtime::DispatchError;
 	use assert_matches::assert_matches;
 	use std::{cell::RefCell, collections::HashMap, rc::Rc};
 	use pretty_assertions::{assert_eq, assert_ne};
 	use pallet_contracts_primitives::ReturnFlags;
+	use frame_support::{assert_ok, assert_err};
 
 	type MockStack<'a> = Stack<'a, Test, MockExecutable>;
 
@@ -1731,7 +1761,7 @@ mod tests {
 		let value = Default::default();
 		let recurse_ch = MockLoader::insert(Call, |ctx, _| {
 			// Try to call into yourself.
-			let r = ctx.ext.call(0, BOB, 0, vec![]);
+			let r = ctx.ext.call(0, BOB, 0, vec![], true);
 
 			REACHED_BOTTOM.with(|reached_bottom| {
 				let mut reached_bottom = reached_bottom.borrow_mut();
@@ -1789,7 +1819,7 @@ mod tests {
 
 			// Call into CHARLIE contract.
 			assert_matches!(
-				ctx.ext.call(0, CHARLIE, 0, vec![]),
+				ctx.ext.call(0, CHARLIE, 0, vec![], true),
 				Ok(_)
 			);
 			exec_success()
@@ -1832,7 +1862,7 @@ mod tests {
 
 			// Call into charlie contract.
 			assert_matches!(
-				ctx.ext.call(0, CHARLIE, 0, vec![]),
+				ctx.ext.call(0, CHARLIE, 0, vec![], true),
 				Ok(_)
 			);
 			exec_success()
@@ -2263,7 +2293,7 @@ mod tests {
 				assert_ne!(original_allowance, changed_allowance);
 				ctx.ext.set_rent_allowance(changed_allowance);
 				assert_eq!(
-					ctx.ext.call(0, CHARLIE, 0, vec![]).map(|v| v.0).map_err(|e| e.0),
+					ctx.ext.call(0, CHARLIE, 0, vec![], true).map(|v| v.0).map_err(|e| e.0),
 					exec_trapped()
 				);
 				assert_eq!(ctx.ext.rent_allowance(), changed_allowance);
@@ -2272,7 +2302,7 @@ mod tests {
 			exec_success()
 		});
 		let code_charlie = MockLoader::insert(Call, |ctx, _| {
-			assert!(ctx.ext.call(0, BOB, 0, vec![99]).is_ok());
+			assert!(ctx.ext.call(0, BOB, 0, vec![99], true).is_ok());
 			exec_trapped()
 		});
 
@@ -2299,7 +2329,7 @@ mod tests {
 	fn recursive_call_during_constructor_fails() {
 		let code = MockLoader::insert(Constructor, |ctx, _| {
 			assert_matches!(
-				ctx.ext.call(0, ctx.ext.address().clone(), 0, vec![]),
+				ctx.ext.call(0, ctx.ext.address().clone(), 0, vec![], true),
 				Err((ExecError{error, ..}, _)) if error == <Error<Test>>::ContractNotFound.into()
 			);
 			exec_success()
@@ -2390,4 +2420,84 @@ mod tests {
 
 		assert_eq!(&String::from_utf8(debug_buffer).unwrap(), "This is a testMore text");
 	}
+
+	#[test]
+	fn call_reentry_direct_recursion() {
+		// call the contract passed as input with disabled reentry
+		let code_bob = MockLoader::insert(Call, |ctx, _| {
+			let dest = Decode::decode(&mut ctx.input_data.as_ref()).unwrap();
+			ctx.ext.call(0, dest, 0, vec![], false).map(|v| v.0).map_err(|e| e.0)
+		});
+
+		let code_charlie = MockLoader::insert(Call, |_, _| {
+			exec_success()
+		});
+
+		ExtBuilder::default().build().execute_with(|| {
+			let schedule = <Test as Config>::Schedule::get();
+			place_contract(&BOB, code_bob);
+			place_contract(&CHARLIE, code_charlie);
+
+			// Calling another contract should succeed
+			assert_ok!(MockStack::run_call(
+				ALICE,
+				BOB,
+				&mut GasMeter::<Test>::new(GAS_LIMIT),
+				&schedule,
+				0,
+				CHARLIE.encode(),
+				None,
+			));
+
+			// Calling into oneself fails
+			assert_err!(
+				MockStack::run_call(
+					ALICE,
+					BOB,
+					&mut GasMeter::<Test>::new(GAS_LIMIT),
+					&schedule,
+					0,
+					BOB.encode(),
+					None,
+				).map_err(|e| e.0.error),
+				<Error<Test>>::ReentranceDenied,
+			);
+		});
+	}
+
+	#[test]
+	fn call_deny_reentry() {
+		let code_bob = MockLoader::insert(Call, |ctx, _| {
+			if ctx.input_data[0] == 0 {
+				ctx.ext.call(0, CHARLIE, 0, vec![], false).map(|v| v.0).map_err(|e| e.0)
+			} else {
+				exec_success()
+			}
+		});
+
+		// call BOB with input set to '1'
+		let code_charlie = MockLoader::insert(Call, |ctx, _| {
+			ctx.ext.call(0, BOB, 0, vec![1], true).map(|v| v.0).map_err(|e| e.0)
+		});
+
+		ExtBuilder::default().build().execute_with(|| {
+			let schedule = <Test as Config>::Schedule::get();
+			place_contract(&BOB, code_bob);
+			place_contract(&CHARLIE, code_charlie);
+
+			// BOB -> CHARLIE -> BOB fails as BOB denies reentry.
+			assert_err!(
+				MockStack::run_call(
+					ALICE,
+					BOB,
+					&mut GasMeter::<Test>::new(GAS_LIMIT),
+					&schedule,
+					0,
+					vec![0],
+					None,
+				).map_err(|e| e.0.error),
+				<Error<Test>>::ReentranceDenied,
+			);
+		});
+	}
 }

frame/contracts/src/exec.rs

@@ -562,12 +562,11 @@ pub mod pallet {
 		ContractTrapped,
 		/// The size defined in `T::MaxValueSize` was exceeded.
 		ValueTooLarge,
-		/// The action performed is not allowed while the contract performing it is already
-		/// on the call stack. Those actions are contract self destruction and restoration
-		/// of a tombstone.
-		ReentranceDenied,
-		/// `seal_input` was called twice from the same contract execution context.
-		InputAlreadyRead,
+		/// Termination of a contract is not allowed while the contract is already
+		/// on the call stack. Can be triggered by `seal_terminate` or `seal_restore_to.
+		TerminatedWhileReentrant,
+		/// `seal_call` forwarded this contracts input. It therefore is no longer available.
+		InputForwarded,
 		/// The subject passed to `seal_random` exceeds the limit.
 		RandomSubjectTooLong,
 		/// The amount of topics passed to `seal_deposit_events` exceeds the limit.
@@ -602,6 +601,8 @@ pub mod pallet {
 		TerminatedInConstructor,
 		/// The debug message specified to `seal_debug_message` does contain invalid UTF-8.
 		DebugMessageInvalidUTF8,
+		/// A call tried to invoke a contract that is flagged as non-reentrant.
+		ReentranceDenied,
 	}
 
 	/// A mapping from an original code hash to the original code, untouched by instrumentation.