Fix EOA check for precompiles (#249)

Author: RomarQ

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

@@ -216,11 +216,19 @@ impl PrecompileHandle for MockHandle {
 		&self.context
 	}
 
+	fn origin(&self) -> H160 {
+		unimplemented!()
+	}
+
 	fn is_static(&self) -> bool {
 		unimplemented!()
 	}
 
 	fn gas_limit(&self) -> Option<u64> {
 		None
 	}
+
+	fn is_contract_being_constructed(&self, _address: H160) -> bool {
+		unimplemented!()
+	}
 }

frame/evm/src/lib.rs

@@ -696,7 +696,6 @@ pub mod pallet {
 					.saturating_add(T::DbWeight::get().reads(1))
 					.saturating_add(account_basic_weight)
 					.saturating_add(min_gas_weight);
-
 			}
 
 			total_weight

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

@@ -111,13 +111,21 @@ impl PrecompileHandle for MockHandle {
 		&self.context
 	}
 
+	fn origin(&self) -> H160 {
+		unimplemented!()
+	}
+
 	fn is_static(&self) -> bool {
 		self.is_static
 	}
 
 	fn gas_limit(&self) -> Option<u64> {
 		self.gas_limit
 	}
+
+	fn is_contract_being_constructed(&self, _address: H160) -> bool {
+		unimplemented!()
+	}
 }
 
 /// Tests a precompile against the ethereum consensus tests defined in the given file at filepath.

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
 		}
@@ -195,6 +199,10 @@ mod tests {
 		}
 
 		fn refund_external_cost(&mut self, _ref_time: Option<u64>, _proof_size: Option<u64>) {}
+
+		fn is_contract_being_constructed(&self, _address: sp_core::H160) -> bool {
+			unimplemented!()
+		}
 	}
 
 	#[test]

precompiles/src/precompile_set.rs

@@ -29,7 +29,7 @@ use alloc::{collections::btree_map::BTreeMap, vec, vec::Vec};
 use core::{cell::RefCell, marker::PhantomData, ops::RangeInclusive};
 use fp_evm::{
 	ExitError, IsPrecompileResult, Precompile, PrecompileFailure, PrecompileHandle,
-	PrecompileResult, PrecompileSet,
+	PrecompileResult, PrecompileSet, ACCOUNT_CODES_METADATA_PROOF_SIZE,
 };
 use frame_support::pallet_prelude::Get;
 use impl_trait_for_tuples::impl_for_tuples;
@@ -304,13 +304,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 is potentially a Smart Contract.
 	Contract,
 }
 
@@ -319,39 +317,27 @@ pub fn get_address_type<R: pallet_evm::Config>(
 	handle: &mut impl PrecompileHandle,
 	address: H160,
 ) -> Result<AddressType, ExitError> {
+	// Check if address is a precompile
+	if let Ok(true) = is_precompile_or_fail::<R>(address, handle.remaining_gas()) {
+		return Ok(AddressType::Precompile);
+	}
+
+	// Contracts under-construction don't have code yet
+	if handle.is_contract_being_constructed(address) {
+		return Ok(AddressType::Contract);
+	}
+
 	// AccountCodesMetadata:
 	// Blake2128(16) + H160(20) + CodeMetadata(40)
-	handle.record_db_read::<R>(76)?;
+	handle.record_db_read::<R>(ACCOUNT_CODES_METADATA_PROOF_SIZE as usize)?;
 	let code_len = pallet_evm::Pallet::<R>::account_code_metadata(address).size;
 
-	// 0 => either EOA or precompile without dummy code
+	// Having no code at this point means that the address is an EOA
 	if code_len == 0 {
 		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] {
-		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,17 +361,27 @@ 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"));
-	}
-
-	// 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())? {
-		return Err(revert("Function not callable by precompiles"));
+	let caller_address_type = get_address_type::<R>(handle, caller)?;
+	match caller_address_type {
+		AddressType::Precompile => {
+			// Is this selector callable from a precompile?
+			let callable_by_precompile =
+				C::callable_by_precompile(caller, selector).unwrap_or(false);
+			if !callable_by_precompile {
+				return Err(revert("Function not callable by precompiles"));
+			}
+		}
+		AddressType::Contract => {
+			// 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 {
+				return Err(revert("Function not callable by smart contracts"));
+			}
+		}
+		AddressType::EOA => {
+			// No check required for EOA
+		}
 	}
 
 	Ok(())
@@ -463,6 +459,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()
 	}
