From 94f8c6dab4313db8dbbeb28d9ce7d6bf3f4b45da Mon Sep 17 00:00:00 2001 From: enitrat Date: Mon, 4 Nov 2024 21:02:53 +0700 Subject: [PATCH] make whitelisted checks based on caller address (instead of code address) and only register succeeded cairo call in message --- cairo_zero/kakarot/interpreter.cairo | 17 +++---- .../kakarot/precompiles/precompiles.cairo | 4 +- .../precompiles/precompiles_helpers.cairo | 10 ++--- .../precompiles/test_precompiles.cairo | 3 -- .../kakarot/precompiles/test_precompiles.py | 44 +++++++++---------- kakarot_scripts/constants.py | 4 -- .../src/CairoPrecompiles/DualVmToken.sol | 19 ++++---- .../src/Security/DualVmTokenHack.sol | 2 +- .../Security/test_dual_vm_token_hack.py | 2 +- tests/end_to_end/conftest.py | 2 +- 10 files changed, 44 insertions(+), 63 deletions(-) diff --git a/cairo_zero/kakarot/interpreter.cairo b/cairo_zero/kakarot/interpreter.cairo index 4f090a319..3bb298135 100644 --- a/cairo_zero/kakarot/interpreter.cairo +++ b/cairo_zero/kakarot/interpreter.cairo @@ -67,20 +67,11 @@ namespace Interpreter { let is_precompile = PrecompilesHelpers.is_precompile(evm.message.code_address.evm); if (is_precompile != FALSE) { let parent_context = evm.message.parent; - let is_parent_zero = Helpers.is_zero(cast(parent_context, felt)); - if (is_parent_zero != FALSE) { - // Case A: The precompile is called straight from an EOA - tempvar caller_code_address = evm.message.caller; - } else { - // Case B: The precompile is called from a contract - tempvar caller_code_address = parent_context.evm.message.code_address.evm; - } tempvar caller_address = evm.message.caller; let (output_len, output, gas_used, revert_code) = Precompiles.exec_precompile( evm.message.code_address.evm, evm.message.calldata_len, evm.message.calldata, - caller_code_address, caller_address, evm.message.address.evm, ); @@ -101,9 +92,15 @@ namespace Interpreter { } let range_check_ptr = [ap - 2]; let evm = cast([ap - 1], model.EVM*); + + // Only count the cairo precompile if it was executed and did not revert. + // If it did revert, we're ensured no state changes were made in the cairo call. let is_cairo_precompile_called = PrecompilesHelpers.is_kakarot_precompile( evm.message.code_address.evm ); + let is_cairo_precompile_executed = is_cairo_precompile_called * ( + 1 - precompile_reverted + ); tempvar message = new model.Message( bytecode=evm.message.bytecode, bytecode_len=evm.message.bytecode_len, @@ -120,7 +117,7 @@ namespace Interpreter { is_create=evm.message.is_create, depth=evm.message.depth, env=evm.message.env, - cairo_precompile_called=is_cairo_precompile_called, + cairo_precompile_called=is_cairo_precompile_executed, ); tempvar evm = new model.EVM( message=message, diff --git a/cairo_zero/kakarot/precompiles/precompiles.cairo b/cairo_zero/kakarot/precompiles/precompiles.cairo index 2d8816e55..4704f2a88 100644 --- a/cairo_zero/kakarot/precompiles/precompiles.cairo +++ b/cairo_zero/kakarot/precompiles/precompiles.cairo @@ -32,7 +32,6 @@ namespace Precompiles { // @param precompile_address The precompile evm_address. // @param input_len The length of the input array. // @param input The input array. - // @param caller_code_address The address of the code of the contract that calls the precompile. // @param caller_address The address of the caller of the precompile. Delegatecall rules apply. // @param message_address The address being executed in the current message. // @return output_len The output length. @@ -48,7 +47,6 @@ namespace Precompiles { precompile_address: felt, input_len: felt, input: felt*, - caller_code_address: felt, caller_address: felt, message_address: felt, ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) { @@ -135,7 +133,7 @@ namespace Precompiles { kakarot_precompile: let is_call_authorized_ = PrecompilesHelpers.is_call_authorized( - precompile_address, caller_code_address, caller_address, message_address + precompile_address, caller_address, message_address ); tempvar is_not_authorized = 1 - is_call_authorized_; tempvar syscall_ptr = syscall_ptr; diff --git a/cairo_zero/kakarot/precompiles/precompiles_helpers.cairo b/cairo_zero/kakarot/precompiles/precompiles_helpers.cairo index bbb7b352b..d86cfd3dc 100644 --- a/cairo_zero/kakarot/precompiles/precompiles_helpers.cairo +++ b/cairo_zero/kakarot/precompiles/precompiles_helpers.cairo @@ -75,25 +75,21 @@ namespace PrecompilesHelpers { // @notice Returns whether the call to the precompile is authorized. // @dev A call is authorized if: - // a. The precompile requires a whitelist AND the CODE_ADDRESS of the caller is whitelisted + // a. The precompile requires a whitelist AND the ADDRESS of the caller is whitelisted // b. The precompile is CAIRO_MULTICALL_PRECOMPILE and the precompile address is the same as the message address (NOT a DELEGATECALL / CALLCODE). // @param precompile_address The address of the precompile. - // @param caller_code_address The code_address of the precompile caller. // @param caller_address The address of the caller. // @param message_address The address being executed in the current message. // @return Whether the call is authorized. func is_call_authorized{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}( - precompile_address: felt, - caller_code_address: felt, - caller_address: felt, - message_address: felt, + precompile_address: felt, caller_address: felt, message_address: felt ) -> felt { alloc_locals; let precompile_requires_whitelist = requires_whitelist(precompile_address); // Ensure that calls to precompiles that require a whitelist are properly authorized. if (precompile_requires_whitelist == TRUE) { - let is_whitelisted = is_caller_whitelisted(caller_code_address); + let is_whitelisted = is_caller_whitelisted(caller_address); tempvar syscall_ptr = syscall_ptr; tempvar pedersen_ptr = pedersen_ptr; tempvar range_check_ptr = range_check_ptr; diff --git a/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo b/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo index 4c0e5562c..c0d88cbdb 100644 --- a/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo +++ b/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo @@ -25,7 +25,6 @@ func test__precompiles_run{ // Given local address; local input_len; - local caller_code_address; local caller_address; local message_address; let (local input) = alloc(); @@ -33,7 +32,6 @@ func test__precompiles_run{ ids.address = program_input["address"] ids.input_len = len(program_input["input"]) segments.write_arg(ids.input, program_input["input"]) - ids.caller_code_address = program_input.get("caller_code_address", 0) ids.caller_address = program_input.get("caller_address", 0) ids.message_address = program_input.get("message_address", 0) %} @@ -43,7 +41,6 @@ func test__precompiles_run{ precompile_address=address, input_len=input_len, input=input, - caller_code_address=caller_code_address, caller_address=caller_address, message_address=message_address, ); diff --git a/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.py b/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.py index e4fc74c46..173f5a97f 100644 --- a/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.py +++ b/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.py @@ -13,8 +13,8 @@ ) from tests.utils.syscall_handler import SyscallHandler -AUTHORIZED_CALLER_CODE = 0xA7071ED -UNAUTHORIZED_CALLER_CODE = 0xC0C0C0 +AUTHORIZED_CALLER_ADDRESS = 0xA7071ED +UNAUTHORIZED_CALLER_ADDRESS = 0xC0C0C0 CALLER_ADDRESS = 0x123ABC432 @@ -80,7 +80,7 @@ def test__p256_verify_precompile( class TestKakarotPrecompiles: @SyscallHandler.patch( "Kakarot_authorized_cairo_precompiles_callers", - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, 1, ) @SyscallHandler.patch_deploy(lambda class_hash, data: [0]) @@ -103,8 +103,7 @@ def test_should_deploy_account_when_sender_starknet_address_zero( + f"{0x60:064x}" # data_offset + f"{0x00:064x}" # data_len ), - caller_code_address=AUTHORIZED_CALLER_CODE, - caller_address=CALLER_ADDRESS, + caller_address=AUTHORIZED_CALLER_ADDRESS, message_address=0x75001, ) assert not bool(reverted) @@ -116,25 +115,26 @@ def test_should_deploy_account_when_sender_starknet_address_zero( @SyscallHandler.patch( "Kakarot_authorized_cairo_precompiles_callers", - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, 1, ) @SyscallHandler.patch( "Kakarot_evm_to_starknet_address", CALLER_ADDRESS, 0x1234 ) + @SyscallHandler.patch_deploy(lambda class_hash, data: [0]) @pytest.mark.parametrize( - "address, caller_code_address, input_data, expected_return_data, expected_reverted", + "address, caller_address, input_data, expected_return_data, expected_reverted", [ ( 0x75001, - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, bytes.fromhex("0abcdef0"), b"Kakarot: OutOfBoundsRead", True, ), # invalid input ( 0x75001, - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, bytes.fromhex( f"{0xc0de:064x}" + f"{get_selector_from_name('inc'):064x}" @@ -146,7 +146,7 @@ def test_should_deploy_account_when_sender_starknet_address_zero( ), # call_contract ( 0x75001, - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, bytes.fromhex( f"{0xc0de:064x}" + f"{get_selector_from_name('get'):064x}" @@ -159,7 +159,7 @@ def test_should_deploy_account_when_sender_starknet_address_zero( ), # call_contract with return data ( 0x75001, - UNAUTHORIZED_CALLER_CODE, + UNAUTHORIZED_CALLER_ADDRESS, bytes.fromhex("0abcdef0"), b"Kakarot: unauthorizedPrecompile", True, @@ -176,7 +176,7 @@ def test__cairo_precompiles( self, cairo_run, address, - caller_code_address, + caller_address, input_data, expected_return_data, expected_reverted, @@ -197,15 +197,14 @@ def test__cairo_precompiles( "test__precompiles_run", address=address, input=input_data, - caller_code_address=caller_code_address, - caller_address=CALLER_ADDRESS, + caller_address=caller_address, message_address=address, ) assert bool(reverted) == expected_reverted assert bytes(return_data) == expected_return_data assert gas_used == ( CAIRO_PRECOMPILE_GAS - if caller_code_address == AUTHORIZED_CALLER_CODE + if caller_address == AUTHORIZED_CALLER_ADDRESS else 0 ) return @@ -213,7 +212,7 @@ def test__cairo_precompiles( class TestKakarotMessaging: @SyscallHandler.patch( "Kakarot_authorized_cairo_precompiles_callers", - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, 1, ) @SyscallHandler.patch( @@ -221,11 +220,11 @@ class TestKakarotMessaging: 0xC0DE, ) @pytest.mark.parametrize( - "address, caller_code_address, input_data, to_address, expected_reverted_return_data, expected_reverted", + "address, caller_address, input_data, to_address, expected_reverted_return_data, expected_reverted", [ ( 0x75002, - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, encode( ["uint160", "bytes"], [0xC0DE, encode(["uint128"], [0x2A])] ), @@ -235,7 +234,7 @@ class TestKakarotMessaging: ), ( 0x75002, - AUTHORIZED_CALLER_CODE, + AUTHORIZED_CALLER_ADDRESS, encode(["uint160", "bytes"], [0xC0DE, 0x2A.to_bytes(1, "big")]), 0xC0DE, b"", @@ -243,7 +242,7 @@ class TestKakarotMessaging: ), ( 0x75002, - UNAUTHORIZED_CALLER_CODE, + UNAUTHORIZED_CALLER_ADDRESS, bytes.fromhex("0abcdef0"), 0xC0DE, b"Kakarot: unauthorizedPrecompile", @@ -258,7 +257,7 @@ class TestKakarotMessaging: ) def test__cairo_message( self, - caller_code_address, + caller_address, cairo_run, address, input_data, @@ -271,8 +270,7 @@ def test__cairo_message( "test__precompiles_run", address=address, input=input_data, - caller_code_address=caller_code_address, - caller_address=CALLER_ADDRESS, + caller_address=caller_address, message_address=address, ) if expected_reverted: diff --git a/kakarot_scripts/constants.py b/kakarot_scripts/constants.py index a7c085cce..47bbe4370 100644 --- a/kakarot_scripts/constants.py +++ b/kakarot_scripts/constants.py @@ -90,10 +90,6 @@ class NetworkType(Enum): "check_interval": 0.01, "max_wait": 3, "relayers": [ - { - "address": 0xE29882A1FCBA1E7E10CAD46212257FEA5C752A4F9B1B1EC683C503A2CF5C8A, - "private_key": 0x14D6672DCB4B77CA36A887E9A11CD9D637D5012468175829E9C6E770C61642, - }, { "address": 0x29873C310FBEFDE666DC32A1554FEA6BB45EECC84F680F8A2B0A8FBB8CB89AF, "private_key": 0xC5B2FCAB997346F3EA1C00B002ECF6F382C5F9C9659A3894EB783C5320F912, diff --git a/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol b/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol index 463ccd88d..66b66534e 100644 --- a/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol +++ b/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol @@ -3,13 +3,12 @@ pragma solidity 0.8.27; import {WhitelistedCallCairoLib} from "./WhitelistedCallCairoLib.sol"; import {CairoLib} from "kakarot-lib/CairoLib.sol"; -import {NoDelegateCall} from "../Security/NoDelegateCall.sol"; /// @notice EVM adapter into a Cairo ERC20 token /// @dev This implementation is highly experimental /// It relies on CairoLib to perform Cairo precompile calls /// Events are emitted in this contract but also in the Starknet token contract -/// @dev External functions are noDelegateCall to prevent a user making an EVM call to a malicious contract, +/// @dev External functions are to prevent a user making an EVM call to a malicious contract, /// with any calldata, that would be able to directly control on their behalf any quantity of any one of the ERC20 /// tokens held by the victim's account contract, with the sole condition that the ERC20 has an /// authorized DualVmToken wrapper. @@ -197,7 +196,7 @@ contract DualVmToken is NoDelegateCall { //////////////////////////////////////////////////////////////*/ /// @dev Approve an evm account spender for a specific amount - function approve(address spender, uint256 amount) external noDelegateCall returns (bool) { + function approve(address spender, uint256 amount) external returns (bool) { uint256[] memory spenderAddressCalldata = new uint256[](1); spenderAddressCalldata[0] = uint256(uint160(spender)); uint256 spenderStarknetAddress = @@ -213,7 +212,7 @@ contract DualVmToken is NoDelegateCall { /// @param spender The starknet address to approve /// @param amount The amount of tokens to approve /// @return True if the approval was successful - function approve(uint256 spender, uint256 amount) external noDelegateCall returns (bool) { + function approve(uint256 spender, uint256 amount) external returns (bool) { _approve(spender, amount); emit Approval(msg.sender, spender, amount); return true; @@ -238,7 +237,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transfer(address to, uint256 amount) external noDelegateCall returns (bool) { + function transfer(address to, uint256 amount) external returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -253,7 +252,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transfer(uint256 to, uint256 amount) external noDelegateCall returns (bool) { + function transfer(uint256 to, uint256 amount) external returns (bool) { _transfer(to, amount); emit Transfer(msg.sender, to, amount); return true; @@ -280,7 +279,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(address from, address to, uint256 amount) external noDelegateCall returns (bool) { + function transferFrom(address from, address to, uint256 amount) external returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -302,7 +301,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(uint256 from, address to, uint256 amount) external noDelegateCall returns (bool) { + function transferFrom(uint256 from, address to, uint256 amount) external returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -319,7 +318,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(address from, uint256 to, uint256 amount) external noDelegateCall returns (bool) { + function transferFrom(address from, uint256 to, uint256 amount) external returns (bool) { uint256[] memory fromAddressCalldata = new uint256[](1); fromAddressCalldata[0] = uint256(uint160(from)); uint256 fromStarknetAddress = @@ -336,7 +335,7 @@ contract DualVmToken is NoDelegateCall { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(uint256 from, uint256 to, uint256 amount) external noDelegateCall returns (bool) { + function transferFrom(uint256 from, uint256 to, uint256 amount) external returns (bool) { _transferFrom(from, to, amount); emit Transfer(from, to, amount); return true; diff --git a/solidity_contracts/src/Security/DualVmTokenHack.sol b/solidity_contracts/src/Security/DualVmTokenHack.sol index c252bd636..26a1622fc 100644 --- a/solidity_contracts/src/Security/DualVmTokenHack.sol +++ b/solidity_contracts/src/Security/DualVmTokenHack.sol @@ -10,7 +10,7 @@ contract DualVmTokenHack { } function tryApproveEvm() external returns (bool success) { - (success,) = target.delegatecall( + (success,) = target.delegatecall{gas: 30000}( abi.encodeWithSelector(bytes4(keccak256("approve(address,uint256)")), address(this), AMOUNT) ); } diff --git a/tests/end_to_end/Security/test_dual_vm_token_hack.py b/tests/end_to_end/Security/test_dual_vm_token_hack.py index 673c4dd9b..3b3446474 100644 --- a/tests/end_to_end/Security/test_dual_vm_token_hack.py +++ b/tests/end_to_end/Security/test_dual_vm_token_hack.py @@ -58,7 +58,7 @@ class TestActions: async def test_malicious_approve_address_should_fail_nodelegatecaltest_malicious_approve_address_should_fail_nodelegatecall( self, dual_vm_token, hack_vm_token, owner ): - result = await hack_vm_token.functions["tryApproveEvm()"]() + result = await hack_vm_token.functions["tryApproveEvm()"](gas_limit=1000000) call_succeeded = int.from_bytes(bytes(result["response"]), "big") assert call_succeeded == 0 diff --git a/tests/end_to_end/conftest.py b/tests/end_to_end/conftest.py index ec9b7200f..d460d8628 100644 --- a/tests/end_to_end/conftest.py +++ b/tests/end_to_end/conftest.py @@ -79,7 +79,7 @@ async def _factory(amount=0): address=get_deployments()["KakarotETH"]["address"], ) gas_price = (await call("kakarot", "get_base_fee")).base_fee - gas_limit = 40_000 + gas_limit = 100_000 tx_cost = gas_limit * gas_price for wallet in deployed: balance = await eth_balance_of(wallet.address)