fix(zk): propagate range check to batch_5 + add amount/receiver checks#927
Conversation
…s (DEM-756) The L2PS batch-5 circuit had a dead-constraint balance guard — `check <== sender_after * sender_after` — that binds nothing. Because `sender_after === sender_before - amount` is BN254 field subtraction, an overdraw wraps `sender_after` to a value about the size of the curve order and the proof still accepts. The same circuit at batch-10 already had the proper `Num2Bits(64)` range check on `sender_after` with an explicit "SECURITY FIX — range check instead of squaring" comment; the fix was never propagated back to batch-5. Plus a related gap on BOTH circuits, surfaced by the PATH-OS L2PS hardening report: only `sender_after` was range-checked. An overflow on the receiver side (`receiver_after`) or an oversized `amount` can also wrap the BN254 field and satisfy the proof. This change: - Replaces the dead constraint on batch_5 with the same `Num2Bits(64)` range check batch_10 already has on `sender_after`. - Adds matching range checks on `amount` and `receiver_after` on BOTH circuits. 64 bits is the documented user-balance limit. - Adds `include "bitify.circom"` to batch_5 (batch_10 already had it). Impact: if anything ever trusts a batch-5 or batch-10 proof on its own (light clients, cross-node aggregation, future bridges), a crafted overdraw / over-receive / oversized-amount transaction can no longer pass verification. The direct executor path already re-validates balances against L1 state, so defence-in-depth on the happy path — but this closes a real soundness hole the moment trust assumptions widen. BLOCKER FOR MERGE: regenerating the proving/verifying keys requires a ZK ceremony. This PR ships the circom source change only. Coordination on the ceremony (rerun trusted setup + commit new `verification_key`) must happen before merge. Source: PATH-OS L2PS hardening report, item 5 (severity: yours to set — defence-in-depth on direct path, critical if anything trusts the batch proof alone). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Disabled knowledge base sources:
WalkthroughThe PR hardens two L2PS batch circuits (batch_5 and batch_10) against field-wrapping exploits in the BalanceTransfer template by introducing 64-bit range checks on pre-state, post-state, and transfer amount signals. Corresponding PLONK verification keys are generated and the parent merkle verification key is updated. ChangesZK Circuit Hardening and Key Regeneration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes two ZK soundness gaps in the L2PS batch circuits: the dead
Confidence Score: 4/5The batch circuit constraint changes are correct, but the partial Merkle verification key update on an unmodified circuit could silently break Merkle proof verification in production. The circom fixes are logically sound and the new PLONK keys are properly committed. The unexplained partial update to verification_key_merkle.json is the main concern — only vk_delta_2 changed in a Groth16 key for a circuit this PR never touches, and no proving key update accompanies it. src/features/zk/keys/verification_key_merkle.json — the partial Groth16 key change on an unmodified circuit needs explanation and the corresponding proving key update included if applicable. Important Files Changed
Reviews (3): Last reviewed commit: "new verification keys" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile P1 on PR #927: the range-check pass only covered `sender_after`, `receiver_after`, and `amount`. Both pre-state inputs — `sender_before` and `receiver_before` — remained unconstrained, which left two field-arithmetic gaps even with the post-state checks in place: 1. `receiver_before = receiver_after - amount (mod p)` — if `receiver_after < amount` as integers, this wraps to a value near the BN254 order (~2^254). The prover can craft a proof where the receiver "started at −X" and still pass the post-state 64-bit bound on `receiver_after`. 2. `sender_before = sender_after + amount` — with both terms bounded to 64 bits, the integer sum is in `[0, 2^65)`, which exceeds the documented user-balance limit even though every individual signal passes its own check. Bind both pre-state inputs through `Num2Bits(64)` on both circuits. With all five magnitudes (`sender_before`, `receiver_before`, `amount`, `sender_after`, `receiver_after`) bounded to 64 bits and the equality constraints in the middle, field arithmetic collapses to integer arithmetic and a malicious prover can no longer satisfy any of the exploit shapes Greptile described. Constraint order is now: pre-state range checks → equality constraints → post-state range checks. Order is informational only (circom enforces all constraints simultaneously) but reads cleanly top-down. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
Summary
Two ZK soundness fixes on the L2PS batch circuits, surfaced by the PATH-OS L2PS hardening report.
1. Dead constraint on batch_5. The only balance guard on
BalanceTransferinl2ps_batch_5.circomwascheck <== sender_after * sender_after, which binds nothing. Sincesender_after === sender_before - amountis BN254 field subtraction, an overdraw wrapssender_afterto a value about the size of the curve order and the proof still accepts.batch_10already has the proper guard —Num2Bits(64)onsender_after, with an explicit "SECURITY FIX — range check instead of squaring" comment — but the fix was never propagated back to batch_5.2. Gap on both circuits. Only
sender_afterwas range-checked. An overflow onreceiver_afteror an oversizedamountcan also wrap the field and satisfy the proof. Range-check both on both circuits — 64 bits is the documented user-balance limit.Impact
If anything ever trusts a batch-5 or batch-10 proof on its own (light clients, cross-node aggregation, future bridges), a crafted overdraw / over-receive / oversized-amount transaction passes verification. The direct executor path re-validates balances against L1 state, so defence-in-depth on the happy path — this closes a real soundness hole the moment trust assumptions widen.
The circom source change is fast, but the proving/verifying keys must be regenerated. Do not merge until the team has rerun the trusted setup and committed the new
verification_keyartefacts.Test plan
Num2Bits(64)range checks onsender_after,receiver_after, andamount. batch_5 also gets the missinginclude \"bitify.circom\".Linked
🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Updates