chore: green light for blackbox_solver audit

acvm-repo/blackbox_solver/src/ecdsa/secp256k1.rs

@@ -11,6 +11,27 @@ use k256::{
 };
 use k256::{Scalar, ecdsa::Signature};
 
+/// Verifies an ECDSA signature over the Secp256k1 elliptic curve.
+///
+/// This function implements ECDSA signature verification on the Secp256k1 curve
+///
+/// # Parameters:
+///
+/// * `hashed_msg` - The 32-byte hash of the message that was signed
+/// * `public_key_x_bytes` - The x-coordinate of the public key (32 bytes, big-endian)
+/// * `public_key_y_bytes` - The y-coordinate of the public key (32 bytes, big-endian)
+/// * `signature` - The 64-byte signature in (r, s) format, where r and s are 32 bytes each
+///
+/// Returns `true` if the signature is valid, `false` otherwise.
+///
+/// The function panic if the following is not true:
+/// - The signature components `r` and `s` must be non-zero
+/// - The public key point must lie on the Secp256k1 curve
+/// - The signature must be "low S" normalized per BIP 0062 to prevent malleability
+///
+/// The function will also panic if `hashed_msg >= k256::Secp256k1::ORDER`.
+/// According to ECDSA specification, the message hash leftmost bits should be truncated
+/// up to the curve order length, and then reduced modulo the curve order.
 pub(super) fn verify_signature(
     hashed_msg: &[u8; 32],
     public_key_x_bytes: &[u8; 32],
@@ -31,15 +52,14 @@ pub(super) fn verify_signature(
     );
 
     let pubkey = PublicKey::from_encoded_point(&point);
-    let pubkey = if pubkey.is_some().into() {
-        pubkey.unwrap()
-    } else {
+    if pubkey.is_none().into() {
         // Public key must sit on the Secp256k1 curve.
         log::warn!("Invalid public key provided for ECDSA verification");
         return false;
-    };
+    }
+    let pubkey = pubkey.unwrap();
 
-    // Note: This is incorrect as it will panic if `hashed_msg >= k256::Secp256k1::ORDER`.
+    // Note: This will panic if `hashed_msg >= k256::Secp256k1::ORDER`.
     // In this scenario we should just take the leftmost bits from `hashed_msg` up to the group order length.
     let z = Scalar::from_repr(*GenericArray::from_slice(hashed_msg)).unwrap();
 

acvm-repo/blackbox_solver/src/ecdsa/secp256r1.rs

@@ -11,6 +11,27 @@ use p256::{
 };
 use p256::{Scalar, ecdsa::Signature};
 
+/// Verifies an ECDSA signature over the Secp256r1 elliptic curve.
+///
+/// This function implements ECDSA signature verification on the Secp256r1 curve
+///
+/// # Parameters:
+///
+/// * `hashed_msg` - The 32-byte hash of the message that was signed
+/// * `public_key_x_bytes` - The x-coordinate of the public key (32 bytes, big-endian)
+/// * `public_key_y_bytes` - The y-coordinate of the public key (32 bytes, big-endian)
+/// * `signature` - The 64-byte signature in (r, s) format, where r and s are 32 bytes each
+///
+/// Returns `true` if the signature is valid, `false` otherwise.
+///
+/// The function panic if the following is not true:
+/// - The signature components `r` and `s` must be non-zero
+/// - The public key point must lie on the Secp256r1 curve
+/// - The signature must be "low S" normalized per BIP 0062 to prevent malleability
+///
+/// The function will also panic if `hashed_msg >= p256::NistP256::ORDER`.
+/// According to ECDSA specification, the message hash leftmost bits should be truncated
+/// up to the curve order length, and then reduced modulo the curve order.
 pub(super) fn verify_signature(
     hashed_msg: &[u8],
     public_key_x_bytes: &[u8; 32],
@@ -31,15 +52,14 @@ pub(super) fn verify_signature(
     );
 
     let pubkey = PublicKey::from_encoded_point(&point);
-    let pubkey = if pubkey.is_some().into() {
-        pubkey.unwrap()
-    } else {
+    if pubkey.is_none().into() {
         // Public key must sit on the Secp256r1 curve.
         log::warn!("Invalid public key provided for ECDSA verification");
         return false;
     };
+    let pubkey = pubkey.unwrap();
 
-    // Note: This is incorrect as it will panic if `hashed_msg >= p256::NistP256::ORDER`.
+    // Note: This will panic if `hashed_msg >= p256::NistP256::ORDER`.
     // In this scenario we should just take the leftmost bits from `hashed_msg` up to the group order length.
     let z = Scalar::from_repr(*GenericArray::from_slice(hashed_msg)).unwrap();
 

acvm-repo/blackbox_solver/src/hash.rs

@@ -32,7 +32,7 @@ pub fn sha256_compression(state: &mut [u32; 8], msg_blocks: &[u32; 16]) {
 }
 
 const KECCAK_LANES: usize = 25;
-
+/// Keccak permutation for a state of size 1600 bits, represented by 25 lanes of 64 bits (25*64 = 1600)
 pub fn keccakf1600(
     mut state: [u64; KECCAK_LANES],
 ) -> Result<[u64; KECCAK_LANES], BlackBoxResolutionError> {

acvm-repo/blackbox_solver/src/logic.rs

@@ -8,16 +8,15 @@ pub fn bit_xor<F: AcirField>(lhs: F, rhs: F, num_bits: u32) -> F {
     bitwise_op(lhs, rhs, num_bits, |lhs_byte, rhs_byte| lhs_byte ^ rhs_byte)
 }
 
+/// Performs a bitwise operation on two field elements by treating them as byte arrays.
+///
+/// Both field elements are converted to little-endian byte arrays and masked to keep only
+/// the lowest `num_bits` bits. The provided operation `op` is then applied byte-by-byte,
+/// and the result is converted back to a field element.
+/// This function works for any `num_bits` value and does not assume it to be a multiple of 8.
 fn bitwise_op<F: AcirField>(lhs: F, rhs: F, num_bits: u32, op: fn(u8, u8) -> u8) -> F {
-    // XXX: Gadgets like SHA256 need to have their input be a multiple of 8
-    // This is not a restriction caused by SHA256, as it works on bits
-    // but most backends assume bytes.
-    // We could implicitly pad, however this may not be intuitive for users.
-    // assert!(
-    //     num_bits % 8 == 0,
-    //     "num_bits is not a multiple of 8, it is {}",
-    //     num_bits
-    // );
+    // We could explicitly expect `num_bits` to be a multiple of 8 as most backends assume bytes:
+    // assert!(num_bits % 8 == 0, "num_bits is not a multiple of 8, it is {num_bits}");
 
     let lhs_bytes = mask_to_le_bytes(lhs, num_bits);
     let rhs_bytes = mask_to_le_bytes(rhs, num_bits);
@@ -50,8 +49,9 @@ fn mask_vector_le(bytes: &mut [u8], num_bits: usize) {
     // Find how many bits to keep in that byte (0-7)
     let mask_power = num_bits % 8;
 
-    // Mask the partial byte if any. Otherwise, if our `num_bits` is a multiple of 8
-    // we will get a `mask_power` of zero and thus zero out the byte at `array_mask_index`.
+    // If `mask_power` is non-zero, this keeps only the lower `mask_power` bits of the byte.
+    // If `mask_power` is zero (when `num_bits` is a multiple of 8), this zeros out the byte,
+    // which is correct since that byte is the first one beyond what we want to keep.
     bytes[array_mask_index] &= 2u8.pow(mask_power as u32) - 1;
 
     // Zero out all remaining bytes