fix(vault): compute share<>asset conversions in a 256-bit intermediate - #38
Open
alejoamiras wants to merge 6 commits into
Open
alejoamiras wants to merge 6 commits into
alejoamiras wants to merge 6 commits into
Conversation
This was referenced Aug 19, 2026
alejoamiras
force-pushed
the
stack/closeout-vault-overflow
branch
from
August 19, 2026 16:16
09aac15 to
5f6f39f
Compare
alejoamiras
force-pushed
the
stack/closeout-nft-notehash
branch
from
August 19, 2026 16:16
d17b64d to
252c352
Compare
alejoamiras
force-pushed
the
stack/closeout-vault-overflow
branch
from
August 19, 2026 19:07
5f6f39f to
b22f58e
Compare
alejoamiras
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alejoamiras
force-pushed
the
stack/closeout-nft-notehash
branch
from
September 28, 2026 14:00
252c352 to
c6a4513
Compare
alejoamiras
force-pushed
the
stack/closeout-vault-overflow
branch
from
September 28, 2026 14:00
b22f58e to
b96059e
Compare
alejoamiras
removed this pull request from stack #39
September 28, 2026 14:01
alejoamiras
force-pushed
the
stack/closeout-vault-overflow
branch
from
September 29, 2026 15:19
b96059e to
3433a70
Compare
alejoamiras
changed the base branch from
stack/closeout-nft-notehash
to
main
September 29, 2026 15:19
The conversions compute `a * b / denominator`. Done natively in u128 the intermediate product overflows for large-but-legitimate inputs; Noir range-checks u128, so the transaction reverts rather than wrapping. That is an availability bug (audit F-004): a vault whose totals reach the range can no longer be deposited to or withdrawn from, permanently locking every participant's funds. Both sites carried TODOs. New `conversion.nr` module with a `mul_div` primitive that widens both operands to noir-bignum's U256 (the same library the escrow's key derivation already uses), multiplies and divides there, then narrows the quotient back to u128, asserting it fits. U256 arithmetic is modulo 2^256 and the largest possible product, (2^128-1)^2, is 2^129-1 short of that modulus, so the product is always exact — no modular wraparound. Rounding is unchanged: both old and new return floor(p/d) + (round_up && p%d != 0). For every input the old code accepted the results are identical; the widening only extends the domain that succeeds. This matters because the vault's economic safety depends on rounding always favouring the vault — a shift in either direction would leak value between the vault and its depositors. `mul_div` also rejects a zero denominator explicitly: noir-bignum's constrained udiv_mod fails on it, but its unconstrained path assumes non-zero and would return a meaningless witness. Unreachable from the vault (denominators are total_assets+1 and total_supply+vault_offset with vault_offset >= 1) but the helper is now safe in isolation. Extracting mul_div into its own module is what makes the overflow boundary testable at all: a real vault cannot be driven to a 2^128 supply in a test, but the primitive can be called directly at its edges. Also adds `ensureVaultContractClassPublished` to the JS test utils. Publishing just the class is what the vault tests actually need and is substantially cheaper in DA gas than deploying a throwaway Vault to get the class published as a side effect. It is idempotent — publication emits a nullifier keyed on the class id, so a second publish of the same class is rejected with "Existing nullifier"; the helper checks registration state first, the same way DeployMethod does. Validated: vault_contract 195 Noir tests (188 pre-existing all still green — the strongest evidence rounding did not shift — plus 7 new covering limb round-trips, rounding direction both ways, products that previously overflowed, max operands, and the two revert guards). aztec compile OK. Codex adversarial review: correct, rounding invariance proven algebraically, no value-leak path; its zero-denominator hardening is applied. Note: the vault README still describes the overflow as a known issue. That warning block is rewritten in PR #24 (unmerged); leaving it there avoids a three-way conflict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`aztec start --local-network` defaults to 3s blocks. With 72s slots that packs 21 blocks into a checkpoint, and the per-tx DA admission limit is `ceil(daBudget / blocks * 1.5)` = 55,882 DA gas. Mainnet runs 6s blocks (10 blocks per checkpoint) and admits 117,668. The protocol picks that 1.5 multiplier deliberately, and says so in aztec stdlib `gas/tx_gas_limits.ts`: it is set "so the largest tx we want to support — a maximal contract class registration (~97k DA gas) — fits a single block under v5 mainnet geometry (72s slots, 6s blocks -> 10 blocks per checkpoint)". The default local geometry therefore advertises a limit well below the ~97k the protocol guarantees for exactly this kind of transaction, and rejects contracts that are valid on the network we ship to. Publishing the Vault contract class costs ~64k DA gas. That is fine on mainnet and inside the protocol's stated envelope, but over the local 55,882 cap. Note this is not specific to any one change: main's Vault already sits at ~54k, i.e. 97% of the local ceiling, so essentially any growth in that contract trips it. Reusable workflows do not inherit the caller's `env`, and aztec-ci-actions' run-tests.yml exposes no knob for this, so the JS job is inlined here (`run-js-tests: false` on the reusable call) purely to own the environment. It still calls the same pinned `setup-aztec` and `js-tests` composite actions, so behaviour is otherwise unchanged. NOTE FOR REVIEWERS: this renames the check from "checks / JS Tests" to "JS Tests". Any branch protection rule naming the old check needs updating, or it will block merges waiting on a check that no longer runs. The better long-term fix is upstream — either the local network should default to mainnet geometry, or run-tests.yml should expose the knob. Worth raising with the Aztec CI folks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alejoamiras
force-pushed
the
stack/closeout-vault-overflow
branch
from
September 30, 2026 13:06
3433a70 to
7f7ca65
Compare
The Vault benchmark's setup publishes the Vault contract class (63,776 DA gas). The local network's default 3s blocks cap a tx at 55,836, and aztec-benchmark's reusable workflows had no way to change that. Pin both workflows to AztecProtocol/aztec-benchmark#3, which adds a block-duration-ms input, and pass 6000 (mainnet's 6s blocks: 117,624). This is a pre-release commit; swap it for the v6.0.0-rc.1 tag commit before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
879fa93 is the squash commit of aztec-benchmark#3 that v6.0.0-rc.1 tags. Its pr-benchmark.yml and update-baseline.yml are byte-identical to the d623993 head they were verified at; only publish.yml differs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
5.0.1 declared @aztec/aztec.js and @aztec/wallets peers (<6) and typed its Benchmark base class against them, so against this repo's @aztec-labs/* v6 packages the peers were unmet and the benchmark files carried 33 type errors (unseen: tsx does not type-check). 6.0.0-rc.1 peers @aztec-labs/* >=6.0.0-0 <7 and the benchmark files type-check clean. Its gitHead and SLSA provenance name 879fa93, the commit both benchmark workflows are pinned to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The benchmark gotcha described 5.0.1's v5 peers as current. 6.0.0-rc.1 is the first @aztec-labs release. A peer mismatch fails nothing (yarn warns, tsconfig skips benchmarks/, tsx doesn't type-check), so document the explicit typecheck. Record that the devDependency and both workflow pins move together, and the local-network DA cap that the Vault class publication hits without 6s blocks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Benchmark Comparison
Contract: escrow
Contract: logic
Contract: multitoken
Contract: nft
Contract: token
Contract: vault
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
alejoamiras
marked this pull request as ready for review
October 1, 2026 18:46
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes F-004 from the audit. The vault's share<>asset conversions (
a * b / denominator) overflowedu128on large but valid inputs. Noir range-checks, so it reverts instead of wrapping - a vault that gets there can't deposit or withdraw anymore and everyone's funds are stuck.conversion.nrwith amul_divthat widens to noir-bignum'sU256(same lib escrow already uses), divides there and narrows back, asserting the result fits.(2^128-1)^2fits in 256 bits, so the product is always exact.floor(p/d) + (round_up && p%d != 0), so every input the old code accepted gives the same result. The 188 existing vault tests still pass, plus 7 new ones on the edges (products that used to overflow, max operands, rounding both ways, zero denominator).mul_divis safe on its own.The bigger Vault pushes its class publish to 63,776 DA gas, over the local network's per-tx cap (55,836 with the default 3s blocks; mainnet's 6s blocks admit ~117k). So CI now runs on mainnet geometry:
SEQ_BLOCK_DURATION_MS=6000. The job is inlined becauserun-tests.ymlhas no knob for it, so the check is nowJS Testsinstead ofchecks / JS Tests.block-duration-ms), and the devDep moves to 6.0.0-rc.1 - same commit,@aztec-labspeers.bump-aztec-versionskill now notes the benchmark peers, pins and DA cap.Rebased on v6 main.
🤖 Generated with Claude Code