From e27ea67883c4f206e68aed066e9960f241e8aae8 Mon Sep 17 00:00:00 2001 From: Michael Zaikin Date: Thu, 15 Aug 2024 13:52:16 +0100 Subject: [PATCH] EVM/Kernel: ensure FA deposit cannot invoke XTZ/FA withdrawal precompiles --- etherlink/CHANGES_KERNEL.md | 1 + .../evm_execution/src/fa_bridge/mod.rs | 3 +- .../evm_execution/src/fa_bridge/test_utils.rs | 7 +- .../evm_execution/src/fa_bridge/tests.rs | 77 +++++++++++++++++-- .../evm_execution/src/precompiles/mod.rs | 28 ++++--- etherlink/kernel_evm/kernel/src/apply.rs | 11 +-- 6 files changed, 103 insertions(+), 24 deletions(-) diff --git a/etherlink/CHANGES_KERNEL.md b/etherlink/CHANGES_KERNEL.md index d75c5ee3ce72..55fc8699b3e9 100644 --- a/etherlink/CHANGES_KERNEL.md +++ b/etherlink/CHANGES_KERNEL.md @@ -98,6 +98,7 @@ L1 blocks, without sacrificing decentralization, and with reduced costs. account. Execution forbid this. (!14317) - Tick model is updated with regards to FA deposits and withdrawals. (!14016) - Gas cost of XTZ/FA withdrawals is increased to prevent abuses. (!14016) +- FA deposits disallowed to invoke XTZ/FA withdrawal precompiles. (!14545) ## Version 4f4457e2527cb227a90bb1c56d3a83f39c0f78fd diff --git a/etherlink/kernel_evm/evm_execution/src/fa_bridge/mod.rs b/etherlink/kernel_evm/evm_execution/src/fa_bridge/mod.rs index 9a4dad75adc4..fcff26d70b3a 100644 --- a/etherlink/kernel_evm/evm_execution/src/fa_bridge/mod.rs +++ b/etherlink/kernel_evm/evm_execution/src/fa_bridge/mod.rs @@ -131,6 +131,7 @@ pub fn execute_fa_deposit<'a, Host: Runtime>( deposit: &FaDeposit, allocated_ticks: u64, tracer_input: Option, + gas_limit: u64, ) -> Result { log!(host, Info, "Going to execute a {}", deposit.display()); @@ -148,7 +149,7 @@ pub fn execute_fa_deposit<'a, Host: Runtime>( tracer_input, ); - handler.begin_initial_transaction(false, Some(FA_DEPOSIT_PROXY_GAS_LIMIT))?; + handler.begin_initial_transaction(false, Some(gas_limit))?; // It's ok if internal proxy call fails, we will update the ticket table anyways. let ticket_owner = if let Some(proxy) = deposit.proxy { diff --git a/etherlink/kernel_evm/evm_execution/src/fa_bridge/test_utils.rs b/etherlink/kernel_evm/evm_execution/src/fa_bridge/test_utils.rs index 51388c37a229..401ec407c9e5 100644 --- a/etherlink/kernel_evm/evm_execution/src/fa_bridge/test_utils.rs +++ b/etherlink/kernel_evm/evm_execution/src/fa_bridge/test_utils.rs @@ -154,9 +154,11 @@ pub fn run_fa_deposit( evm_account_storage: &mut EthereumAccountStorage, deposit: &FaDeposit, caller: &H160, + gas_limit: u64, + enable_fa_withdrawals: bool, ) -> ExecutionOutcome { let block = dummy_block_constants(); - let precompiles = precompile_set::(false); + let precompiles = precompile_set::(enable_fa_withdrawals); execute_fa_deposit( host, @@ -166,8 +168,9 @@ pub fn run_fa_deposit( Config::shanghai(), *caller, deposit, - 1_000_000_000, + 100_000_000_000, None, + gas_limit, ) .expect("Failed to execute deposit") } diff --git a/etherlink/kernel_evm/evm_execution/src/fa_bridge/tests.rs b/etherlink/kernel_evm/evm_execution/src/fa_bridge/tests.rs index ca7891f1722e..1b4a723c09e7 100644 --- a/etherlink/kernel_evm/evm_execution/src/fa_bridge/tests.rs +++ b/etherlink/kernel_evm/evm_execution/src/fa_bridge/tests.rs @@ -10,11 +10,16 @@ use tezos_evm_runtime::runtime::MockKernelHost; use crate::{ account_storage::{account_path, init_account_storage}, - fa_bridge::test_utils::{ - convert_h160, convert_log, convert_u256, deploy_mock_wrapper, dummy_fa_deposit, - dummy_fa_withdrawal, dummy_ticket, fa_bridge_precompile_call_withdraw, - get_storage_flag, kernel_wrapper, run_fa_deposit, ticket_balance_add, - ticket_balance_get, token_wrapper, withdrawal_counter_next, + fa_bridge::{ + deposit::ticket_hash, + test_utils::{ + convert_h160, convert_log, convert_u256, deploy_mock_wrapper, + deploy_reentrancy_tester, dummy_fa_deposit, dummy_fa_withdrawal, + dummy_ticket, fa_bridge_precompile_call_withdraw, get_storage_flag, + kernel_wrapper, run_fa_deposit, ticket_balance_add, ticket_balance_get, + token_wrapper, withdrawal_counter_next, + }, + FA_DEPOSIT_PROXY_GAS_LIMIT, }, handler::ExecutionResult, }; @@ -43,6 +48,8 @@ fn fa_deposit_reached_wrapper_contract() { &mut evm_account_storage, &deposit, &caller, + FA_DEPOSIT_PROXY_GAS_LIMIT, + false, ); assert!(res.is_success()); assert_eq!(2, res.logs.len()); @@ -113,6 +120,8 @@ fn fa_deposit_refused_due_non_existing_contract() { &mut evm_account_storage, &deposit, &caller, + FA_DEPOSIT_PROXY_GAS_LIMIT, + false, ); assert_eq!(1, res.logs.len()); @@ -177,6 +186,8 @@ fn fa_deposit_refused_non_compatible_interface() { &mut evm_account_storage, &deposit, &caller, + FA_DEPOSIT_PROXY_GAS_LIMIT, + false, ); assert_eq!(1, res.logs.len()); @@ -254,6 +265,8 @@ fn fa_deposit_proxy_state_reverted_if_ticket_balance_overflows() { &mut evm_account_storage, &deposit, &caller, + FA_DEPOSIT_PROXY_GAS_LIMIT, + false, ); assert!(!res.is_success()); assert!(res.logs.is_empty()); @@ -453,3 +466,57 @@ fn fa_withdrawal_fails_due_to_insufficient_balance() { matches!(res.result, ExecutionResult::Error(ExitError::Other(err)) if err.contains("Insufficient ticket balance")) ); } + +#[test] +fn fa_deposit_cannot_call_fa_withdrawal_precompile() { + let mut mock_runtime = MockKernelHost::default(); + let mut evm_account_storage = init_account_storage().unwrap(); + + let caller = H160::zero(); + let ticket = dummy_ticket(); + + let proxy = deploy_reentrancy_tester( + &mut mock_runtime, + &mut evm_account_storage, + &ticket, + &caller, + U256::one(), + U256::one(), + ) + .new_address() + .expect("Failed to deploy reentrancy tester"); + + ticket_balance_add( + &mut mock_runtime, + &mut evm_account_storage, + &ticket_hash(&ticket).unwrap(), + &proxy, + U256::one(), + ); + + let deposit = dummy_fa_deposit(ticket, Some(proxy)); + + // First let's show that it's possible to withdraw from the inner call + let res = run_fa_deposit( + &mut mock_runtime, + &mut evm_account_storage, + &deposit, + &caller, + 100_000_000, + true, + ); + assert!(res.is_success()); + assert!(!res.withdrawals.is_empty()); + + // Now let's do the same but without enabling the withdrawal precompile + let res = run_fa_deposit( + &mut mock_runtime, + &mut evm_account_storage, + &deposit, + &caller, + 100_000_000, + false, + ); + assert!(res.is_success()); + assert!(res.withdrawals.is_empty()); +} diff --git a/etherlink/kernel_evm/evm_execution/src/precompiles/mod.rs b/etherlink/kernel_evm/evm_execution/src/precompiles/mod.rs index fea4fa772972..85aa0e3ff50e 100644 --- a/etherlink/kernel_evm/evm_execution/src/precompiles/mod.rs +++ b/etherlink/kernel_evm/evm_execution/src/precompiles/mod.rs @@ -160,11 +160,10 @@ pub const WITHDRAWAL_ADDRESS: H160 = H160([ 0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, ]); -/// Factory function for generating the precompileset that the EVM kernel uses. -pub fn precompile_set( - enable_fa_withdrawals: bool, -) -> PrecompileBTreeMap { - let mut precompiles = BTreeMap::from([ +/// Factory function for generating the pure precompile set that does not mutate kernel state. +/// This does not include XTZ/FA withdrawal precompiles (for reentrancy protection). +pub fn pure_precompile_set() -> PrecompileBTreeMap { + BTreeMap::from([ ( H160::from_low_u64_be(1u64), ecrecover_precompile as PrecompileFn, @@ -201,11 +200,19 @@ pub fn precompile_set( H160::from_low_u64_be(9u64), blake2f_precompile as PrecompileFn, ), - ( - WITHDRAWAL_ADDRESS, - withdrawal_precompile as PrecompileFn, - ), - ]); + ]) +} + +/// Factory function for generating the precompileset that the EVM kernel uses. +pub fn precompile_set( + enable_fa_withdrawals: bool, +) -> PrecompileBTreeMap { + let mut precompiles = pure_precompile_set(); + + precompiles.insert( + WITHDRAWAL_ADDRESS, + withdrawal_precompile as PrecompileFn, + ); if enable_fa_withdrawals { precompiles.insert( @@ -213,7 +220,6 @@ pub fn precompile_set( fa_bridge_precompile as PrecompileFn, ); } - precompiles } diff --git a/etherlink/kernel_evm/kernel/src/apply.rs b/etherlink/kernel_evm/kernel/src/apply.rs index 7795a091850b..be0163319888 100644 --- a/etherlink/kernel_evm/kernel/src/apply.rs +++ b/etherlink/kernel_evm/kernel/src/apply.rs @@ -9,11 +9,11 @@ use ethereum::Log; use evm_execution::account_storage::{EthereumAccount, EthereumAccountStorage}; use evm_execution::fa_bridge::deposit::FaDeposit; -use evm_execution::fa_bridge::execute_fa_deposit; +use evm_execution::fa_bridge::{execute_fa_deposit, FA_DEPOSIT_PROXY_GAS_LIMIT}; use evm_execution::handler::{ ExecutionOutcome, ExecutionResult as ExecutionOutcomeResult, RouterInterface, }; -use evm_execution::precompiles::PrecompileBTreeMap; +use evm_execution::precompiles::{self, PrecompileBTreeMap}; use evm_execution::run_transaction; use evm_execution::storage::tracer; use evm_execution::trace::TracerInput::CallTracer; @@ -463,22 +463,24 @@ fn apply_fa_deposit( evm_account_storage: &mut EthereumAccountStorage, fa_deposit: &FaDeposit, block_constants: &BlockConstants, - precompiles: &PrecompileBTreeMap, allocated_ticks: u64, transaction: &Transaction, tracer_input: Option, ) -> Result, Error> { let caller = H160::zero(); + // Prevent inner calls to XTZ/FA withdrawal precompiles + let precompiles = precompiles::pure_precompile_set(); let outcome = execute_fa_deposit( host, block_constants, evm_account_storage, - precompiles, + &precompiles, CONFIG, caller, fa_deposit, allocated_ticks, tracer_input, + FA_DEPOSIT_PROXY_GAS_LIMIT, ) .map_err(Error::InvalidRunTransaction)?; @@ -648,7 +650,6 @@ pub fn apply_transaction( evm_account_storage, fa_deposit, block_constants, - precompiles, allocated_ticks, transaction, tracer_input, -- GitLab