[KGA-98] QA report 66 (#1633)

code-423n4/2024-09-kakarot-findings#66

*  Missing boundary check in EVM transaction decoding
adds a check to ensure [call_array].data_offset + [call_array].data_len
<= calldata_len
* Several of the stack_size_diff values defined in constants.cairo are
off
   put the right values
* After Kakarot's native token is changed, an approval from
account_contract instances can't be re-triggered
   remove the setter, this will not change
* Some invalid values for the v field in Ethereum signatures are
accepted
   add a check to ensure y_parity is 0 or 1

cairo_zero/kakarot/accounts/account_contract.cairo

@@ -5,7 +5,7 @@
 from openzeppelin.access.ownable.library import Ownable, Ownable_owner
 from starkware.cairo.common.alloc import alloc
 from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, SignatureBuiltin
-from starkware.cairo.common.math import assert_le, assert_not_zero
+from starkware.cairo.common.math import assert_le, assert_not_zero, assert_le_felt
 from starkware.cairo.common.math_cmp import is_nn
 from starkware.cairo.common.uint256 import Uint256
 from starkware.starknet.common.syscalls import (
@@ -121,6 +121,11 @@ func execute_from_outside{
         assert bytecode_len = 0;
     }
 
+    // Ensure the call_array is within the bounds of the calldata.
+    with_attr error_message("EOA: call_array out of bounds") {
+        assert_le_felt([call_array].data_offset + [call_array].data_len, calldata_len);
+    }
+
     // Unpack the tx data
     let packed_tx_data_len = [call_array].data_len;
     with_attr error_message("Execute from outside: packed_tx_data_len is zero or out of range") {

cairo_zero/kakarot/accounts/library.cairo

@@ -200,7 +200,7 @@ namespace AccountContract {
             msg_hash=msg_hash,
             r=r,
             s=s,
-            v=y_parity,
+            y_parity=y_parity,
             eth_address=address,
             helpers_class=helpers_class,
         );

cairo_zero/kakarot/constants.cairo

@@ -98,13 +98,13 @@ dw 0x08;
 dw Gas.MID;
 dw 3;
 dw 3;
-dw -1;
+dw -2;
 // MULMOD
 dw 0x09;
 dw Gas.MID;
 dw 3;
 dw 3;
-dw -1;
+dw -2;
 // EXP
 dw 0x0a;
 dw Gas.EXPONENTIATION;
@@ -200,7 +200,7 @@ dw 0x19;
 dw Gas.VERY_LOW;
 dw 1;
 dw 1;
-dw -1;
+dw 0;
 // BYTE
 dw 0x1a;
 dw Gas.VERY_LOW;
@@ -380,7 +380,7 @@ dw 0x37;
 dw Gas.VERY_LOW;
 dw 3;
 dw 3;
-dw 0;
+dw -3;
 // CODESIZE
 dw 0x38;
 dw Gas.BASE;
@@ -392,7 +392,7 @@ dw 0x39;
 dw Gas.VERY_LOW;
 dw 3;
 dw 3;
-dw 0;
+dw -3;
 // GASPRICE
 dw 0x3a;
 dw Gas.BASE;
@@ -422,7 +422,7 @@ dw 0x3e;
 dw Gas.VERY_LOW;
 dw 3;
 dw 3;
-dw 0;
+dw -3;
 // EXTCODEHASH
 dw 0x3f;
 dw 0;

cairo_zero/kakarot/interfaces/interfaces.cairo

@@ -112,9 +112,6 @@ namespace IKakarot {
     func get_owner() -> (owner: felt) {
     }
 
-    func set_native_token(native_token_address: felt) {
-    }
-
     func get_native_token() -> (native_token_address: felt) {
     }
 

cairo_zero/kakarot/kakarot.cairo

@@ -108,17 +108,6 @@ func get_owner{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
     return Ownable_owner.read();
 }
 
-// @notice Set the native token used by kakarot
-// @dev Set the native token which will emulate the role of ETH on Ethereum
-// @param native_token_address The address of the native token
-@external
-func set_native_token{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
-    native_token_address: felt
-) {
-    Ownable.assert_only_owner();
-    return Kakarot.set_native_token(native_token_address);
-}
-
 // @notice Get the native token address
 // @dev Return the address used to emulate the role of ETH on Ethereum
 // @return native_token_address The address of the native token

cairo_zero/kakarot/library.cairo

@@ -138,16 +138,6 @@ namespace Kakarot {
         return (chain_id=chain_id);
     }
 
-    // @notice Set the native Starknet ERC20 token used by kakarot.
-    // @dev Set the native token which will emulate the role of ETH on Ethereum.
-    // @param native_token_address The address of the native token.
-    func set_native_token{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
-        native_token_address: felt
-    ) {
-        Kakarot_native_token_address.write(native_token_address);
-        return ();
-    }
-
     // @notice Get the native token address
     // @dev Return the address used to emulate the role of ETH on Ethereum
     // @return native_token_address The address of the native token

cairo_zero/tests/src/kakarot/accounts/test_account_contract.cairo

@@ -166,7 +166,7 @@ func test__execute_from_outside_entrypoint{
                 call_.values() for call_ in program_input["call_array"]
             ])),
         )
-        ids.calldata_len = len(program_input["calldata"])
+        ids.calldata_len = program_input["calldata_len"]
         segments.write_arg(ids.calldata, program_input["calldata"])
         ids.signature_len = len(program_input["signature"])
         segments.write_arg(ids.signature, program_input["signature"])

cairo_zero/tests/src/kakarot/accounts/test_account_contract.py

@@ -302,7 +302,17 @@ def test_should_raise_with_wrong_signature(self, cairo_run):
                 cairo_run(
                     "test__execute_from_outside",
                     tx_data=[1],
-                    signature=list(range(5)),
+                    signature=[0, 1, 2, 3, 0],
+                    chain_id=CHAIN_ID,
+                )
+
+        @given(y_parity=integers(min_value=2))
+        def test_should_raise_with_invalid_y_parity(self, cairo_run, y_parity):
+            with cairo_error(message="Invalid y_parity"):
+                cairo_run(
+                    "test__execute_from_outside",
+                    tx_data=[1],
+                    signature=[0, 1, 2, 3, y_parity],
                     chain_id=CHAIN_ID,
                 )
 
@@ -354,7 +364,7 @@ def test_should_raise_unauthorized_pre_eip155_tx(self, cairo_run):
                     chain_id=CHAIN_ID,
                 )
 
