EIP-3607 added check to make sure a contract account cannot transfer funds as an EOA account (#9717)

fixes #9570

---------

Co-authored-by: Robert van Eerdewijk <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>

Author: 4 people

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

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::<Test>::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::<Test>::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 = <Test as pallet_revive::Config>::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 =
+				<Test as pallet_revive::Config>::AddressMapper::to_address(&owner).0.into();
 			let data = IERC20::balanceOfCall { account }.abi_encode();
 
 			let data = pallet_revive::Pallet::<Test>::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);

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<T: Config, E: Executable<T>>(address: &H160) -> bool
+where
+	BalanceOf<T>: Into<U256> + TryFrom<U256>,
+	MomentOf<T>: Into<U256>,
+	T::Hash: frame_support::traits::IsType<H256>,
+{
+	<AllPrecompiles<T>>::get::<Stack<'_, T, E>>(address.as_fixed_bytes()).is_some()
+}
+
 mod sealing {
 	use super::*;
 

substrate/frame/revive/src/lib.rs

@@ -1193,6 +1193,9 @@ where
 		storage_deposit_limit: DepositLimit<BalanceOf<T>>,
 		data: Vec<u8>,
 	) -> ContractResult<ExecReturnValue, BalanceOf<T>> {
+		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<InstantiateReturnValue, BalanceOf<T>> {
+		// 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<ReturnValue>(
+		origin: &OriginFor<T>,
+	) -> Result<(), ContractResult<ReturnValue, BalanceOf<T>>> {
+		use crate::exec::is_precompile;
+		let Ok(who) = ensure_signed(origin.clone()) else { return Ok(()) };
+		let address = <T::AddressMapper as AddressMapper<T>>::to_address(&who);
+
+		// EIP_1052: precompile can never be used as EOA.
+		if is_precompile::<T, ContractBlob<T>>(&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 <AccountInfo<T>>::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<T: Config> Pallet<T> {

substrate/frame/revive/src/storage.rs

@@ -152,7 +152,7 @@ impl<T: Config> From<ContractInfo<T>> for AccountType<T> {
 
 impl<T: Config> AccountInfo<T> {
 	/// Returns true if the account is a contract.
-	fn is_contract(address: &H160) -> bool {
+	pub fn is_contract(address: &H160) -> bool {
 		let Some(info) = <AccountInfoOf<T>>::get(address) else { return false };
 		matches!(info.account_type, AccountType::Contract(_))
 	}

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 _ = <Test as Config>::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::<Test>::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 = <Test as Config>::AddressMapper::to_account_id(&precompile_addr);
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+		let _ = <Test as Config>::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 = <Test as Config>::AddressMapper::to_account_id(&precompile_addr);
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+		let _ = <Test as Config>::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);
+	});
+}