[KGA-109] [KGA-137] fix: validate s <= N//2 when recovering tx sender (…

…#1635)

Fixes the checks on `s` value when recovering the transaction sender.

code-423n4/2024-09-kakarot-findings#98
code-423n4/2024-09-kakarot-findings#125

---------

Co-authored-by: Oba <[email protected]>

cairo_zero/kakarot/accounts/library.cairo

@@ -7,7 +7,7 @@ from starkware.cairo.common.dict_access import DictAccess
 from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
 from starkware.cairo.common.math import split_int
 from starkware.cairo.common.memcpy import memcpy
-from starkware.cairo.common.uint256 import Uint256
+from starkware.cairo.common.uint256 import Uint256, uint256_lt
 from starkware.cairo.common.math_cmp import is_nn, is_le_felt
 from starkware.starknet.common.syscalls import (
     StorageRead,
@@ -72,6 +72,9 @@ func transaction_executed(response_len: felt, response: felt*, success: felt, ga
 
 const BYTES_PER_FELT = 31;
 
+const SECP256K1N_DIV_2_LOW = 0x5d576e7357a4501ddfe92f46681b20a0;
+const SECP256K1N_DIV_2_HIGH = 0x7fffffffffffffffffffffffffffffff;
+
 // @title Account main library file.
 // @notice This file contains the EVM account representation logic.
 // @dev: Both EOAs and Contract Accounts are represented by this contract. Owner is expected to be Kakarot.
@@ -182,6 +185,16 @@ namespace AccountContract {
         }
         let range_check_ptr = [ap - 1];
 
+        // Signature validation
+        // `verify_eth_signature_uint256` verifies that r and s are in the range [1, N[
+        // TX validation imposes s to be the range [1, N//2], see EIP-2
+        let (is_invalid_upper_s) = uint256_lt(
+            Uint256(SECP256K1N_DIV_2_LOW, SECP256K1N_DIV_2_HIGH), s
+        );
+        with_attr error_message("Invalid s value") {
+            assert is_invalid_upper_s = FALSE;
+        }
+
         let (local words: felt*) = alloc();
         let (words_len, last_word, last_word_num_bytes) = bytes_to_bytes8_little_endian(
             words, tx_data_len, tx_data

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

@@ -6,6 +6,7 @@
 import rlp
 from eth_account.account import Account
 from eth_utils import keccak
+from ethereum.crypto.elliptic_curve import SECP256K1N
 from hypothesis import given, settings
 from hypothesis.strategies import binary, composite, integers, lists, permutations
 from starkware.cairo.lang.cairo_constants import DEFAULT_PRIME
@@ -315,6 +316,34 @@ def test_should_raise_with_invalid_y_parity(self, cairo_run, y_parity):
                     chain_id=CHAIN_ID,
                 )
 
+        @given(s_value=integers(min_value=SECP256K1N // 2 + 1, max_value=SECP256K1N))
+        def test_should_raise_with_high_s_values(self, cairo_run, s_value):
+            """Test that signatures with s values > secp256k1n/2 are rejected (EIP-2)."""
+            transaction = TRANSACTIONS[0]
+            private_key = generate_random_private_key()
+            address = int(private_key.public_key.to_checksum_address(), 16)
+            signed = Account.sign_transaction(transaction, private_key)
+
+            # Override the s value with our test value while keeping r and v
+            signature = [
+                *int_to_uint256(signed.r),
+                *int_to_uint256(s_value),
+                signed.v,
+            ]
+            tx_data = list(rlp_encode_signed_data(transaction))
+
+            with (
+                cairo_error(message="Invalid s value"),
+                SyscallHandler.patch("Account_evm_address", address),
+                SyscallHandler.patch("Account_nonce", transaction.get("nonce", 0)),
+            ):
+                cairo_run(
+                    "test__execute_from_outside",
+                    tx_data=tx_data,
+                    signature=signature,
+                    chain_id=CHAIN_ID,
+                )
+
         @composite
         def draw_signature_not_in_range(draw):
             # create signature with 4 elements < 2 ** 128 and one > 2 ** 128