diff --git a/prdoc/pr_10100.prdoc b/prdoc/pr_10100.prdoc new file mode 100644 index 0000000000000..c6cf1ef2320d5 --- /dev/null +++ b/prdoc/pr_10100.prdoc @@ -0,0 +1,12 @@ +title: 'pallet_revive: Only enforce EIP-3607 for dispatchables' +doc: +- audience: Runtime Dev + description: |- + The EIP states that contract addresses cannot be the origin of a transaction. However, we are enforcing this rule for all contract execution (i.e all public function on the pallet). This is a problem for code that uses `pallet_revive` and explicitly wants to allow this. + + This PR now only enforces this check for dispatchables so that all the `bare_*` functions are unaffected. + + As a drive-by a regrouped the functions on the `Pallet` so that the public functions are no longer interleaved with the private ones. This got mixed up when we resolved some merge conflicts. +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 407ff66eabebd..df7ae903f7571 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -1036,6 +1036,7 @@ pub mod pallet { #[pallet::compact] storage_deposit_limit: BalanceOf, data: Vec, ) -> DispatchResultWithPostInfo { + Self::ensure_non_contract_if_signed(&origin)?; let mut output = Self::bare_call( origin, dest, @@ -1072,6 +1073,7 @@ pub mod pallet { data: Vec, salt: Option<[u8; 32]>, ) -> DispatchResultWithPostInfo { + Self::ensure_non_contract_if_signed(&origin)?; let data_len = data.len() as u32; let mut output = Self::bare_instantiate( origin, @@ -1136,6 +1138,7 @@ pub mod pallet { data: Vec, salt: Option<[u8; 32]>, ) -> DispatchResultWithPostInfo { + Self::ensure_non_contract_if_signed(&origin)?; let code_len = code.len() as u32; let data_len = data.len() as u32; let mut output = Self::bare_instantiate( @@ -1197,6 +1200,7 @@ pub mod pallet { encoded_len: u32, ) -> DispatchResultWithPostInfo { let origin = Self::ensure_eth_origin(origin)?; + Self::ensure_non_contract_if_signed(&origin)?; let mut call = Call::::eth_instantiate_with_code { value, gas_limit, @@ -1267,6 +1271,7 @@ pub mod pallet { encoded_len: u32, ) -> DispatchResultWithPostInfo { let origin = Self::ensure_eth_origin(origin)?; + Self::ensure_non_contract_if_signed(&origin)?; let mut call = Call::::eth_call { dest, value, @@ -1342,6 +1347,7 @@ pub mod pallet { code: Vec, #[pallet::compact] storage_deposit_limit: BalanceOf, ) -> DispatchResult { + Self::ensure_non_contract_if_signed(&origin)?; Self::bare_upload_code(origin, code, storage_deposit_limit).map(|_| ()) } @@ -1403,6 +1409,7 @@ pub mod pallet { #[pallet::call_index(7)] #[pallet::weight(::WeightInfo::map_account())] pub fn map_account(origin: OriginFor) -> DispatchResult { + Self::ensure_non_contract_if_signed(&origin)?; let origin = ensure_signed(origin)?; T::AddressMapper::map(&origin) } @@ -1435,6 +1442,7 @@ pub mod pallet { origin: OriginFor, call: Box<::RuntimeCall>, ) -> DispatchResultWithPostInfo { + Self::ensure_non_contract_if_signed(&origin)?; let origin = ensure_signed(origin)?; let unmapped_account = T::AddressMapper::to_fallback_account_id(&T::AddressMapper::to_address(&origin)); @@ -1475,9 +1483,6 @@ impl Pallet { data: Vec, exec_config: ExecConfig, ) -> ContractResult> { - if let Err(contract_result) = Self::ensure_non_contract_if_signed(&origin) { - return contract_result; - } let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); @@ -1534,10 +1539,6 @@ impl Pallet { salt: Option<[u8; 32]>, exec_config: ExecConfig, ) -> ContractResult> { - // Enforce EIP-3607 for top-level signed origins: deny signed contract addresses. - if let Err(contract_result) = Self::ensure_non_contract_if_signed(&origin) { - return contract_result; - } let mut gas_meter = GasMeter::new(gas_limit); let mut storage_deposit = Default::default(); let try_instantiate = || { @@ -2084,6 +2085,39 @@ impl Pallet { .map_err(ContractAccessError::StorageWriteFailed) } + /// Pallet account, used to hold funds for contracts upload deposit. + pub fn account_id() -> T::AccountId { + use frame_support::PalletId; + use sp_runtime::traits::AccountIdConversion; + PalletId(*b"py/reviv").into_account_truncating() + } + + /// The address of the validator that produced the current block. + pub fn block_author() -> H160 { + use frame_support::traits::FindAuthor; + + let digest = >::digest(); + let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime()); + + T::FindAuthor::find_author(pre_runtime_digests) + .map(|account_id| T::AddressMapper::to_address(&account_id)) + .unwrap_or_default() + } + + /// Returns the code at `address`. + /// + /// This takes pre-compiles into account. + pub fn code(address: &H160) -> Vec { + use precompiles::{All, Precompiles}; + if let Some(code) = >::code(address.as_fixed_bytes()) { + return code.into() + } + AccountInfo::::load_contract(&address) + .and_then(|contract| >::get(contract.code_hash)) + .map(|code| code.into()) + .unwrap_or_default() + } + /// Uploads new code and returns the Vm binary contract blob and deposit amount collected. fn try_upload_pvm_code( origin: T::AccountId, @@ -2121,80 +2155,6 @@ impl Pallet { T::FeeInfo::weight_to_fee(&weight, Combinator::Max).into() } - /// Ensure the origin has no code deplyoyed if it is a signed origin. - fn ensure_non_contract_if_signed( - origin: &OriginFor, - ) -> Result<(), ContractResult>> { - use crate::exec::is_precompile; - let Ok(who) = ensure_signed(origin.clone()) else { return Ok(()) }; - let address = >::to_address(&who); - - // EIP_1052: precompile can never be used as EOA. - if is_precompile::>(&address) { - log::debug!( - target: crate::LOG_TARGET, - "EIP-3607: reject externally-signed tx from precompile account {:?}", - address - ); - return Err(ContractResult { - result: Err(DispatchError::BadOrigin), - gas_consumed: Weight::default(), - gas_required: Weight::default(), - storage_deposit: Default::default(), - }); - } - - // Deployed code exists when hash is neither zero (no account) nor EMPTY_CODE_HASH - // (account exists but no code). - if >::is_contract(&address) { - log::debug!( - target: crate::LOG_TARGET, - "EIP-3607: reject externally-signed tx from contract account {:?}", - address - ); - return Err(ContractResult { - result: Err(DispatchError::BadOrigin), - gas_consumed: Weight::default(), - gas_required: Weight::default(), - storage_deposit: Default::default(), - }); - } - Ok(()) - } - - /// Pallet account, used to hold funds for contracts upload deposit. - pub fn account_id() -> T::AccountId { - use frame_support::PalletId; - use sp_runtime::traits::AccountIdConversion; - PalletId(*b"py/reviv").into_account_truncating() - } - - /// The address of the validator that produced the current block. - pub fn block_author() -> H160 { - use frame_support::traits::FindAuthor; - - let digest = >::digest(); - let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime()); - - T::FindAuthor::find_author(pre_runtime_digests) - .map(|account_id| T::AddressMapper::to_address(&account_id)) - .unwrap_or_default() - } - - /// Returns the code at `address`. - /// - /// This takes pre-compiles into account. - pub fn code(address: &H160) -> Vec { - use precompiles::{All, Precompiles}; - if let Some(code) = >::code(address.as_fixed_bytes()) { - return code.into() - } - AccountInfo::::load_contract(&address) - .and_then(|contract| >::get(contract.code_hash)) - .map(|code| code.into()) - .unwrap_or_default() - } - /// Transfer a deposit from some account to another. /// /// `from` is usually the transaction origin and `to` a contract or @@ -2321,6 +2281,30 @@ impl Pallet { _ => Err(BadOrigin.into()), } } + + /// Ensure that the origin is neither a pre-compile nor a contract. + /// + /// This enforces EIP-3607. + fn ensure_non_contract_if_signed(origin: &OriginFor) -> DispatchResult { + let Some(address) = origin + .as_system_ref() + .and_then(|o| o.as_signed()) + .map(>::to_address) + else { + return Ok(()) + }; + if exec::is_precompile::>(&address) || + >::is_contract(&address) + { + log::debug!( + target: crate::LOG_TARGET, + "EIP-3607: reject tx as pre-compile or account exist at {address:?}", + ); + Err(DispatchError::BadOrigin) + } else { + Ok(()) + } + } } /// The address used to call the runtime's pallets dispatchables diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index d32ee0033299a..76e8aced8f4b3 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -4942,74 +4942,70 @@ fn storage_deposit_from_hold_works() { }); } -/// EIP-3607 -/// Test that a top-level signed transaction that uses a contract address as the signer is rejected. #[test] -fn reject_signed_tx_from_contract_address() { +fn eip3607_reject_tx_from_contract_or_precompile() { let (binary, _code_hash) = compile_module("dummy").unwrap(); ExtBuilder::default().existential_deposit(200).build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let Contract { addr: contract_addr, account_id: contract_account_id, .. } = + // the origins from which we try to call a dispatchable + let Contract { addr: contract_addr, .. } = builder::bare_instantiate(Code::Upload(binary)).build_and_unwrap_contract(); + assert!(>::is_contract(&contract_addr)); + let blake2_addr = H160::from_low_u64_be(9); + let system_addr = H160::from_low_u64_be(0x900); + let addresses = [contract_addr, blake2_addr, system_addr]; - assert!(AccountInfoOf::::contains_key(&contract_addr)); + // used to test `dispatch_as_fallback_account` + let call = Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_all { + dest: EVE, + keep_alive: false, + })); - let call_result = builder::bare_call(BOB_ADDR) - .native_value(1) - .origin(RuntimeOrigin::signed(contract_account_id.clone())) - .build(); - assert_err!(call_result.result, DispatchError::BadOrigin); + for address in addresses.iter() { + let origin = ::AddressMapper::to_fallback_account_id(address); - let instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) - .origin(RuntimeOrigin::signed(contract_account_id)) - .build(); - assert_err!(instantiate_result.result, DispatchError::BadOrigin); - }); -} + let result = + builder::call(BOB_ADDR).origin(RuntimeOrigin::signed(origin.clone())).build(); + assert_err!(result, DispatchError::BadOrigin); -#[test] -fn reject_signed_tx_from_primitive_precompile_address() { - ExtBuilder::default().existential_deposit(200).build().execute_with(|| { - // blake2f precompile address - let precompile_addr = H160::from_low_u64_be(9); - let fake_account = ::AddressMapper::to_account_id(&precompile_addr); - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let _ = ::Currency::set_balance(&fake_account, 1_000_000); + let result = builder::eth_call(BOB_ADDR) + .origin(RuntimeOrigin::signed(origin.clone())) + .build(); + assert_err!(result, DispatchError::BadOrigin); - let call_result = builder::bare_call(BOB_ADDR) - .native_value(1) - .origin(RuntimeOrigin::signed(fake_account.clone())) - .build(); - assert_err!(call_result.result, DispatchError::BadOrigin); + let result = builder::instantiate(Default::default()) + .origin(RuntimeOrigin::signed(origin.clone())) + .build(); + assert_err!(result, DispatchError::BadOrigin); - let instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) - .origin(RuntimeOrigin::signed(fake_account)) - .build(); - assert_err!(instantiate_result.result, DispatchError::BadOrigin); - }); -} + let result = builder::eth_instantiate_with_code(Default::default()) + .origin(RuntimeOrigin::signed(origin.clone())) + .build(); + assert_err!(result, DispatchError::BadOrigin); -#[test] -fn reject_signed_tx_from_builtin_precompile_address() { - ExtBuilder::default().existential_deposit(200).build().execute_with(|| { - // system precompile address - let precompile_addr = H160::from_low_u64_be(0x900); - let fake_account = ::AddressMapper::to_account_id(&precompile_addr); - let _ = ::Currency::set_balance(&ALICE, 1_000_000); - let _ = ::Currency::set_balance(&fake_account, 1_000_000); + let result = builder::instantiate_with_code(Default::default()) + .origin(RuntimeOrigin::signed(origin.clone())) + .build(); + assert_err!(result, DispatchError::BadOrigin); - let call_result = builder::bare_call(BOB_ADDR) - .native_value(1) - .origin(RuntimeOrigin::signed(fake_account.clone())) - .build(); - assert_err!(call_result.result, DispatchError::BadOrigin); + let result = >::upload_code( + RuntimeOrigin::signed(origin.clone()), + Default::default(), + >::MAX, + ); + assert_err!(result, DispatchError::BadOrigin); - let instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) - .origin(RuntimeOrigin::signed(fake_account)) - .build(); - assert_err!(instantiate_result.result, DispatchError::BadOrigin); + let result = >::map_account(RuntimeOrigin::signed(origin.clone())); + assert_err!(result, DispatchError::BadOrigin); + + let result = >::dispatch_as_fallback_account( + RuntimeOrigin::signed(origin.clone()), + call.clone(), + ); + assert_err!(result, DispatchError::BadOrigin); + } }); }