From b5b01b9ee34375ddb06e996ec3f9d34372ffb774 Mon Sep 17 00:00:00 2001 From: toints Date: Thu, 16 May 2024 15:36:36 +0800 Subject: [PATCH 1/3] perf(*): optimized evm contract bound with substrate precompile contract 1. Fixed potential vulnerabilities with DELEGATECALL and CALLCODE 2. Provided an interface to support administrators in adding EVM contract addresses --- pallets/assets-bridge/src/lib.rs | 53 +++++++++++++++ .../precompile/transfer-to-magnet/src/lib.rs | 7 +- .../transfer-to-magnet/src/tests.rs | 68 ++++++++++++++++++- runtime/src/precompiles.rs | 18 ++++- 4 files changed, 141 insertions(+), 5 deletions(-) diff --git a/pallets/assets-bridge/src/lib.rs b/pallets/assets-bridge/src/lib.rs index 0b2e3a1..f973b67 100644 --- a/pallets/assets-bridge/src/lib.rs +++ b/pallets/assets-bridge/src/lib.rs @@ -263,6 +263,10 @@ pub mod pallet { #[pallet::getter(fn emergencies)] pub(super) type Emergencies = StorageValue<_, Vec, ValueQuery>; + #[pallet::storage] + #[pallet::getter(fn evm_contracts)] + pub type EvmContracts = StorageValue<_, BTreeSet, ValueQuery>; + #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] pub struct GenesisConfig { @@ -315,6 +319,11 @@ pub mod pallet { UnPausedAll, // (asset_id, remove) BackForeign(T::AssetId, bool), + + ///(new_evm_contract) + AddNewContract(H160), + ///(evm contract) + RemoveContract(H160), } /// Error for evm accounts module. @@ -630,6 +639,50 @@ pub mod pallet { Ok(Pays::No.into()) } + + /// Add evm token contracts which can call precompile + /// Note: for admin + /// + /// - `new_contract`: + #[pallet::call_index(8)] + #[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))] + pub fn add_evm_contract( + origin: OriginFor, + new_contract: H160, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + ensure!(Some(who) == Self::admin_key(), Error::::RequireAdmin); + + EvmContracts::::mutate(|contracts| { + contracts.insert(new_contract); + }); + + Self::deposit_event(Event::AddNewContract(new_contract.clone())); + + Ok(Pays::No.into()) + } + + /// Remove evm token contracts which can call precompile + /// Note: for admin + /// + /// - `new_contract`: + #[pallet::call_index(9)] + #[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))] + pub fn remove_evm_contract( + origin: OriginFor, + contract: H160, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + ensure!(Some(who) == Self::admin_key(), Error::::RequireAdmin); + + EvmContracts::::mutate(|contracts| { + contracts.remove(&contract); + }); + + Self::deposit_event(Event::RemoveContract(contract)); + + Ok(Pays::No.into()) + } } } diff --git a/pallets/evm/precompile/transfer-to-magnet/src/lib.rs b/pallets/evm/precompile/transfer-to-magnet/src/lib.rs index 1fd7383..961d95d 100644 --- a/pallets/evm/precompile/transfer-to-magnet/src/lib.rs +++ b/pallets/evm/precompile/transfer-to-magnet/src/lib.rs @@ -93,10 +93,11 @@ where &target_gas ); let caller = context.caller.clone(); - if T::EvmAdmins::get().contains(&caller) == false { - log::error!("Caller is not the admin: {:?}", caller); + if pallet_assets_bridge::EvmContracts::::get().contains(&caller) == false { + log::error!("Caller {:?} is not in the admin allow set.", caller); + //log::error!("EvmContracts:{:?}", pallet_assets_bridge::EvmContracts::::get()); return Err(PrecompileFailure::Error { - exit_status: ExitError::Other("Caller is not the admin".into()), + exit_status: ExitError::Other("Caller is not in the admin allow set".into()), }); } diff --git a/pallets/evm/precompile/transfer-to-magnet/src/tests.rs b/pallets/evm/precompile/transfer-to-magnet/src/tests.rs index 6e0b0d3..6344bf1 100644 --- a/pallets/evm/precompile/transfer-to-magnet/src/tests.rs +++ b/pallets/evm/precompile/transfer-to-magnet/src/tests.rs @@ -21,6 +21,7 @@ use super::*; use crate::mock::*; use frame_support::assert_ok; use frame_support::traits::Currency; +use frame_system::RawOrigin; use pallet_evm::{AddressMapping, CallInfo, Error, ExitError, ExitReason, Runner}; use codec::Encode; @@ -280,6 +281,19 @@ fn transfer_to_substrate_works() { let asset_id = create_and_register_asset(token_addr); log::info!("asset id:{:?}", asset_id); + let admin_key = AccountId32::from([1u8; 32]); + let root_origin: frame_system::Origin = frame_system::RawOrigin::Root; + let _ = + pallet_assets_bridge::Pallet::::set_admin(root_origin.into(), admin_key.clone()); + let r = pallet_assets_bridge::Pallet::::add_evm_contract( + RawOrigin::Signed(admin_key.clone()).into(), + bob_evm, + ); + log::info!("add evm contract result:{:?}", r); + + let callers = pallet_assets_bridge::EvmContracts::::get(); + log::info!("callers:{:?}", callers); + let alice_token_amount_before = Assets::balance(asset_id, &ALICE); log::info!("alice token amount before mint:{:?}", alice_token_amount_before); @@ -390,6 +404,19 @@ fn gas_not_enough_error_works() { let token_addr = deploy_contract(bob_evm); log::info!("token addr:{:?}", token_addr); + let admin_key = AccountId32::from([1u8; 32]); + let root_origin: frame_system::Origin = frame_system::RawOrigin::Root; + let _ = + pallet_assets_bridge::Pallet::::set_admin(root_origin.into(), admin_key.clone()); + let r = pallet_assets_bridge::Pallet::::add_evm_contract( + RawOrigin::Signed(admin_key.clone()).into(), + bob_evm, + ); + log::info!("add evm contract result:{:?}", r); + + let callers = pallet_assets_bridge::EvmContracts::::get(); + log::info!("callers:{:?}", callers); + let asset_id = create_and_register_asset(token_addr); log::info!("asset id:{:?}", asset_id); @@ -491,6 +518,19 @@ fn gas_price_too_low_error_works() { let token_addr = deploy_contract(bob_evm); log::info!("token addr:{:?}", token_addr); + let admin_key = AccountId32::from([1u8; 32]); + let root_origin: frame_system::Origin = frame_system::RawOrigin::Root; + let _ = + pallet_assets_bridge::Pallet::::set_admin(root_origin.into(), admin_key.clone()); + let r = pallet_assets_bridge::Pallet::::add_evm_contract( + RawOrigin::Signed(admin_key.clone()).into(), + bob_evm, + ); + log::info!("add evm contract result:{:?}", r); + + let callers = pallet_assets_bridge::EvmContracts::::get(); + log::info!("callers:{:?}", callers); + let asset_id = create_and_register_asset(token_addr); log::info!("asset id:{:?}", asset_id); @@ -745,6 +785,19 @@ fn ss58address_error_works() { let token_addr = deploy_contract(bob_evm); log::info!("token addr:{:?}", token_addr); + let admin_key = AccountId32::from([1u8; 32]); + let root_origin: frame_system::Origin = frame_system::RawOrigin::Root; + let _ = + pallet_assets_bridge::Pallet::::set_admin(root_origin.into(), admin_key.clone()); + let r = pallet_assets_bridge::Pallet::::add_evm_contract( + RawOrigin::Signed(admin_key.clone()).into(), + bob_evm, + ); + log::info!("add evm contract result:{:?}", r); + + let callers = pallet_assets_bridge::EvmContracts::::get(); + log::info!("callers:{:?}", callers); + let asset_id = create_and_register_asset(token_addr); log::info!("asset id:{:?}", asset_id); @@ -929,7 +982,7 @@ fn not_evm_admin_works() { assert_eq!( reason, ExitReason::Error(ExitError::Other(std::borrow::Cow::Borrowed( - "Caller is not the admin" + "Caller is not in the admin allow set" ))) ); }, @@ -967,6 +1020,19 @@ fn token_and_assets_not_bound_works() { let token_addr = deploy_contract(bob_evm); log::info!("token addr:{:?}", token_addr); + let admin_key = AccountId32::from([1u8; 32]); + let root_origin: frame_system::Origin = frame_system::RawOrigin::Root; + let _ = + pallet_assets_bridge::Pallet::::set_admin(root_origin.into(), admin_key.clone()); + let r = pallet_assets_bridge::Pallet::::add_evm_contract( + RawOrigin::Signed(admin_key.clone()).into(), + bob_evm, + ); + log::info!("add evm contract result:{:?}", r); + + let callers = pallet_assets_bridge::EvmContracts::::get(); + log::info!("callers:{:?}", callers); + let asset_id = create_without_register_asset(); log::info!("asset id:{:?}", asset_id); diff --git a/runtime/src/precompiles.rs b/runtime/src/precompiles.rs index c7e7560..a69590a 100644 --- a/runtime/src/precompiles.rs +++ b/runtime/src/precompiles.rs @@ -1,5 +1,6 @@ use pallet_evm::{ - IsPrecompileResult, Precompile, PrecompileHandle, PrecompileResult, PrecompileSet, + ExitRevert, IsPrecompileResult, Precompile, PrecompileFailure, PrecompileHandle, + PrecompileResult, PrecompileSet, }; use sp_core::{crypto::AccountId32, H160, U256}; use sp_runtime::traits::UniqueSaturatedInto; @@ -45,6 +46,21 @@ where U256: UniqueSaturatedInto>, { fn execute(&self, handle: &mut impl PrecompileHandle) -> Option { + let remaining_gas = handle.remaining_gas(); + + let is_precompile_result = self.is_precompile(handle.code_address(), remaining_gas); + if let IsPrecompileResult::Answer { is_precompile, .. } = is_precompile_result { + if is_precompile + && handle.code_address() > hash(5) + && handle.code_address() != handle.context().address + { + return Some(Err(PrecompileFailure::Revert { + exit_status: ExitRevert::Reverted, + output: "cannot be called with DELEGATECALL or CALLCODE".into(), + })); + } + } + match handle.code_address() { // Ethereum precompiles : a if a == hash(1) => Some(ECRecover::execute(handle)), From e9d6f4fe2bdd156d0252c89f8cbe9cdf10ec6cc9 Mon Sep 17 00:00:00 2001 From: toints Date: Tue, 21 May 2024 13:44:42 +0800 Subject: [PATCH 2/3] perf: removed hardcode of evm admins --- pallets/assets-bridge/src/lib.rs | 4 ---- pallets/assets-bridge/src/mock.rs | 7 ------- pallets/evm/precompile/transfer-to-magnet/src/mock.rs | 6 ------ runtime/src/lib.rs | 7 ------- 4 files changed, 24 deletions(-) diff --git a/pallets/assets-bridge/src/lib.rs b/pallets/assets-bridge/src/lib.rs index f973b67..df53215 100644 --- a/pallets/assets-bridge/src/lib.rs +++ b/pallets/assets-bridge/src/lib.rs @@ -212,10 +212,6 @@ pub mod pallet { /// How much should be locked up in order to claim account. #[pallet::constant] type ClaimBond: Get>; - - /// The assets-bridge's evm contract deployer. - #[pallet::constant] - type EvmAdmins: Get>; } /// The Substrate Account for Evm Addresses diff --git a/pallets/assets-bridge/src/mock.rs b/pallets/assets-bridge/src/mock.rs index a4e226a..092d380 100644 --- a/pallets/assets-bridge/src/mock.rs +++ b/pallets/assets-bridge/src/mock.rs @@ -129,12 +129,6 @@ parameter_types! { // 0x1111111111111111111111111111111111111111 pub EvmCaller: H160 = H160::from_slice(&[17u8;20][..]); pub ClaimBond: u128 = 2; - //pub EvmAdmin: H160 = H160([0x05, 0xF9, 0xb8, 0xC7, 0x6E, 0x89, 0x87, 0xB8, 0x15, 0xC9, 0x3C, 0x27, 0xD1, 0x45, 0x20, 0xb6, 0xeD, 0x57, 0x39, 0x02]); - pub EvmAdmins: BTreeSet = { - let mut set = BTreeSet::new(); - set.insert(H160([0x05, 0xF9, 0xb8, 0xC7, 0x6E, 0x89, 0x87, 0xB8, 0x15, 0xC9, 0x3C, 0x27, 0xD1, 0x45, 0x20, 0xb6, 0xeD, 0x57, 0x39, 0x02])); - set - }; pub const WeightPerGas: Weight = Weight::from_parts(20_000, 0); pub const GasLimitPovSizeRatio: u64 = BLOCK_GAS_LIMIT.saturating_div(MAX_POV_SIZE); @@ -193,7 +187,6 @@ impl assets_bridge::Config for Test { type RuntimeEvent = RuntimeEvent; type EvmCaller = EvmCaller; type ClaimBond = ClaimBond; - type EvmAdmins = EvmAdmins; } pub const ALICE: [u8; 32] = [1u8; 32]; diff --git a/pallets/evm/precompile/transfer-to-magnet/src/mock.rs b/pallets/evm/precompile/transfer-to-magnet/src/mock.rs index fab01a2..d1e16eb 100644 --- a/pallets/evm/precompile/transfer-to-magnet/src/mock.rs +++ b/pallets/evm/precompile/transfer-to-magnet/src/mock.rs @@ -137,18 +137,12 @@ parameter_types! { // 0x1111111111111111111111111111111111111111 pub EvmCaller: H160 = H160::from_slice(&[17u8;20][..]); pub ClaimBond: u64 = 10_000_000_000_000_000; - pub EvmAdmins: BTreeSet = { - let mut set = BTreeSet::new(); - set.insert(H160([0x05, 0xF9, 0xb8, 0xC7, 0x6E, 0x89, 0x87, 0xB8, 0x15, 0xC9, 0x3C, 0x27, 0xD1, 0x45, 0x20, 0xb6, 0xeD, 0x57, 0x39, 0x02])); - set - }; } impl pallet_assets_bridge::Config for Test { type RuntimeEvent = RuntimeEvent; type EvmCaller = EvmCaller; type ClaimBond = ClaimBond; - type EvmAdmins = EvmAdmins; } pub const UNIT: u64 = 1_000_000_000_000_000_000; diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index ce93b1b..a345083 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -901,18 +901,11 @@ parameter_types! { // 0x1111111111111111111111111111111111111111 pub EvmCaller: H160 = H160::from_slice(&[17u8;20][..]); pub ClaimBond: Balance = 10 * EXISTENTIAL_DEPOSIT; - //pub EvmAdmin: H160 = H160([0x05, 0xF9, 0xb8, 0xC7, 0x6E, 0x89, 0x87, 0xB8, 0x15, 0xC9, 0x3C, 0x27, 0xD1, 0x45, 0x20, 0xb6, 0xeD, 0x57, 0x39, 0x02]); - pub EvmAdmins: BTreeSet = { - let mut set = BTreeSet::new(); - set.insert(H160([0x05, 0xF9, 0xb8, 0xC7, 0x6E, 0x89, 0x87, 0xB8, 0x15, 0xC9, 0x3C, 0x27, 0xD1, 0x45, 0x20, 0xb6, 0xeD, 0x57, 0x39, 0x02])); - set - }; } impl pallet_assets_bridge::Config for Runtime { type RuntimeEvent = RuntimeEvent; type EvmCaller = EvmCaller; type ClaimBond = ClaimBond; - type EvmAdmins = EvmAdmins; } impl pallet_evm_utils::Config for Runtime { From c2ad63539702b5a760ff9f96feb2d0e92dfe7960 Mon Sep 17 00:00:00 2001 From: toints Date: Tue, 21 May 2024 14:19:42 +0800 Subject: [PATCH 3/3] perf: removed useless code and fixed log level --- pallets/assets-bridge/src/mock.rs | 5 +---- .../precompile/transfer-to-magnet/src/lib.rs | 18 ++++++++---------- .../precompile/transfer-to-magnet/src/mock.rs | 1 - 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/pallets/assets-bridge/src/mock.rs b/pallets/assets-bridge/src/mock.rs index 092d380..4bec7f4 100644 --- a/pallets/assets-bridge/src/mock.rs +++ b/pallets/assets-bridge/src/mock.rs @@ -12,9 +12,7 @@ // along with this program. If not, see . pub use crate as assets_bridge; -pub use assets_bridge::{Config, Error, Event as AssetsBridgeEvent}; - -use sp_std::collections::btree_set::BTreeSet; +pub use assets_bridge::{Error, Event as AssetsBridgeEvent}; use frame_support::{ derive_impl, @@ -27,7 +25,6 @@ use frame_system::EnsureSigned; use sp_core::{H160, H256}; pub use sp_runtime::{ - testing::Header, traits::{BlakeTwo256, IdentityLookup}, AccountId32, BuildStorage, }; diff --git a/pallets/evm/precompile/transfer-to-magnet/src/lib.rs b/pallets/evm/precompile/transfer-to-magnet/src/lib.rs index 961d95d..7c66d67 100644 --- a/pallets/evm/precompile/transfer-to-magnet/src/lib.rs +++ b/pallets/evm/precompile/transfer-to-magnet/src/lib.rs @@ -86,7 +86,7 @@ where let target_gas = handle.gas_limit(); let context = handle.context(); - log::info!( + log::debug!( "codeAddress:{:?}, input:{:?}, targetGas:{:?}", &code_address, &input, @@ -103,9 +103,7 @@ where let token_addr = solidity::decode_arguments::
(&input[4..36])?; let amount = solidity::decode_arguments::(&input[36..68])?; - log::info!("Caller:{:?}, tokenAddr:{:?}, amount:{:?}", &caller, &token_addr, &amount); - - log::info!("who bstr data len:{:?}", &input[100..].len()); + log::debug!("Caller:{:?}, tokenAddr:{:?}, amount:{:?}", &caller, &token_addr, &amount); if handle.is_static() { log::error!("Can't be static call error"); @@ -123,12 +121,12 @@ where }); }, }; - log::info!("to who ss58:{:?}", &to_who_ss58); + log::debug!("to who ss58:{:?}", &to_who_ss58); let to_who_id32 = AccountId32::from_ss58check(&to_who_ss58).map_err(|_| PrecompileFailure::Error { exit_status: ExitError::Other("AccountId32 from ss58check(string) failed".into()), })?; - log::info!("to_who_ss58:{:?}, to_who_id32:{:?}", &to_who_ss58, &to_who_id32); + log::debug!("to_who_ss58:{:?}, to_who_id32:{:?}", &to_who_ss58, &to_who_id32); let to_who: ::AccountId = to_who_id32.clone().into(); @@ -153,7 +151,7 @@ where let amount_saturated: T::Balance = amount.into(); - log::info!( + log::debug!( "Preparing to mint: AssetId: {:?}, Beneficiary: {:?}, Amount: {:?}", &asset_id, &to_who_ss58, @@ -238,9 +236,9 @@ where } let length_bytes = &input[offset..offset + 32]; - log::info!("length_bytes:{:?}", &length_bytes); + log::debug!("length_bytes:{:?}", &length_bytes); let length = u32::from_be_bytes(length_bytes[28..32].try_into().unwrap()) as usize; - log::info!("ss58 string len:{:?}", length); + log::debug!("ss58 string len:{:?}", length); if input.len() < offset + 32 + length { return Err("Input too short to contain string data"); @@ -249,7 +247,7 @@ where let string_data_start = offset + 32; let string_data_end = string_data_start + length; let string_data = &input[string_data_start..string_data_end]; - log::info!("ss58 string data:{:?}", &string_data); + log::debug!("ss58 string data:{:?}", &string_data); let mut result_string = from_utf8(string_data) .map_err(|_| "String data is not valid UTF-8")? diff --git a/pallets/evm/precompile/transfer-to-magnet/src/mock.rs b/pallets/evm/precompile/transfer-to-magnet/src/mock.rs index d1e16eb..b1a9f47 100644 --- a/pallets/evm/precompile/transfer-to-magnet/src/mock.rs +++ b/pallets/evm/precompile/transfer-to-magnet/src/mock.rs @@ -35,7 +35,6 @@ use sp_runtime::{ traits::{BlakeTwo256, IdentityLookup}, BuildStorage, }; -use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use std::str::FromStr; use xcm::latest::prelude::BodyId;