Repository navigation
fix(vault): compute share<>asset conversions in a 256-bit intermediate - #38
Merged
Merged
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
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>
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
None of the called workflows (aztec-ci-actions run-tests.yml, aztec-benchmark pr-benchmark.yml and update-baseline.yml, at both the v5.0.1 and v6.0.0-rc.1 pins) declare `on.workflow_call.secrets` or read `secrets.*`, and the repo has no repo, org or environment secrets. `inherit` passes nothing today, but it would hand any secret added later to workflows that live in other repos, so a future pin bump that starts reading one gets it without us ever opting in. A called workflow always gets `github.token` / `secrets.GITHUB_TOKEN`, scoped by the caller job's `permissions`, so the benchmark PR comment keeps working. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Vitest and tsx (which runs the benchmarks) only strip types, so a type error in either never failed CI. Add `yarn typecheck` (`tsc --noEmit`) and run it in both js-tests jobs, after the setup step's compile + codegen write src/artifacts. tsconfig changes so the project type-checks clean: - `target/**/*` matched no JSON: an include glob only picks up .json files when the pattern itself ends in `.json`. Every generated src/artifacts/*.ts imports its `target/*.json`, which `composite` then rejects with TS6307. Now `target/**/*.json`. - include `benchmarks`, which nothing type-checked before. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…the contract path The conversions still add `vault_offset` to the share supply and 1 to total assets in u128 before `mul_div`. A deposit can leave the supply above 2^128 - 1 - vault_offset, after which every conversion reverts, withdrawals included; with a large offset that takes few assets. The READMEs had dropped the overflow warning entirely: restore it as a precise second known issue, and correct comments that called the remaining overflow impossible. `conversion.nr`'s tests run constrained; the vault runs it in public. Add a 2^100-scale deposit and redeem through the contract (fails on the old u128 math) and a case that only the `hi < 256` narrowing bound rejects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It still described a temporary tsconfig and said tsconfig.json skips benchmarks/, both stale since typecheck joined CI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Benchmark Comparison
Contract: escrow
Contract: logic
Contract: multitoken
Contract: nft
Contract: token
Contract: vault
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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 9 new ones: products that used to overflow, max operands, rounding both ways, zero denominator, the exact-2^128 narrowing bound, and a 2^100-scale deposit and redeem through the contract, which runsmul_divin public as the vault does. That last one fails on the old math.mul_divis safe on its own.vault_offsetto the share supply (and 1 to total assets) inu128beforemul_div. A deposit can leave the supply above2^128 - 1 - vault_offset, after which every conversion reverts, withdrawals included. At offset 1 that takes the asset's whole possible supply; at offset ≥ 2^127, one unit. Widening those additions overflows the 256-bit intermediate, so the vault README keeps it as a second known issue with the exact bounds, next to the still-open ARC-403 hook reentrancy (F-001/F-002).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, and points atyarn typecheck.Rebased on v6 main. Also carries #47 (drop
secrets: inherit) and #48 (typecheck), which were stacked on this branch.🤖 Generated with Claude Code