fix: ecrecover validation (#1565)

Fixes validation of ecrecover `r` and `s` values.
Fixes the library call 'mock' to throw error if an invalid values
reaches this point.

Fix for:
code-423n4/2024-09-kakarot-findings#13

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg"
height="34" align="absmiddle"
alt="Reviewable"/>](https://reviewable.io/reviews/kkrt-labs/kakarot/1565)
<!-- Reviewable:end -->

Co-authored-by: Clément Walter <clement0walter@gmail.com>

cairo_zero/kakarot/precompiles/ec_recover.cairo

@@ -4,14 +4,15 @@ from starkware.cairo.common.alloc import alloc
 from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
 from starkware.cairo.common.cairo_keccak.keccak import finalize_keccak
 from starkware.cairo.common.bool import FALSE
-from starkware.cairo.common.math_cmp import RC_BOUND
+from starkware.cairo.common.math_cmp import RC_BOUND, is_nn
 from starkware.cairo.common.cairo_secp.ec import EcPoint
 from starkware.cairo.common.cairo_secp.bigint import BigInt3
 from starkware.cairo.common.cairo_secp.signature import (
     recover_public_key,
     public_key_point_to_eth_address,
 )
-from starkware.cairo.common.uint256 import Uint256, uint256_reverse_endian
+from starkware.cairo.common.uint256 import Uint256, uint256_reverse_endian, uint256_lt
+from utils.uint256 import uint256_eq
 from starkware.cairo.common.cairo_secp.bigint import bigint_to_uint256
 from starkware.cairo.common.keccak_utils.keccak_utils import keccak_add_uint256s
 from starkware.cairo.common.memset import memset
@@ -31,6 +32,9 @@ namespace PrecompileEcRecover {
     const PRECOMPILE_ADDRESS = 0x01;
     const GAS_COST_EC_RECOVER = 3000;
 
+    const SECP256K1N_HIGH = 0xfffffffffffffffffffffffffffffffe;
+    const SECP256K1N_LOW = 0xbaaedce6af48a03bbfd25e8cd0364141;
+
     // @notice Run the precompile.
     // @param input_len The length of input array.
     // @param input The input array.
@@ -62,6 +66,23 @@ namespace PrecompileEcRecover {
         let r = Helpers.bytes_to_uint256(32, input_padded + 32 * 2);
         let s = Helpers.bytes_to_uint256(32, input_padded + 32 * 3);
 
+        let SECP256K1N = Uint256(low=SECP256K1N_LOW, high=SECP256K1N_HIGH);
+        let (is_valid_upper_r) = uint256_lt(r, SECP256K1N);
+        let (is_valid_upper_s) = uint256_lt(s, SECP256K1N);
+        let is_valid_upper_bound = is_valid_upper_r * is_valid_upper_s;
+        if (is_valid_upper_bound == FALSE) {
+            let (output) = alloc();
+            return (0, output, GAS_COST_EC_RECOVER, 0);
+        }
+
+        let (is_invalid_lower_r) = uint256_eq(r, Uint256(low=0, high=0));
+        let (is_invalid_lower_s) = uint256_eq(s, Uint256(low=0, high=0));
+        let is_invalid_lower_bound = is_invalid_lower_r + is_invalid_lower_s;
+        if (is_invalid_lower_bound != FALSE) {
+            let (output) = alloc();
+            return (0, output, GAS_COST_EC_RECOVER, 0);
+        }
+
         // v - 27, see recover_public_key comment
         let (helpers_class) = Kakarot_cairo1_helpers_class_hash.read();
         let (success, recovered_address) = ICairo1Helpers.library_call_recover_eth_address(

cairo_zero/tests/src/kakarot/precompiles/test_ec_recover.py

@@ -3,6 +3,8 @@
 from ethereum.crypto.elliptic_curve import SECP256K1N, secp256k1_recover
 from ethereum.crypto.hash import Hash32, keccak256
 from ethereum.utils.byte import left_pad_zero_bytes
+from hypothesis import given
+from hypothesis import strategies as st
 
 from tests.utils.helpers import ec_sign, generate_random_private_key
 
@@ -34,9 +36,11 @@ def ecrecover(data):
 
 @pytest.mark.EC_RECOVER
 class TestEcRecover:
-    def test_should_return_evm_address_in_bytes32(self, cairo_run):
+    @given(message=st.binary(min_size=1, max_size=1000))
+    def test_valid_signature(self, message, cairo_run):
+        """Test with valid signatures generated from random messages."""
         private_key = generate_random_private_key()
-        msg = keccak256(b"test message")
+        msg = keccak256(message)
         (v, r, s) = ec_sign(msg, private_key)
 
         input_data = [
@@ -47,25 +51,53 @@ def test_should_return_evm_address_in_bytes32(self, cairo_run):
         ]
 
         padded_address, _ = ecrecover(input_data)
-
         [output] = cairo_run("test__ec_recover", input=input_data)
-
         assert bytes(output) == bytes(padded_address)
 
-    def test_should_fail_when_input_len_is_not_128(self, cairo_run):
-        [output] = cairo_run("test__ec_recover", input=[])
+    @given(input_length=st.integers(min_value=0, max_value=127))
+    def test_invalid_input_length(self, input_length, cairo_run):
+        """Test with various invalid input lengths."""
+        input_data = [0] * input_length
+        [output] = cairo_run("test__ec_recover", input=input_data)
         assert output == []
 
-    def test_should_fail_when_recovery_identifier_is_neither_27_nor_28(self, cairo_run):
-        private_key = generate_random_private_key()
-        msg = keccak256(b"test message")
-        (_, r, s) = ec_sign(msg, private_key)
-        v = 1
+    @given(
+        v=st.integers(min_value=0, max_value=26) | st.integers(min_value=29),
+        msg=st.binary(min_size=32, max_size=32),
+        r=st.integers(min_value=1, max_value=SECP256K1N - 1),
+        s=st.integers(min_value=1, max_value=SECP256K1N - 1),
+    )
+    def test_invalid_v(self, v, msg, r, s, cairo_run):
+        """Test with invalid v values."""
         input_data = [
             *msg,
             *v.to_bytes(32, "big"),
-            *r,
-            *s,
+            *r.to_bytes(32, "big"),
+            *s.to_bytes(32, "big"),
         ]
         [output] = cairo_run("test__ec_recover", input=input_data)
         assert output == []
+
+    @given(
+        v=st.integers(min_value=27, max_value=28),
+        msg=st.binary(min_size=32, max_size=32),
+        r=st.integers(min_value=0, max_value=2**256 - 1),
+        s=st.integers(min_value=0, max_value=2**256 - 1),
+    )
+    def test_parameter_boundaries(self, cairo_run, v, msg, r, s):
+        """Test `r` and `s` parameter validation including boundary conditions."""
+        input_data = [
+            *msg,
+            *v.to_bytes(32, "big"),
+            *r.to_bytes(32, "big"),
+            *s.to_bytes(32, "big"),
+        ]
+
+        py_result = ecrecover(input_data)
+        [cairo_result] = cairo_run("test__ec_recover", input=input_data)
+
+        if py_result is None:
+            assert cairo_result == []
+        else:
+            py_address, _ = py_result
+            assert bytes(cairo_result) == bytes(py_address)

tests/utils/syscall_handler.py

@@ -44,10 +44,11 @@ def cairo_recover_eth_address(class_hash, calldata):
     s = U256(uint256_to_int(calldata[4], calldata[5]))
     y_parity = U256(calldata[6])
 
+    # r and s must have been validated by the precompile preparation
     if 0 >= r or r >= SECP256K1N:
-        return [0, 0]
+        raise ValueError("Invalid r value")
     if 0 >= s or s >= SECP256K1N:
-        return [0, 0]
+        raise ValueError("Invalid s value")
     try:
         public_key = secp256k1_recover(r, s, y_parity, msg_hash)
     except Exception: