diff --git a/Cargo.lock b/Cargo.lock index b7c7a813fede1..90de9fe457e60 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5597,6 +5597,7 @@ dependencies = [ "assert_matches", "bitflags", "env_logger 0.9.3", + "environmental", "frame-benchmarking", "frame-support", "frame-system", diff --git a/frame/contracts/CHANGELOG.md b/frame/contracts/CHANGELOG.md index ab3998e6dc4f7..dcb9d6d4d2b20 100644 --- a/frame/contracts/CHANGELOG.md +++ b/frame/contracts/CHANGELOG.md @@ -20,6 +20,9 @@ In other words: Upgrading this pallet will not break pre-existing contracts. ### Added +- Forbid calling back to contracts after switching to runtime +[#13443](https://github.com/paritytech/substrate/pull/13443) + - Allow contracts to dispatch calls into the runtime (**unstable**) [#9276](https://github.com/paritytech/substrate/pull/9276) diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index cd41ae2e39b55..e19326b785b2b 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -35,6 +35,7 @@ rand = { version = "0.8", optional = true, default-features = false } rand_pcg = { version = "0.3", optional = true } # Substrate Dependencies +environmental = { version = "1.1.4", default-features = false } frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } @@ -80,6 +81,7 @@ std = [ "log/std", "rand/std", "wasmparser/std", + "environmental/std", ] runtime-benchmarks = [ "frame-benchmarking/runtime-benchmarks", diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a115cf119ceef..4f5d7ba305fc1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -100,13 +100,14 @@ pub mod weights; mod tests; use crate::{ - exec::{AccountIdOf, ExecError, Executable, Stack as ExecStack}, + exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Stack as ExecStack}, gas::GasMeter, storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract}, wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate}, weights::WeightInfo, }; use codec::{Codec, Encode, HasCompact}; +use environmental::*; use frame_support::{ dispatch::{Dispatchable, GetDispatchInfo, Pays, PostDispatchInfo}, ensure, @@ -616,16 +617,16 @@ pub mod pallet { let gas_limit: Weight = gas_limit.into(); let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; - let mut output = Self::internal_call( + let common = CommonInput { origin, - dest, value, - gas_limit, - storage_deposit_limit.map(Into::into), data, - None, - Determinism::Deterministic, - ); + gas_limit, + storage_deposit_limit: storage_deposit_limit.map(Into::into), + debug_message: None, + }; + let mut output = CallInput:: { dest, determinism: Determinism::Deterministic } + .run_guarded(common); if let Ok(retval) = &output.result { if retval.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -678,16 +679,16 @@ pub mod pallet { let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let mut output = Self::internal_instantiate( + let common = CommonInput { origin, value, - gas_limit, - storage_deposit_limit.map(Into::into), - Code::Upload(code), data, - salt, - None, - ); + gas_limit, + storage_deposit_limit: storage_deposit_limit.map(Into::into), + debug_message: None, + }; + let mut output = + InstantiateInput:: { code: Code::Upload(code), salt }.run_guarded(common); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -720,16 +721,16 @@ pub mod pallet { let origin = ensure_signed(origin)?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let mut output = Self::internal_instantiate( + let common = CommonInput { origin, value, - gas_limit, - storage_deposit_limit.map(Into::into), - Code::Existing(code_hash), data, - salt, - None, - ); + gas_limit, + storage_deposit_limit: storage_deposit_limit.map(Into::into), + debug_message: None, + }; + let mut output = + InstantiateInput:: { code: Code::Existing(code_hash), salt }.run_guarded(common); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -872,6 +873,9 @@ pub mod pallet { /// This can be triggered by a call to `seal_terminate`. TerminatedInConstructor, /// A call tried to invoke a contract that is flagged as non-reentrant. + /// The only other cause is that a call from a contract into the runtime tried to call back + /// into `pallet-contracts`. This would make the whole pallet reentrant with regard to + /// contract code execution which is not supported. ReentranceDenied, /// Origin doesn't have enough balance to pay the required storage deposits. StorageDepositNotEnoughFunds, @@ -951,11 +955,27 @@ pub mod pallet { StorageValue<_, BoundedVec, ValueQuery>; } -/// Return type of the private [`Pallet::internal_call`] function. -type InternalCallOutput = InternalOutput; +/// Context of a contract invocation. +struct CommonInput<'a, T: Config> { + origin: T::AccountId, + value: BalanceOf, + data: Vec, + gas_limit: Weight, + storage_deposit_limit: Option>, + debug_message: Option<&'a mut DebugBufferVec>, +} -/// Return type of the private [`Pallet::internal_instantiate`] function. -type InternalInstantiateOutput = InternalOutput, ExecReturnValue)>; +/// Input specific to a call into contract. +struct CallInput { + dest: T::AccountId, + determinism: Determinism, +} + +/// Input specific to a contract instantiation invocation. +struct InstantiateInput { + code: Code>, + salt: Vec, +} /// Return type of private helper functions. struct InternalOutput { @@ -967,6 +987,164 @@ struct InternalOutput { result: Result, } +/// Helper trait to wrap contract execution entry points into a signle function +/// [`Invokable::run_guarded`]. +trait Invokable { + /// What is returned as a result of a successful invocation. + type Output; + + /// Single entry point to contract execution. + /// Downstream execution flow is branched by implementations of [`Invokable`] trait: + /// + /// - [`InstantiateInput::run`] runs contract instantiation, + /// - [`CallInput::run`] runs contract call. + /// + /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a + /// global reference. + fn run_guarded(&self, common: CommonInput) -> InternalOutput { + // Set up a global reference to the boolean flag used for the re-entrancy guard. + environmental!(executing_contract: bool); + + let gas_limit = common.gas_limit; + executing_contract::using_once(&mut false, || { + executing_contract::with(|f| { + // Fail if already entered contract execution + if *f { + return Err(()) + } + // We are entering contract execution + *f = true; + Ok(()) + }) + .expect("Returns `Ok` if called within `using_once`. It is syntactically obvious that this is the case; qed") + .map_or_else( + |_| InternalOutput { + gas_meter: GasMeter::new(gas_limit), + storage_deposit: Default::default(), + result: Err(ExecError { + error: >::ReentranceDenied.into(), + origin: ErrorOrigin::Caller, + }), + }, + // Enter contract call. + |_| self.run(common, GasMeter::new(gas_limit)), + ) + }) + } + + /// Method that does the actual call to a contract. It can be either a call to a deployed + /// contract or a instantiation of a new one. + /// + /// Called by dispatchables and public functions through the [`Invokable::run_guarded`]. + fn run( + &self, + common: CommonInput, + gas_meter: GasMeter, + ) -> InternalOutput; +} + +impl Invokable for CallInput { + type Output = ExecReturnValue; + + fn run( + &self, + common: CommonInput, + mut gas_meter: GasMeter, + ) -> InternalOutput { + let mut storage_meter = + match StorageMeter::new(&common.origin, common.storage_deposit_limit, common.value) { + Ok(meter) => meter, + Err(err) => + return InternalOutput { + result: Err(err.into()), + gas_meter, + storage_deposit: Default::default(), + }, + }; + let schedule = T::Schedule::get(); + let CallInput { dest, determinism } = self; + let CommonInput { origin, value, data, debug_message, .. } = common; + let result = ExecStack::>::run_call( + origin.clone(), + dest.clone(), + &mut gas_meter, + &mut storage_meter, + &schedule, + value, + data.clone(), + debug_message, + *determinism, + ); + InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(&origin), result } + } +} + +impl Invokable for InstantiateInput { + type Output = (AccountIdOf, ExecReturnValue); + + fn run( + &self, + mut common: CommonInput, + mut gas_meter: GasMeter, + ) -> InternalOutput { + let mut storage_deposit = Default::default(); + let try_exec = || { + let schedule = T::Schedule::get(); + let (extra_deposit, executable) = match &self.code { + Code::Upload(binary) => { + let executable = PrefabWasmModule::from_code( + binary.clone(), + &schedule, + common.origin.clone(), + Determinism::Deterministic, + TryInstantiate::Skip, + ) + .map_err(|(err, msg)| { + common + .debug_message + .as_mut() + .map(|buffer| buffer.try_extend(&mut msg.bytes())); + err + })?; + // The open deposit will be charged during execution when the + // uploaded module does not already exist. This deposit is not part of the + // storage meter because it is not transferred to the contract but + // reserved on the uploading account. + (executable.open_deposit(), executable) + }, + Code::Existing(hash) => ( + Default::default(), + PrefabWasmModule::from_storage(*hash, &schedule, &mut gas_meter)?, + ), + }; + let mut storage_meter = StorageMeter::new( + &common.origin, + common.storage_deposit_limit, + common.value.saturating_add(extra_deposit), + )?; + + let InstantiateInput { salt, .. } = self; + let CommonInput { origin, value, data, debug_message, .. } = common; + let result = ExecStack::>::run_instantiate( + origin.clone(), + executable, + &mut gas_meter, + &mut storage_meter, + &schedule, + value, + data.clone(), + &salt, + debug_message, + ); + storage_deposit = storage_meter + .into_deposit(&origin) + .saturating_add(&StorageDeposit::Charge(extra_deposit)); + result + }; + InternalOutput { result: try_exec(), gas_meter, storage_deposit } + } +} + impl Pallet { /// Perform a call to a specified contract. /// @@ -991,16 +1169,15 @@ impl Pallet { determinism: Determinism, ) -> ContractExecResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let output = Self::internal_call( + let common = CommonInput { origin, - dest, value, + data, gas_limit, storage_deposit_limit, - data, - debug_message.as_mut(), - determinism, - ); + debug_message: debug_message.as_mut(), + }; + let output = CallInput:: { dest, determinism }.run_guarded(common); ContractExecResult { result: output.result.map_err(|r| r.error), gas_consumed: output.gas_meter.gas_consumed(), @@ -1033,16 +1210,15 @@ impl Pallet { debug: bool, ) -> ContractInstantiateResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let output = Self::internal_instantiate( + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit, - code, - data, - salt, - debug_message.as_mut(), - ); + debug_message: debug_message.as_mut(), + }; + let output = InstantiateInput:: { code, salt }.run_guarded(common); ContractInstantiateResult { result: output .result @@ -1132,113 +1308,6 @@ impl Pallet { self::wasm::reinstrument(module, schedule).map(|_| ()) } - /// Internal function that does the actual call. - /// - /// Called by dispatchables and public functions. - fn internal_call( - origin: T::AccountId, - dest: T::AccountId, - value: BalanceOf, - gas_limit: Weight, - storage_deposit_limit: Option>, - data: Vec, - debug_message: Option<&mut DebugBufferVec>, - determinism: Determinism, - ) -> InternalCallOutput { - let mut gas_meter = GasMeter::new(gas_limit); - let mut storage_meter = match StorageMeter::new(&origin, storage_deposit_limit, value) { - Ok(meter) => meter, - Err(err) => - return InternalCallOutput { - result: Err(err.into()), - gas_meter, - storage_deposit: Default::default(), - }, - }; - let schedule = T::Schedule::get(); - let result = ExecStack::>::run_call( - origin.clone(), - dest, - &mut gas_meter, - &mut storage_meter, - &schedule, - value, - data, - debug_message, - determinism, - ); - InternalCallOutput { - result, - gas_meter, - storage_deposit: storage_meter.into_deposit(&origin), - } - } - - /// Internal function that does the actual instantiation. - /// - /// Called by dispatchables and public functions. - fn internal_instantiate( - origin: T::AccountId, - value: BalanceOf, - gas_limit: Weight, - storage_deposit_limit: Option>, - code: Code>, - data: Vec, - salt: Vec, - mut debug_message: Option<&mut DebugBufferVec>, - ) -> InternalInstantiateOutput { - let mut storage_deposit = Default::default(); - let mut gas_meter = GasMeter::new(gas_limit); - let try_exec = || { - let schedule = T::Schedule::get(); - let (extra_deposit, executable) = match code { - Code::Upload(binary) => { - let executable = PrefabWasmModule::from_code( - binary, - &schedule, - origin.clone(), - Determinism::Deterministic, - TryInstantiate::Skip, - ) - .map_err(|(err, msg)| { - debug_message.as_mut().map(|buffer| buffer.try_extend(&mut msg.bytes())); - err - })?; - // The open deposit will be charged during execution when the - // uploaded module does not already exist. This deposit is not part of the - // storage meter because it is not transferred to the contract but - // reserved on the uploading account. - (executable.open_deposit(), executable) - }, - Code::Existing(hash) => ( - Default::default(), - PrefabWasmModule::from_storage(hash, &schedule, &mut gas_meter)?, - ), - }; - let mut storage_meter = StorageMeter::new( - &origin, - storage_deposit_limit, - value.saturating_add(extra_deposit), - )?; - let result = ExecStack::>::run_instantiate( - origin.clone(), - executable, - &mut gas_meter, - &mut storage_meter, - &schedule, - value, - data, - &salt, - debug_message, - ); - storage_deposit = storage_meter - .into_deposit(&origin) - .saturating_add(&StorageDeposit::Charge(extra_deposit)); - result - }; - InternalInstantiateOutput { result: try_exec(), gas_meter, storage_deposit } - } - /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. fn deposit_event(topics: Vec, event: Event) { >::deposit_event_indexed( diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index bf1bd4f65442f..7144dcf2b5d8c 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -2805,12 +2805,9 @@ fn gas_estimation_call_runtime() { // Call something trivial with a huge gas limit so that we can observe the effects // of pre-charging. This should create a difference between consumed and required. - let call = RuntimeCall::Contracts(crate::Call::call { + let call = RuntimeCall::Balances(pallet_balances::Call::transfer { dest: addr_callee, - value: 0, - gas_limit: GAS_LIMIT / 3, - storage_deposit_limit: None, - data: vec![], + value: min_balance * 10, }); let result = Contracts::bare_call( ALICE, @@ -2844,6 +2841,71 @@ fn gas_estimation_call_runtime() { }); } +#[test] +fn gas_call_runtime_reentrancy_guarded() { + let (caller_code, _caller_hash) = compile_module::("call_runtime").unwrap(); + let (callee_code, _callee_hash) = compile_module::("dummy").unwrap(); + ExtBuilder::default().existential_deposit(50).build().execute_with(|| { + let min_balance = ::Currency::minimum_balance(); + let _ = Balances::deposit_creating(&ALICE, 1000 * min_balance); + let _ = Balances::deposit_creating(&CHARLIE, 1000 * min_balance); + + let addr_caller = Contracts::bare_instantiate( + ALICE, + min_balance * 100, + GAS_LIMIT, + None, + Code::Upload(caller_code), + vec![], + vec![0], + false, + ) + .result + .unwrap() + .account_id; + + let addr_callee = Contracts::bare_instantiate( + ALICE, + min_balance * 100, + GAS_LIMIT, + None, + Code::Upload(callee_code), + vec![], + vec![1], + false, + ) + .result + .unwrap() + .account_id; + + // Call pallet_contracts call() dispatchable + let call = RuntimeCall::Contracts(crate::Call::call { + dest: addr_callee, + value: 0, + gas_limit: GAS_LIMIT / 3, + storage_deposit_limit: None, + data: vec![], + }); + + // Call runtime to re-enter back to contracts engine by + // calling dummy contract + let result = Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + GAS_LIMIT, + None, + call.encode(), + false, + Determinism::Deterministic, + ) + .result + .unwrap(); + // Call to runtime should fail because of the re-entrancy guard + assert_return_code!(result, RuntimeReturnCode::CallRuntimeFailed); + }); +} + #[test] fn ecdsa_recover() { let (wasm, _code_hash) = compile_module::("ecdsa_recover").unwrap();