[KGA-29] [KGA-136] fix: cases with Cairo precompiles and delegatecalls (

#1567) Fixes the issue raised during the audit about eventual honeypots that could lure a user and manipulate starknet tokens without explicit approvals 1. Remove dependence on the `code_address` for whitelisting, which were reported dangerous. 2. Disable ability of having nesting a delegatecall to 0x75001 at protocol level. (no longer possible to do User A -call-> contract C -delegatecall->DualVmToken -delegatecall->0x75001 as a byproduct of 1. 3. Disable ability of calling 0x75002 with a DELEGATECALL at protocol level 4. Added noDelegateCall modifier to L2KakarotMessaging and DualVMToken for extra security 4. Added associated tests under `Security/` code-423n4/2024-09-kakarot-findings#38 Closes #1562  - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/kkrt-labs/kakarot/1567)  --------- Co-authored-by: Oba <[email protected]> Co-authored-by: Clément Walter <[email protected]>
kkrt-labs · Nov 15, 2024 · 8e37d05 · 8e37d05
1 parent cbe1c81
commit 8e37d05
Show file tree

Hide file tree

Showing 19 changed files with 783 additions and 84 deletions.
diff --git 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
@@ -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
@@ -75,25 +75,30 @@ 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);
 
+        tempvar is_not_delegatecall: felt = Helpers.is_zero(message_address - 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);
+            // If the call is not a DELEGATECALL / CALLCODE, the actual caller is the caller_address
+            // Otherwise, the actual caller is the message_address.
+            if (is_not_delegatecall != FALSE) {
+                tempvar actual_caller_address = caller_address;
+            } else {
+                tempvar actual_caller_address = message_address;
+            }
+            let is_whitelisted = is_caller_whitelisted(actual_caller_address);
             tempvar syscall_ptr = syscall_ptr;
             tempvar pedersen_ptr = pedersen_ptr;
             tempvar range_check_ptr = range_check_ptr;
@@ -109,11 +114,10 @@ namespace PrecompilesHelpers {
         let range_check_ptr = [ap - 2];
         let authorized = [ap - 1];
 
-        // Ensure that calls to CAIRO_CALL_PRECOMPILE or CAIRO_CALL_PRECOMPILE are not made through
+        // Ensure that calls to CAIRO_CALL_PRECOMPILE or CAIRO_MULTICALL_PRECOMPILE are not made through
         // a delegatecall / callcode.
         let is_delegatecall_protected_ = is_delegatecall_protected(precompile_address);
         if (is_delegatecall_protected_ != FALSE) {
-            let is_not_delegatecall = Helpers.is_zero(message_address - precompile_address);
             tempvar authorized = authorized * is_not_delegatecall;
         } else {
             tempvar authorized = authorized;

diff --git a/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo b/cairo_zero/tests/src/kakarot/precompiles/test_precompiles.cairo
@@ -25,15 +25,13 @@ func test__precompiles_run{
     // Given
     local address;
     local input_len;
-    local caller_code_address;
     local caller_address;
     local message_address;
     let (local input) = alloc();
     %{
         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
@@ -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 *_: [0])
@@ -103,38 +103,37 @@ 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)
                 assert bytes(return_data) == b""
                 assert gas_used == CAIRO_PRECOMPILE_GAS
 
-                SyscallHandler.mock_deploy.assert_called_once()
-                return
+                assert SyscallHandler.mock_deploy.call_count == 1
 
             @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 *_: [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 +145,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 +158,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 +175,7 @@ def test__cairo_precompiles(
                 self,
                 cairo_run,
                 address,
-                caller_code_address,
+                caller_address,
                 input_data,
                 expected_return_data,
                 expected_reverted,
@@ -197,35 +196,34 @@ 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
 
         class TestKakarotMessaging:
             @SyscallHandler.patch(
                 "Kakarot_authorized_cairo_precompiles_callers",
-                AUTHORIZED_CALLER_CODE,
+                AUTHORIZED_CALLER_ADDRESS,
                 1,
             )
             @SyscallHandler.patch(
                 "Kakarot_l1_messaging_contract_address",
                 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,15 +233,15 @@ class TestKakarotMessaging:
                     ),
                     (
                         0x75002,
-                        AUTHORIZED_CALLER_CODE,
+                        AUTHORIZED_CALLER_ADDRESS,
                         encode(["uint160", "bytes"], [0xC0DE, 0x2A.to_bytes(1, "big")]),
                         0xC0DE,
                         b"",
                         False,
                     ),
                     (
                         0x75002,
-                        UNAUTHORIZED_CALLER_CODE,
+                        UNAUTHORIZED_CALLER_ADDRESS,
                         bytes.fromhex("0abcdef0"),
                         0xC0DE,
                         b"Kakarot: unauthorizedPrecompile",
@@ -258,7 +256,7 @@ class TestKakarotMessaging:
             )
             def test__cairo_message(
                 self,
-                caller_code_address,
+                caller_address,
                 cairo_run,
                 address,
                 input_data,
@@ -271,8 +269,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/utils/kakarot.py b/kakarot_scripts/utils/kakarot.py
@@ -712,8 +712,7 @@ async def eth_send_transaction(
     typed_transaction = TypedTransaction.from_dict(payload)
 
     evm_tx = EvmAccount.sign_transaction(
-        typed_transaction.as_dict(),
-        hex(evm_account.signer.private_key),
+        typed_transaction.as_dict(), f"{evm_account.signer.private_key:064x}"
     )
 
     if WEB3.is_connected():

diff --git a/solidity_contracts/src/CairoPrecompiles/CallCairoPrecompileTest.sol b/solidity_contracts/src/CairoPrecompiles/CallCairoPrecompileTest.sol
@@ -115,7 +115,7 @@ library Internals {
             mstore(0x40, add(result, add(0x20, size)))
         }
 
-        require(success, string(abi.encodePacked("CairoLib: call_contract failed with: ", result)));
+        require(success, string(abi.encodePacked("CairoLib: cairo call failed with: ", result)));
 
         return result;
     }