From ddf1821af9350b7c224c94c26cc05b6a70bd8521 Mon Sep 17 00:00:00 2001
From: Oba <obatirou@gmail.com>
Date: Tue, 3 Dec 2024 18:28:58 +0100
Subject: [PATCH] fix: validate s <= N//2 when recovering tx sender (#206)

Closes #161

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

Note to reviewer: this is a fix imported from C4 mitigation, ensure the
fix was correctly ported by looking at the corresponding issue and PR.
---
 cairo/src/utils/transaction.cairo | 15 ++++++++++++++-
 cairo/tests/programs/test_os.py   | 29 +++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/cairo/src/utils/transaction.cairo b/cairo/src/utils/transaction.cairo
index ac9a567c..cb3f9fa5 100644
--- a/cairo/src/utils/transaction.cairo
+++ b/cairo/src/utils/transaction.cairo
@@ -4,7 +4,7 @@ from starkware.cairo.common.cairo_builtins import BitwiseBuiltin, HashBuiltin, K
 from starkware.cairo.common.math_cmp import is_not_zero, is_nn
 from starkware.cairo.common.math import assert_not_zero, assert_nn
 from starkware.cairo.common.memcpy import memcpy
-from starkware.cairo.common.uint256 import Uint256
+from starkware.cairo.common.uint256 import Uint256, uint256_lt
 
 from src.model import model
 from src.constants import Constants
@@ -20,6 +20,9 @@ const TX_CREATE_COST = 32000;
 const TX_ACCESS_LIST_ADDRESS_COST = 2400;
 const TX_ACCESS_LIST_STORAGE_KEY_COST = 1900;
 
+const SECP256K1N_DIV_2_LOW = 0x5d576e7357a4501ddfe92f46681b20a0;
+const SECP256K1N_DIV_2_HIGH = 0x7fffffffffffffffffffffffffffffff;
+
 // @title Transaction utils
 // @notice This file contains utils for decoding eth transactions
 // @custom:namespace Transaction
@@ -396,6 +399,16 @@ namespace Transaction {
         }
         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 msg_hash = keccak(tx.rlp_len, tx.rlp);
 
         Signature.verify_eth_signature_uint256(
diff --git a/cairo/tests/programs/test_os.py b/cairo/tests/programs/test_os.py
index 82a4a3f7..72b132fb 100644
--- a/cairo/tests/programs/test_os.py
+++ b/cairo/tests/programs/test_os.py
@@ -1,8 +1,13 @@
 from eth_abi import encode
+from hypothesis import given
+from hypothesis.strategies import integers
 
+from ethereum.crypto.elliptic_curve import SECP256K1N
 from ethereum.crypto.hash import keccak256
+from src.utils.uint256 import int_to_uint256
 from tests.utils.constants import COINBASE, OTHER, OWNER
 from tests.utils.data import block
+from tests.utils.errors import cairo_error
 from tests.utils.models import State
 from tests.utils.solidity import get_contract
 
@@ -121,3 +126,27 @@ def test_block_hint(self, cairo_run):
                 else []
             ),
         ]
+
+    @given(s_value=integers(min_value=SECP256K1N // 2 + 1, max_value=SECP256K1N))
+    def test_should_raise_on_invalid_s_value(self, cairo_run, s_value):
+        initial_state = {
+            OWNER: {
+                "code": [],
+                "storage": {},
+                "balance": int(1e18),
+                "nonce": 0,
+            }
+        }
+        transactions = [{"to": OTHER, "data": "0x6001", "value": 0, "signer": OWNER}]
+
+        low, high = int_to_uint256(s_value)
+        block_to_pass = block(transactions)
+        block_to_pass.transactions[0].signature[2] = low
+        block_to_pass.transactions[0].signature[3] = high
+
+        with cairo_error("Invalid s value"):
+            cairo_run(
+                "test_os",
+                block=block_to_pass,
+                state=State.model_validate(initial_state),
+            )