-        def test_should_raise_invalid_signature_for_invalid_chain_id_when_tx_type0_not_pre_eip155(
+        def test_should_raise_invalid_y_parity_for_invalid_chain_id_when_tx_type0_not_pre_eip155(
             self, cairo_run
         ):
             transaction = {
@@ -374,7 +384,7 @@ def test_should_raise_invalid_signature_for_invalid_chain_id_when_tx_type0_not_p
 
             with (
                 SyscallHandler.patch("Account_evm_address", address),
-                cairo_error(message="Invalid signature."),
+                cairo_error(message="Invalid y_parity"),
             ):
                 cairo_run(
                     "test__execute_from_outside",
@@ -521,6 +531,7 @@ def test_should_raise_when_call_array_is_empty(self, cairo_run):
                     },
                     call_array=[],
                     calldata=[],
+                    calldata_len=0,
                     signature=[],
                 )
 
@@ -536,6 +547,7 @@ def test_should_raise_when_call_array_has_more_than_one_call(self, cairo_run):
                     },
                     call_array=[{}, {}],
                     calldata=[],
+                    calldata_len=0,
                     signature=[],
                 )
 
@@ -554,6 +566,7 @@ def test_should_raise_when_tx_version_is_zero(self, cairo_run):
                         {"to": 0, "selector": 0, "data_offset": 0, "data_len": 0},
                     ],
                     calldata=[],
+                    calldata_len=0,
                     signature=[],
                 )
 
@@ -572,10 +585,31 @@ def test_should_raise_when_eoa_has_code(self, cairo_run):
                         {"to": 0, "selector": 0, "data_offset": 0, "data_len": 0},
                     ],
                     calldata=[],
+                    calldata_len=0,
+                    signature=[],
+                )
+
+        def test_should_raise_when_call_array_not_within_bounds_calldata(
+            self, cairo_run
+        ):
+            with cairo_error(message="EOA: call_array out of bounds"):
+                cairo_run(
+                    "test__execute_from_outside_entrypoint",
+                    outside_execution={
+                        "caller": SyscallHandler.caller_address,
+                        "nonce": 0,
+                        "execute_after": 0,
+                        "execute_before": SyscallHandler.block_timestamp + 1,
+                    },
+                    call_array=[
+                        {"to": 0, "selector": 0, "data_offset": 1, "data_len": 1},
+                    ],
+                    calldata=[],
+                    calldata_len=0,
                     signature=[],
                 )
 
