diff --git a/Cargo.lock b/Cargo.lock index 03bb4473f6..cb075119f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7474,6 +7474,7 @@ dependencies = [ "fp-evm", "frame-support", "frame-system", + "hex", "hex-literal", "log", "num_enum 0.5.11", @@ -8241,6 +8242,7 @@ name = "pallet-xvm" version = "0.2.2" dependencies = [ "astar-primitives", + "environmental", "fp-evm", "frame-benchmarking", "frame-support", diff --git a/chain-extensions/types/xvm/src/lib.rs b/chain-extensions/types/xvm/src/lib.rs index c026fc52dc..c42b71c4d5 100644 --- a/chain-extensions/types/xvm/src/lib.rs +++ b/chain-extensions/types/xvm/src/lib.rs @@ -38,10 +38,11 @@ impl From for XvmExecutionResult { // `0` is reserved for `Ok` let error_code = match input { InvalidVmId => 1, - SameVmCallNotAllowed => 2, + SameVmCallDenied => 2, InvalidTarget => 3, InputTooLarge => 4, - ExecutionFailed(_) => 5, + ReentranceDenied => 5, + ExecutionFailed(_) => 6, }; Self::Err(error_code) } diff --git a/pallets/xvm/Cargo.toml b/pallets/xvm/Cargo.toml index 6b83517d55..d9f9bec944 100644 --- a/pallets/xvm/Cargo.toml +++ b/pallets/xvm/Cargo.toml @@ -7,6 +7,7 @@ homepage.workspace = true repository.workspace = true [dependencies] +environmental = { workspace = true } log = { workspace = true } serde = { workspace = true, optional = true } @@ -42,6 +43,7 @@ sp-io = { workspace = true } [features] default = ["std"] std = [ + "environmental/std", "log/std", "parity-scale-codec/std", "frame-support/std", diff --git a/pallets/xvm/src/lib.rs b/pallets/xvm/src/lib.rs index d10d141520..0f15c8b7d8 100644 --- a/pallets/xvm/src/lib.rs +++ b/pallets/xvm/src/lib.rs @@ -37,7 +37,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use frame_support::{ensure, traits::Currency}; +use frame_support::{ensure, traits::Currency, weights::Weight}; use pallet_contracts::{CollectEvents, DebugInfo, Determinism}; use pallet_evm::GasWeightMapping; use parity_scale_codec::Decode; @@ -66,6 +66,8 @@ pub use pallet::*; pub type WeightInfoOf = ::WeightInfo; +environmental::thread_local_impl!(static IN_XVM: environmental::RefCell = environmental::RefCell::new(false)); + #[frame_support::pallet] pub mod pallet { use super::*; @@ -120,25 +122,53 @@ where value: Balance, skip_execution: bool, ) -> CallResult { + let overheads = match vm_id { + VmId::Evm => WeightInfoOf::::evm_call_overheads(), + VmId::Wasm => WeightInfoOf::::wasm_call_overheads(), + }; + ensure!( context.source_vm_id != vm_id, CallErrorWithWeight { - error: CallError::SameVmCallNotAllowed, - used_weight: match vm_id { - VmId::Evm => WeightInfoOf::::evm_call_overheads(), - VmId::Wasm => WeightInfoOf::::wasm_call_overheads(), - }, + error: CallError::SameVmCallDenied, + used_weight: overheads, } ); - match vm_id { - VmId::Evm => { - Pallet::::evm_call(context, source, target, input, value, skip_execution) - } - VmId::Wasm => { - Pallet::::wasm_call(context, source, target, input, value, skip_execution) - } + // Set `IN_XVM` to true & check reentrance. + if IN_XVM.with(|in_xvm| in_xvm.replace(true)) { + return Err(CallErrorWithWeight { + error: CallError::ReentranceDenied, + used_weight: overheads, + }); } + + let res = match vm_id { + VmId::Evm => Pallet::::evm_call( + context, + source, + target, + input, + value, + overheads, + skip_execution, + ), + VmId::Wasm => Pallet::::wasm_call( + context, + source, + target, + input, + value, + overheads, + skip_execution, + ), + }; + + // Set `IN_XVM` to false. + // We should make sure that this line is executed whatever the execution path. + let _ = IN_XVM.with(|in_xvm| in_xvm.take()); + + res } fn evm_call( @@ -147,6 +177,7 @@ where target: Vec, input: Vec, value: Balance, + overheads: Weight, skip_execution: bool, ) -> CallResult { log::trace!( @@ -159,24 +190,22 @@ where target.len() == H160::len_bytes(), CallErrorWithWeight { error: CallError::InvalidTarget, - used_weight: WeightInfoOf::::evm_call_overheads(), + used_weight: overheads, } ); let target_decoded = Decode::decode(&mut target.as_ref()).map_err(|_| CallErrorWithWeight { error: CallError::InvalidTarget, - used_weight: WeightInfoOf::::evm_call_overheads(), + used_weight: overheads, })?; let bounded_input = EthereumTxInput::try_from(input).map_err(|_| CallErrorWithWeight { error: CallError::InputTooLarge, - used_weight: WeightInfoOf::::evm_call_overheads(), + used_weight: overheads, })?; let value_u256 = U256::from(value); // With overheads, less weight is available. - let weight_limit = context - .weight_limit - .saturating_sub(WeightInfoOf::::evm_call_overheads()); + let weight_limit = context.weight_limit.saturating_sub(overheads); let gas_limit = U256::from(T::GasWeightMapping::weight_to_gas(weight_limit)); let source = T::AccountMapping::into_h160(source); @@ -193,7 +222,7 @@ where if skip_execution { return Ok(CallInfo { output: vec![], - used_weight: WeightInfoOf::::evm_call_overheads(), + used_weight: overheads, }); } @@ -208,7 +237,7 @@ where let used_weight = post_dispatch_info .actual_weight .unwrap_or_default() - .saturating_add(WeightInfoOf::::evm_call_overheads()); + .saturating_add(overheads); CallInfo { output: call_info.value, used_weight, @@ -219,7 +248,7 @@ where .post_info .actual_weight .unwrap_or_default() - .saturating_add(WeightInfoOf::::evm_call_overheads()); + .saturating_add(overheads); CallErrorWithWeight { error: CallError::ExecutionFailed(Into::<&str>::into(e.error).into()), used_weight, @@ -233,6 +262,7 @@ where target: Vec, input: Vec, value: Balance, + overheads: Weight, skip_execution: bool, ) -> CallResult { log::trace!( @@ -244,23 +274,21 @@ where let dest = { let error = CallErrorWithWeight { error: CallError::InvalidTarget, - used_weight: WeightInfoOf::::wasm_call_overheads(), + used_weight: overheads, }; let decoded = Decode::decode(&mut target.as_ref()).map_err(|_| error.clone())?; T::Lookup::lookup(decoded).map_err(|_| error) }?; // With overheads, less weight is available. - let weight_limit = context - .weight_limit - .saturating_sub(WeightInfoOf::::wasm_call_overheads()); + let weight_limit = context.weight_limit.saturating_sub(overheads); // Note the skip execution check should be exactly before `pallet_contracts::bare_call` // to benchmark the correct overheads. if skip_execution { return Ok(CallInfo { output: vec![], - used_weight: WeightInfoOf::::wasm_call_overheads(), + used_weight: overheads, }); } @@ -277,9 +305,7 @@ where ); log::trace!(target: "xvm::wasm_call", "WASM call result: {:?}", call_result); - let used_weight = call_result - .gas_consumed - .saturating_add(WeightInfoOf::::wasm_call_overheads()); + let used_weight = call_result.gas_consumed.saturating_add(overheads); match call_result.result { Ok(success) => Ok(CallInfo { output: success.data, diff --git a/pallets/xvm/src/tests.rs b/pallets/xvm/src/tests.rs index aedb8b46d4..ba7b32b585 100644 --- a/pallets/xvm/src/tests.rs +++ b/pallets/xvm/src/tests.rs @@ -49,7 +49,7 @@ fn calling_into_same_vm_is_not_allowed() { value ), CallErrorWithWeight { - error: CallError::SameVmCallNotAllowed, + error: CallError::SameVmCallDenied, used_weight: evm_used_weight }, ); @@ -66,7 +66,7 @@ fn calling_into_same_vm_is_not_allowed() { assert_noop!( Xvm::call(wasm_context, wasm_vm_id, ALICE, wasm_target, input, value), CallErrorWithWeight { - error: CallError::SameVmCallNotAllowed, + error: CallError::SameVmCallDenied, used_weight: wasm_used_weight }, ); diff --git a/precompiles/utils/src/testing.rs b/precompiles/utils/src/testing.rs index 9eb1023a20..721681ec87 100644 --- a/precompiles/utils/src/testing.rs +++ b/precompiles/utils/src/testing.rs @@ -303,6 +303,11 @@ impl<'p, P: PrecompileSet> PrecompilesTester<'p, P> { self } + pub fn with_gas_limit(mut self, gas_limit: u64) -> Self { + self.handle.gas_limit = gas_limit; + self + } + pub fn expect_cost(mut self, cost: u64) -> Self { self.expected_cost = Some(cost); self diff --git a/precompiles/xvm/Cargo.toml b/precompiles/xvm/Cargo.toml index 397a412373..c64e26b3b2 100644 --- a/precompiles/xvm/Cargo.toml +++ b/precompiles/xvm/Cargo.toml @@ -8,6 +8,7 @@ homepage.workspace = true repository.workspace = true [dependencies] +hex = { workspace = true } log = { workspace = true } num_enum = { workspace = true } precompile-utils = { workspace = true } diff --git a/precompiles/xvm/src/lib.rs b/precompiles/xvm/src/lib.rs index 5ccdfcb342..6bb033dc1b 100644 --- a/precompiles/xvm/src/lib.rs +++ b/precompiles/xvm/src/lib.rs @@ -83,8 +83,12 @@ where id.try_into().map_err(|_| revert("invalid vm id")) }?; - let remaining_gas = handle.remaining_gas(); - let weight_limit = R::GasWeightMapping::gas_to_weight(remaining_gas, true); + let mut gas_limit = handle.remaining_gas(); + // If user specified a gas limit, make sure it's not exceeded. + if let Some(user_limit) = handle.gas_limit() { + gas_limit = gas_limit.min(user_limit); + } + let weight_limit = R::GasWeightMapping::gas_to_weight(gas_limit, true); let xvm_context = Context { source_vm_id: VmId::Evm, weight_limit, diff --git a/precompiles/xvm/src/mock.rs b/precompiles/xvm/src/mock.rs index 7115a43467..33fd5d1a78 100644 --- a/precompiles/xvm/src/mock.rs +++ b/precompiles/xvm/src/mock.rs @@ -38,6 +38,7 @@ use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, }; +use sp_std::cell::RefCell; use astar_primitives::xvm::{CallError::*, CallErrorWithWeight, CallInfo, CallResult}; @@ -236,10 +237,29 @@ impl pallet_evm::Config for Runtime { type GasLimitPovSizeRatio = ConstU64<4>; } +thread_local! { + static WEIGHT_LIMIT: RefCell = RefCell::new(Weight::zero()); +} + +pub(crate) struct WeightLimitCalledWith; +impl WeightLimitCalledWith { + pub(crate) fn get() -> Weight { + WEIGHT_LIMIT.with(|gas_limit| *gas_limit.borrow()) + } + + pub(crate) fn set(value: Weight) { + WEIGHT_LIMIT.with(|gas_limit| *gas_limit.borrow_mut() = value) + } + + pub(crate) fn reset() { + Self::set(Weight::zero()); + } +} + struct MockXvmWithArgsCheck; impl XvmCall for MockXvmWithArgsCheck { fn call( - _context: Context, + context: Context, vm_id: VmId, _source: AccountId, target: Vec, @@ -249,7 +269,7 @@ impl XvmCall for MockXvmWithArgsCheck { ensure!( vm_id != VmId::Evm, CallErrorWithWeight { - error: SameVmCallNotAllowed, + error: SameVmCallDenied, used_weight: Weight::zero() } ); @@ -267,6 +287,9 @@ impl XvmCall for MockXvmWithArgsCheck { used_weight: Weight::zero() } ); + + WeightLimitCalledWith::set(context.weight_limit); + Ok(CallInfo { output: vec![], used_weight: Weight::zero(), @@ -297,6 +320,8 @@ impl ExtBuilder { .build_storage::() .expect("Frame system builds valid default genesis config"); + WeightLimitCalledWith::reset(); + let mut ext = sp_io::TestExternalities::new(t); ext.execute_with(|| System::set_block_number(1)); ext diff --git a/precompiles/xvm/src/tests.rs b/precompiles/xvm/src/tests.rs index dc09f55f99..37bbc56cf1 100644 --- a/precompiles/xvm/src/tests.rs +++ b/precompiles/xvm/src/tests.rs @@ -82,3 +82,54 @@ fn correct_arguments_works() { ); }) } + +#[test] +fn weight_limit_is_min_of_remaining_and_user_limit() { + ExtBuilder::default().build().execute_with(|| { + // The caller didn't set a limit. + precompiles() + .prepare_test( + TestAccount::Alice, + PRECOMPILE_ADDRESS, + EvmDataWriter::new_with_selector(Action::XvmCall) + .write(0x1Fu8) + .write(Bytes( + hex::decode("0000000000000000000000000000000000000000") + .expect("invalid hex"), + )) + .write(Bytes(b"".to_vec())) + .write(U256::one()) + .build(), + ) + .expect_no_logs() + .execute_some(); + assert_eq!( + WeightLimitCalledWith::get(), + ::GasWeightMapping::gas_to_weight(u64::MAX, true) + ); + + // The caller set a limit. + let gas_limit = 1_000; + precompiles() + .prepare_test( + TestAccount::Alice, + PRECOMPILE_ADDRESS, + EvmDataWriter::new_with_selector(Action::XvmCall) + .write(0x1Fu8) + .write(Bytes( + hex::decode("0000000000000000000000000000000000000000") + .expect("invalid hex"), + )) + .write(Bytes(b"".to_vec())) + .write(U256::one()) + .build(), + ) + .with_gas_limit(gas_limit) + .expect_no_logs() + .execute_some(); + assert_eq!( + WeightLimitCalledWith::get(), + ::GasWeightMapping::gas_to_weight(gas_limit, true) + ); + }); +} diff --git a/primitives/src/xvm.rs b/primitives/src/xvm.rs index 38de257e27..4b08aa6967 100644 --- a/primitives/src/xvm.rs +++ b/primitives/src/xvm.rs @@ -61,11 +61,13 @@ pub enum CallError { /// Invalid VM id. InvalidVmId, /// Calling the contracts in the same VM is not allowed. - SameVmCallNotAllowed, + SameVmCallDenied, /// Target contract address is invalid. InvalidTarget, /// Input is too large. InputTooLarge, + /// Reentrance is not allowed. + ReentranceDenied, /// The call failed on EVM or WASM execution. ExecutionFailed(Vec), } diff --git a/runtime/local/src/lib.rs b/runtime/local/src/lib.rs index 765dc47b84..524b5085d0 100644 --- a/runtime/local/src/lib.rs +++ b/runtime/local/src/lib.rs @@ -466,11 +466,6 @@ impl pallet_ethereum_checked::Config for Runtime { type WeightInfo = pallet_ethereum_checked::weights::SubstrateWeight; } -parameter_types! { - pub EvmId: u8 = 0x0F; - pub WasmId: u8 = 0x1F; -} - impl pallet_xvm::Config for Runtime { type GasWeightMapping = pallet_evm::FixedGasWeightMapping; type AccountMapping = HashedAccountMapping; diff --git a/tests/integration/src/xvm.rs b/tests/integration/src/xvm.rs index a92ff4c382..fb3ba9b72b 100644 --- a/tests/integration/src/xvm.rs +++ b/tests/integration/src/xvm.rs @@ -69,6 +69,7 @@ mod payable { } */ +const WASM_PAYABLE_NAME: &'static str = "payable"; /* Call WASM payable: @@ -151,6 +152,7 @@ impl Environment for CustomEnvironment { } */ +const CALL_EVM_PAYBLE_NAME: &'static str = "call_xvm_payable"; #[test] fn evm_payable_call_via_xvm_works() { @@ -197,7 +199,7 @@ fn evm_payable_call_via_xvm_works() { #[test] fn wasm_payable_call_via_xvm_works() { new_test_ext().execute_with(|| { - let contract_addr = deploy_wasm_contract("payable"); + let contract_addr = deploy_wasm_contract(WASM_PAYABLE_NAME); let prev_balance = Balances::free_balance(&contract_addr); let value = UNIT; @@ -223,7 +225,7 @@ fn wasm_payable_call_via_xvm_works() { #[test] fn calling_wasm_payable_from_evm_fails_if_caller_contract_balance_below_ed() { new_test_ext().execute_with(|| { - let wasm_payable_addr = deploy_wasm_contract("payable"); + let wasm_payable_addr = deploy_wasm_contract(WASM_PAYABLE_NAME); let call_wasm_payable_addr = deploy_evm_contract(CALL_WASM_PAYBLE); let value = 1_000_000_000; @@ -262,7 +264,7 @@ fn calling_wasm_payable_from_evm_fails_if_caller_contract_balance_below_ed() { #[test] fn calling_wasm_payable_from_evm_works() { new_test_ext().execute_with(|| { - let wasm_payable_addr = deploy_wasm_contract("payable"); + let wasm_payable_addr = deploy_wasm_contract(WASM_PAYABLE_NAME); let call_wasm_payable_addr = deploy_evm_contract(CALL_WASM_PAYBLE); let _ = Balances::deposit_creating(&account_id_from(call_wasm_payable_addr.clone()), ExistentialDeposit::get()); @@ -293,7 +295,7 @@ fn calling_wasm_payable_from_evm_works() { fn calling_evm_payable_from_wasm_works() { new_test_ext().execute_with(|| { let evm_payable_addr = deploy_evm_contract(EVM_PAYABLE); - let wasm_address = deploy_wasm_contract("call_xvm_payable"); + let wasm_address = deploy_wasm_contract(CALL_EVM_PAYBLE_NAME); let value = UNIT; @@ -316,7 +318,7 @@ fn calling_evm_payable_from_wasm_works() { RuntimeOrigin::signed(ALICE), MultiAddress::Id(wasm_address.clone()), value, - Weight::from_parts(10_000_000_000, 10 * 1024 * 1024), + Weight::from_parts(10_000_000_000, 1024 * 1024), None, input, )); @@ -332,3 +334,45 @@ fn calling_evm_payable_from_wasm_works() { assert_eq!(Balances::free_balance(&mock_unified_wasm_account), 0); }); } + +#[test] +fn reentrance_not_allowed() { + new_test_ext().execute_with(|| { + // Call path: WASM -> EVM -> WASM + let call_evm_payable_address = deploy_wasm_contract(CALL_EVM_PAYBLE_NAME); + let call_wasm_payable_addr = deploy_evm_contract(CALL_WASM_PAYBLE); + let _ = deploy_wasm_contract(WASM_PAYABLE_NAME); + + // to: 0x00a8f69d59df362b69a8d4acdb9001eb3e1b8d067b8fdaa70081aed945bde5c48c + // input: 0x0000002a (deposit) + // value: 1000000000 + let call_wasm_payable_input = hex::decode("4012b914000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000003b9aca00000000000000000000000000000000000000000000000000000000000000002100a8f69d59df362b69a8d4acdb9001eb3e1b8d067b8fdaa70081aed945bde5c48c0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000002a00000000000000000000000000000000000000000000000000000000").expect("invalid call input hex"); + let input = hex::decode("0000002a") + .expect("invalid selector hex") + .iter() + .chain(call_wasm_payable_addr.as_ref().to_vec().encode().iter()) + .chain(call_wasm_payable_input.encode().iter()) + .cloned() + .collect::>(); + assert_ok!( + Contracts::call( + RuntimeOrigin::signed(ALICE), + MultiAddress::Id(call_evm_payable_address.clone()), + 0, + Weight::from_parts(10_000_000_000, 1024 * 1024), + None, + input, + ) + ); + + // TODO: after XVM error propagation finished, replace with assert `ReentranceDenied` + // error. + let wasm_entrance_count = System::events().iter().filter(|record| { + match record.event { + RuntimeEvent::Contracts(pallet_contracts::Event::Called { .. }) => true, + _ => false, + } + }).count(); + assert_eq!(wasm_entrance_count, 1); + }); +}