DO NOT MERGE - zkpassport circuit audit comments

package.json

@@ -48,5 +48,6 @@
     "ts-jest": "^29.1.2",
     "tsx": "^4.19.3",
     "typescript": "^5.0.0"
-  }
+  },
+  "packageManager": "[email protected]+sha512.8cd0e31bd60779ef4ca92b855fb3462c7ec35ce8b345752b7349a68239776417f46d41d79c8047242d9c93b48a1516f64c7444ebe747d9a02bf26868e6fa1f2b"
 }

src/noir/bin/bind/evm/src/main.nr

@@ -1,6 +1,7 @@
 use bind::calculate_param_commitment_sha2;
 use commitment::nullify;
 
+// Visually inspected.
 fn main(
     comm_in: pub Field,
     salt: Field,
@@ -9,7 +10,7 @@ fn main(
     // @committed
     // The data is public (via the parameter commitment) so verifiers can check the data
     // provided to the proof is correct
-    data: [u8; 500],
+    data: [u8; 500], // Why the choice of 500 bytes? I'd have expected a nice round number like 256 or 512, to enable neat hashing of large amounts of bind data.
     service_scope: pub Field,
     service_subscope: pub Field,
 ) -> pub (Field, Field) {

src/noir/bin/compare/age/evm/src/main.nr

@@ -9,6 +9,14 @@ fn main(
     // @committed
     // The current date timestamp is public (via the parameter commitment) so verifiers can check the date
     // provided to the proof is correct
+    // Danger: need to assert that this `current_date` is equal to the `current_date` public input 
+    // of the "Data Integrity Check" circuit. I can't find where that check is happening in the codebase.
+    // It's not happening in the "outer circuit". Perhaps it happens in the Verifier code?
+    // I read somewhere that as a lovely optimisation, a user generates the first 3 proofs when they
+    // first scan their passport, and then they only need to generate the disclosure proof & outer proof on demand.
+    // This particular disclosure circuit might not be amenable to that optimisation, since the Data Integrity Check
+    // circuit would have to be re-proven with the current date.
+    // Maybe you've already thought of all this :)
     current_date: u64,
     // @committed
     // The minimum age required is public (via the parameter commitment) so verifiers can check

src/noir/bin/compare/birthdate/evm/src/main.nr

@@ -2,6 +2,7 @@ use commitment::nullify;
 use compare_date_lib::{calculate_param_commitment_sha2, compare_date, get_birthdate};
 use utils::PROOF_TYPE_BIRTHDATE;
 
+// Visually inspected.
 fn main(
     comm_in: pub Field,
     salt: Field,
@@ -10,6 +11,7 @@ fn main(
     // @committed
     // The current date timestamp is public (via the parameter commitment) so verifiers can check the date
     // provided to the proof is correct
+    // Danger: see concerns in `compare/age/evm/src/main.nr`.
     current_date: u64,
     // @committed
     // The minimum date required is public (via the parameter commitment) so verifiers can check

src/noir/bin/compare/citizenship/src/main.nr

@@ -1,13 +1,27 @@
 use commitment::nullify;
 use compare_citizenship::compare_citizenship;
 
+// Q: Is this superfluous, given that this can be achieved with `disclose`?
 fn main(
     comm_in: pub Field,
     salt: Field,
     private_nullifier: Field,
     dg1: [u8; 95],
     // The country is public so verifiers can check
     // the country provided to the proof is correct
+    // Danger: these public inputs are inconsistent with those of other disclosure circuits that I've looked at so far.
+    // In particular, I suspect this is breaking the rigid ordering that is expected by the Outer circuit.
+    // The outer circuit expects public inputs of disclosure proofs to be in the order:
+    // - comm_in
+    // - service_scope
+    // - service_subscope
+    // - param_commitment
+    // - scoped_nullifier
+    //
+    // But this circuit has no `param_commitment`, and although the `country` could feasibly replace
+    // that `param_commitment` field, it is in the wrong position, and it is a str<3> type, which I
+    // presume is naively serialized into 3 distinct fields.
+    // So how is this circuit's public inputs "shape" compatible with the Outer circuit?
     country: pub str<3>,
     service_scope: pub Field,
     service_subscope: pub Field,

src/noir/bin/compare/expiry/evm/src/main.nr

@@ -10,6 +10,7 @@ fn main(
     // @committed
     // The current date timestamp is public so verifiers can check the date
     // provided to the proof is correct
+    // Danger: see my similar concern in `compare/age/evm/src/main.nr` 
     current_date: u64,
     // @committed
     // The minimum date required is public so verifiers can check

src/noir/bin/data-check/expiry/src/main.nr

@@ -1,5 +1,7 @@
 use data_check_expiry::check_expiry;
 
+// Suggestion: Use DG1Data type
+// Question: How is this circuit used, and how does it fit in with all the other circuits?
 fn main(dg1: [u8; 95], current_date_timestamp: pub u64) {
     // Check the ID is not expired first
     check_expiry(dg1, current_date_timestamp);

src/noir/bin/data-check/integrity/sa_sha256/dg_sha256/src/main.nr

@@ -18,6 +18,10 @@ fn main(
     // Get the length of e_content by parsing the ASN.1
     // Safety: This is safe because the length must be correct for econtent to hash to
     // the expected digest in signed attributes as checked below in check_signed_attributes_sha256
+    // Warning: A pattern in other circuits has been to constrain that the length returned from one of these unconstrained calls
+    // is somewhat correct, by asserting that all elements _after_ the claimed length are all zero.
+    // I can't see that being done within any of the functions of this circuit, so e_content might contain
+    // nonzero entries after this `e_content_size`.
     let e_content_size = unsafe { utils::unsafe_get_asn1_element_length(e_content) };
     // Check the integrity of the data
     check_dg1_sha256(dg1, e_content, e_content_size);

src/noir/bin/disclose/bytes/evm/src/main.nr

@@ -7,13 +7,13 @@ fn main(
     private_nullifier: Field,
     dg1: [u8; 95],
     // @committed
-    // The disclose mask is public (via the parameter commitment) so verifiers can check the mask
+    // The disclose mask is public (via the `param_commitment`) so verifiers can check the mask
     disclose_mask: [u8; 90],
     service_scope: pub Field,
     service_subscope: pub Field,
 ) -> pub (Field, Field) {
     // @committed
-    // The disclose bytes are public (via the parameter commitment) so verifiers can check the bytes
+    // The disclosed bytes are public (via the `param_commitment`) so verifiers can check the bytes
     let disclosed_bytes = get_disclosed_bytes(dg1, disclose_mask);
     let scoped_nullifier = nullify(
         comm_in,

src/noir/bin/disclose/bytes/standard/src/main.nr

@@ -5,10 +5,12 @@ fn main(
     comm_in: pub Field,
     salt: Field,
     private_nullifier: Field,
+// Suggestion: Use DG1Data type
     dg1: [u8; 95],
     // @committed
     // The disclose mask is public (via the parameter commitment) so verifiers can check the mask
-    disclose_mask: [u8; 90],
+    // ^^^ I see. I was going to comment that these masks should be `bool` types, or the existing `u8` types should be constrained to be `0 or 1` only, to prevent malicious usage of the `mask[i] * mrz[i]` calculation in `get_disclosed_bytes`.
+    disclose_mask: [u8; 90], // Consider replacing `90` with a global constant.
     service_scope: pub Field,
     service_subscope: pub Field,
 ) -> pub (Field, Field) {

src/noir/bin/disclose/flags/src/main.nr

@@ -2,10 +2,13 @@ use commitment::nullify;
 use disclose::get_disclosed_data;
 use utils::{DisclosedData, DiscloseFlags};
 
+// Apparently this is not used. Consider removing. 
+// Not audited.
 fn main(
     comm_in: pub Field,
     salt: Field,
     private_nullifier: Field,
+// Suggestion: Use DG1Data type
     dg1: [u8; 95],
     disclose_flags: DiscloseFlags,
     service_scope: pub Field,

src/noir/bin/exclusion-check/issuing-country/evm/src/main.nr

@@ -2,6 +2,7 @@ use commitment::nullify;
 use exclusion_check_country::{calculate_param_commitment_sha2, check_issuing_country_exclusion};
 use utils::PROOF_TYPE_ISSUING_COUNTRY_EXCLUSION;
 
+// Visually inspected
 fn main(
     comm_in: pub Field,
     salt: Field,

src/noir/bin/exclusion-check/nationality/evm/src/main.nr

@@ -2,6 +2,7 @@ use commitment::nullify;
 use exclusion_check_country::{calculate_param_commitment_sha2, check_nationality_exclusion};
 use utils::PROOF_TYPE_NATIONALITY_EXCLUSION;
 
+// Visually inspected
 fn main(
     comm_in: pub Field,
     salt: Field,

src/noir/bin/exclusion-check/sanctions/evm/src/main.nr

@@ -5,6 +5,7 @@ use exclusion_check_sanctions::{
 };
 use utils::PROOF_TYPE_SANCTIONS_EXCLUSION;
 
+
 fn main(
     comm_in: pub Field,
     salt: Field,
@@ -15,6 +16,15 @@ fn main(
     service_scope: pub Field,
     service_subscope: pub Field,
 ) -> pub (Field, Field) {
+    // So is the plan to always hard-code the root in the circuit, or to instead
+    // have the root be a public input (or committed into param_commitment) so that
+    // the list can be more-easily updated onchain?
+    // Iiuc, it might make more sense to not have a hard-coded root, and instead
+    // instantiate this as:
+    // `trees = SanctionsSparseMerkleTrees { root };` using the circuit private input `root`,
+    // then checking that root within the Verifier.
+    // Also "trees" (plural) is confusing, because it appears to be just one big tree,
+    // which contains leaves that might represent one of three different concepts.
     let trees = SanctionsSparseMerkleTrees::default();
     assert(trees.root() == root);
     // Check that nationality of the passport holder is not in the list of countries

src/noir/bin/inclusion-check/issuing-country/evm/src/main.nr

@@ -2,6 +2,7 @@ use commitment::nullify;
 use inclusion_check_country::{calculate_param_commitment_sha2, check_issuing_country_inclusion};
 use utils::PROOF_TYPE_ISSUING_COUNTRY_INCLUSION;
 
+// Visually inspected.
 fn main(
     comm_in: pub Field,
     salt: Field,

src/noir/bin/inclusion-check/nationality/evm/src/main.nr

@@ -2,6 +2,7 @@ use commitment::nullify;
 use inclusion_check_country::{calculate_param_commitment_sha2, check_nationality_inclusion};
 use utils::PROOF_TYPE_NATIONALITY_INCLUSION;
 
+// Visually inspected.
 fn main(
     comm_in: pub Field,
     salt: Field,

src/noir/bin/main/outer/count_9/src/main.nr

@@ -30,6 +30,7 @@ use outer_lib::{
 use std::verify_proof_with_type;
 global PROOF_TYPE_HONK_ZK: u32 = 7;
 
+// Visually inspected.
 fn verify_subproofs(
     // Root of the certificate merkle tree
     certificate_registry_root: Field,
@@ -128,8 +129,8 @@ fn verify_subproofs(
 
     // Commitment out from CSC to DSC circuit == commitment in from DSC to ID Data circuit
     assert_eq(
-        csc_to_dsc_proof.public_inputs[0],
-        dsc_to_id_data_proof.public_inputs[0],
+        csc_to_dsc_proof.public_inputs[0], // comm_out
+        dsc_to_id_data_proof.public_inputs[0], // comm_in
         "Commitment out from CSC to DSC circuit != commitment in from DSC to ID Data circuit",
     );
 
@@ -146,8 +147,8 @@ fn verify_subproofs(
 
     // Commitment out from DSC to ID Data circuit == commitment in from integrity check circuit
     assert_eq(
-        dsc_to_id_data_proof.public_inputs[1],
-        integrity_check_proof.public_inputs[0],
+        dsc_to_id_data_proof.public_inputs[1], // comm_out
+        integrity_check_proof.public_inputs[0], // comm_in
         "Commitment out from DSC to ID Data circuit != commitment in from integrity check circuit",
     );
 
@@ -166,8 +167,8 @@ fn verify_subproofs(
     for i in 0..disclosure_proofs.len() {
         // Commitment out from integrity check circuit == commitment in from disclosure circuit
         assert_eq(
-            integrity_check_proof.public_inputs[1],
-            disclosure_proofs[i].public_inputs[0],
+            integrity_check_proof.public_inputs[1], // comm_out
+            disclosure_proofs[i].public_inputs[0], // comm_in
             "Commitment out from integrity check circuit != commitment in from disclosure circuit",
         );
 

src/noir/bin/sig-check/dsc/tbs_700/ecdsa/nist/p384/sha256/src/main.nr

@@ -4,6 +4,7 @@ use sig_check_common::sha256_and_check_data_to_sign;
 use sig_check_ecdsa::verify_nist_p384;
 use utils::{concat_array, split_array};
 
+// Visually inspected this example.
 fn main(
     certificate_registry_root: pub Field,
     certificate_registry_index: Field,
@@ -18,6 +19,8 @@ fn main(
 ) -> pub Field {
     // Get the length of tbs_certificate by parsing the ASN.1
     // Safety: This is safe because the length must be correct for the hash and signature to be valid
+    // There's a check that the tbs_certificate is all zeroes after `tbs_certificate_len` (within `verify..()`). Consider moving
+    // that check to immediately after this unsafe call, to make it clear that the check is being done.
     let tbs_certificate_len = unsafe { utils::unsafe_get_asn1_element_length(tbs_certificate) };
     let (r, s) = split_array(dsc_signature);
     let msg_hash = sha256_and_check_data_to_sign(tbs_certificate, tbs_certificate_len);

src/noir/bin/sig-check/id-data/tbs_700/ecdsa/nist/p384/sha256/src/main.nr

@@ -5,11 +5,13 @@ use sig_check_common::sha256_and_check_data_to_sign;
 use sig_check_ecdsa::verify_nist_p384;
 use utils::split_array;
 
+// Visually inspected this binary, as an example.
+// Consider using globals for all of the magic numbers that are the same for all circuit variants (95, 700), or better still: custom types, as has been begun in `types.nr`.
 fn main(
     comm_in: pub Field,
     salt_in: Field,
     salt_out: Field,
-    dg1: [u8; 95],
+    dg1: [u8; 95], // Consider using the DG1Data type
     dsc_pubkey_x: [u8; 48],
     dsc_pubkey_y: [u8; 48],
     sod_signature: [u8; 96],

src/noir/lib/commitment/common/src/lib.nr

@@ -40,6 +40,7 @@ fn has_value_in_tags<let BIT_SIZE: u32, let TAGS_LEN: u32>(
     result
 }
 
+// Visually inspected.
 pub fn calculate_certificate_registry_leaf<let N: u32>(
     tags: [Field; 3],
     cert_type: u8,
@@ -48,14 +49,15 @@ pub fn calculate_certificate_registry_leaf<let N: u32>(
 ) -> Field {
     // Pack certificate type and country code into a single field
     let country_bytes: [u8; 3] = country.as_bytes();
+    // No need to cast cert_type `as u8`.
     let unpacked_bytes: [u8; 4] =
         [cert_type as u8, country_bytes[0], country_bytes[1], country_bytes[2]];
     // Prepare and hash Poseidon2 inputs
-    let mut poseidon_inputs: [Field; 4 + (N + 30) / 31] = [0; 4 + (N + 30) / 31];
+    let mut poseidon_inputs: [Field; 4 + (N + 30) / 31] = [0; 4 + (N + 30) / 31]; // [Field; 4 + ceil(N/31)]
     poseidon_inputs[0] = tags[0];
     poseidon_inputs[1] = tags[1];
     poseidon_inputs[2] = tags[2];
-    poseidon_inputs[3] = utils::pack_be_bytes_into_fields::<4, 1, 31>(unpacked_bytes)[0];
+    poseidon_inputs[3] = utils::pack_be_bytes_into_fields::<4, 1, 31>(unpacked_bytes)[0]; // consider using pack_be_bytes_into_field (singular)
     let packed_pubkey = utils::pack_be_bytes_into_fields::<N, (N + 30) / 31, 31>(public_key);
     for i in 0..packed_pubkey.len() {
         poseidon_inputs[4 + i] = packed_pubkey[i] as Field;
@@ -159,8 +161,10 @@ pub fn test_calculate_certificate_registry_leaf() {
     assert(leaf_hash == 0x10370abe147332c5b039fb820a32964f7567998e9c54f93595bb95a1dc4ba62f);
 }
 
+// Duplicate of `get_issuing_country_from_mrz` in utils/src/lib.nr. Consider deleting this one and only using that one. (Also see my comments on that one).
 pub fn get_country_from_dg1(dg1: [u8; 95]) -> str<3> {
     // There 5 padding bytes in the dg1 before the actual MRZ
+    // Consider globals for the magic numbers 5, 3.
     let country_offset = 5 + PASSPORT_MRZ_COUNTRY_INDEX;
     let mut country_bytes: [u8; 3] = [0; 3];
     for i in 0..3 {
@@ -174,16 +178,18 @@ pub fn get_country_from_dg1(dg1: [u8; 95]) -> str<3> {
     country
 }
 
+// Visually inspected.
 pub fn hash_salt_country_tbs<let TBS_MAX_SIZE: u32>(
     salt: Field,
     country: str<3>,
     tbs: [u8; TBS_MAX_SIZE],
 ) -> Field {
     let country_bytes: [u8; 3] = country.as_bytes();
-    let mut result: [Field; 2 + ((TBS_MAX_SIZE + 30) / 31)] = [0; 2 + ((TBS_MAX_SIZE + 30) / 31)];
+    // Consider renaming `result` to something like `hash_input` or `commitment_preimage`
+    let mut result: [Field; 2 + ((TBS_MAX_SIZE + 30) / 31)] = [0; 2 + ((TBS_MAX_SIZE + 30) / 31)]; // [Field; 2 + ceil(TBS_MAX_SIZE/31)]
     result[0] = salt;
 
-    let packed_country: Field = pack_be_bytes_into_fields::<3, 1, 31>(country_bytes)[0];
+    let packed_country: Field = pack_be_bytes_into_fields::<3, 1, 31>(country_bytes)[0]; // Technically already packed within `calculate_certificate_registry_leaf` -- albeit it was mixed-in with `cert_type` in that fn.
     result[1] = packed_country;
 
     let packed_tbs: [Field; (TBS_MAX_SIZE + 30) / 31] =
@@ -200,6 +206,8 @@ pub fn hash_salt_dg1_private_nullifier<let N: u32>(
     dg1: [u8; N],
     private_nullifier: Field,
 ) -> Field {
+    // You might be able to just write `= std::mem::zeroed()` to reduce verbosity.
+    // Or `let mut result = [0 as Field; 2 + ((N + 30) / 31)];`
     let mut result: [Field; 2 + ((N + 30) / 31)] = [0; 2 + ((N + 30) / 31)];
     result[0] = salt;
 
@@ -211,11 +219,13 @@ pub fn hash_salt_dg1_private_nullifier<let N: u32>(
     Poseidon2::hash(result, 2 + ((N + 30) / 31))
 }
 
+// Visually inspected.
 pub fn calculate_private_nullifier<let DG1: u32, let ECONTENT: u32, let SIG: u32>(
     dg1: [u8; DG1],
     e_content: [u8; ECONTENT],
     sod_sig: [u8; SIG],
 ) -> Field {
+    // You might be able to just write `= std::mem::zeroed()` to reduce verbosity.
     let mut result: [Field; (DG1 + 30) / 31 + (ECONTENT + 30) / 31 + (SIG + 30) / 31] =
         [0; (DG1 + 30) / 31 + (ECONTENT + 30) / 31 + (SIG + 30) / 31];
 
@@ -241,6 +251,7 @@ pub fn calculate_private_nullifier<let DG1: u32, let ECONTENT: u32, let SIG: u32
     )
 }
 
+// Visually inspected.
 pub fn hash_salt_country_signed_attr_dg1_e_content_private_nullifier<let SA: u32, let ECONTENT: u32, let DG1: u32>(
     salt: Field,
     country: str<3>,
@@ -252,6 +263,7 @@ pub fn hash_salt_country_signed_attr_dg1_e_content_private_nullifier<let SA: u32
 ) -> Field {
     let country_bytes: [u8; 3] = country.as_bytes();
 
+    // You might be able to just write `= std::mem::zeroed()` to reduce verbosity.
     let mut result: [Field; 4 + (SA + 30) / 31 + ((DG1 + 30) / 31) + ((ECONTENT + 30) / 31)] =
         [0; 4 + (SA + 30) / 31 + ((DG1 + 30) / 31) + ((ECONTENT + 30) / 31)];
     result[0] = salt;
@@ -285,6 +297,7 @@ pub fn hash_salt_country_signed_attr_dg1_e_content_private_nullifier<let SA: u32
     )
 }
 
+// Visually inspected.
 // Returns the merkle root of the tree from the provided leaf, index and hash_path, using the Poseidon2 hash function
 // Arity is expected to be 2 and the the tree depth is equal to the hash_path array length
 pub fn compute_merkle_root<let N: u32>(leaf: Field, index: Field, hash_path: [Field; N]) -> Field {
@@ -302,6 +315,8 @@ pub fn compute_merkle_root<let N: u32>(leaf: Field, index: Field, hash_path: [Fi
     current
 }
 
+// Recommendation: a negative test that fails a merkle root inclusion check.
+
 #[test]
 fn test_compute_merkle_root1() {
     let leaf = 0x2fe190f39de3fcf4cbc2eb334d0dc76e4d06f2350aa6056c91ff5f11ded9fb4a;