From de5b7d8fe843f11589ca5d0b33671f3b840f36a1 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Fri, 5 Jul 2024 11:30:32 +0200 Subject: [PATCH] feat: whitelist 'compute_starknet_address' from reentrancy (#1254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Time spent on this PR: ## Pull request type Please check the type of change your PR introduces: - [ ] Bugfix - [ ] Feature - [ ] Code style update (formatting, renaming) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] Documentation content changes - [ ] Other (please describe): ## What is the current behavior? Resolves # ## What is the new behavior? - - - - - - This change is [Reviewable](https://reviewable.io/reviews/kkrt-labs/kakarot/1254) --- src/kakarot/accounts/account_contract.cairo | 15 ++++++++--- src/kakarot/errors.cairo | 26 +++++++++++++++++++ .../precompiles/kakarot_precompiles.cairo | 23 +++++++++------- src/kakarot/precompiles/precompiles.cairo | 2 +- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/kakarot/accounts/account_contract.cairo b/src/kakarot/accounts/account_contract.cairo index ebe305301..38527e8d7 100644 --- a/src/kakarot/accounts/account_contract.cairo +++ b/src/kakarot/accounts/account_contract.cairo @@ -16,6 +16,8 @@ from kakarot.accounts.library import ( Account_authorized_message_hashes, ) from kakarot.accounts.model import CallArray +from utils.utils import Helpers +from kakarot.errors import Errors from kakarot.interfaces.interfaces import IKakarot, IAccount from starkware.starknet.common.syscalls import ( get_tx_info, @@ -26,6 +28,8 @@ from starkware.starknet.common.syscalls import ( from starkware.cairo.common.math import assert_le, unsigned_div_rem from starkware.cairo.common.alloc import alloc +const COMPUTE_STARKNET_ADDRESS_SELECTOR = 0x0ad7772990f7f5a506d84e5723efd1242e989c23f45653870d49d6d107f6e7; + // @title EVM smart contract account representation. @constructor func constructor{ @@ -317,9 +321,14 @@ func execute_starknet_call{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range ) -> (retdata_len: felt, retdata: felt*, success: felt) { Ownable.assert_only_owner(); let (kakarot_address) = Ownable.owner(); - if (kakarot_address == to) { - let (retdata) = alloc(); - return (0, retdata, FALSE); + let is_compute_starknet_address = Helpers.is_zero( + COMPUTE_STARKNET_ADDRESS_SELECTOR - function_selector + ); + let is_kakarot = Helpers.is_zero(kakarot_address - to); + tempvar is_forbidden = is_kakarot * (1 - is_compute_starknet_address); + if (is_forbidden != FALSE) { + let (error_len, error) = Errors.kakarotReentrancy(); + return (error_len, error, FALSE); } let (retdata_len, retdata) = call_contract(to, function_selector, calldata_len, calldata); return (retdata_len, retdata, TRUE); diff --git a/src/kakarot/errors.cairo b/src/kakarot/errors.cairo index 5a832bdfa..04b6fdd8b 100644 --- a/src/kakarot/errors.cairo +++ b/src/kakarot/errors.cairo @@ -744,4 +744,30 @@ namespace Errors { dw 'o'; dw 'r'; } + + func kakarotReentrancy() -> (error_len: felt, error: felt*) { + let (error) = get_label_location(kakarot_reentrancy_error_message); + return (19, error); + + kakarot_reentrancy_error_message: + dw 'K'; + dw 'a'; + dw 'k'; + dw 'a'; + dw 'r'; + dw 'o'; + dw 't'; + dw ':'; + dw ' '; + dw 'r'; + dw 'e'; + dw 'e'; + dw 'n'; + dw 't'; + dw 'r'; + dw 'a'; + dw 'n'; + dw 'c'; + dw 'y'; + } } diff --git a/src/kakarot/precompiles/kakarot_precompiles.cairo b/src/kakarot/precompiles/kakarot_precompiles.cairo index 744d981dd..03c6f49cb 100644 --- a/src/kakarot/precompiles/kakarot_precompiles.cairo +++ b/src/kakarot/precompiles/kakarot_precompiles.cairo @@ -6,7 +6,7 @@ from starkware.cairo.common.math import assert_not_zero from starkware.cairo.common.alloc import alloc from starkware.starknet.common.syscalls import call_contract, library_call, get_caller_address from starkware.starknet.common.messages import send_message_to_l1 -from starkware.cairo.common.bool import FALSE +from starkware.cairo.common.bool import FALSE, TRUE from kakarot.errors import Errors from kakarot.interfaces.interfaces import IAccount @@ -48,9 +48,7 @@ namespace KakarotPrecompiles { let is_input_invalid = is_le(input_len, 99); if (is_input_invalid != 0) { let (revert_reason_len, revert_reason) = Errors.outOfBoundsRead(); - return ( - revert_reason_len, revert_reason, CAIRO_PRECOMPILE_GAS, Errors.EXCEPTIONAL_HALT - ); + return (revert_reason_len, revert_reason, CAIRO_PRECOMPILE_GAS, TRUE); } // Input is formatted as: @@ -100,10 +98,15 @@ namespace KakarotPrecompiles { let (retdata_len, retdata, success) = IAccount.execute_starknet_call( caller_starknet_address, to_starknet_address, starknet_selector, data_len, data ); + if (success == FALSE) { + // skip formatting to bytes32 array and return revert reason directly + return (retdata_len, retdata, CAIRO_PRECOMPILE_GAS, TRUE); + } + let (output) = alloc(); let output_len = retdata_len * 32; Helpers.felt_array_to_bytes32_array(retdata_len, retdata, output); - return (output_len, output, CAIRO_PRECOMPILE_GAS, 1 - success); + return (output_len, output, CAIRO_PRECOMPILE_GAS, FALSE); } if (selector == LIBRARY_CALL_SOLIDITY_SELECTOR) { @@ -113,11 +116,11 @@ namespace KakarotPrecompiles { let (output) = alloc(); let output_len = retdata_len * 32; Helpers.felt_array_to_bytes32_array(retdata_len, retdata, output); - return (output_len, output, CAIRO_PRECOMPILE_GAS, 0); + return (output_len, output, CAIRO_PRECOMPILE_GAS, FALSE); } let (revert_reason_len, revert_reason) = Errors.invalidCairoSelector(); - return (revert_reason_len, revert_reason, CAIRO_PRECOMPILE_GAS, Errors.EXCEPTIONAL_HALT); + return (revert_reason_len, revert_reason, CAIRO_PRECOMPILE_GAS, TRUE); } // @notice Sends a message to a message to L1. @@ -138,7 +141,7 @@ namespace KakarotPrecompiles { let is_input_invalid = is_le(input_len, 95); if (is_input_invalid != 0) { let (revert_reason_len, revert_reason) = Errors.outOfBoundsRead(); - return (revert_reason_len, revert_reason, CAIRO_MESSAGE_GAS, Errors.EXCEPTIONAL_HALT); + return (revert_reason_len, revert_reason, CAIRO_MESSAGE_GAS, TRUE); } // Input is formatted as: @@ -151,7 +154,7 @@ namespace KakarotPrecompiles { let data_fits_in_input = is_le(data_bytes_len, input_len - 3 * 32); if (data_fits_in_input == 0) { let (revert_reason_len, revert_reason) = Errors.outOfBoundsRead(); - return (revert_reason_len, revert_reason, CAIRO_MESSAGE_GAS, Errors.EXCEPTIONAL_HALT); + return (revert_reason_len, revert_reason, CAIRO_MESSAGE_GAS, TRUE); } let data_ptr = input + 3 * 32; @@ -160,6 +163,6 @@ namespace KakarotPrecompiles { send_message_to_l1(target_address, data_bytes_len, data_ptr); let (output) = alloc(); - return (0, output, CAIRO_MESSAGE_GAS, 0); + return (0, output, CAIRO_MESSAGE_GAS, FALSE); } } diff --git a/src/kakarot/precompiles/precompiles.cairo b/src/kakarot/precompiles/precompiles.cairo index c06466ccb..868f08b57 100644 --- a/src/kakarot/precompiles/precompiles.cairo +++ b/src/kakarot/precompiles/precompiles.cairo @@ -175,7 +175,7 @@ namespace Precompiles { bitwise_ptr: BitwiseBuiltin*, }() -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) { let (revert_reason_len, revert_reason) = Errors.unauthorizedPrecompile(); - return (revert_reason_len, revert_reason, 0, Errors.EXCEPTIONAL_HALT); + return (revert_reason_len, revert_reason, 0, Errors.REVERT); } // @notice A placeholder for precompile that don't exist.