fix: ecrecover validation + migrate to KeccakBuiltin-based ecrecover (#…

…88) Fixes the ecrecover validatoin issues raised in audit + migrate ecrecover to use the KeccakBuiltin.
kkrt-labs · Nov 4, 2024 · 96829dc · 96829dc
1 parent b1d8e01
commit 96829dc
Show file tree

Hide file tree

Showing 27 changed files with 355 additions and 105 deletions.
diff --git a/.github/workflows/python_tests.yml b/.github/workflows/python_tests.yml
@@ -13,13 +13,10 @@ jobs:
       HYPOTHESIS_PROFILE: ci
     steps:
       - uses: actions/checkout@v4
-      - uses: astral-sh/setup-uv@v2
+      - uses: astral-sh/setup-uv@v3
         with:
           enable-cache: true
           cache-dependency-glob: uv.lock
-      - uses: actions/setup-python@v5
-        with:
-          python-version-file: .python-version
       - run: |
           cd cairo
           uv run compile

diff --git a/.gitignore b/.gitignore
@@ -30,3 +30,6 @@ cairo/tests/ef_tests/test_data
 
 # Ignore proptest regression files
 **/proptest-regressions/*
+
+# Ide settings
+.vscode/
diff --git a/.python-version b/.python-version
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -12,5 +12,9 @@
   "python.testing.unittestEnabled": false,
   "python.testing.pytestEnabled": true,
   "jupyter.notebookFileRoot": "${workspaceFolder}/cairo",
-  "githubPullRequests.remotes": ["kakarot", "upstream"]
+  "githubPullRequests.remotes": [
+    "kakarot",
+    "upstream"
+  ],
+  "cairols.sourceDir": "cairo"
 }
diff --git a/cairo/src/interpreter.cairo b/cairo/src/interpreter.cairo
@@ -1,6 +1,6 @@
 from starkware.cairo.common.alloc import alloc
 from starkware.cairo.common.bool import FALSE, TRUE
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.math_cmp import is_not_zero, is_nn, is_le_felt
 from starkware.cairo.common.math import split_felt
 from starkware.cairo.common.default_dict import default_dict_new
@@ -45,6 +45,7 @@ namespace Interpreter {
         pedersen_ptr: HashBuiltin*,
         range_check_ptr,
         bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
         stack: model.Stack*,
         memory: model.Memory*,
         state: model.State*,
@@ -693,6 +694,9 @@ namespace Interpreter {
         let evm = cast([ap - 1], model.EVM*);
         let evm_prev = cast([fp - 3], model.EVM*);
 
+        // keccak is still unused in regular opcode execution
+        let keccak_ptr = cast([fp - 7], KeccakBuiltin*);
+
         if (evm_prev.message.depth == evm.message.depth) {
             let evm = EVM.increment_program_counter(evm, 1);
             return evm;
@@ -709,6 +713,9 @@ namespace Interpreter {
         let state = cast([ap - 2], model.State*);
         let evm = cast([ap - 1], model.EVM*);
 
+        // keccak is still unused in regular opcode execution
+        let keccak_ptr = cast([fp - 7], KeccakBuiltin*);
+
         return evm;
     }
 
@@ -735,6 +742,7 @@ namespace Interpreter {
         pedersen_ptr: HashBuiltin*,
         range_check_ptr,
         bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
         stack: model.Stack*,
         memory: model.Memory*,
         state: model.State*,
@@ -811,7 +819,12 @@ namespace Interpreter {
     // @return gas_used the gas used by the transaction
     // @return required_gas The amount of gas required by the transaction to successfully execute. This is different
     // from the gas used by the transaction as it doesn't take into account any refunds.
-    func execute{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
+    func execute{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(
         env: model.Environment*,
         address: felt,
         is_deploy_tx: felt,

diff --git a/cairo/src/precompiles/blake2f.cairo b/cairo/src/precompiles/blake2f.cairo
@@ -1,6 +1,5 @@
 from starkware.cairo.common.alloc import alloc
-from starkware.cairo.common.cairo_builtins import HashBuiltin
-from starkware.cairo.common.cairo_builtins import BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.registers import get_fp_and_pc, get_label_location
 from starkware.cairo.common.math_cmp import is_nn
 from starkware.cairo.common.bool import FALSE
@@ -23,9 +22,14 @@ namespace PrecompileBlake2f {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
         local rounds_bytes_len = 4;
         local word_bytes_len = 8;

diff --git a/cairo/src/precompiles/datacopy.cairo b/cairo/src/precompiles/datacopy.cairo
@@ -1,4 +1,4 @@
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 
 from src.model import model
 from src.utils.utils import Helpers
@@ -19,9 +19,14 @@ namespace PrecompileDataCopy {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         let (minimum_word_size) = Helpers.minimum_word_count(input_len);
         return (input_len, input, 3 * minimum_word_size + GAS_COST_DATACOPY, 0);
     }

diff --git a/cairo/src/precompiles/ec_recover.cairo b/cairo/src/precompiles/ec_recover.cairo
@@ -1,22 +1,20 @@
 from starkware.cairo.common.alloc import alloc
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.builtin_keccak.keccak import keccak_uint256s_bigend
 from starkware.cairo.common.bool import FALSE
 from starkware.cairo.common.math_cmp import RC_BOUND
 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.cairo_secp.bigint import bigint_to_uint256
+from starkware.cairo.common.cairo_secp.signature import recover_public_key
+from starkware.cairo.common.uint256 import Uint256, uint256_reverse_endian, uint256_lt
+from starkware.cairo.common.cairo_secp.bigint import bigint_to_uint256, uint256_to_bigint
 from starkware.cairo.common.memset import memset
 
 from src.errors import Errors
-from src.interfaces.interfaces import ICairo1Helpers
+from src.utils.uint256 import uint256_eq
 from src.utils.utils import Helpers
 from src.utils.array import slice
+from src.utils.signature import Signature
 from src.utils.maths import unsigned_div_rem
 
 // @title EcRecover Precompile related functions.
@@ -28,15 +26,23 @@ 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.
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
 
         let (input_padded) = alloc();
@@ -50,19 +56,47 @@ namespace PrecompileEcRecover {
             return (0, output, GAS_COST_EC_RECOVER, 0);
         }
 
-        let msg_hash = Helpers.bytes_to_uint256(32, input_padded);
+        let msg_hash_bigint = Helpers.bytes32_to_bigint(input_padded);
         let r = Helpers.bytes_to_uint256(32, input_padded + 32 * 2);
         let s = Helpers.bytes_to_uint256(32, input_padded + 32 * 3);
 
-        // v - 27, see recover_public_key comment
-        let (success, recovered_address) = ICairo1Helpers.recover_eth_address(
-            msg_hash=msg_hash, r=r, s=s, y_parity=v - 27
-        );
+        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);
+        }
 
+        let (r_bigint) = uint256_to_bigint(r);
+        let (s_bigint) = uint256_to_bigint(s);
+        let (public_key_point, success) = Signature.try_recover_public_key(
+            msg_hash_bigint, r_bigint, s_bigint, v - 27
+        );
         if (success == 0) {
             let (output) = alloc();
             return (0, output, GAS_COST_EC_RECOVER, 0);
         }
+        let (is_public_key_invalid) = EcRecoverHelpers.ec_point_equal(
+            public_key_point, EcPoint(BigInt3(0, 0, 0), BigInt3(0, 0, 0))
+        );
+        if (is_public_key_invalid != 0) {
+            let (output) = alloc();
+            return (0, output, GAS_COST_EC_RECOVER, 0);
+        }
+
+        let (recovered_address) = EcRecoverHelpers.public_key_point_to_eth_address(
+            public_key_point
+        );
 
         let (output) = alloc();
         memset(output, 0, 12);
@@ -83,8 +117,12 @@ namespace EcRecoverHelpers {
     }
 
     // @notice Convert a public key point to the corresponding Ethereum address.
+    // @dev Uses the `KeccakBuiltin` builtin, while the one in Starkware's CairoZero library does not.
     func public_key_point_to_eth_address{
-        pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
     }(public_key_point: EcPoint) -> (eth_address: felt) {
         alloc_locals;
         let (local elements: Uint256*) = alloc();

diff --git a/cairo/src/precompiles/ecadd.cairo b/cairo/src/precompiles/ecadd.cairo
@@ -1,4 +1,4 @@
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.cairo_secp.bigint import BigInt3
 from starkware.cairo.common.uint256 import Uint256
 from starkware.cairo.common.memcpy import memcpy
@@ -22,9 +22,14 @@ namespace PrecompileEcAdd {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
 
         let x0: BigInt3 = Helpers.bytes32_to_bigint(input);

diff --git a/cairo/src/precompiles/ecmul.cairo b/cairo/src/precompiles/ecmul.cairo
@@ -1,4 +1,4 @@
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.cairo_secp.bigint import BigInt3
 from starkware.cairo.common.uint256 import Uint256
 from starkware.cairo.common.memcpy import memcpy
@@ -22,9 +22,14 @@ namespace PrecompileEcMul {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
 
         let x: BigInt3 = Helpers.bytes32_to_bigint(input);

diff --git a/cairo/src/precompiles/kakarot_precompiles.cairo b/cairo/src/precompiles/kakarot_precompiles.cairo
@@ -1,4 +1,4 @@
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.math_cmp import is_nn
 from starkware.cairo.common.math import assert_not_zero
 from starkware.cairo.common.alloc import alloc
@@ -20,7 +20,10 @@ namespace KakarotPrecompiles {
     // @param input The input data.
     // @param caller_address The address of the caller of the precompile. Delegatecall rules apply.
     func cairo_precompile{
-        pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
     }(input_len: felt, input: felt*, caller_address: felt) -> (
         output_len: felt, output: felt*, gas_used: felt, reverted: felt
     ) {

diff --git a/cairo/src/precompiles/modexp.cairo b/cairo/src/precompiles/modexp.cairo
@@ -1,4 +1,4 @@
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.uint256 import Uint256
 from starkware.cairo.common.alloc import alloc
 
@@ -20,9 +20,14 @@ namespace PrecompileModExpUint256 {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
 
         let b_size: Uint256 = Helpers.bytes32_to_uint256(input);

diff --git a/cairo/src/precompiles/p256verify.cairo b/cairo/src/precompiles/p256verify.cairo
@@ -1,5 +1,5 @@
 from starkware.cairo.common.alloc import alloc
-from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
+from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin, KeccakBuiltin
 from starkware.cairo.common.memset import memset
 
 from src.utils.utils import Helpers
@@ -20,9 +20,14 @@ namespace PrecompileP256Verify {
     // @return output_len The output length.
     // @return output The output array.
     // @return gas_used The gas usage of precompile.
-    func run{pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*}(
-        _address: felt, input_len: felt, input: felt*
-    ) -> (output_len: felt, output: felt*, gas_used: felt, reverted: felt) {
+    func run{
+        pedersen_ptr: HashBuiltin*,
+        range_check_ptr,
+        bitwise_ptr: BitwiseBuiltin*,
+        keccak_ptr: KeccakBuiltin*,
+    }(_address: felt, input_len: felt, input: felt*) -> (
+        output_len: felt, output: felt*, gas_used: felt, reverted: felt
+    ) {
         alloc_locals;
 
         let (output) = alloc();