ecrecover soundness (address not recovered) (#930)

* wip: preliminary work sig circuit/lookp

* feat: handle panics and edge cases in sig circuit

* fix: handle v not boolean

* fix: fq modulus not assigned and bus-mapping handle overflowing r/s

* fix: sig_v handle invalid values (lookup) and is_valid dev_load fix

* refactor (review comment)

* fix: no sig table lookup for invalid sig_v

* remove wrong test case (assertion fail)

Author: roynalnaruto

Cargo.lock

@@ -20,6 +20,7 @@ poseidon-circuit = { git = "https://github.com/scroll-tech/poseidon-circuit.git"
 itertools = "0.10"
 lazy_static = "1.4"
 log = "0.4.14"
+num = "0.4"
 rand = { version = "0.8", optional = true }
 serde = {version = "1.0.130", features = ["derive"] }
 serde_json = "1.0.66"

bus-mapping/Cargo.toml

@@ -1,8 +1,13 @@
 use eth_types::{
-    sign_types::{recover_pk, SignData},
+    sign_types::{biguint_to_32bytes_le, recover_pk, SignData, SECP256K1_Q},
     Bytes, ToBigEndian, ToLittleEndian,
 };
-use halo2_proofs::halo2curves::secp256k1::Fq;
+use ethers_core::k256::elliptic_curve::PrimeField;
+use halo2_proofs::halo2curves::{
+    group::prime::PrimeCurveAffine,
+    secp256k1::{Fq, Secp256k1Affine},
+};
+use num::{BigUint, Integer};
 
 use crate::{
     circuit_input_builder::PrecompileEvent,
@@ -23,41 +28,41 @@ pub(crate) fn opt_data(
     });
     let aux_data = EcrecoverAuxData::new(input_bytes, output_bytes);
 
-    // only if sig_v was a valid recovery ID, then we proceed to populate the ecrecover events.
+    // We skip the validation through sig circuit if r or s was not in canonical form.
+    let opt_sig_r: Option<Fq> = Fq::from_bytes(&aux_data.sig_r.to_le_bytes()).into();
+    let opt_sig_s: Option<Fq> = Fq::from_bytes(&aux_data.sig_s.to_le_bytes()).into();
+    if opt_sig_r.zip(opt_sig_s).is_none() {
+        return (None, Some(PrecompileAuxData::Ecrecover(aux_data)));
+    }
+
     if let Some(sig_v) = aux_data.recovery_id() {
-        if let Ok(recovered_pk) = recover_pk(
+        let recovered_pk = recover_pk(
             sig_v,
             &aux_data.sig_r,
             &aux_data.sig_s,
             &aux_data.msg_hash.to_be_bytes(),
-        ) {
-            let sign_data = SignData {
-                signature: (
-                    Fq::from_bytes(&aux_data.sig_r.to_le_bytes()).unwrap(),
-                    Fq::from_bytes(&aux_data.sig_s.to_le_bytes()).unwrap(),
-                    sig_v,
-                ),
-                pk: recovered_pk,
-                msg: Bytes::default(),
-                msg_hash: Fq::from_bytes(&aux_data.msg_hash.to_le_bytes()).unwrap(),
-            };
-            assert_eq!(aux_data.recovered_addr, sign_data.get_addr());
-            (
-                Some(PrecompileEvent::Ecrecover(sign_data)),
-                Some(PrecompileAuxData::Ecrecover(aux_data)),
-            )
-        } else {
-            log::warn!(
-                "could not recover pubkey. ecrecover aux_data={:?}",
-                aux_data
-            );
-            (None, Some(PrecompileAuxData::Ecrecover(aux_data)))
-        }
+        )
+        .unwrap_or(Secp256k1Affine::identity());
+        let sign_data = SignData {
+            signature: (
+                Fq::from_bytes(&aux_data.sig_r.to_le_bytes()).unwrap(),
+                Fq::from_bytes(&aux_data.sig_s.to_le_bytes()).unwrap(),
+                sig_v,
+            ),
+            pk: recovered_pk,
+            msg: Bytes::default(),
+            msg_hash: {
+                let msg_hash = BigUint::from_bytes_be(&aux_data.msg_hash.to_be_bytes());
+                let msg_hash = msg_hash.mod_floor(&*SECP256K1_Q);
+                let msg_hash_le = biguint_to_32bytes_le(msg_hash);
+                Fq::from_repr(msg_hash_le).unwrap()
+            },
+        };
+        (
+            Some(PrecompileEvent::Ecrecover(sign_data)),
+            Some(PrecompileAuxData::Ecrecover(aux_data)),
+        )
     } else {
-        log::warn!(
-            "invalid recoveryId for ecrecover. sig_v={:?}",
-            aux_data.sig_v
-        );
         (None, Some(PrecompileAuxData::Ecrecover(aux_data)))
     }
 }

bus-mapping/src/evm/opcodes/precompiles/ecrecover.rs

@@ -15,6 +15,7 @@ use halo2_proofs::{
     halo2curves::{
         group::{
             ff::{Field as GroupField, PrimeField},
+            prime::PrimeCurveAffine,
             Curve,
         },
         secp256k1::{self, Secp256k1Affine},
@@ -101,6 +102,9 @@ pub fn get_dummy_tx() -> (TransactionRequest, Signature) {
 impl SignData {
     /// Recover address of the signature
     pub fn get_addr(&self) -> Address {
+        if self.pk == Secp256k1Affine::identity() {
+            return Address::zero();
+        }
         let pk_le = pk_bytes_le(&self.pk);
         let pk_be = pk_bytes_swap_endianness(&pk_le);
         let pk_hash = keccak256(pk_be);

eth-types/src/sign_types.rs

@@ -622,10 +622,10 @@ impl<F: Field, const XI_0: i64> EccCircuit<F, XI_0> {
         let infinity = EcPoint::construct(
             ecc_chip
                 .field_chip()
-                .load_private(ctx, Value::known(0.into())),
+                .load_constant(ctx, fe_to_biguint(&Fq::zero())),
             ecc_chip
                 .field_chip()
-                .load_private(ctx, Value::known(0.into())),
+                .load_constant(ctx, fe_to_biguint(&Fq::zero())),
         );
         // for invalid case, take a random point.
         let dummy_g1 = ecc_chip.load_random_point::<G1Affine>(ctx);