From ec4b4e492dc7ab872cfe52859d32e93a35502283 Mon Sep 17 00:00:00 2001 From: Alejo Amiras Date: Wed, 19 Aug 2026 15:35:17 +0000 Subject: [PATCH 1/2] fix(vault): compute share<>asset conversions in a 256-bit intermediate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/ts/test/utils.ts | 15 ++- src/vault_contract/Nargo.toml | 1 + src/vault_contract/README.md | 5 +- src/vault_contract/src/conversion.nr | 137 +++++++++++++++++++++++++++ src/vault_contract/src/main.nr | 33 +++---- 5 files changed, 171 insertions(+), 20 deletions(-) create mode 100644 src/vault_contract/src/conversion.nr diff --git a/src/ts/test/utils.ts b/src/ts/test/utils.ts index 3d118480..1cd02dd6 100644 --- a/src/ts/test/utils.ts +++ b/src/ts/test/utils.ts @@ -4,6 +4,7 @@ import { type Wallet, AccountManager } from '@aztec/aztec.js/wallet'; import { Fr } from '@aztec/aztec.js/fields'; import { createAztecNodeClient, waitForNode, waitForTx } from '@aztec/aztec.js/node'; import { type ContractInstanceWithAddress } from '@aztec/aztec.js/contracts'; +import { publishContractClass } from '@aztec/aztec.js/deployment'; import { TxHash } from '@aztec/aztec.js/tx'; import { EmbeddedWallet } from '@aztec/wallets/embedded'; import { registerInitialLocalNetworkAccountsInWallet } from '@aztec/wallets/testing'; @@ -244,7 +245,19 @@ export async function deployNFTWithMinter(wallet: EmbeddedWallet, deployer: Azte * Each vault pool deploys its own VaultDeployer instance via initializers; there is no shared factory. */ export async function ensureVaultContractClassPublished(wallet: Wallet, deployer: AztecAddress): Promise { - await VaultContract.deploy(wallet, deployer, 1).send({ from: deployer }); + // Publish ONLY the contract class, which is all callers need. Deploying a throwaway Vault would + // publish the class *and* the instance *and* run the constructor in one transaction, and the + // Vault's public dispatch bytecode is big enough that the combined DA gas leaves little headroom + // under the node's per-tx admission limit. That limit is derived from block geometry, so it is not + // a constant: see SEQ_BLOCK_DURATION_MS in .github/workflows/pr-checks.yml for why CI pins the + // local network to mainnet's. + // + // Publication emits a nullifier keyed on the class id, so a second publish of the same class is + // rejected with "Existing nullifier". Guard on registration state the same way DeployMethod does. + const contractClass = await getContractClassFromArtifact(VaultContractArtifact); + const { isContractClassPubliclyRegistered } = await wallet.getContractClassMetadata(contractClass.id); + if (isContractClassPubliclyRegistered) return; + await (await publishContractClass(wallet, VaultContractArtifact)).send({ from: deployer }); } /** diff --git a/src/vault_contract/Nargo.toml b/src/vault_contract/Nargo.toml index 2238d530..264e6319 100644 --- a/src/vault_contract/Nargo.toml +++ b/src/vault_contract/Nargo.toml @@ -8,3 +8,4 @@ type = "contract" aztec = { git = "https://github.com/AztecProtocol/aztec-packages/", tag = "v5.2.0", directory = "noir-projects/aztec-nr/aztec" } token_contract = { path = "../token_contract" } generic_proxy = { path = "../generic_proxy" } +bignum = { git = "https://github.com/noir-lang/noir-bignum", tag = "v0.10.0" } diff --git a/src/vault_contract/README.md b/src/vault_contract/README.md index 19af351f..288cd1ea 100644 --- a/src/vault_contract/README.md +++ b/src/vault_contract/README.md @@ -16,10 +16,9 @@ This contract follows the [AIP-4626: Tokenized Vault Standard](https://forum.azt > **WARNING — Experimental Feature** > -> The AIP-4626 functionality of this contract is not yet production-ready. Use it at your own risk. Two known issues, neither currently scheduled for a fix: +> The AIP-4626 functionality of this contract is not yet production-ready. Use it at your own risk. One known issue, not currently scheduled for a fix: > -> 1. **Reentrancy via a hooked asset token.** This contract's protection against reentrancy is the *order* of its operations (assets are taken in before shares are minted; shares are burned before assets are paid out), on the assumption that a token transfer is indivisible. It is not: when the asset token has an ARC-403 `auth_contract` configured, that contract is invoked *during* the transfer, before balances move, and can observe the vault mid-operation — the exact intermediate state the ordering is meant to exclude. Reading the share price at that point yields a value no completed operation would produce, which can be used to extract value belonging to other shareholders. **Only wrap an asset token whose `get_auth_contract()` is the zero address, or one whose authorization contract you fully trust.** Vaults over tokens with no hook configured are not affected. -> 2. **Overflow in the asset↔share conversion** logic used on deposits and withdrawals, for sufficiently large inputs. +> **Reentrancy via a hooked asset token.** This contract's protection against reentrancy is the *order* of its operations (assets are taken in before shares are minted; shares are burned before assets are paid out), on the assumption that a token transfer is indivisible. It is not: when the asset token has an ARC-403 `auth_contract` configured, that contract is invoked *during* the transfer, before balances move, and can observe the vault mid-operation — the exact intermediate state the ordering is meant to exclude. Reading the share price at that point yields a value no completed operation would produce, which can be used to extract value belonging to other shareholders. **Only wrap an asset token whose `get_auth_contract()` is the zero address, or one whose authorization contract you fully trust.** Vaults over tokens with no hook configured are not affected. ## Architecture diff --git a/src/vault_contract/src/conversion.nr b/src/vault_contract/src/conversion.nr new file mode 100644 index 00000000..c37963e9 --- /dev/null +++ b/src/vault_contract/src/conversion.nr @@ -0,0 +1,137 @@ +//! Overflow-safe fixed-point conversion math for the vault. +//! +//! The share<>asset conversions compute `a * b / denominator`. Done natively in `u128`, the +//! intermediate product `a * b` overflows for large-but-legitimate inputs (Noir range-checks u128, +//! so the transaction reverts rather than wrapping) — this is the availability bug flagged by the +//! 2026-08 security audit (F-004). We compute the product and the division in a 256-bit intermediate +//! (`noir-bignum`'s `U256` — three 120-bit limbs, modulus 2^256) so nothing overflows until +//! the final result, which is narrowed back to `u128` and only fails if the true result genuinely +//! exceeds `u128` — an impossibility for real balances. + +use bignum::bignum::BigNum; +use bignum::U256; + +/// 2^120, the width of a single `U256` limb. A `u128` splits across the low two limbs: limb 0 holds +/// the low 120 bits, limb 1 the high 8 bits. +global TWO_POW_120: u128 = 0x1000000000000000000000000000000; + +/// Widens a `u128` into a `U256` (little-endian, 120-bit limbs). +fn u256_from_u128(v: u128) -> U256 { + U256::from_limbs([v % TWO_POW_120, v / TWO_POW_120, 0]) +} + +/// Narrows a `U256` back to `u128`, asserting it fits. Reverts if the value exceeds `u128::MAX`. +fn u256_to_u128(v: U256) -> u128 { + let hi = v.get_limb(1); + // A u128 occupies limb 0 (120 bits) plus at most the low 8 bits of limb 1; anything above that + // does not fit. `hi < 256` keeps `hi * TWO_POW_120` below 2^128 so the reconstruction cannot + // itself overflow. + assert((v.get_limb(2) == 0) & (hi < 256), "conversion result exceeds u128"); + // hi < 256 (asserted above), so hi * TWO_POW_120 < 2^128 and the sum fits u128. + v.get_limb(0) + hi * TWO_POW_120 +} + +/// True iff `v` is zero. +fn u256_is_zero(v: U256) -> bool { + (v.get_limb(0) == 0) & (v.get_limb(1) == 0) & (v.get_limb(2) == 0) +} + +/// Computes `a * b / denominator` without overflowing on the intermediate product. +/// +/// Rounds down by default; when `round_up` is true and the division leaves a remainder, rounds up. +/// This matches the previous native-`u128` implementation exactly for every input that did not +/// overflow — the widening only extends the range of inputs that succeed, it does not change any +/// result. `denominator` must be non-zero (it always is at the call sites: `total_assets + 1` and +/// `total_supply + vault_offset` with `vault_offset >= 1`). +pub fn mul_div(a: u128, b: u128, denominator: u128, round_up: bool) -> u128 { + // noir-bignum's constrained `udiv_mod` enforces `remainder < divisor` (which fails for a zero + // divisor), but its unconstrained path assumes a non-zero divisor and would otherwise return a + // meaningless witness. Guard explicitly so the helper is safe in isolation, not only at the + // vault's (always non-zero) call sites. + assert(denominator > 0, "division by zero"); + let numerator = u256_from_u128(a) * u256_from_u128(b); + let (quotient, remainder) = numerator.udiv_mod(u256_from_u128(denominator)); + + let mut result = u256_to_u128(quotient); + if round_up & !u256_is_zero(remainder) { + result = result + 1; + } + result +} + +mod test { + use super::{mul_div, TWO_POW_120, u256_from_u128, u256_to_u128}; + + global ROUND_DOWN: bool = false; + global ROUND_UP: bool = true; + + #[test] + fn round_trip_u128_boundaries() { + // The widen/narrow pair is identity across the u128 range. + for v in [0, 1, TWO_POW_120 - 1, TWO_POW_120, 0xffffffffffffffffffffffffffffffff] { + assert_eq(u256_to_u128(u256_from_u128(v)), v); + } + } + + #[test] + fn matches_native_for_small_values() { + // Exact division: no rounding either way. + assert_eq(mul_div(1000, 3, 3, ROUND_DOWN), 1000); + assert_eq(mul_div(1000, 3, 3, ROUND_UP), 1000); + // 1:1 ratio, the vault's initial state. + assert_eq(mul_div(1000, 1, 1, ROUND_DOWN), 1000); + // Zero numerator. + assert_eq(mul_div(0, 12345, 7, ROUND_UP), 0); + } + + #[test] + fn rounding_direction_is_preserved() { + // 7 / 2 = 3 remainder 1: down floors, up ceils. This is the invariant the vault's economic + // safety depends on — rounding must never shift, or value leaks to or from depositors. + assert_eq(mul_div(7, 1, 2, ROUND_DOWN), 3); + assert_eq(mul_div(7, 1, 2, ROUND_UP), 4); + // 10 / 3 = 3 remainder 1. + assert_eq(mul_div(5, 2, 3, ROUND_DOWN), 3); + assert_eq(mul_div(5, 2, 3, ROUND_UP), 4); + // No remainder => up does NOT add one. + assert_eq(mul_div(9, 1, 3, ROUND_UP), 3); + } + + #[test] + fn product_that_overflowed_u128_now_converts() { + // a * b here is 2^200, far beyond u128::MAX (2^128 - 1); the native `a * b` reverted. + // 2^100 * 2^100 / 2^100 = 2^100, which fits u128. + let two_pow_100: u128 = 0x10000000000000000000000000; + assert_eq(mul_div(two_pow_100, two_pow_100, two_pow_100, ROUND_DOWN), two_pow_100); + + // A non-trivial ratio at the same scale: (2^100 * (3 * 2^100)) / (2 * 2^100) = 3 * 2^100 / 2 + // = 1.5 * 2^100 => floor is 2^100 + 2^99, ceil the same (exact, no remainder). + let expected: u128 = two_pow_100 + (two_pow_100 / 2); + assert_eq(mul_div(two_pow_100, 3 * two_pow_100, 2 * two_pow_100, ROUND_DOWN), expected); + assert_eq(mul_div(two_pow_100, 3 * two_pow_100, 2 * two_pow_100, ROUND_UP), expected); + } + + #[test] + fn max_operands_do_not_overflow_the_intermediate() { + // Both operands at u128::MAX: native `a * b` = (2^128-1)^2 reverts; here it divides cleanly. + let max: u128 = 0xffffffffffffffffffffffffffffffff; + // max * max / max = max. + assert_eq(mul_div(max, max, max, ROUND_DOWN), max); + } + + #[test(should_fail_with = "division by zero")] + fn zero_denominator_reverts() { + // The helper guards a zero denominator explicitly (unreachable from the vault, whose + // denominators are total_assets+1 / total_supply+vault_offset with vault_offset >= 1). + let _ = mul_div(100, 5, 0, ROUND_DOWN); + } + + #[test(should_fail_with = "conversion result exceeds u128")] + fn result_exceeding_u128_reverts_cleanly() { + // max * max / 1 = (2^128-1)^2, which genuinely does not fit u128. Reverts with our message + // rather than a raw range-check failure. Unreachable for real balances (would require a + // denominator far below the operands), but proves the narrowing guard is present. + let max: u128 = 0xffffffffffffffffffffffffffffffff; + let _ = mul_div(max, max, 1, ROUND_DOWN); + } +} diff --git a/src/vault_contract/src/main.nr b/src/vault_contract/src/main.nr index 0db56f24..ee1eec27 100644 --- a/src/vault_contract/src/main.nr +++ b/src/vault_contract/src/main.nr @@ -1,3 +1,4 @@ +pub mod conversion; pub mod test; use aztec::macros::aztec; @@ -1451,7 +1452,6 @@ pub contract Vault { /// @param vault_offset The vault offset /// @param rounding The rounding direction (ROUND_UP or ROUND_DOWN) /// @return The equivalent amount of shares - // TODO: The multiplication `assets * (total_supply + vault_offset)` can overflow u128 for large values. #[contract_library_method] fn _convert_to_shares( assets: u128, @@ -1460,13 +1460,14 @@ pub contract Vault { vault_offset: u128, rounding: bool, ) -> u128 { - let mul_term = assets * (total_supply + vault_offset); - let denominator = total_assets + 1; - let mut shares = mul_term / denominator; - if (rounding == ROUND_UP) & (mul_term % denominator > 0) { - shares = shares + 1; - } - shares + // `assets * (total_supply + vault_offset)` overflows u128 for large inputs; compute it in a + // 512-bit intermediate. Rounding is unchanged (see `conversion::mul_div`). + crate::conversion::mul_div( + assets, + total_supply + vault_offset, + total_assets + 1, + rounding == ROUND_UP, + ) } /// @notice Converts an amount of shares to assets using the given exchange rate parameters @@ -1476,7 +1477,6 @@ pub contract Vault { /// @param vault_offset The vault offset /// @param rounding The rounding direction (ROUND_UP or ROUND_DOWN) /// @return The equivalent amount of assets - // TODO: The multiplication `shares * (total_assets + 1)` can overflow u128 for large values. #[contract_library_method] fn _convert_to_assets( shares: u128, @@ -1485,13 +1485,14 @@ pub contract Vault { vault_offset: u128, rounding: bool, ) -> u128 { - let mul_term = shares * (total_assets + 1); - let denominator = total_supply + vault_offset; - let mut assets = mul_term / denominator; - if (rounding == ROUND_UP) & (mul_term % denominator > 0) { - assets = assets + 1; - } - assets + // `shares * (total_assets + 1)` overflows u128 for large inputs; compute it in a 512-bit + // intermediate. Rounding is unchanged (see `conversion::mul_div`). + crate::conversion::mul_div( + shares, + total_assets + 1, + total_supply + vault_offset, + rounding == ROUND_UP, + ) } /** ========================================================== From b22f58ec52dccd31566828b39742dc11bc56c6a2 Mon Sep 17 00:00:00 2001 From: Alejo Amiras Date: Wed, 19 Aug 2026 19:06:21 +0000 Subject: [PATCH 2/2] ci: run JS tests against mainnet block geometry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .github/workflows/main-tests.yml | 28 +++++++++++++++++++++++++ .github/workflows/pr-checks.yml | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/.github/workflows/main-tests.yml b/.github/workflows/main-tests.yml index 919252df..dcf74380 100644 --- a/.github/workflows/main-tests.yml +++ b/.github/workflows/main-tests.yml @@ -14,4 +14,32 @@ jobs: uses: AztecProtocol/aztec-ci-actions/.github/workflows/run-tests.yml@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 with: runner: ubuntu-latest + # JS tests run in the `js-tests` job below instead, which needs a job-level env var + # that a reusable workflow cannot inherit from its caller. + run-js-tests: false secrets: inherit + + # Kept in lockstep with the `js-tests` job in pr-checks.yml — see the rationale for + # SEQ_BLOCK_DURATION_MS there. + js-tests: + name: JS Tests + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + # 6s blocks = mainnet geometry (10 blocks/checkpoint, 117,668 DA gas per tx). The local + # default of 3s blocks caps a tx at 55,882, below the ~97k the protocol guarantees for a + # maximal contract class registration, which the Vault class publication needs. + SEQ_BLOCK_DURATION_MS: '6000' + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + fetch-depth: 0 + + - name: Setup Aztec environment + uses: AztecProtocol/aztec-ci-actions/actions/setup-aztec@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 + with: + start-pxe: 'true' + run-codegen: 'true' + + - name: Run JS tests + uses: AztecProtocol/aztec-ci-actions/actions/js-tests@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 3053f025..857ce309 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -49,8 +49,43 @@ jobs: uses: AztecProtocol/aztec-ci-actions/.github/workflows/run-tests.yml@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 with: runner: ubuntu-latest + # JS tests run in the `js-tests` job below instead, which needs a job-level env var + # that a reusable workflow cannot inherit from its caller. + run-js-tests: false secrets: inherit + # Mirrors the `js-tests` job of aztec-ci-actions/run-tests.yml, inlined for one reason: + # SEQ_BLOCK_DURATION_MS below. Reusable workflows do not inherit the caller's `env`, and + # run-tests.yml exposes no knob for it, so the only way to reach the node is to own the job. + js-tests: + name: JS Tests + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + # `aztec start --local-network` defaults to 3s blocks. With 72s slots that is 21 blocks per + # checkpoint, and the per-tx DA admission limit is ceil(budget / blocks * 1.5) = 55,882. + # Mainnet runs 6s blocks (10 blocks per checkpoint -> 117,668); the protocol picks that + # multiplier precisely so "a maximal contract class registration (~97k DA gas) fits a single + # block under v5 mainnet geometry" (aztec stdlib, gas/tx_gas_limits.ts). + # + # Publishing the Vault contract class costs ~64k DA gas: comfortably valid on mainnet and + # under the ~97k the protocol guarantees, but rejected by the default local geometry. Pinning + # 6s blocks makes CI reproduce mainnet rather than a network stricter than the one we ship to. + SEQ_BLOCK_DURATION_MS: '6000' + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + fetch-depth: 0 + + - name: Setup Aztec environment + uses: AztecProtocol/aztec-ci-actions/actions/setup-aztec@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 + with: + start-pxe: 'true' + run-codegen: 'true' + + - name: Run JS tests + uses: AztecProtocol/aztec-ci-actions/actions/js-tests@431859e477234b8690eb1f80d1305d2e34f10f1b # v0.1.1 + benchmark: # Skipping also skips this branch's baseline-artifact upload; a PR stacked on # top of a skipped branch falls back to `if_no_artifact_found: warn` downstream.