DO NOT MERGE - zkpassport circuit audit comments #96
Conversation
| // 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. |
There was a problem hiding this comment.
It was a rather arbitrary choice. It can be changed to 512, which should be enough for most use cases
| 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. |
There was a problem hiding this comment.
This circuit is no longer used since it can be done with the disclose circuit. So we'll discard it for clarity as part of a larger PR to clean unused code.
| @@ -1,5 +1,7 @@ | |||
| use data_check_expiry::check_expiry; | |||
|
|
|||
| // Suggestion: Use DG1Data type | |||
There was a problem hiding this comment.
This circuit is actually not used on its own. The expiry date is checked directly within the data integrity check circuit. So, this is a circuit that should be discarded as well
| 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 |
There was a problem hiding this comment.
The plan is to have a non-hard-coded root that can be passed as inputs, as there will be multiple sanction lists, and they should be updated regularly. They will be managed similarly to other registries (i.e., certificates and circuits registries). For now, it was hard-coded to have a first working version before we can deploy the rest of the logic to make it dynamic
| csc_pubkey: [u8; CSC_KEY_SIZE], | ||
| ) -> Field { | ||
| // Verify csc_pubkey exists in certificate registry | ||
| // How is this registry merkle tree secured / maintained? |
There was a problem hiding this comment.
The registry is stored on AWS with its root settled onchain in the Registry smart contract, which the onchain verifier uses to check against the latest valid roots. The historical roots can be found here https://registry.zkpassport.id/certificates/history
For now, only we have the right to update the registry, but long-term, it would be better to involve other participants to spread the responsibility to other parties. Each certificate has a list of tags that describes from which source it was pulled (i.e., which masterlists), so it can be independently checked that we got it from official sources such as the ICAO or masterlists from governments such as that of Germany, the Netherlands, Italy, or Sweden
| // when adding a number of years that is not a multiple of 4 (or when landing on century years that are not a multiple of 400). | ||
| assert(current_date.lt(birthdate.add_years(max_age as u32)), "Age is not below max age"); | ||
| } else { | ||
| // I don't understand the approach for `max_age` in this `else` block. It's inconsistent with the previous |
There was a problem hiding this comment.
This is actually intentional. This is to allow the use of this clause to cover equality checks, that is, age = expected age, which can be achieved by setting max age and min age to the same value
| "Age is not above or equal to min age", | ||
| ); | ||
|
|
||
| // If the max_age is specified as 50, based on the comments and docs, you only want to |
There was a problem hiding this comment.
Right, this was actually the expected behaviour when this circuit was made, but it looks like the upper bound inclusivity was mixed up when doing the logic on the SDK side. It makes more sense to make everything inclusive then and handle other cases (exclusive bounds at the level of the SDK)
| // Copyright 2025 ZKPassport | ||
| pragma solidity >=0.8.21; | ||
|
|
||
| // Is this not updateable? |
There was a problem hiding this comment.
It will be soon, hard-coded for now to have a first working testable version of the sanction checks
| * @title IRootRegistry | ||
| * @dev Interface for a root registry | ||
| */ | ||
| // I guess the impl is in some other repo? |
There was a problem hiding this comment.
| address public admin; | ||
| bool public paused; | ||
|
|
||
| // Which vkhashes are these? vkhashes of outer circuits? Please comment storage variables. |
There was a problem hiding this comment.
The vkey hashes of the verifier for each variant of the outer circuit from 4 to 12 subproofs
Please find included:
Thanks!