Fix EOA check for precompiles

frame/evm/precompile/dispatch/src/mock.rs

@@ -217,6 +217,10 @@ impl PrecompileHandle for MockHandle {
 		&self.context
 	}
 
+	fn origin(&self) -> H160 {
+		unimplemented!()
+	}
+
 	fn is_static(&self) -> bool {
 		unimplemented!()
 	}

frame/evm/test-vector-support/src/lib.rs

@@ -111,6 +111,10 @@ impl PrecompileHandle for MockHandle {
 		&self.context
 	}
 
+	fn origin(&self) -> H160 {
+		unimplemented!()
+	}
+
 	fn is_static(&self) -> bool {
 		self.is_static
 	}

precompiles/src/evm/handle.rs

@@ -177,6 +177,10 @@ mod tests {
 			unimplemented!()
 		}
 
+		fn origin(&self) -> sp_core::H160 {
+			unimplemented!()
+		}
+
 		fn is_static(&self) -> bool {
 			true
 		}

precompiles/src/precompile_set.rs

@@ -21,7 +21,6 @@
 //! default and must be disabled explicely through type annotations.
 
 use crate::{
-	evm::handle::PrecompileHandleExt,
 	solidity::{codec::String, revert::revert},
 	EvmResult,
 };
@@ -36,6 +35,10 @@ use impl_trait_for_tuples::impl_for_tuples;
 use pallet_evm::AddressMapping;
 use sp_core::{H160, H256};
 
+// This is the simplest bytecode to revert without returning any data.
+// (PUSH1 0x00 PUSH1 0x00 REVERT)
+pub const REVERT_BYTECODE: [u8; 5] = [0x60, 0x00, 0x60, 0x00, 0xfd];
+
 /// Trait representing checks that can be made on a precompile call.
 /// Types implementing this trait are made to be chained in a tuple.
 ///
