security: circuit hardening from internal audit#3
Conversation
Tier 1 — Constraint additions (no layout changes): - Add merkle index binary constraints (all circuits with Merkle proofs) - Add Field-to-u64 range checks for all timestamp/coordinate comparisons - Add boolean constraints on lat/lon sign flags - Add path_depth range check in selective_disclosure - Add claim_length/assertion_length/sig_structure_length bounds checks - Add leaf_key_type consistency with cert_algorithm in proof_a - Add P-384 ECDSA r/s non-zero checks - Add cert_not_before/cert_not_after cross-check in image_aggregator - Add content_hash_offset cross-check in image_aggregator - Add link_commit non-zero check in image_aggregator Tier 2 — Public input layout changes: - Make skip_content_hash_binding and skip_assertion_hash_verification public inputs in all ProofA variants (PROOF_A_PUBLIC_VALUES: 53 → 55) - Expose vk_a_hash and vk_b_hash as public outputs from image_aggregator (IMAGE_AGG_PUBLIC_VALUES: 10 → 12) - Expose vk_hash_left and vk_hash_right as public outputs from tree_aggregator (maintains 12 total public values) Tier 3 — Document known limitations in SECURITY.md: - Metadata values client-extracted, not parsed in-circuit - Certificate timestamps witness-provided - bytes_to_field BN254 overflow (future breaking fix) - 2-cert chain intermediate_leaf unconstrained - Cross-equator bounding box limitation All 12 modified circuits compile and pass tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds SECURITY.md and broad circuit hardening: public-input layout extensions for aggregators, cross-proof equality checks, buffer-length bounds, Field↔u64 roundtrip assertions, boolean/binary constraints, Merkle-index validation, non-zero ECDSA scalar checks, and stricter verifier monotonicity. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ProofA as ProofA
participant ProofB as ProofB
participant ImageAgg as ImageAggregator
participant TreeAgg as TreeAggregator
participant Verifier as AztecVerifier
Client->>ProofA: submit ProofA + public_values (55)
Client->>ProofB: submit ProofB + public_values
ProofA->>ImageAgg: verify_honk_proof(55) → expose commits + vk_a_hash/vk_b_hash
ProofB->>ImageAgg: verify_honk_proof(...) → provide matching cert/time/offset publics
ImageAgg->>TreeAgg: forward child public_values_left/right (12) incl. vk_hashes
TreeAgg->>Verifier: combined_commitment, vk_hash_left, vk_hash_right
Verifier->>Verifier: enforce u64-fit for sizes and require new_tree_size > current_size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
proof_a/src/main.nr (1)
445-458: Edit time u64 checks placed outside conditional block - inconsistent with other variants.Unlike
proof_a_rsa_4096,proof_a_ecdsa_p384, andproof_a_skipwhich place these checks inside theif exact_edit_timestamp != 0block, this circuit validates edit time fields unconditionally. While functionally safe (validates unused values when no edit), this creates inconsistency.Consider moving lines 445-448 inside the
ifblock for consistency:♻️ Suggested refactor for consistency
- assert(exact_edit_timestamp as u64 as Field == exact_edit_timestamp, "exact_edit_timestamp exceeds u64"); - assert(edit_time_min as u64 as Field == edit_time_min, "edit_time_min exceeds u64"); - assert(edit_time_max as u64 as Field == edit_time_max, "edit_time_max exceeds u64"); - if exact_edit_timestamp != 0 { + assert(exact_edit_timestamp as u64 as Field == exact_edit_timestamp, "exact_edit_timestamp exceeds u64"); + assert(edit_time_min as u64 as Field == edit_time_min, "edit_time_min exceeds u64"); + assert(edit_time_max as u64 as Field == edit_time_max, "edit_time_max exceeds u64"); let edit_ts = exact_edit_timestamp as u64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proof_a/src/main.nr` around lines 445 - 458, The three unconditional assertions that cast and bound-check exact_edit_timestamp, edit_time_min, and edit_time_max (the assert(...) lines that check "... exceeds u64") should be moved inside the existing if exact_edit_timestamp != 0 branch so the u64 casts/asserts for exact_edit_timestamp, edit_time_min, and edit_time_max are only executed when an edit exists; update the code so the sequence is: if exact_edit_timestamp != 0 { perform the u64 casts/asserts for exact_edit_timestamp, edit_time_min, edit_time_max, then assign edit_ts/edit_min/edit_max and check edit_ts >= edit_min and edit_ts <= edit_max } else { keep the existing zero checks for edit_time_min and edit_time_max } to match the pattern used in proof_a_rsa_4096, proof_a_ecdsa_p384, and proof_a_skip.proof_a_ecdsa_p256/src/main.nr (1)
231-233: Consider validating edit time bounds unconditionally.When
exact_edit_timestamp != 0, the range checks foredit_time_minandedit_time_maxare applied. However, a malicious prover could setexact_edit_timestamp = 0while providing non-zeroedit_time_min/edit_time_maxvalues that exceed u64 range. The else branch at lines 240-242 only checks equality to zero, not representability.This is a minor gap since the else branch enforces
edit_time_min == 0andedit_time_max == 0, but adding u64 range checks unconditionally (outside the if/else) would be more defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proof_a_ecdsa_p256/src/main.nr` around lines 231 - 233, The u64 representability assertions for edit_time_min/edit_time_max are currently only applied when exact_edit_timestamp != 0, leaving a gap if exact_edit_timestamp == 0; move or duplicate the assertions so that assert(edit_time_min as u64 as Field == edit_time_min, "edit_time_min exceeds u64") and assert(edit_time_max as u64 as Field == edit_time_max, "edit_time_max exceeds u64") run unconditionally (outside the if/else), while keeping the existing else checks that enforce edit_time_min == 0 and edit_time_max == 0; reference the symbols exact_edit_timestamp, edit_time_min, and edit_time_max to locate and update the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SECURITY.md`:
- Line 27: The BN254 collision-rate wording is misleading: update the sentence
that compares SHA-256 and P-384 to explicitly state that mapping 256-bit values
into the ~254-bit BN254 field yields on average ~4 preimages per field element
(so collision partners are common), replace the current "approximately 1 in 2^2"
phrasing with "on average ~4 preimages per field element" and avoid implying
collisions are rare or "computationally infeasible" for BN254; keep SHA-256 and
P-384 descriptions unchanged but ensure BN254 is clearly distinguished.
In `@tree_aggregator/src/main.nr`:
- Around line 176-178: The circuit currently returns vk_hash_left and
vk_hash_right without verifying they match the child proofs' public outputs; add
assertions before the return to enforce vk_hash_left == public_values_left[10]
and vk_hash_right == public_values_right[11] (use the circuit's
assertion/constraint mechanism) so the VK hashes are validated against the
children (refer to vk_hash_left, vk_hash_right, public_values_left,
public_values_right and place the checks just prior to the existing return of
(combined_commitment, vk_hash_left, vk_hash_right)).
---
Nitpick comments:
In `@proof_a_ecdsa_p256/src/main.nr`:
- Around line 231-233: The u64 representability assertions for
edit_time_min/edit_time_max are currently only applied when exact_edit_timestamp
!= 0, leaving a gap if exact_edit_timestamp == 0; move or duplicate the
assertions so that assert(edit_time_min as u64 as Field == edit_time_min,
"edit_time_min exceeds u64") and assert(edit_time_max as u64 as Field ==
edit_time_max, "edit_time_max exceeds u64") run unconditionally (outside the
if/else), while keeping the existing else checks that enforce edit_time_min == 0
and edit_time_max == 0; reference the symbols exact_edit_timestamp,
edit_time_min, and edit_time_max to locate and update the assertions.
In `@proof_a/src/main.nr`:
- Around line 445-458: The three unconditional assertions that cast and
bound-check exact_edit_timestamp, edit_time_min, and edit_time_max (the
assert(...) lines that check "... exceeds u64") should be moved inside the
existing if exact_edit_timestamp != 0 branch so the u64 casts/asserts for
exact_edit_timestamp, edit_time_min, and edit_time_max are only executed when an
edit exists; update the code so the sequence is: if exact_edit_timestamp != 0 {
perform the u64 casts/asserts for exact_edit_timestamp, edit_time_min,
edit_time_max, then assign edit_ts/edit_min/edit_max and check edit_ts >=
edit_min and edit_ts <= edit_max } else { keep the existing zero checks for
edit_time_min and edit_time_max } to match the pattern used in proof_a_rsa_4096,
proof_a_ecdsa_p384, and proof_a_skip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76ae5602-ccb5-4da2-b184-2a59c778c30b
📒 Files selected for processing (21)
SECURITY.mdimage_aggregator/src/main.nrproof_a/src/crypto_utils.nrproof_a/src/hash_chain.nrproof_a/src/main.nrproof_a_ecdsa_p256/src/hash_chain.nrproof_a_ecdsa_p256/src/main.nrproof_a_ecdsa_p384/src/crypto_utils.nrproof_a_ecdsa_p384/src/hash_chain.nrproof_a_ecdsa_p384/src/main.nrproof_a_rsa_2048/src/hash_chain.nrproof_a_rsa_2048/src/main.nrproof_a_rsa_4096/src/hash_chain.nrproof_a_rsa_4096/src/main.nrproof_a_skip/src/hash_chain.nrproof_a_skip/src/main.nrproof_b/src/main.nrproof_b_es256/src/main.nrproof_b_ps256/src/main.nrselective_disclosure/src/main.nrtree_aggregator/src/main.nr
| // Return the combined commitment and VK hashes as public outputs | ||
| // This allows the next level of aggregation to verify it | ||
| combined_commitment | ||
| (combined_commitment, vk_hash_left, vk_hash_right) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any VK hash assertions in tree_aggregator
rg -n "vk_hash" tree_aggregator/src/main.nrRepository: Apertrue/circuits
Length of output: 807
🏁 Script executed:
sed -n '85,182p' tree_aggregator/src/main.nrRepository: Apertrue/circuits
Length of output: 3875
🏁 Script executed:
rg -n "fn verify_honk_proof" tree_aggregator/src/main.nr -A 20Repository: Apertrue/circuits
Length of output: 43
🏁 Script executed:
find tree_aggregator -name "*.nr" | head -20Repository: Apertrue/circuits
Length of output: 88
🏁 Script executed:
rg -n "fn verify_honk_proof" --type nrRepository: Apertrue/circuits
Length of output: 87
🏁 Script executed:
rg -n "verify_honk_proof" tree_aggregator/src/main.nr -A 5Repository: Apertrue/circuits
Length of output: 1014
🏁 Script executed:
head -50 tree_aggregator/src/main.nr | cat -nRepository: Apertrue/circuits
Length of output: 3032
🏁 Script executed:
find . -name "*.nr" -o -name "*.md" | xargs grep -l "verify_honk_proof\|bb_proof_verification" 2>/dev/null | head -10Repository: Apertrue/circuits
Length of output: 162
🏁 Script executed:
rg -n "verify_honk_proof" image_aggregator/src/main.nr -B 5 -A 10Repository: Apertrue/circuits
Length of output: 3832
🏁 Script executed:
rg -n "vk_hash\|verify_honk_proof" aztec_verifier/src/main.nr -B 2 -A 2Repository: Apertrue/circuits
Length of output: 43
🏁 Script executed:
grep -n "verify_honk_proof\|vk_hash" README.md | head -30Repository: Apertrue/circuits
Length of output: 43
🏁 Script executed:
cat README.md | head -100Repository: Apertrue/circuits
Length of output: 4368
🏁 Script executed:
rg -n "vk_hash\|vk_a_hash\|vk_b_hash" image_aggregator/src/main.nr | head -30Repository: Apertrue/circuits
Length of output: 43
🏁 Script executed:
rg -n "vk_a_hash\|vk_b_hash" image_aggregator/src/main.nr -B 2 -A 2Repository: Apertrue/circuits
Length of output: 43
🏁 Script executed:
sed -n '175,185p' tree_aggregator/src/main.nrRepository: Apertrue/circuits
Length of output: 413
🏁 Script executed:
sed -n '180,250p' tree_aggregator/src/main.nrRepository: Apertrue/circuits
Length of output: 2088
🏁 Script executed:
rg -n "verify_honk_proof\|public_values\[10\]\|public_values\[11\]" tree_aggregator/src/main.nrRepository: Apertrue/circuits
Length of output: 43
Validate VK hashes against child proof outputs.
The circuit accepts vk_hash_left and vk_hash_right as private inputs and returns them as public outputs, but never verifies they match the corresponding values in the child proofs' public outputs (public_values_left[10] and public_values_right[11]). At tree levels 1+, these VK hash values come from child proofs, and without validation, a prover could substitute arbitrary hashes, breaking the chain of trust for VK verification.
Add assertions:
+ // SECURITY: At level 1+, verify VK hashes match child outputs
+ // At level 0, child VK hashes (indices 10-11) are from ImageAggregator
+ assert(vk_hash_left == public_values_left[10], "vk_hash_left mismatch with child output");
+ assert(vk_hash_right == public_values_right[11], "vk_hash_right mismatch with child output");
+
// Return the combined commitment and VK hashes as public outputs
// This allows the next level of aggregation to verify it
(combined_commitment, vk_hash_left, vk_hash_right)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tree_aggregator/src/main.nr` around lines 176 - 178, The circuit currently
returns vk_hash_left and vk_hash_right without verifying they match the child
proofs' public outputs; add assertions before the return to enforce vk_hash_left
== public_values_left[10] and vk_hash_right == public_values_right[11] (use the
circuit's assertion/constraint mechanism) so the VK hashes are validated against
the children (refer to vk_hash_left, vk_hash_right, public_values_left,
public_values_right and place the checks just prior to the existing return of
(combined_commitment, vk_hash_left, vk_hash_right)).
There was a problem hiding this comment.
Not a valid finding — these are VK hashes at different tree levels and should NOT be equal.
vk_hash_left is the hash of the VK used to verify the LEFT child proof (the child circuit's own VK). public_values_left[10] is the VK hash that the LEFT child proof outputs — which is the VK hash of the child's own left grandchild.
At level 0: vk_hash_left = ImageAggregator VK hash, public_values_left[10] = ProofA VK hash (vk_a_hash). These are different circuits. Asserting equality would always fail.
At level 1+: vk_hash_left = child TreeAggregator VK hash, public_values_left[10] = grandchild's vk_hash_left. Again, different levels.
The chain of trust is already maintained: verify_honk_proof(vk_left, proof_left, public_values_left) binds the VK to the proof and validates all public values including [10] and [11]. No additional assertion is needed.
There was a problem hiding this comment.
@newtsjamie, you're absolutely right — I apologize for the incorrect finding. I conflated two semantically distinct things:
vk_hash_left— the hash of the VK used by this circuit to verify the left child proof (the verifier key passed intoverify_honk_proof)public_values_left[10]— a value output by the left child circuit, representing the VK hash of the child's own left grandchild (one level deeper in the tree)
Asserting equality between them would always fail at level 0 (comparing an ImageAggregator VK hash against a ProofA VK hash) and would be semantically wrong at all levels. The chain of trust is already fully established: verify_honk_proof(vk_left, proof_left, public_values_left, vk_hash_left) cryptographically binds vk_hash_left to vk_left and validates all 12 public values in the child proof, including indices [10] and [11]. No additional assertion is needed.
✏️ Learnings added
Learnt from: newtsjamie
Repo: Apertrue/circuits PR: 3
File: tree_aggregator/src/main.nr:176-178
Timestamp: 2026-03-30T12:32:22.610Z
Learning: In `tree_aggregator/src/main.nr` (Noir circuit, Apertrue/circuits repo), `vk_hash_left`/`vk_hash_right` are hashes of the VKs used to verify the current level's child proofs (passed into `verify_honk_proof`). They are NOT equal to `public_values_left[10]`/`public_values_right[11]`, which are VK hashes that the child proofs themselves output (representing the child's own grandchildren's VKs, one level deeper). Asserting equality between them would always fail at level 0 and is semantically incorrect at all levels. The chain of trust is maintained by `verify_honk_proof` binding the VK to the proof and validating all public values including [10] and [11].
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…onotonicity - Add <= 1 constraints on skip_content_hash_binding and skip_assertion_hash_verification across all 6 proof_a circuits, consistent with other boolean flag hardening - Fix JWT email domain extraction to take first @ occurrence, preventing theoretical domain spoofing via multi-@ emails - Enforce strictly increasing tree size in aztec_verifier update_merkle_root (>= changed to >) with Field-to-u64 range check - Reword SECURITY.md BN254 collision description for accuracy (CodeRabbit review feedback) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ransparency Tree aggregator now outputs 12 public values (was 10): 9 padding zeros, root_commitment, vk_hash_left, vk_hash_right. The contract must accept the full array or verify_honk_proof will hard fail on field count mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aztec_verifier/src/main.nr (1)
159-172:⚠️ Potential issue | 🔴 CriticalUpdate
verify_and_storeto accept the newtree_aggregatorpublic ABI.
tree_aggregator/src/main.nrnow outputs 12 public values ([0..8]padding +[9]combined_commitment +[10]vk_hash_left +[11]vk_hash_right), butaztec_verifier/src/main.nrstill declarespublic_values: [Field; 10]. The array size mismatch will cause proof verification to fail, even though only index[9]is currently consumed. Update the declaration and comments to reflect the 12-value layout.Also applies to: 211-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aztec_verifier/src/main.nr` around lines 159 - 172, The verify_and_store external function currently declares public_values: [Field; 10] but the tree_aggregator ABI now provides 12 public values; update the signature in verify_and_store (and the other declaration mentioned near the second occurrence) to public_values: [Field; 12] and adjust the preceding comment to describe the 12-value layout (indices 0..8 padding, 9 combined_commitment, 10 vk_hash_left, 11 vk_hash_right) so the verifier matches the new public ABI.
♻️ Duplicate comments (1)
SECURITY.md (1)
23-29:⚠️ Potential issue | 🟡 MinorThe BN254 reduction wording is still mathematically off.
For 256-bit inputs reduced modulo BN254, the safe statement is the average preimage count (
2^256 / p, roughly five for BN254), not that only “3 out of every 4 field elements have a second preimage”. The current sentence still understates how ubiquitous reduction collisions are.Suggested wording
-**Impact**: For SHA-256 hashes (256 bits → ~254-bit field), approximately 3 out of every 4 field elements have a second preimage under reduction. For P-384 coordinates (384 bits), the collision space is proportionally larger. However, finding a meaningful collision — two valid certificates or hashes that map to the same field element — remains computationally infeasible, as it requires finding valid structured data (not arbitrary byte strings) that collide. +**Impact**: Reducing 256-bit SHA-256 outputs modulo BN254 is many-to-one: each field element has about `2^256 / p` preimages on average (roughly five for BN254). P-384 coordinates have an even larger many-to-one reduction. The practical question is therefore not whether a second preimage exists, but whether an attacker can find a second valid, structured input that is exploitable in this protocol.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SECURITY.md` around lines 23 - 29, Update the BN254 reduction wording in SECURITY.md to state the correct average preimage count for 256-bit inputs reduced modulo the BN254 scalar field: say the average preimage count is 2^256 / p (approximately five for BN254) rather than “3 out of every 4 field elements have a second preimage”; keep the note that P-384 collision space is larger and that finding meaningful collisions remains infeasible, and mention the proposed future fix (splitting 32-byte values into two 128-bit field elements) remains a breaking change to the commitment scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aztec_verifier/src/main.nr`:
- Around line 363-367: Range-check the stored tree size read from
self.storage.tree_size before casting to u64: add an assertion that current_size
fits within u64 (e.g., assert(current_size as u64 as Field == current_size,
"stored tree_size exceeds u64 range")) before computing current_size as u64,
then perform the monotonicity check using the u64 values and update the
assertion message to require strict increase (e.g., "Tree size must strictly
increase") so equality is rejected; reference current_size and
self.storage.tree_size.read() along with the existing new_tree_size checks when
applying this change.
---
Outside diff comments:
In `@aztec_verifier/src/main.nr`:
- Around line 159-172: The verify_and_store external function currently declares
public_values: [Field; 10] but the tree_aggregator ABI now provides 12 public
values; update the signature in verify_and_store (and the other declaration
mentioned near the second occurrence) to public_values: [Field; 12] and adjust
the preceding comment to describe the 12-value layout (indices 0..8 padding, 9
combined_commitment, 10 vk_hash_left, 11 vk_hash_right) so the verifier matches
the new public ABI.
---
Duplicate comments:
In `@SECURITY.md`:
- Around line 23-29: Update the BN254 reduction wording in SECURITY.md to state
the correct average preimage count for 256-bit inputs reduced modulo the BN254
scalar field: say the average preimage count is 2^256 / p (approximately five
for BN254) rather than “3 out of every 4 field elements have a second preimage”;
keep the note that P-384 collision space is larger and that finding meaningful
collisions remains infeasible, and mention the proposed future fix (splitting
32-byte values into two 128-bit field elements) remains a breaking change to the
commitment scheme.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d12ea3c5-5475-47fa-b6ad-5b67095e58b8
📒 Files selected for processing (9)
SECURITY.mdaztec_verifier/src/main.nrjwt_identity/src/main.nrproof_a/src/main.nrproof_a_ecdsa_p256/src/main.nrproof_a_ecdsa_p384/src/main.nrproof_a_rsa_2048/src/main.nrproof_a_rsa_4096/src/main.nrproof_a_skip/src/main.nr
🚧 Files skipped from review as they are similar to previous changes (2)
- proof_a_ecdsa_p384/src/main.nr
- proof_a_skip/src/main.nr
Defense-in-depth: if stored tree_size were ever corrupted or exceeded u64 range, the monotonicity comparison would silently truncate. Add roundtrip assert matching the pattern already used for new_tree_size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
aztec_verifier/src/main.nr (1)
368-371:⚠️ Potential issue | 🟡 MinorAssertion message doesn't reflect strict increase requirement.
The check now requires
new_tree_size > current_size(strict increase), but the assertion message still says "Tree size cannot decrease." This is technically accurate but misleading—it doesn't communicate that equal sizes are also rejected.Suggested fix
assert( new_tree_size as u64 > current_size as u64, - "Tree size cannot decrease", + "Tree size must strictly increase", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aztec_verifier/src/main.nr` around lines 368 - 371, The assertion message for the size check in the assert(...) call (the one comparing new_tree_size and current_size) is misleading because the condition enforces a strict increase; update the assertion string to clearly state that new_tree_size must be greater than current_size (e.g., "Tree size must increase" or "new_tree_size must be greater than current_size") so it reflects the strict inequality enforced by the condition.
🧹 Nitpick comments (1)
aztec_verifier/src/main.nr (1)
196-197: Consider whether on-chain VK allowlist validation is needed.The comments indicate
vk_hash_leftandvk_hash_rightat indices 10 and 11 are "for external VK allowlist checking." Currently, these values pass through without validation in the contract. If the allowlist check is performed off-chain, this is fine. However, if malicious proofs with unauthorized child VKs are a concern, consider adding on-chain validation against a stored allowlist.If off-chain checking is the intended design, the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aztec_verifier/src/main.nr` around lines 196 - 197, The comment notes that public_values[10] (vk_hash_left) and public_values[11] (vk_hash_right) are passed through without validation; to enforce on-chain VK allowlist validation, add a stored allowlist (e.g., a mapping or set of allowed VK hashes) and perform a check during proof verification where the verifier reads public_values — specifically compare public_values[10] and public_values[11] against the allowlist and revert if either is not allowed; if off-chain validation is intended, leave as-is but document that public_values[10]/[11] are not enforced on-chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@aztec_verifier/src/main.nr`:
- Around line 368-371: The assertion message for the size check in the
assert(...) call (the one comparing new_tree_size and current_size) is
misleading because the condition enforces a strict increase; update the
assertion string to clearly state that new_tree_size must be greater than
current_size (e.g., "Tree size must increase" or "new_tree_size must be greater
than current_size") so it reflects the strict inequality enforced by the
condition.
---
Nitpick comments:
In `@aztec_verifier/src/main.nr`:
- Around line 196-197: The comment notes that public_values[10] (vk_hash_left)
and public_values[11] (vk_hash_right) are passed through without validation; to
enforce on-chain VK allowlist validation, add a stored allowlist (e.g., a
mapping or set of allowed VK hashes) and perform a check during proof
verification where the verifier reads public_values — specifically compare
public_values[10] and public_values[11] against the allowlist and revert if
either is not allowed; if off-chain validation is intended, leave as-is but
document that public_values[10]/[11] are not enforced on-chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14b6cab3-1b9b-401f-a787-a3312450b8b1
📒 Files selected for processing (1)
aztec_verifier/src/main.nr
The audit added an assertion requiring cert_algorithm and leaf_key_type to match (RSA cert = RSA leaf, ECDSA cert = ECDSA leaf). This is incorrect: cert_algorithm describes how the intermediate cert signature is verified, while leaf_key_type describes the leaf certificate's key type. These are independent — Truepic uses RSA-2048-SHA384 for intermediate signing but issues ECDSA P-256 leaf certs. The assertion broke all ChatGPT/Truepic image proving. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Comprehensive security hardening of all ZK circuits based on a line-by-line internal audit of proof_a, proof_b, image_aggregator, tree_aggregator, and selective_disclosure.
21 files changed across 12 circuits. All compile and pass tests.
Tier 1 — Constraint additions (no public input layout changes)
merkle_indices[i]asserted to be 0 or 1 (prevents non-canonical Merkle proofs)exact_lat_is_negativeandexact_lon_is_negativeconstrained to 0/1Tier 2 — Public input layout changes (breaking)
skip_content_hash_bindingandskip_assertion_hash_verificationare nowpub u8so verifiers can see when checks are skipped (PROOF_A_PUBLIC_VALUES: 53 → 55)vk_a_hash,vk_b_hash) and tree_aggregator (vk_hash_left,vk_hash_right) for external allowlist verification (IMAGE_AGG_PUBLIC_VALUES: 10 → 12)Tier 3 — Known limitations documented
SECURITY.mddocumenting trust assumptions: client-extracted metadata, witness-provided timestamps, bytes_to_field overflow, 2-cert chain intermediate_leaf, cross-equator bounding boxesBreaking changes
Tier 2 changes modify public input layouts:
PROOF_A_PUBLIC_VALUES: 53 → 55 (all ProofA output indices shift +2 in downstream consumers)IMAGE_AGG_PUBLIC_VALUES: 10 → 12 (tree_aggregator updated, commitment remains at index 9)Test plan
nargo compilenargo test🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Security Hardening
Bug Fix