-        @given(data_len=integers(min_value=2**128, max_value=DEFAULT_PRIME))
+        @given(data_len=integers(min_value=2**128, max_value=DEFAULT_PRIME - 1))
         def test_should_raise_when_packed_data_len_is_zero_or_out_of_range(
             self, cairo_run, data_len
         ):
@@ -595,10 +629,11 @@ def test_should_raise_when_packed_data_len_is_zero_or_out_of_range(
                             "to": 0,
                             "selector": 0,
                             "data_offset": 0,
-                            "data_len": data_len % DEFAULT_PRIME,
+                            "data_len": data_len,
                         },
                     ],
                     calldata=[],
+                    calldata_len=data_len,
                     signature=[],
                 )
 
@@ -623,5 +658,6 @@ def test_should_raise_when_tx_data_len_is_out_of_range(self, cairo_run):
                         },
                     ],
                     calldata=[2**128],
+                    calldata_len=1,
                     signature=[],
                 )

cairo_zero/tests/src/kakarot/test_kakarot.cairo

@@ -8,7 +8,6 @@ from kakarot.library import Kakarot
 from kakarot.kakarot import (
     eth_send_raw_unsigned_tx,
     register_account,
-    set_native_token,
     set_base_fee,
     set_coinbase,
     set_prev_randao,
@@ -134,15 +133,6 @@ func test__transfer_ownership{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, ra
     return ();
 }
 
-func test__set_native_token{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}() {
-    tempvar address;
-
-    %{ ids.address = program_input["address"] %}
-
-    set_native_token(address);
-    return ();
-}
-
 func test__set_coinbase{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}() {
     tempvar coinbase;
 

cairo_zero/tests/src/kakarot/test_kakarot.py

@@ -105,22 +105,6 @@ def test_should_unpause(self, cairo_run):
                 address=get_storage_var_address("Pausable_paused"), value=0
             )
 
-    class TestNativeToken:
-        @pytest.mark.slow
-        @SyscallHandler.patch("Ownable_owner", 0xDEAD)
-        def test_should_assert_only_owner(self, cairo_run):
-            with cairo_error(message="Ownable: caller is not the owner"):
-                cairo_run("test__set_native_token", address=0xABC)
-
-        @SyscallHandler.patch("Ownable_owner", SyscallHandler.caller_address)
-        def test_should_set_native_token(self, cairo_run):
-            token_address = 0xABCDE12345
-            cairo_run("test__set_native_token", address=token_address)
-            SyscallHandler.mock_storage.assert_any_call(
-                address=get_storage_var_address("Kakarot_native_token_address"),
-                value=token_address,
-            )
-
     class TestTransferOwnership:
         @SyscallHandler.patch("Ownable_owner", 0xDEAD)
         def test_should_assert_only_owner(self, cairo_run):

cairo_zero/utils/signature.cairo

@@ -12,7 +12,14 @@ namespace Signature {
     // using the Cairo1 helpers class.
     func verify_eth_signature_uint256{
         syscall_ptr: felt*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*
-    }(msg_hash: Uint256, r: Uint256, s: Uint256, v: felt, eth_address: felt, helpers_class: felt) {
+    }(
+        msg_hash: Uint256,
+        r: Uint256,
+        s: Uint256,
+        y_parity: felt,
+        eth_address: felt,
+        helpers_class: felt,
+    ) {
         alloc_locals;
         let (msg_hash_bigint: BigInt3) = uint256_to_bigint(msg_hash);
         let (r_bigint: BigInt3) = uint256_to_bigint(r);
@@ -23,9 +30,13 @@ namespace Signature {
             validate_signature_entry(s_bigint);
         }
 
+        with_attr error_message("Invalid y_parity") {
+            assert (1 - y_parity) * y_parity = 0;
+        }
+
         with_attr error_message("Invalid signature.") {
             let (success, recovered_address) = ICairo1Helpers.library_call_recover_eth_address(
-                class_hash=helpers_class, msg_hash=msg_hash, r=r, s=s, y_parity=v
+                class_hash=helpers_class, msg_hash=msg_hash, r=r, s=s, y_parity=y_parity
             );
             assert success = 1;
             assert eth_address = recovered_address;