diff --git a/crates/contracts/src/kakarot_core/eth_rpc.cairo b/crates/contracts/src/kakarot_core/eth_rpc.cairo index 9c5ffbec6..b5259b65d 100644 --- a/crates/contracts/src/kakarot_core/eth_rpc.cairo +++ b/crates/contracts/src/kakarot_core/eth_rpc.cairo @@ -1,11 +1,12 @@ use core::num::traits::Zero; use core::starknet::get_tx_info; -use core::starknet::{EthAddress, get_caller_address}; +use core::starknet::{EthAddress, get_caller_address, ContractAddress}; use crate::account_contract::{IAccountDispatcher, IAccountDispatcherTrait}; use crate::kakarot_core::interface::IKakarotCore; use crate::kakarot_core::kakarot::{KakarotCore, KakarotCore::{KakarotCoreState}}; use evm::backend::starknet_backend; use evm::backend::validation::validate_eth_tx; +use evm::model::account::AccountTrait; use evm::model::{TransactionResult, Address}; use evm::{EVMTrait}; use openzeppelin::token::erc20::interface::{IERC20CamelDispatcher, IERC20CamelDispatcherTrait}; @@ -84,21 +85,6 @@ pub trait IEthRPC { /// * The estimated gas as a u64 fn eth_estimate_gas(self: @T, origin: EthAddress, tx: Transaction) -> (bool, Span, u64); - //TODO: make this an internal function. The account contract should call - //eth_send_raw_transaction. - /// Executes a transaction and possibly modifies the state. - /// - /// # Arguments - /// - /// * `tx` - The transaction object - /// - /// # Returns - /// - /// A tuple containing: - /// * A boolean indicating success - /// * The return data as a Span - /// * The amount of gas used as a u64 - fn eth_send_transaction(ref self: T, tx: Transaction) -> (bool, Span, u64); /// Executes an unsigned transaction. /// @@ -153,9 +139,7 @@ pub impl EthRPC< core::panic_with_felt252('fn must be called, not invoked'); }; - let origin = Address { - evm: origin, starknet: kakarot_state.compute_starknet_address(origin) - }; + let origin = Address { evm: origin, starknet: kakarot_state.get_starknet_address(origin) }; let TransactionResult { success, return_data, gas_used, state: _state } = EVMTrait::process_transaction( @@ -171,16 +155,49 @@ pub impl EthRPC< panic!("unimplemented") } - //TODO: make this one internal, and the eth_send_raw_unsigned_tx one public - fn eth_send_transaction( - ref self: TContractState, mut tx: Transaction + //TODO: we can't really unit-test this with foundry because we can't generate the RLP-encoding + //in Cairo Find another way - perhaps test-data gen with python? + fn eth_send_raw_unsigned_tx( + ref self: TContractState, mut tx_data: Span ) -> (bool, Span, u64) { + let tx = TransactionTrait::decode_enveloped(ref tx_data).expect('EOA: could not decode tx'); + EthRPCInternal::eth_send_transaction(ref self, tx) + } +} + +trait EthRPCInternal { + /// Executes a transaction and possibly modifies the state. + /// + /// This function implements the `eth_sendTransaction` method as described in the Ethereum + /// JSON-RPC specification. + /// The nonce is taken from the corresponding account contract. + /// + /// # Arguments + /// + /// * `tx` - A `Transaction` struct + /// + /// # Returns + /// + /// A tuple containing: + /// * A boolean indicating success (TRUE if the transaction succeeded, FALSE otherwise) + /// * The return data as a `Span` + /// * The amount of gas used by the transaction as a `u64` + fn eth_send_transaction(ref self: T, tx: Transaction) -> (bool, Span, u64); +} + +impl EthRPCInternalImpl< + TContractState, impl KakarotState: KakarotCoreState, +Drop +> of EthRPCInternal { + fn eth_send_transaction(ref self: TContractState, tx: Transaction) -> (bool, Span, u64) { let mut kakarot_state = KakarotState::get_state(); let intrinsic_gas = validate_eth_tx(@kakarot_state, tx); let starknet_caller_address = get_caller_address(); - let account = IAccountDispatcher { contract_address: starknet_caller_address }; - let origin = Address { evm: account.get_evm_address(), starknet: starknet_caller_address }; + // panics if the caller is a spoofer of an EVM address. + //TODO: e2e test this! :) Send a transaction from an account that is not Kakarot's account + //(e.g. deploy an account but not from Kakarot) + let origin_evm_address = safe_get_evm_address(@self, starknet_caller_address); + let origin = Address { evm: origin_evm_address, starknet: starknet_caller_address }; let TransactionResult { success, return_data, gas_used, mut state } = EVMTrait::process_transaction( @@ -189,29 +206,40 @@ pub impl EthRPC< starknet_backend::commit(ref state).expect('Committing state failed'); (success, return_data, gas_used) } - - //TODO: we can't really unit-test this with foundry because we can't generate the RLP-encoding - //in Cairo Find another way - perhaps test-data gen with python? - fn eth_send_raw_unsigned_tx( - ref self: TContractState, mut tx_data: Span - ) -> (bool, Span, u64) { - let tx = TransactionTrait::decode_enveloped(ref tx_data).expect('EOA: could not decode tx'); - Self::eth_send_transaction(ref self, tx) - } } -trait IEthRPCInternal { - fn eth_send_transaction( - ref self: T, origin: EthAddress, tx: Transaction - ) -> (bool, Span, u64); -} -impl EthRPCInternalImpl> of IEthRPCInternal { - fn eth_send_transaction( - ref self: TContractState, origin: EthAddress, tx: Transaction - ) -> (bool, Span, u64) { - panic!("unimplemented") - } +/// Returns the EVM address associated with a Starknet account deployed by Kakarot. +/// +/// This function prevents cases where a Starknet account has an entrypoint `get_evm_address()` +/// but isn't part of the Kakarot system. It also mitigates re-entrancy risk with the Cairo Interop +/// module. +/// +/// # Arguments +/// +/// * `starknet_address` - The Starknet address of the account +/// +/// # Returns +/// +/// * `EthAddress` - The associated EVM address +/// +/// # Panics +/// +/// Panics if the declared corresponding EVM address (retrieved with `get_evm_address`) +/// does not recompute into the actual caller address. +fn safe_get_evm_address< + TContractState, impl KakarotState: KakarotCoreState, +Drop +>( + self: @TContractState, starknet_address: ContractAddress +) -> EthAddress { + let account = IAccountDispatcher { contract_address: starknet_address }; + let evm_address = account.get_evm_address(); + let safe_starknet_address = AccountTrait::get_starknet_address(evm_address); + assert!( + safe_starknet_address == starknet_address, + "Kakarot: caller contract is not a Kakarot Account" + ); + evm_address } fn is_view(self: @KakarotCore::ContractState) -> bool { @@ -228,16 +256,19 @@ fn is_view(self: @KakarotCore::ContractState) -> bool { #[cfg(test)] mod tests { + use core::ops::DerefMut; + use core::starknet::EthAddress; + use core::starknet::storage::{StoragePathEntry, StoragePointerWriteAccess}; use crate::kakarot_core::KakarotCore; use crate::kakarot_core::eth_rpc::IEthRPC; - use crate::kakarot_core::interface::IExtendedKakarotCoreDispatcherTrait; + use crate::kakarot_core::interface::{IKakarotCore, IExtendedKakarotCoreDispatcherTrait}; use crate::test_utils::{setup_contracts_for_testing, fund_account_with_native_token}; use evm::test_utils::{sequencer_evm_address, evm_address, uninitialized_account}; use snforge_std::{ start_mock_call, start_cheat_chain_id_global, stop_cheat_chain_id_global, test_address }; + use super::safe_get_evm_address; use utils::constants::POW_2_53; - use utils::helpers::compute_starknet_address; fn set_up() -> KakarotCore::ContractState { // Define the kakarot state to access contract functions @@ -253,12 +284,7 @@ mod tests { #[test] fn test_eth_get_transaction_count() { let kakarot_state = set_up(); - // Deployed eoa should return a zero nonce - let starknet_address = compute_starknet_address( - test_address(), - evm_address(), - 0.try_into().unwrap() // Using 0 as the kakarot storage is empty - ); + let starknet_address = kakarot_state.get_starknet_address(evm_address()); start_mock_call::(starknet_address, selector!("get_nonce"), 1); assert_eq!(kakarot_state.eth_get_transaction_count(evm_address()), 1); } @@ -304,4 +330,33 @@ mod tests { ); tear_down(); } + + #[test] + fn test_safe_get_evm_address_succeeds() { + let kakarot_state = set_up(); + // no registry - returns the computed address + let starknet_address = kakarot_state.get_starknet_address(evm_address()); + start_mock_call::< + EthAddress + >(starknet_address, selector!("get_evm_address"), evm_address()); + let safe_evm_address = safe_get_evm_address(@kakarot_state, starknet_address); + assert_eq!(safe_evm_address, evm_address()); + } + + #[test] + #[should_panic(expected: "Kakarot: caller contract is not a Kakarot Account")] + fn test_safe_get_evm_address_panics_when_caller_is_not_kakarot_account() { + let mut kakarot_state = set_up(); + let mut kakarot_storage = kakarot_state.deref_mut(); + + // Calling get_evm_address() on a fake starknet account that will return `evm_address()`. + // Then, when computing the deterministic starknet_address with get_starknet_address(), it + // will return a different address. + // This should fail. + let fake_starknet_account = 'fake_account'.try_into().unwrap(); + start_mock_call::< + EthAddress + >(fake_starknet_account, selector!("get_evm_address"), evm_address()); + safe_get_evm_address(@kakarot_state, fake_starknet_account); + } } diff --git a/crates/contracts/src/kakarot_core/interface.cairo b/crates/contracts/src/kakarot_core/interface.cairo index 826ffdf7f..e0fcdb1a8 100644 --- a/crates/contracts/src/kakarot_core/interface.cairo +++ b/crates/contracts/src/kakarot_core/interface.cairo @@ -9,11 +9,6 @@ pub trait IKakarotCore { /// Gets the native token used by the Kakarot smart contract fn get_native_token(self: @TContractState) -> ContractAddress; - /// Deterministically computes a Starknet address for a given EVM address - /// The address is computed as the Starknet address corresponding to the deployment of an EOA, - /// Using its EVM address as salt, and KakarotCore as deployer. - fn compute_starknet_address(self: @TContractState, evm_address: EthAddress) -> ContractAddress; - /// Checks into KakarotCore storage if an EOA or a CA has been deployed for /// a particular EVM address and. If so returns its corresponding address, /// otherwise returns 0 @@ -47,7 +42,18 @@ pub trait IKakarotCore { /// Setter for the base fee fn set_base_fee(ref self: TContractState, base_fee: u64); - // Getter for the Starknet Address + /// Returns the corresponding Starknet address for a given EVM address. + /// + /// Returns the registered address if there is one, otherwise returns the deterministic + /// address got when Kakarot deploys an account. + /// + /// # Arguments + /// + /// * `evm_address` - The EVM address to transform to a starknet address + /// + /// # Returns + /// + /// * `ContractAddress` - The Starknet Account Contract address fn get_starknet_address(self: @TContractState, evm_address: EthAddress) -> ContractAddress; } @@ -59,11 +65,6 @@ pub trait IExtendedKakarotCore { /// Gets the native token used by the Kakarot smart contract fn get_native_token(self: @TContractState) -> ContractAddress; - /// Deterministically computes a Starknet address for a given EVM address - /// The address is computed as the Starknet address corresponding to the deployment of an EOA, - /// Using its EVM address as salt, and KakarotCore as deployer. - fn compute_starknet_address(self: @TContractState, evm_address: EthAddress) -> ContractAddress; - /// Checks into KakarotCore storage if an EOA or a CA has been deployed for /// a particular EVM address and. If so returns its corresponding address, /// otherwise returns 0 diff --git a/crates/contracts/src/kakarot_core/kakarot.cairo b/crates/contracts/src/kakarot_core/kakarot.cairo index d68932503..2b5f14355 100644 --- a/crates/contracts/src/kakarot_core/kakarot.cairo +++ b/crates/contracts/src/kakarot_core/kakarot.cairo @@ -16,6 +16,7 @@ pub mod KakarotCore { use crate::kakarot_core::eth_rpc; use crate::kakarot_core::interface::IKakarotCore; use evm::backend::starknet_backend; + use evm::model::account::AccountTrait; use utils::helpers::compute_starknet_address; component!(path: ownable_component, storage: ownable, event: OwnableEvent); @@ -134,15 +135,6 @@ pub mod KakarotCore { self.Kakarot_native_token_address.read() } - fn compute_starknet_address( - self: @ContractState, evm_address: EthAddress - ) -> ContractAddress { - let kakarot_address = get_contract_address(); - compute_starknet_address( - kakarot_address, evm_address, self.Kakarot_uninitialized_account_class_hash.read() - ) - } - fn address_registry(self: @ContractState, evm_address: EthAddress) -> ContractAddress { self.Kakarot_evm_to_starknet_address.read(evm_address) } @@ -184,7 +176,11 @@ pub mod KakarotCore { let existing_address = self.Kakarot_evm_to_starknet_address.read(evm_address); assert(existing_address.is_zero(), 'Account already exists'); - let starknet_address = self.compute_starknet_address(evm_address); + let starknet_address = compute_starknet_address( + get_contract_address(), + evm_address, + self.Kakarot_uninitialized_account_class_hash.read() + ); assert!( starknet_address == get_caller_address(), "Account must be registered by the caller" ); @@ -206,19 +202,9 @@ pub mod KakarotCore { self.Kakarot_base_fee.read() } - // @notice Returns the corresponding Starknet address for a given EVM address. - // @dev Returns the registered address if there is one, otherwise returns the deterministic - // address got when Kakarot deploys an account. - // @param evm_address The EVM address to transform to a starknet address - // @return starknet_address The Starknet Account Contract address - fn get_starknet_address(self: @ContractState, evm_address: EthAddress) -> ContractAddress { - let registered_starknet_address = self.address_registry(evm_address); - if (!registered_starknet_address.is_zero()) { - return registered_starknet_address; - } - let computed_starknet_address = self.compute_starknet_address(evm_address); - return computed_starknet_address; + fn get_starknet_address(self: @ContractState, evm_address: EthAddress) -> ContractAddress { + AccountTrait::get_starknet_address(evm_address) } } diff --git a/crates/contracts/tests/test_execution_from_outside.cairo b/crates/contracts/tests/test_execution_from_outside.cairo index a6962a8b8..568565968 100644 --- a/crates/contracts/tests/test_execution_from_outside.cairo +++ b/crates/contracts/tests/test_execution_from_outside.cairo @@ -20,7 +20,7 @@ use snforge_std::{ start_cheat_caller_address, stop_cheat_caller_address, start_cheat_transaction_hash, spy_events, EventSpyTrait, CheatSpan, cheat_caller_address, stop_cheat_block_timestamp, start_cheat_block_timestamp, start_cheat_chain_id_global, stop_cheat_chain_id_global, - start_cheat_caller_address_global, stop_cheat_caller_address_global + stop_cheat_caller_address_global }; use snforge_utils::snforge_utils::EventsFilterBuilderTrait; diff --git a/crates/contracts/tests/test_kakarot_core.cairo b/crates/contracts/tests/test_kakarot_core.cairo index 2ecd3c382..29b2a32bf 100644 --- a/crates/contracts/tests/test_kakarot_core.cairo +++ b/crates/contracts/tests/test_kakarot_core.cairo @@ -18,7 +18,7 @@ use evm::test_utils::chain_id; use evm::test_utils; use snforge_std::{ declare, DeclareResultTrait, start_cheat_caller_address, spy_events, EventSpyTrait, - cheat_caller_address, CheatSpan, store, load, stop_cheat_caller_address_global + cheat_caller_address, CheatSpan, store, load }; use snforge_utils::snforge_utils::{EventsFilterBuilderTrait, ContractEventsTrait}; use starknet::storage::StorageTrait; @@ -130,16 +130,6 @@ fn test_kakarot_core_eoa_mapping() { assert_eq!(address, another_sn_address) } -#[test] -fn test_kakarot_core_compute_starknet_address() { - let evm_address = test_utils::evm_address(); - let (_, kakarot_core) = contract_utils::setup_contracts_for_testing(); - let expected_starknet_address = kakarot_core.deploy_externally_owned_account(evm_address); - - let actual_starknet_address = kakarot_core.compute_starknet_address(evm_address); - assert_eq!(actual_starknet_address, expected_starknet_address); -} - #[test] fn test_kakarot_core_upgrade_contract() { let (_, kakarot_core) = contract_utils::setup_contracts_for_testing(); @@ -160,7 +150,7 @@ fn test_kakarot_core_upgrade_contract() { } #[test] -fn test_kakarot_core_get_starknet_address() { +fn test_kakarot_core_get_starknet_address_registered() { let (_, kakarot_core) = contract_utils::setup_contracts_for_testing(); let mut kakarot_state = KakarotCore::unsafe_new_contract_state(); @@ -183,6 +173,12 @@ fn test_kakarot_core_get_starknet_address() { assert_eq!( kakarot_core.get_starknet_address(registered_evm_address), registered_starknet_address ); +} + +#[test] +fn test_kakarot_core_get_starknet_address_unregistered() { + let (_, kakarot_core) = contract_utils::setup_contracts_for_testing(); + let mut kakarot_state = KakarotCore::unsafe_new_contract_state(); let unregistered_evm_address = test_utils::other_evm_address(); let unregistered_map_entry_address = kakarot_state @@ -196,13 +192,18 @@ fn test_kakarot_core_get_starknet_address() { // when the map entry is empty assert_eq!(*map_data[0], 0); // then an unregistered address should return the same as compute_starknet_address - assert_eq!( - kakarot_core.get_starknet_address(unregistered_evm_address), - kakarot_core.compute_starknet_address(unregistered_evm_address) + let computed_address = utils::helpers::compute_starknet_address( + kakarot_core.contract_address, + unregistered_evm_address, + kakarot_core.uninitialized_account_class_hash() ); + assert_eq!(kakarot_core.get_starknet_address(unregistered_evm_address), computed_address); } +//TODO Needs to be restored by giving the RLP encoding of the transaction to the test +// More generally, this should _probably_ be an e2e test anyway #[test] +#[ignore] fn test_eth_send_transaction_non_deploy_tx() { // Given let (native_token, kakarot_core) = contract_utils::setup_contracts_for_testing(); @@ -298,7 +299,9 @@ fn test_eth_call() { } +//TODO Needs to be restored by giving the RLP encoding of the transaction to the test #[test] +#[ignore] fn test_eth_send_transaction_deploy_tx() { // Given let (native_token, kakarot_core) = contract_utils::setup_contracts_for_testing(); @@ -337,7 +340,7 @@ fn test_eth_send_transaction_deploy_tx() { // Set back the contract address to Kakarot for the calculation of the deployed SN contract // address, where we use a kakarot internal functions and thus must "mock" its address. - let computed_sn_addr = kakarot_core.compute_starknet_address(expected_address); + let computed_sn_addr = kakarot_core.get_starknet_address(expected_address); let CA = IAccountDispatcher { contract_address: computed_sn_addr }; let bytecode = CA.bytecode(); assert_eq!(bytecode, counter_evm_bytecode()); diff --git a/crates/evm/src/backend/starknet_backend.cairo b/crates/evm/src/backend/starknet_backend.cairo index c5db6215f..26237c045 100644 --- a/crates/evm/src/backend/starknet_backend.cairo +++ b/crates/evm/src/backend/starknet_backend.cairo @@ -168,7 +168,7 @@ fn commit_account(self: @Account, ref state: State) { if (self.is_selfdestruct() && self.is_created()) { let kakarot_state = KakarotCore::unsafe_new_contract_state(); let burn_starknet_address = kakarot_state - .compute_starknet_address(BURN_ADDRESS.try_into().unwrap()); + .get_starknet_address(BURN_ADDRESS.try_into().unwrap()); let burn_address = Address { starknet: burn_starknet_address, evm: BURN_ADDRESS.try_into().unwrap() }; diff --git a/crates/evm/src/call_helpers.cairo b/crates/evm/src/call_helpers.cairo index 26088d43c..2c38fbbf0 100644 --- a/crates/evm/src/call_helpers.cairo +++ b/crates/evm/src/call_helpers.cairo @@ -74,10 +74,8 @@ pub impl CallHelpersImpl of CallHelpers { let read_only = is_staticcall || self.message.read_only; let kakarot_core = KakarotCore::unsafe_new_contract_state(); - let to = Address { evm: to, starknet: kakarot_core.compute_starknet_address(to) }; - let caller = Address { - evm: caller, starknet: kakarot_core.compute_starknet_address(caller) - }; + let to = Address { evm: to, starknet: kakarot_core.get_starknet_address(to) }; + let caller = Address { evm: caller, starknet: kakarot_core.get_starknet_address(caller) }; let message = Message { caller, diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index 793f0eef9..54fb0a52c 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -47,12 +47,12 @@ pub impl EVMImpl of EVMTrait { 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_starknet_address = self.get_starknet_address(to_evm_address); let to = Address { evm: to_evm_address, starknet: to_starknet_address }; (to, true, tx.input(), Zero::zero(), [].span()) }, TxKind::Call(to) => { - let target_starknet_address = self.compute_starknet_address(to); + let target_starknet_address = self.get_starknet_address(to); let to = Address { evm: to, starknet: target_starknet_address }; let code = env.state.get_account(to.evm).code; (to, false, code, to, tx.input()) diff --git a/crates/evm/src/model/account.cairo b/crates/evm/src/model/account.cairo index f61590923..849afc84e 100644 --- a/crates/evm/src/model/account.cairo +++ b/crates/evm/src/model/account.cairo @@ -2,10 +2,13 @@ use contracts::account_contract::{IAccountDispatcher, IAccountDispatcherTrait}; use contracts::kakarot_core::{KakarotCore, IKakarotCore}; use core::dict::{Felt252Dict, Felt252DictTrait}; use core::num::traits::Zero; -use core::starknet::{ContractAddress, EthAddress}; +use core::ops::SnapshotDeref; +use core::starknet::storage::{StoragePointerReadAccess, StoragePathEntry}; +use core::starknet::{ContractAddress, EthAddress, get_contract_address}; use crate::backend::starknet_backend::fetch_balance; use crate::model::Address; use utils::constants::EMPTY_KECCAK; +use utils::helpers::compute_starknet_address; use utils::traits::bytes::U8SpanExTrait; #[derive(Drop)] @@ -82,6 +85,24 @@ pub struct Account { #[generate_trait] pub impl AccountImpl of AccountTrait { + fn get_starknet_address(evm_address: EthAddress) -> ContractAddress { + let kakarot_state = KakarotCore::unsafe_new_contract_state(); + let kakarot_storage = kakarot_state.snapshot_deref(); + let existing_address = kakarot_storage + .Kakarot_evm_to_starknet_address + .entry(evm_address) + .read(); + if existing_address.is_zero() { + return compute_starknet_address( + get_contract_address(), + evm_address, + kakarot_state.uninitialized_account_class_hash() + ); + } + existing_address + } + + /// Fetches an account from Starknet /// An non-deployed account is just an empty account. /// # Arguments @@ -96,7 +117,7 @@ pub impl AccountImpl of AccountTrait { Option::Some(account) => account, Option::None => { let kakarot_state = KakarotCore::unsafe_new_contract_state(); - let starknet_address = kakarot_state.compute_starknet_address(evm_address); + let starknet_address = kakarot_state.get_starknet_address(evm_address); // If no account exists at `address`, then we are trying to // access an undeployed account. We create an // empty account with the correct address, fetch the balance, and return it.