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

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

@@ -2027,18 +2027,7 @@ where
 	}
 
 	fn code_hash(&self, address: &H160) -> H256 {
-		if let Some(code) = <AllPrecompiles<T>>::code(address.as_fixed_bytes()) {
-			return sp_io::hashing::keccak_256(code).into()
-		}
-
-		<AccountInfo<T>>::load_contract(&address)
-			.map(|contract| contract.code_hash)
-			.unwrap_or_else(|| {
-				if System::<T>::account_exists(&T::AddressMapper::to_account_id(address)) {
-					return EMPTY_CODE_HASH;
-				}
-				H256::zero()
-			})
+		code_hash::<T>(address)
 	}
 
 	fn code_size(&self, address: &H160) -> u64 {
@@ -2188,6 +2177,32 @@ where
 	}
 }
 
+/// Returns the code hash of the contract at `address`.
+pub fn code_hash<T: Config>(address: &H160) -> H256 {
+	// EIP_1052: The EXTCODEHASH of a precompiled contract is either c5d246... or 0.
+	// But EXTCODEHASH for a *custom* precompile should return the hash of its code.
+	// EVM has no concept of a custom precompile.
+	if let Some(code) = <AllPrecompiles<T>>::code(address.as_fixed_bytes()) {
+		return sp_io::hashing::keccak_256(code).into();
+	}
+
+	// check stored contract info or account existence
+	<AccountInfo<T>>::load_contract(address)
+		.map(|contract| contract.code_hash)
+		.unwrap_or_else(|| {
+			if System::<T>::account_exists(&T::AddressMapper::to_account_id(address)) {
+				EMPTY_CODE_HASH
+			} else {
+				H256::zero()
+			}
+		})
+}
+
+/// Returns true if the address has a precompile contract, else false.
+pub fn is_precompile<T: Config>(address: &H160) -> bool {
+	<AllPrecompiles<T>>::code(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();
@@ -1715,6 +1722,50 @@ 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::{code_hash, is_precompile, EMPTY_CODE_HASH};
+		if let Ok(who) = ensure_signed(origin.clone()) {
+			let address = <T::AddressMapper as AddressMapper<T>>::to_address(&who);
+
+			// EIP_1052: precompile can never be used as EOA.
+			if is_precompile::<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(),
+				});
+			}
+
+			let code_hash = code_hash::<T>(&address);
+
+			// Deployed code exists when hash is neither zero (no account) nor EMPTY_CODE_HASH
+			// (account exists but no code).
+			if !(code_hash == H256::zero() || code_hash == EMPTY_CODE_HASH) {
+				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/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);
+	});
+}