@@ -484,6 +484,10 @@ impl<'a, H: PrecompileHandle> PrecompileHandle for RestrictiveHandle<'a, H> {
 	fn refund_external_cost(&mut self, ref_time: Option<u64>, proof_size: Option<u64>) {
 		self.handle.refund_external_cost(ref_time, proof_size)
 	}
+
+	fn is_contract_being_constructed(&self, address: H160) -> bool {
+		self.handle.is_contract_being_constructed(address)
+	}
 }
 
 /// Allows to know if a precompile is active or not.

precompiles/src/testing/handle.rs

@@ -22,6 +22,8 @@ use evm::{ExitRevert, ExitSucceed};
 use fp_evm::{Context, ExitError, ExitReason, Log, PrecompileHandle, Transfer};
 use sp_core::{H160, H256};
 
+use super::Alice;
+
 #[derive(Debug, Clone)]
 pub struct Subcall {
 	pub address: H160,
@@ -213,4 +215,12 @@ impl PrecompileHandle for MockHandle {
 	}
 
 	fn refund_external_cost(&mut self, _ref_time: Option<u64>, _proof_size: Option<u64>) {}
+
+	fn origin(&self) -> H160 {
+		Alice.into()
+	}
+
+	fn is_contract_being_constructed(&self, _address: H160) -> bool {
+		false
+	}
 }

precompiles/tests-external/lib.rs

@@ -34,7 +34,7 @@ use sp_runtime::{
 };
 // Frontier
 use fp_evm::{ExitReason, ExitRevert, PrecompileFailure, PrecompileHandle};
-use pallet_evm::{EnsureAddressNever, EnsureAddressRoot};
+use pallet_evm::{CodeMetadata, EnsureAddressNever, EnsureAddressRoot};
 use precompile_utils::{
 	precompile_set::*,
 	solidity::{codec::Writer, revert::revert},
@@ -147,7 +147,16 @@ impl MockPrecompile {
 	}
 }
 
-struct MockPrecompileHandle;
+#[derive(Default)]
+struct MockPrecompileHandle {
+	contracts_being_constructed: Vec<H160>,
+}
+impl MockPrecompileHandle {
+	fn with_contracts_being_constructed(mut self, contracts_being_constructed: Vec<H160>) -> Self {
+		self.contracts_being_constructed = contracts_being_constructed;
+		self
+	}
+}
 impl PrecompileHandle for MockPrecompileHandle {
 	fn call(
 		&mut self,
@@ -177,7 +186,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!()
+		0
 	}
 
 	fn log(&mut self, _: H160, _: Vec<H256>, _: Vec<u8>) -> Result<(), evm::ExitError> {
@@ -196,13 +205,21 @@ impl PrecompileHandle for MockPrecompileHandle {
 		unimplemented!()
 	}
 
+	fn origin(&self) -> H160 {
+		Alice.into()
+	}
+
 	fn is_static(&self) -> bool {
 		true
 	}
 
 	fn gas_limit(&self) -> Option<u64> {
 		unimplemented!()
 	}
+
+	fn is_contract_being_constructed(&self, address: H160) -> bool {
+		self.contracts_being_constructed.contains(&address)
+	}
 }
 
 pub type Precompiles<R> = PrecompileSetBuilder<
@@ -384,58 +401,63 @@ 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();
+
 		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 address = H160::repeat_byte(0x1d);
+		pallet_evm::AccountCodesMetadata::<Runtime>::insert(
+			address,
+			CodeMetadata {
+				hash: Default::default(),
+				size: 1,
+			},
 		);
 
-		// length < 5
-		pallet_evm::AccountCodes::<Runtime>::insert(addr, vec![0x60, 0x00, 0x60]);
+		let mut handle = MockPrecompileHandle::default();
 		assert_eq!(
 			AddressType::Contract,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
+			get_address_type::<Runtime>(&mut handle, address).expect("OOG")
 		);
 	})
 }
 
 #[test]
-fn get_address_type_works_for_unknown() {
+fn get_address_type_works_for_smart_contract_being_constructed() {
 	ExtBuilder::default().build().execute_with(|| {
-		let addr = H160::repeat_byte(0x1d);
-		pallet_evm::AccountCodes::<Runtime>::insert(addr, vec![0x11, 0x00, 0x60, 0x00, 0xfd]);
+		let contract_being_constucted = H160::repeat_byte(0x1d);
+		let mut handle = MockPrecompileHandle::default()
+			.with_contracts_being_constructed(vec![contract_being_constucted]);
+
 		assert_eq!(
-			AddressType::Unknown,
-			get_address_type::<Runtime>(&mut MockPrecompileHandle, addr).expect("OOG")
+			AddressType::Contract,
+			get_address_type::<Runtime>(&mut handle, contract_being_constucted).expect("OOG")
 		);
 	})
 }