diff --git a/prdoc/pr_9717.prdoc b/prdoc/pr_9717.prdoc new file mode 100644 index 0000000000000..0296bbd167371 --- /dev/null +++ b/prdoc/pr_9717.prdoc @@ -0,0 +1,10 @@ +title: EIP-3607 added check to make sure a contract account cannot transfer funds + as an EOA account +doc: +- audience: Runtime Dev + description: fixes https://github.com/paritytech/polkadot-sdk/issues/9570 +crates: +- name: pallet-revive + bump: patch +- name: pallet-assets + bump: patch diff --git a/substrate/frame/assets/src/precompiles.rs b/substrate/frame/assets/src/precompiles.rs index 107145a524d81..3a7915dfd241d 100644 --- a/substrate/frame/assets/src/precompiles.rs +++ b/substrate/frame/assets/src/precompiles.rs @@ -352,8 +352,8 @@ mod test { hex::const_decode_to_array(b"0000000000000000000000000000000001200000").unwrap(), ); - let from = 1; - let to = 2; + let from = 123456789; + let to = 987654321; Balances::make_free_balance_be(&from, 100); Balances::make_free_balance_be(&to, 100); @@ -367,7 +367,7 @@ mod test { IERC20::transferCall { to: to_addr.0.into(), value: U256::from(10) }.abi_encode(); pallet_revive::Pallet::::bare_call( - RuntimeOrigin::signed(1), + RuntimeOrigin::signed(from), H160::from(asset_addr), 0u32.into(), Weight::MAX, @@ -396,14 +396,16 @@ mod test { let asset_addr = hex::const_decode_to_array(b"0000000000000000000000000000000001200000").unwrap(); - Balances::make_free_balance_be(&1, 100); - assert_ok!(Assets::force_create(RuntimeOrigin::root(), asset_id, 1, true, 1)); - assert_ok!(Assets::mint(RuntimeOrigin::signed(1), asset_id, 1, 1000)); + let owner = 123456789; + + Balances::make_free_balance_be(&owner, 100); + assert_ok!(Assets::force_create(RuntimeOrigin::root(), asset_id, owner, true, 1)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(owner), asset_id, owner, 1000)); let data = IERC20::totalSupplyCall {}.abi_encode(); let data = pallet_revive::Pallet::::bare_call( - RuntimeOrigin::signed(1), + RuntimeOrigin::signed(owner), H160::from(asset_addr), 0u32.into(), Weight::MAX, @@ -426,15 +428,17 @@ mod test { let asset_addr = hex::const_decode_to_array(b"0000000000000000000000000000000001200000").unwrap(); - Balances::make_free_balance_be(&1, 100); - assert_ok!(Assets::force_create(RuntimeOrigin::root(), asset_id, 1, true, 1)); - assert_ok!(Assets::mint(RuntimeOrigin::signed(1), asset_id, 1, 1000)); + let owner = 123456789; - let account = ::AddressMapper::to_address(&1).0.into(); + assert_ok!(Assets::force_create(RuntimeOrigin::root(), asset_id, owner, true, 1)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(owner), asset_id, owner, 1000)); + + let account = + ::AddressMapper::to_address(&owner).0.into(); let data = IERC20::balanceOfCall { account }.abi_encode(); let data = pallet_revive::Pallet::::bare_call( - RuntimeOrigin::signed(1), + RuntimeOrigin::signed(owner), H160::from(asset_addr), 0u32.into(), Weight::MAX, @@ -460,9 +464,9 @@ mod test { hex::const_decode_to_array(b"0000000000000000000000000000000001200000").unwrap(), ); - let owner = 1; - let spender = 2; - let other = 3; + let owner = 123456789; + let spender = 987654321; + let other = 1122334455; Balances::make_free_balance_be(&owner, 100); Balances::make_free_balance_be(&spender, 100); diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 5fa338230a78e..6547a15386439 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -2189,6 +2189,16 @@ where } } +/// Returns true if the address has a precompile contract, else false. +pub fn is_precompile>(address: &H160) -> bool +where + BalanceOf: Into + TryFrom, + MomentOf: Into, + T::Hash: frame_support::traits::IsType, +{ + >::get::>(address.as_fixed_bytes()).is_some() +} + mod sealing { use super::*; diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 377f9370f8ab5..a759d41b59f99 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -1193,6 +1193,9 @@ where storage_deposit_limit: DepositLimit>, data: Vec, ) -> 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(); @@ -1250,6 +1253,10 @@ where salt: Option<[u8; 32]>, bump_nonce: BumpNonce, ) -> 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 unchecked_deposit_limit = storage_deposit_limit.is_unchecked(); @@ -1727,6 +1734,47 @@ where .saturating_mul(T::NativeToEthRatio::get().into()) .saturating_add(dust.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(()) + } } impl Pallet { diff --git a/substrate/frame/revive/src/storage.rs b/substrate/frame/revive/src/storage.rs index 42e4397574c15..cdfbd976797b8 100644 --- a/substrate/frame/revive/src/storage.rs +++ b/substrate/frame/revive/src/storage.rs @@ -152,7 +152,7 @@ impl From> for AccountType { impl AccountInfo { /// Returns true if the account is a contract. - fn is_contract(address: &H160) -> bool { + pub fn is_contract(address: &H160) -> bool { let Some(info) = >::get(address) else { return false }; matches!(info.account_type, AccountType::Contract(_)) } diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index d4d42b20d03f8..26eef5f7e58aa 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -4992,3 +4992,74 @@ fn return_data_limit_is_enforced() { } }); } + +/// 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() { + 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, .. } = + builder::bare_instantiate(Code::Upload(binary)).build_and_unwrap_contract(); + + assert!(AccountInfoOf::::contains_key(&contract_addr)); + + 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); + + let instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) + .origin(RuntimeOrigin::signed(contract_account_id)) + .build(); + assert_err!(instantiate_result.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 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 instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) + .origin(RuntimeOrigin::signed(fake_account)) + .build(); + assert_err!(instantiate_result.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 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 instantiate_result = builder::bare_instantiate(Code::Upload(Vec::new())) + .origin(RuntimeOrigin::signed(fake_account)) + .build(); + assert_err!(instantiate_result.result, DispatchError::BadOrigin); + }); +}