From 2b1cd7fc30579174dc4d89e78c3f0baf7d42b6d6 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:24:59 +0200 Subject: [PATCH] fix: origin nonce used in deploy tx & available balance in tx (#957) * fix: origin nonce used in deploy tx * fix: withdraw max_fee balance prior to tx * remove prints --- crates/evm/src/backend/starknet_backend.cairo | 2 +- .../environmental_information.cairo | 4 +- crates/evm/src/interpreter.cairo | 177 +++++++++++++++--- crates/evm/src/model.cairo | 2 +- crates/evm/src/model/account.cairo | 13 +- crates/evm/src/state.cairo | 2 +- crates/evm/src/test_utils.cairo | 11 +- 7 files changed, 174 insertions(+), 37 deletions(-) diff --git a/crates/evm/src/backend/starknet_backend.cairo b/crates/evm/src/backend/starknet_backend.cairo index 5298e5c36..105480c5c 100644 --- a/crates/evm/src/backend/starknet_backend.cairo +++ b/crates/evm/src/backend/starknet_backend.cairo @@ -71,7 +71,7 @@ pub fn get_bytecode(evm_address: EthAddress) -> Span { } /// Populate an Environment with Starknet syscalls. -pub fn get_env(origin: EthAddress, gas_price: u128) -> Environment { +pub fn get_env(origin: Address, gas_price: u128) -> Environment { let kakarot_state = KakarotCore::unsafe_new_contract_state(); let kakarot_storage = kakarot_state.snapshot_deref(); let block_info = get_block_info().unbox(); diff --git a/crates/evm/src/instructions/environmental_information.cairo b/crates/evm/src/instructions/environmental_information.cairo index 1f5bc4da6..bc209eec9 100644 --- a/crates/evm/src/instructions/environmental_information.cairo +++ b/crates/evm/src/instructions/environmental_information.cairo @@ -46,7 +46,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { /// # Specification: https://www.evm.codes/#32?fork=shanghai fn exec_origin(ref self: VM) -> Result<(), EVMError> { self.charge_gas(gas::BASE)?; - self.stack.push(self.env.origin.into()) + self.stack.push(self.env.origin.evm.into()) } /// 0x33 - CALLER @@ -448,7 +448,7 @@ mod tests { // Then assert_eq!(vm.stack.len(), 1); - assert_eq!(vm.stack.peek().unwrap(), origin().into()); + assert_eq!(vm.stack.peek().unwrap(), vm.env.origin.evm.into()); } diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index 5775b5fcd..cb7f0bfc6 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -1,3 +1,4 @@ +use contracts::account_contract::{IAccountDispatcher, IAccountDispatcherTrait}; use contracts::kakarot_core::KakarotCore; use contracts::kakarot_core::interface::IKakarotCore; use core::num::traits::Zero; @@ -16,7 +17,7 @@ use evm::instructions::{ MemoryOperationTrait, Sha3Trait }; -use evm::model::account::{AccountTrait}; +use evm::model::account::{Account, AccountTrait}; use evm::model::vm::{VM, VMTrait}; use evm::model::{ Message, Environment, Transfer, ExecutionSummary, ExecutionSummaryTrait, ExecutionResult, @@ -35,30 +36,22 @@ use utils::set::{Set, SetTrait}; #[generate_trait] pub impl EVMImpl of EVMTrait { - fn process_transaction( - ref self: KakarotCore::ContractState, origin: Address, tx: Transaction, intrinsic_gas: u64 - ) -> TransactionResult { - // Charge the cost of intrinsic gas - which has been verified to be <= gas_limit. - let block_base_fee = self.snapshot_deref().Kakarot_base_fee.read(); - let gas_price = tx.effective_gas_price(Option::Some(block_base_fee.into())); - let gas_left = tx.gas_limit() - intrinsic_gas; - let mut env = starknet_backend::get_env(origin.evm, gas_price); - - let mut sender_account = env.state.get_account(origin.evm); - sender_account.set_nonce(sender_account.nonce() + 1); - env.state.set_account(sender_account); - - // Handle deploy/non-deploy transaction cases + fn prepare_message( + self: @KakarotCore::ContractState, + tx: @Transaction, + sender_account: @Account, + ref env: Environment, + gas_left: u64 + ) -> (Message, bool) { let (to, is_deploy_tx, code, code_address, calldata) = match tx.kind() { TxKind::Create => { - // Deploy tx case. - let mut origin_nonce: u64 = get_tx_info().unbox().nonce.try_into().unwrap(); - let to_evm_address = compute_contract_address(origin.evm, origin_nonce); + let origin_nonce: u64 = sender_account.nonce(); + let to_evm_address = compute_contract_address( + sender_account.address().evm, origin_nonce + ); let to_starknet_address = self.compute_starknet_address(to_evm_address); let to = Address { evm: to_evm_address, starknet: to_starknet_address }; - let code = tx.input(); - let calldata = [].span(); - (to, true, code, Zero::zero(), calldata) + (to, true, tx.input(), Zero::zero(), [].span()) }, TxKind::Call(to) => { let target_starknet_address = self.compute_starknet_address(to); @@ -71,7 +64,7 @@ pub impl EVMImpl of EVMTrait { let mut accessed_addresses: Set = Default::default(); accessed_addresses.add(env.coinbase); accessed_addresses.add(to.evm); - accessed_addresses.add(origin.evm); + accessed_addresses.add(env.origin.evm); accessed_addresses.extend(eth_precompile_addresses().spanset()); let mut accessed_storage_keys: Set<(EthAddress, u256)> = Default::default(); @@ -86,7 +79,7 @@ pub impl EVMImpl of EVMTrait { }; let message = Message { - caller: origin, + caller: env.origin, target: to, gas_limit: gas_left, data: calldata, @@ -99,8 +92,44 @@ pub impl EVMImpl of EVMTrait { accessed_addresses: accessed_addresses.spanset(), accessed_storage_keys: accessed_storage_keys.spanset(), }; + + (message, is_deploy_tx) + } + + fn process_transaction( + ref self: KakarotCore::ContractState, origin: Address, tx: Transaction, intrinsic_gas: u64 + ) -> TransactionResult { + // Charge the cost of intrinsic gas - which has been verified to be <= gas_limit. + let block_base_fee = self.snapshot_deref().Kakarot_base_fee.read(); + let gas_price = tx.effective_gas_price(Option::Some(block_base_fee.into())); + let gas_left = tx.gas_limit() - intrinsic_gas; + let max_fee = tx.gas_limit().into() * gas_price; + let mut env = starknet_backend::get_env(origin, gas_price); + + let (message, is_deploy_tx) = { + let mut sender_account = env.state.get_account(origin.evm); + // Charge the intrinsic gas to the sender so that it's not available for the execution + // of the transaction but don't trigger any actual transfer, as only the actual consumde + // gas is charged at the end of the transaction + sender_account.set_balance(sender_account.balance() - max_fee.into()); + + let (message, is_deploy_tx) = self + .prepare_message(@tx, @sender_account, ref env, gas_left); + + // Increment nonce of sender AFTER computing eventual created address + sender_account.set_nonce(sender_account.nonce() + 1); + + env.state.set_account(sender_account); + (message, is_deploy_tx) + }; + let mut summary = Self::process_message_call(message, env, is_deploy_tx); + // Cancel the max_fee that was taken from the sender to prevent double charging + let mut sender_account = summary.state.get_account(origin.evm); + sender_account.set_balance(sender_account.balance() + max_fee.into()); + summary.state.set_account(sender_account); + // Gas refunds let gas_used = tx.gas_limit() - summary.gas_left; let gas_refund = core::cmp::min(gas_used / 5, summary.gas_refund); @@ -978,3 +1007,105 @@ pub impl EVMImpl of EVMTrait { return Result::Err(EVMError::InvalidOpcode(opcode)); } } + +#[cfg(test)] +mod tests { + use contracts::kakarot_core::KakarotCore; + use core::num::traits::Zero; + use evm::model::{Account, Environment, Message}; + use evm::state::StateTrait; + use evm::test_utils::{dual_origin, test_dual_address}; + use super::EVMTrait; + use utils::constants::EMPTY_KECCAK; + use utils::eth_transaction::common::TxKind; + use utils::eth_transaction::legacy::TxLegacy; + use utils::eth_transaction::transaction::{Transaction, TransactionTrait}; + + fn setup() -> (KakarotCore::ContractState, Account, Environment) { + let state = KakarotCore::contract_state_for_testing(); + let sender_account = Account { + address: test_dual_address(), + nonce: 5, + balance: 1000000000000000000_u256, // 1 ETH + code: array![].span(), + code_hash: EMPTY_KECCAK, + selfdestruct: false, + is_created: true, + }; + let mut env = Environment { + origin: dual_origin(), + gas_price: 20000000000_u128, // 20 Gwei + chain_id: 1_u64, + prevrandao: 0_u256, + block_number: 12345_u64, + block_gas_limit: 30000000_u64, + block_timestamp: 1634567890_u64, + coinbase: 0x0000000000000000000000000000000000000000.try_into().unwrap(), + base_fee: 0_u64, + state: Default::default(), + }; + env.state.set_account(sender_account); + (state, sender_account, env) + } + + #[test] + fn test_prepare_message_create() { + let (mut state, sender_account, mut env) = setup(); + let tx = Transaction::Legacy( + TxLegacy { + chain_id: Option::Some(1), + nonce: 5, + gas_price: 20000000000_u128, // 20 Gwei + gas_limit: 1000000_u64, + to: TxKind::Create, + value: 0_u256, + input: array![0x60, 0x80, 0x60, 0x40, 0x52].span(), // Simple contract bytecode + } + ); + + let (message, is_deploy_tx) = state + .prepare_message(@tx, @sender_account, ref env, tx.gas_limit()); + + assert_eq!(is_deploy_tx, true); + assert_eq!(message.code, tx.input()); + assert_eq!( + message.target.evm, 0xf50541960eec6df5caa295adee1a1a95c3c3241c.try_into().unwrap() + ); // compute_contract_address('evm_address', 5); + assert_eq!(message.code_address, Zero::zero()); + assert_eq!(message.data, [].span()); + assert_eq!(message.gas_limit, tx.gas_limit()); + assert_eq!(message.depth, 0); + assert_eq!(message.should_transfer_value, true); + assert_eq!(message.value, 0_u256); + } + + #[test] + fn test_prepare_message_call() { + let (mut state, sender_account, mut env) = setup(); + let target_address = sender_account.address; + let tx = Transaction::Legacy( + TxLegacy { + chain_id: Option::Some(1), + nonce: 5, + gas_price: 20000000000_u128, // 20 Gwei + gas_limit: 1000000_u64, + to: TxKind::Call(target_address.evm), + value: 1000000000000000000_u256, // 1 ETH + input: array![0x12, 0x34, 0x56, 0x78].span(), // Some calldata + } + ); + + let (message, is_deploy_tx) = state + .prepare_message(@tx, @sender_account, ref env, tx.gas_limit()); + + assert_eq!(is_deploy_tx, false); + assert_eq!(message.target.evm, target_address.evm); + assert_eq!(message.code_address.evm, target_address.evm); + assert_eq!(message.code, sender_account.code); + assert_eq!(message.data, tx.input()); + assert_eq!(message.gas_limit, tx.gas_limit()); + assert_eq!(message.depth, 0); + assert_eq!(message.should_transfer_value, true); + assert_eq!(message.value, 1000000000000000000_u256); + } +} diff --git a/crates/evm/src/model.cairo b/crates/evm/src/model.cairo index bf70863fe..97a64ec6f 100644 --- a/crates/evm/src/model.cairo +++ b/crates/evm/src/model.cairo @@ -17,7 +17,7 @@ use utils::traits::{EthAddressDefault, ContractAddressDefault, SpanDefault}; #[derive(Destruct, Default)] pub struct Environment { - pub origin: EthAddress, + pub origin: Address, pub gas_price: u128, pub chain_id: u64, pub prevrandao: u256, diff --git a/crates/evm/src/model/account.cairo b/crates/evm/src/model/account.cairo index 6494dc652..87a5b2fae 100644 --- a/crates/evm/src/model/account.cairo +++ b/crates/evm/src/model/account.cairo @@ -162,6 +162,11 @@ pub impl AccountImpl of AccountTrait { *self.balance } + #[inline(always)] + fn set_balance(ref self: Account, value: u256) { + self.balance = value; + } + #[inline(always)] fn address(self: @Account) -> Address { *self.address @@ -268,14 +273,6 @@ pub impl AccountImpl of AccountTrait { } } -#[generate_trait] -pub(crate) impl AccountInternals of AccountInternalTrait { - #[inline(always)] - fn set_balance(ref self: Account, value: u256) { - self.balance = value; - } -} - #[cfg(test)] mod tests { mod test_has_code_or_nonce { diff --git a/crates/evm/src/state.cairo b/crates/evm/src/state.cairo index 5414c2758..ed455d10a 100644 --- a/crates/evm/src/state.cairo +++ b/crates/evm/src/state.cairo @@ -8,7 +8,7 @@ use core::starknet::{EthAddress}; use evm::backend::starknet_backend::fetch_original_storage; use evm::errors::{ensure, EVMError, BALANCE_OVERFLOW}; -use evm::model::account::{AccountTrait, AccountInternalTrait}; +use evm::model::account::{AccountTrait}; use evm::model::{Event, Transfer, Account}; use utils::set::{Set, SetTrait}; diff --git a/crates/evm/src/test_utils.cairo b/crates/evm/src/test_utils.cairo index be3de3fad..90f0fc510 100644 --- a/crates/evm/src/test_utils.cairo +++ b/crates/evm/src/test_utils.cairo @@ -131,6 +131,14 @@ pub fn origin() -> EthAddress { 'origin'.try_into().unwrap() } +pub fn dual_origin() -> Address { + let origin_evm = origin(); + let origin_starknet = utils::helpers::compute_starknet_address( + test_address(), origin_evm, uninitialized_account() + ); + Address { evm: origin_evm, starknet: origin_starknet } +} + pub fn caller() -> EthAddress { 'caller'.try_into().unwrap() } @@ -253,8 +261,9 @@ pub fn preset_message() -> Message { pub fn preset_environment() -> Environment { let block_info = starknet::get_block_info().unbox(); + Environment { - origin: origin(), + origin: dual_origin(), gas_price: gas_price(), chain_id: chain_id(), prevrandao: 0,