diff --git a/modules/asset-registry/src/lib.rs b/modules/asset-registry/src/lib.rs index 535164351c..891f60a27c 100644 --- a/modules/asset-registry/src/lib.rs +++ b/modules/asset-registry/src/lib.rs @@ -643,7 +643,7 @@ where fn calculate_rate(location: MultiLocation) -> Option { let currency = key_to_currency(location); match currency { - Some(CurrencyId::Erc20(address)) if !is_system_contract(address) => { + Some(CurrencyId::Erc20(address)) if !is_system_contract(&address) => { if let Some(asset_metadata) = Pallet::::asset_metadatas(AssetIds::Erc20(address)) { let minimum_balance = asset_metadata.minimal_balance.into(); let rate = @@ -898,7 +898,7 @@ impl Erc20InfoMapping for EvmErc20InfoMapping { // If is CurrencyId::DexShare and contain DexShare::Erc20, // will use the u32 to get the DexShare::Erc20 from the mapping. fn decode_evm_address(addr: EvmAddress) -> Option { - if !is_system_contract(addr) { + if !is_system_contract(&addr) { return Some(CurrencyId::Erc20(addr)); } diff --git a/modules/evm/src/lib.rs b/modules/evm/src/lib.rs index 782c652519..5ea57ae804 100644 --- a/modules/evm/src/lib.rs +++ b/modules/evm/src/lib.rs @@ -59,8 +59,8 @@ pub use module_support::{ pub use orml_traits::{currency::TransferAll, MultiCurrency}; pub use primitives::{ evm::{ - convert_decimals_from_evm, convert_decimals_to_evm, decode_gas_limit, CallInfo, CreateInfo, EvmAddress, - ExecutionInfo, Vicinity, MIRRORED_NFT_ADDRESS_START, MIRRORED_TOKENS_ADDRESS_START, + convert_decimals_from_evm, convert_decimals_to_evm, decode_gas_limit, is_system_contract, CallInfo, CreateInfo, + EvmAddress, ExecutionInfo, Vicinity, MIRRORED_NFT_ADDRESS_START, MIRRORED_TOKENS_ADDRESS_START, }, task::TaskResult, Balance, CurrencyId, Nonce, ReserveIdentifier, @@ -526,6 +526,8 @@ pub mod module { InvalidDecimals, /// Strict call failed StrictCallFailed, + /// Caller is not externally owned account + NotEOA, } #[pallet::pallet] @@ -623,6 +625,8 @@ pub mod module { let who = ensure_signed(origin)?; let source = T::AddressMapping::get_or_create_evm_address(&who); + Self::ensure_eoa(&source)?; + let outcome = T::Runner::call( source, source, @@ -810,6 +814,8 @@ pub mod module { let who = ensure_signed(origin)?; let source = T::AddressMapping::get_or_create_evm_address(&who); + Self::ensure_eoa(&source)?; + let outcome = T::Runner::create( source, input, @@ -888,6 +894,8 @@ pub mod module { let who = ensure_signed(origin)?; let source = T::AddressMapping::get_or_create_evm_address(&who); + Self::ensure_eoa(&source)?; + let outcome = T::Runner::create2( source, input, @@ -1263,6 +1271,8 @@ pub mod module { let who = ensure_signed(origin)?; let source = T::AddressMapping::get_or_create_evm_address(&who); + Self::ensure_eoa(&source)?; + match T::Runner::call( source, source, @@ -1315,6 +1325,20 @@ pub mod module { } impl Pallet { + /// EIP-3607: https://eips.ethereum.org/EIPS/eip-3607 + /// Do not allow transactions for which `tx.sender` has any code deployed. + // + /// We extend the principle of this EIP to also prevent `tx.sender` to be the address + /// of a precompile. While mainnet Ethereum currently only has stateless precompiles, + /// Acala EVM+ can have stateful precompiles that can manage funds or + /// which calls other contracts that expects this precompile address to be trustworthy. + fn ensure_eoa(caller: &EvmAddress) -> DispatchResult { + if is_system_contract(caller) || Self::is_contract(caller) { + return Err(Error::::NotEOA.into()); + } + Ok(()) + } + /// Get StorageDepositPerByte of actual decimals pub fn get_storage_deposit_per_byte() -> BalanceOf { // StorageDepositPerByte decimals is 18, KAR/ACA decimals is 12, convert to 12 here. diff --git a/modules/evm/src/mock.rs b/modules/evm/src/mock.rs index e67caaa466..abe2d20839 100644 --- a/modules/evm/src/mock.rs +++ b/modules/evm/src/mock.rs @@ -29,7 +29,7 @@ use frame_system::EnsureSignedBy; use module_support::mocks::MockAddressMapping; use orml_traits::parameter_type_with_key; use primitives::{define_combined_task, Amount, BlockNumber, CurrencyId, ReserveIdentifier, TokenSymbol}; -use sp_core::{H160, H256}; +use sp_core::{bytes::from_hex, H160, H256}; use sp_runtime::{ traits::{BlakeTwo256, BlockNumberProvider, IdentityLookup}, AccountId32, BuildStorage, @@ -266,10 +266,17 @@ pub fn new_test_ext() -> sp_io::TestExternalities { let mut accounts = BTreeMap::new(); + // pragma solidity >=0.8.2 <0.9.0; + // contract Test {} + let contract = from_hex( + "0x6080604052348015600f57600080fd5b50603f80601d6000396000f3fe6080604052600080fdfea2646970667358221220199b6fd928fecd2e7ce866eb76c49927191c7a839fd75192acc84b773e5dbf1e64736f6c63430008120033" + ).unwrap(); + accounts.insert( contract_a(), GenesisAccount { nonce: 1, + code: contract.clone(), ..Default::default() }, ); diff --git a/modules/evm/src/tests.rs b/modules/evm/src/tests.rs index 995e5a60a8..d3f3e1d309 100644 --- a/modules/evm/src/tests.rs +++ b/modules/evm/src/tests.rs @@ -75,7 +75,7 @@ fn inc_nonce_if_needed() { #[test] fn fail_call_return_ok_and_inc_nonce() { new_test_ext().execute_with(|| { - let mut data = [0u8; 32]; + let mut data = [5u8; 32]; data[0..4].copy_from_slice(b"evm:"); let signer: AccountId32 = AccountId32::from(data); let alice = MockAddressMapping::get_or_create_evm_address(&signer); @@ -2829,3 +2829,153 @@ fn aggregated_storage_logs_works() { })); }) } + +#[allow(deprecated)] +#[test] +fn should_not_allow_contracts_send_tx() { + new_test_ext().execute_with(|| { + let origin = RuntimeOrigin::signed(MockAddressMapping::get_account_id(&contract_a())); + assert_noop!( + EVM::eth_call( + origin.clone(), + TransactionAction::Call(contract_a()), + vec![], + 0, + 1_000_000, + 100, + vec![], + 0 + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call( + origin.clone(), + TransactionAction::Create, + vec![], + 0, + 1_000_000, + 100, + vec![], + 0 + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call_v2( + origin.clone(), + TransactionAction::Call(contract_a()), + vec![], + 0, + 1_000_000, + 100, + vec![] + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call_v2( + origin.clone(), + TransactionAction::Create, + vec![], + 0, + 1_000_000, + 100, + vec![] + ), + Error::::NotEOA + ); + assert_noop!( + EVM::call(origin.clone(), contract_a(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::create(origin.clone(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::create2(origin.clone(), vec![], Default::default(), 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::strict_call(origin, contract_a(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + }); +} + +#[allow(deprecated)] +#[test] +fn should_not_allow_system_contracts_send_tx() { + new_test_ext().execute_with(|| { + let origin = RuntimeOrigin::signed(MockAddressMapping::get_account_id( + &H160::from_str("000000000000000000ffffffffffffffffffffff").unwrap(), + )); + assert_noop!( + EVM::eth_call( + origin.clone(), + TransactionAction::Call(contract_a()), + vec![], + 0, + 1_000_000, + 100, + vec![], + 0 + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call( + origin.clone(), + TransactionAction::Create, + vec![], + 0, + 1_000_000, + 100, + vec![], + 0 + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call_v2( + origin.clone(), + TransactionAction::Call(contract_a()), + vec![], + 0, + 1_000_000, + 100, + vec![] + ), + Error::::NotEOA + ); + assert_noop!( + EVM::eth_call_v2( + origin.clone(), + TransactionAction::Create, + vec![], + 0, + 1_000_000, + 100, + vec![] + ), + Error::::NotEOA + ); + assert_noop!( + EVM::call(origin.clone(), contract_a(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::create(origin.clone(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::create2(origin.clone(), vec![], Default::default(), 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + assert_noop!( + EVM::strict_call(origin, contract_a(), vec![], 0, 1_000_000, 100, vec![]), + Error::::NotEOA + ); + }); +} diff --git a/primitives/src/evm.rs b/primitives/src/evm.rs index 22d607ddcc..312cf657b5 100644 --- a/primitives/src/evm.rs +++ b/primitives/src/evm.rs @@ -175,7 +175,7 @@ pub const SYSTEM_CONTRACT_ADDRESS_PREFIX: [u8; 9] = [0u8; 9]; /// Check if the given `address` is a system contract. /// /// It's system contract if the address starts with SYSTEM_CONTRACT_ADDRESS_PREFIX. -pub fn is_system_contract(address: EvmAddress) -> bool { +pub fn is_system_contract(address: &EvmAddress) -> bool { address.as_bytes().starts_with(&SYSTEM_CONTRACT_ADDRESS_PREFIX) } diff --git a/primitives/src/tests.rs b/primitives/src/tests.rs index ec24324636..fecef8aed7 100644 --- a/primitives/src/tests.rs +++ b/primitives/src/tests.rs @@ -169,18 +169,18 @@ fn generate_function_selector_works() { #[test] fn is_system_contract_works() { - assert!(is_system_contract(H160::from_low_u64_be(0))); - assert!(is_system_contract(H160::from_low_u64_be(u64::max_value()))); + assert!(is_system_contract(&H160::from_low_u64_be(0))); + assert!(is_system_contract(&H160::from_low_u64_be(u64::max_value()))); let mut bytes = [0u8; 20]; bytes[SYSTEM_CONTRACT_ADDRESS_PREFIX.len() - 1] = 1u8; - assert!(!is_system_contract(bytes.into())); + assert!(!is_system_contract(&bytes.into())); bytes = [0u8; 20]; bytes[0] = 1u8; - assert!(!is_system_contract(bytes.into())); + assert!(!is_system_contract(&bytes.into())); } #[test] diff --git a/runtime/acala/src/xcm_config.rs b/runtime/acala/src/xcm_config.rs index e7a2cbe61b..4c7e1655cd 100644 --- a/runtime/acala/src/xcm_config.rs +++ b/runtime/acala/src/xcm_config.rs @@ -293,7 +293,7 @@ impl Convert> for CurrencyIdConvert { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAP) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } - Erc20(address) if !is_system_contract(address) => { + Erc20(address) if !is_system_contract(&address) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } LiquidCrowdloan(_lease) => native_currency_location(ParachainInfo::get().into(), id.encode()), @@ -328,7 +328,7 @@ impl Convert> for CurrencyIdConvert { // check `currency_id` is cross-chain asset match currency_id { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAP) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), LiquidCrowdloan(_lease) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, @@ -350,7 +350,7 @@ impl Convert> for CurrencyIdConvert { let currency_id = CurrencyId::decode(&mut &*key).ok()?; match currency_id { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAP) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), LiquidCrowdloan(_lease) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, diff --git a/runtime/common/src/lib.rs b/runtime/common/src/lib.rs index af9a321397..c9d7f4b4a7 100644 --- a/runtime/common/src/lib.rs +++ b/runtime/common/src/lib.rs @@ -97,7 +97,7 @@ parameter_types! { pub struct SystemContractsFilter; impl PrecompileCallerFilter for SystemContractsFilter { fn is_allowed(caller: H160) -> bool { - is_system_contract(caller) + is_system_contract(&caller) } } diff --git a/runtime/karura/src/xcm_config.rs b/runtime/karura/src/xcm_config.rs index e80a81f616..e1ecd435ad 100644 --- a/runtime/karura/src/xcm_config.rs +++ b/runtime/karura/src/xcm_config.rs @@ -387,7 +387,7 @@ impl Convert> for CurrencyIdConvert { Token(KAR) | Token(KUSD) | Token(LKSM) | Token(TAI) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } - Erc20(address) if !is_system_contract(address) => { + Erc20(address) if !is_system_contract(&address) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } StableAssetPoolToken(_pool_id) => native_currency_location(ParachainInfo::get().into(), id.encode()), @@ -437,7 +437,7 @@ impl Convert> for CurrencyIdConvert { // check `currency_id` is cross-chain asset match currency_id { Token(KAR) | Token(KUSD) | Token(LKSM) | Token(TAI) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, } @@ -462,7 +462,7 @@ impl Convert> for CurrencyIdConvert { let currency_id = CurrencyId::decode(&mut &*key).ok()?; match currency_id { Token(KAR) | Token(KUSD) | Token(LKSM) | Token(TAI) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, } diff --git a/runtime/mandala/src/xcm_config.rs b/runtime/mandala/src/xcm_config.rs index 4041a81e0e..4c7a80d6dd 100644 --- a/runtime/mandala/src/xcm_config.rs +++ b/runtime/mandala/src/xcm_config.rs @@ -279,7 +279,7 @@ impl Convert> for CurrencyIdConvert { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAI) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } - Erc20(address) if !is_system_contract(address) => { + Erc20(address) if !is_system_contract(&address) => { native_currency_location(ParachainInfo::get().into(), id.encode()) } StableAssetPoolToken(_pool_id) => native_currency_location(ParachainInfo::get().into(), id.encode()), @@ -312,7 +312,7 @@ impl Convert> for CurrencyIdConvert { // check if `currency_id` is cross-chain asset match currency_id { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAI) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, } @@ -329,7 +329,7 @@ impl Convert> for CurrencyIdConvert { if let Ok(currency_id) = CurrencyId::decode(&mut &*key) { match currency_id { Token(ACA) | Token(AUSD) | Token(LDOT) | Token(TAI) => Some(currency_id), - Erc20(address) if !is_system_contract(address) => Some(currency_id), + Erc20(address) if !is_system_contract(&address) => Some(currency_id), StableAssetPoolToken(_pool_id) => Some(currency_id), _ => None, }