@@ -304,13 +307,11 @@ impl<T: SelectorFilter> PrecompileChecks for CallableByPrecompile<T> {
 #[derive(PartialEq)]
 #[cfg_attr(feature = "std", derive(Debug))]
 pub enum AddressType {
-	/// The code stored at the address is less than 5 bytes, but not well known.
-	Unknown,
 	/// No code is stored at the address, therefore is EOA.
 	EOA,
 	/// The 5-byte magic constant for a precompile is stored at the address.
 	Precompile,
-	/// The code is greater than 5-bytes, potentially a Smart Contract.
+	/// Every address that is not a EOA or a Precompile it potentially a Smart Contract.
 	Contract,
 }
 
@@ -319,39 +320,18 @@ pub fn get_address_type<R: pallet_evm::Config>(
 	handle: &mut impl PrecompileHandle,
 	address: H160,
 ) -> Result<AddressType, ExitError> {
-	// AccountCodesMetadata:
-	// Blake2128(16) + H160(20) + CodeMetadata(40)
-	handle.record_db_read::<R>(76)?;
-	let code_len = pallet_evm::Pallet::<R>::account_code_metadata(address).size;
-
-	// 0 => either EOA or precompile without dummy code
-	if code_len == 0 {
+	// It is an Externally Owned Account (EOA)
+	// - When the transaction origin is equal to the address
+	if handle.origin() == address {
 		return Ok(AddressType::EOA);
 	}
 
-	// dummy code is 5 bytes long, so any other len means it is a contract.
-	if code_len != 5 {
-		return Ok(AddressType::Contract);
-	}
-
-	// check code matches dummy code
-	handle.record_db_read::<R>(code_len as usize)?;
-	let code = pallet_evm::AccountCodes::<R>::get(address);
-	if code == [0x60, 0x00, 0x60, 0x00, 0xfd] {
+	// Check if address is a precompile
+	if let Ok(true) = is_precompile_or_fail::<R>(address, handle.remaining_gas()) {
 		return Ok(AddressType::Precompile);
 	}
 
-	Ok(AddressType::Unknown)
-}
-
-fn is_address_eoa_or_precompile<R: pallet_evm::Config>(
-	handle: &mut impl PrecompileHandle,
-	address: H160,
-) -> Result<bool, ExitError> {
-	match get_address_type::<R>(handle, address)? {
-		AddressType::EOA | AddressType::Precompile => Ok(true),
-		_ => Ok(false),
-	}
+	Ok(AddressType::Contract)
 }
 
 /// Common checks for precompile and precompile sets.
@@ -375,19 +355,23 @@ fn common_checks<R: pallet_evm::Config, C: PrecompileChecks>(
 		u32::from_be_bytes(buffer)
 	});
 
-	// Is this selector callable from a smart contract?
-	let callable_by_smart_contract =
-		C::callable_by_smart_contract(caller, selector).unwrap_or(false);
-	if !callable_by_smart_contract && !is_address_eoa_or_precompile::<R>(handle, caller)? {
-		return Err(revert("Function not callable by smart contracts"));
-	}
+	let caller_address_type = get_address_type::<R>(handle, caller)?;
 
 	// Is this selector callable from a precompile?
 	let callable_by_precompile = C::callable_by_precompile(caller, selector).unwrap_or(false);
-	if !callable_by_precompile && is_precompile_or_fail::<R>(caller, handle.remaining_gas())? {
+	let is_precompile = caller_address_type == AddressType::Precompile;
+	if !callable_by_precompile && is_precompile {
 		return Err(revert("Function not callable by precompiles"));
 	}
 
+	// Is this selector callable from a smart contract?
+	let callable_by_smart_contract =
+		C::callable_by_smart_contract(caller, selector).unwrap_or(false);
+	let is_smart_contract = caller_address_type == AddressType::Contract;
+	if !callable_by_smart_contract && is_smart_contract {
+		return Err(revert("Function not callable by smart contracts"));
+	}
+
 	Ok(())
 }
 
@@ -463,6 +447,10 @@ impl<'a, H: PrecompileHandle> PrecompileHandle for RestrictiveHandle<'a, H> {
 		self.handle.context()
 	}
 
+	fn origin(&self) -> H160 {
+		self.handle.origin()
+	}
+
 	fn is_static(&self) -> bool {
 		self.handle.is_static()
 	}

precompiles/src/testing/execution.rs

@@ -72,6 +72,11 @@ impl<'p, P: PrecompileSet> PrecompilesTester<'p, P> {
 		}
 	}
 
+	pub fn with_origin(mut self, origin: impl Into<H160>) -> Self {
+		self.handle.origin = origin.into();
+		self
+	}
+
 	pub fn with_value(mut self, value: impl Into<U256>) -> Self {
 		self.handle.context.apparent_value = value.into();
 		self

precompiles/src/testing/handle.rs

@@ -77,6 +77,7 @@ pub type SubcallHandle = Box<dyn SubcallTrait>;
 
 /// Mock handle to write tests for precompiles.
 pub struct MockHandle {
+	pub origin: H160,
 	pub gas_limit: u64,
 	pub gas_used: u64,
 	pub logs: Vec<PrettyLog>,
@@ -90,6 +91,7 @@ pub struct MockHandle {
 impl MockHandle {
 	pub fn new(code_address: H160, context: Context) -> Self {
 		Self {
+			origin: context.caller,
 			gas_limit: u64::MAX,
 			gas_used: 0,
 			logs: vec![],
@@ -193,6 +195,10 @@ impl PrecompileHandle for MockHandle {
 		&self.context
 	}
 
+	fn origin(&self) -> H160 {
+		self.origin
+	}
+
 	/// Is the precompile call is done statically.
 	fn is_static(&self) -> bool {
 		self.is_static

precompiles/tests-external/lib.rs

@@ -146,7 +146,21 @@ impl MockPrecompile {
 	}
 }
 
-struct MockPrecompileHandle;
+#[derive(Default)]
+struct MockPrecompileHandle {
+	origin: H160,
+	remaining_gas: u64,
+}
+impl MockPrecompileHandle {
+	fn with_origin(mut self, origin: H160) -> Self {
+		self.origin = origin;
+		self
+	}
+	fn with_remaining_gas(mut self, remaining_gas: u64) -> Self {
+		self.remaining_gas = remaining_gas;
+		self
+	}
+}
 impl PrecompileHandle for MockPrecompileHandle {
 	fn call(
 		&mut self,
@@ -176,7 +190,7 @@ impl PrecompileHandle for MockPrecompileHandle {
 	fn refund_external_cost(&mut self, _ref_time: Option<u64>, _proof_size: Option<u64>) {}
 
 	fn remaining_gas(&self) -> u64 {
-		unimplemented!()
+		self.remaining_gas
 	}
 
 	fn log(&mut self, _: H160, _: Vec<H256>, _: Vec<u8>) -> Result<(), evm::ExitError> {
@@ -195,6 +209,10 @@ impl PrecompileHandle for MockPrecompileHandle {
 		unimplemented!()
 	}
 
+	fn origin(&self) -> H160 {
+		self.origin
+	}
+
 	fn is_static(&self) -> bool {
 		true
 	}
@@ -325,6 +343,25 @@ fn default_checks_revert_when_called_by_contract() {
 	})
 }
 
+#[test]
+fn default_checks_revert_when_called_by_contract_during_construction() {
+	ExtBuilder::default().build().execute_with(|| {
+		// Condition: tx.origin != msg.sender
+		precompiles()
+			.prepare_test(Zero, H160::from_low_u64_be(1), PCall::success {})
+			.with_origin(Alice)
+			.with_subcall_handle(|Subcall { .. }| panic!("there should be no subcall"))
+			.execute_reverts(|r| r == b"Function not callable by smart contracts");
+
+		// Condition: tx.origin == msg.sender
+		precompiles()
+			.prepare_test(Alice, H160::from_low_u64_be(1), PCall::success {})
+			.with_origin(Alice)
+			.with_subcall_handle(|Subcall { .. }| panic!("there should be no subcall"))
+			.execute_returns(())
+	})
+}
+
 #[test]
 fn default_checks_revert_when_doing_subcall() {
 	ExtBuilder::default().build().execute_with(|| {
@@ -385,58 +422,45 @@ fn subcalls_works_when_allowed() {
 #[test]
 fn get_address_type_works_for_eoa() {
 	ExtBuilder::default().build().execute_with(|| {
-		let addr = H160::repeat_byte(0x1d);
+		let externally_owned_account: H160 = Alice.into();
+		let mut handle = MockPrecompileHandle::default().with_origin(externally_owned_account);
+
 		assert_eq!(
 			AddressType::EOA,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
+			get_address_type::<Runtime>(&mut handle, externally_owned_account).expect("OOG")
 		);
 	})
 }
 
 #[test]
 fn get_address_type_works_for_precompile() {
 	ExtBuilder::default().build().execute_with(|| {
-		let addr = H160::repeat_byte(0x1d);
-		pallet_evm::AccountCodes::<Runtime>::insert(addr, vec![0x60, 0x00, 0x60, 0x00, 0xfd]);
-		assert_eq!(
-			AddressType::Precompile,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
-		);
+		let precompiles: Vec<H160> = Precompiles::<Runtime>::used_addresses_h160().collect();
+		// We expect 4 precompiles
+		assert_eq!(precompiles.len(), 4);
+
+		let mut handle = MockPrecompileHandle::default();
+		precompiles.iter().cloned().for_each(|precompile| {
+			assert_eq!(
+				AddressType::Precompile,
+				get_address_type::<Runtime>(&mut handle, precompile).expect("OOG")
+			);
+		});
 	})
 }
 
 #[test]
 fn get_address_type_works_for_smart_contract() {
 	ExtBuilder::default().build().execute_with(|| {
-		let addr = H160::repeat_byte(0x1d);
-
-		// length > 5
-		pallet_evm::AccountCodes::<Runtime>::insert(
-			addr,
-			vec![0x60, 0x00, 0x60, 0x00, 0xfd, 0xff, 0xff],
-		);
-		assert_eq!(
-			AddressType::Contract,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
-		);
+		let externally_owned_account: H160 = Alice.into();
+		let other_address = H160::repeat_byte(0x1d);
+		let mut handle = MockPrecompileHandle::default()
+			.with_origin(externally_owned_account)
+			.with_remaining_gas(100);
 
-		// length < 5
-		pallet_evm::AccountCodes::<Runtime>::insert(addr, vec![0x60, 0x00, 0x60]);
 		assert_eq!(
 			AddressType::Contract,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
-		);
-	})
-}
-
-#[test]
-fn get_address_type_works_for_unknown() {
-	ExtBuilder::default().build().execute_with(|| {
-		let addr = H160::repeat_byte(0x1d);
-		pallet_evm::AccountCodes::<Runtime>::insert(addr, vec![0x11, 0x00, 0x60, 0x00, 0xfd]);
-		assert_eq!(
-			AddressType::Unknown,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
+			get_address_type::<Runtime>(&mut handle, other_address).expect("OOG")
 		);
 	})
 }