# Audit report > Audit BunkerVault, a shared post-quantum authorized vault for public ERC-20 and ETH balances (Foundry repo, solc 0.8.30, Cancun). Read README.md and docs/CONTRACT.md (specification, invariants, trust assumptions) first. > > SCOPE > Primary: src/BunkerVault.sol, src/AuthorizationMessage.sol, src/interfaces/. Secondary: how the vault relies on the vendored research ML-DSA-44 verifier test/vendor/fireblocks/MLDSA44Verifier.sol (return handling, code-hash and helper pinning, 20,545-byte key blob layout); report a verifier bug only if it lets a signature verify without the key. Out of scope: lib/ (vendored forge-std, OpenZeppelin 5.6.1), test/mocks/, script/, deployments/, gas optimizations without security impact. > > DESIGN > One contract holds many vaults. vaultId = keccak(domain, keccak(1312-byte ML-DSA public key)). createVault: the payer submits it themselves, an ML-DSA proof of possession over the full registration intent is verified BEFORE any fee-token call, then exactly 10**decimals fee tokens are pulled: 90% stay locked forever (totalLockedFees), 10% go to an immutable developer. The payer becomes controllerOf[vaultId], permanently. Every later operation (depositERC20, withdrawERC20, depositETH, withdrawETH, rotateKey) goes through _consumeAuthorization: msg.sender must be the controller AND a valid ML-DSA signature must cover chain id, vault address, vaultId, key hash, key code hash, operation constant, paramsHash of all arguments, exact next nonce, expiry (max 1 hour) and key epoch. The nonce is consumed before token/ETH calls, under nonReentrant. ETH is wrapped into one constructor-pinned WETH; receive() accepts ETH only from WETH during withdrawETH. A token's runtime code hash is pinned at its first deposit. rotateKey needs the old key's signature and the new key's possession signature; nonce stays monotonic, epoch increments, old keys stay burned. No owner, admin, upgrade, rescue, approval or ECDSA fallback exists. > > THREAT MODEL > The vault's key file is secret. The attacker may control the controller wallet (e.g. an ECDSA break), any other EOA, contracts, malicious or reentrant tokens, the mempool and block ordering, and may register their own vaults and keys. Main question: can anyone WITHOUT a vault's ML-DSA key move, lock, freeze or misdirect that vault's assets, or take assets from other vaults or the locked fees? > > INVARIANTS TO BREAK > 1. No state change of a vault without a valid signature from its current key plus a transaction from its controller. > 2. A signature authorizes exactly one operation with exactly its signed parameters, once (no replay across nonces, epochs, operations, vaults, contracts or chains; no ambiguous encodings). > 3. For every token: sum of publicBalanceOf over vaults == totalPublicLiability <= token.balanceOf(vault). One vault, or a malicious token, cannot reduce another vault's withdrawable balance of a well-behaved token. > 4. totalLockedFees only grows; the fee token can never leave except the developer share at registration; the fee token is never a custody asset. > 5. The contract holds no ETH between calls except ETH forced in, which is never credited to anyone. > 6. Any failed operation reverts completely, including nonce consumption, asset admission and fee settlement. > 7. Registration and rotation reject wrong key sizes, blob size/prefix/code hash, reused keys, and run PoP before any external fee call. > > FOCUS PER SPECIALIST > - Math: fee split for decimals 1..18, balance-delta checks in deposits/withdrawals/fee settlement, overflow/underflow paths, heldTokens paging bounds. > - Permissions: _consumeAuthorization and every caller of it, controller binding, executor/feePayer checks in createVault and rotateKey, domain separation in AuthorizationMessage and paramsHash encodings, rotation logic, absence of privileged paths, abuse of open admission (registering someone else's key or blob) beyond the disclosed key-squatting risk. > - Economics: fee collection and locking, griefing (blocking a payer slot, front-running registration or rotation, permanent freezes), donations, forced ETH, cost to an attacker vs harm to victims. > - Control flow: reentrancy through tokens, WETH and ETH recipients, the unwrapping flag and receive()/fallback(), ordering of checks/effects/interactions, tokens whose code changes after pinning (metamorphic or proxy), tokens that lie in balanceOf/totalSupply, ERC-777 style hooks, revert and return-data handling of the verifier and SafeERC20. > > KNOWN AND ACCEPTED, see docs/CONTRACT.md section 9 (report only if the impact goes beyond what is stated) > Balances and amounts are public. The contract cannot prove a blob equals prepare(pk) or that a key came from honest KeyGen (clients must check this off-chain); anyone can register a victim's public key first with a blob they control, which only denies that key. Losing or compromising the controller wallet cannot be fixed (no controller change by design). The controller wallet and Ethereum are not quantum safe. The fee token is trusted and reviewed separately. A malicious token can lie about itself; accepting a token is not certifying it, but harm must stay confined to that token. The ML-DSA verifier is unaudited research code. Each verification costs about 1.35M gas. > > HOW TO WORK > forge build; forge test (103 tests). test/utils/VaultFixture.sol builds a vault with a mock verifier that accepts exactly the (key blob, message, signature) it was armed with: use authorize(...) and arm(...) to craft authorizations. test/utils/RealVerifierBase.sol deploys the real verifier. forge test --ffi adds real-key end-to-end tests (needs npm ci in test/ffi, 122 tests). > > DELIVERABLE > Each finding: severity (Critical, High, Medium, Low, Informational), file:line, root cause, exact attack sequence and preconditions, impact on which vaults/assets, a Foundry test under test/ that fails on the current code (or a precise reason why one cannot be written), and a minimal fix. Mark any finding that only restates an accepted risk. Also list the invariants above you checked and could not break, with the reasoning. | | | |---|---| | Repository | https://github.com/BaixoT/imd-hackathon.git | | Commit | `89a1d0814263e2900529314e70ce372f04efa3de` | | Job | `8302b160-5df9-4cda-b173-6c1976a0f8ac` | | Judged | 2026-10-11 23:56 UTC | | Findings | 2 low · 2 info | Four agents audited the code as it is at `89a1d08`, each in one area (math, permissions, economics, control flow), and a judge reproduced, merged and ranked what they found, then read the code once more itself. Nothing in the repository was changed or deployed. ## Findings ### 1. Low: heldTokens paging reverts with Panic(0x11) for an in-range start and a large count instead of clamping to the list end (merged: permissions, economics, math, flow) `src/BunkerVault.sol:298` ``` uint256 end = start + count > all.length ? all.length : start + count; ``` Root cause: the clamp compares `start + count` to `all.length`, but the sum is evaluated in checked arithmetic before the comparison. The early return on line 297 only covers start >= length. For any in-range start >= 1 and count > type(uint256).max - start the addition overflows and the view reverts with Panic(0x11) instead of returning the tail page that the NatSpec ('Out-of-range pages are empty') and docs/CONTRACT.md section 6 promise. The natural client idiom 'everything from start' (count = type(uint256).max) works for start = 0 and reverts for every later page. Impact: availability of the documented enumeration view only; no vault state, nonce, balance or authorization is involved, and an integrating contract that pages with a max count is wedged. Attack sequence: none needed; any caller triggers it. Minimal fix: compute the end without the overflowing sum, e.g. `uint256 remaining = all.length - start; uint256 end = count >= remaining ? all.length : start + count;` (start < all.length is already established so the subtraction cannot underflow and the sum is only evaluated when it is below all.length). Reproduced by the judge: both specialist proofs (Proof_207325c01b74, Proof_355c2cff5a95) fail on the current code with `panic: arithmetic underflow or overflow (0x11)`; the attached proof is the shorter one. **Reproduction** Register one vault, deposit two distinct ERC-20s so heldTokenCount(id) == 2. heldTokens(id, 0, type(uint256).max) returns 2 entries. heldTokens(id, 1, type(uint256).max): expected a one-element page holding the second token; actual: revert Panic(0x11) at line 298. Verified with `forge test --match-path test/scratch/Proof_207325c01b74.t.sol` -> [FAIL: panic: arithmetic underflow or overflow (0x11)]. **Proof**: a Foundry test that fails on this code and passes once it is fixed. ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.30; import {Test} from "forge-std/Test.sol"; import {BunkerVault} from "src/BunkerVault.sol"; import {IMLDSAVerifier} from "src/interfaces/IMLDSAVerifier.sol"; /// Minimal ERC20 (18 decimals by default) used both as fee token and as custody asset. contract MiniToken { uint8 public decimals; uint256 public totalSupply; mapping(address => uint256) public balanceOf; mapping(address => mapping(address => uint256)) public allowance; constructor(uint8 d) { decimals = d; } function mint(address to, uint256 v) external { balanceOf[to] += v; totalSupply += v; } function approve(address s, uint256 v) external returns (bool) { allowance[msg.sender][s] = v; return true; } function transfer(address to, uint256 v) external returns (bool) { balanceOf[msg.sender] -= v; balanceOf[to] += v; return true; } function transferFrom(address f, address to, uint256 v) external returns (bool) { allowance[f][msg.sender] -= v; balanceOf[f] -= v; balanceOf[to] += v; return true; } } /// Minimal WETH9-like contract (only what the constructor needs). contract MiniWeth is MiniToken { constructor() MiniToken(18) {} function deposit() external payable { balanceOf[msg.sender] += msg.value; } function withdraw(uint256 w) external { balanceOf[msg.sender] -= w; payable(msg.sender).transfer(w); } } /// Programmable verifier: accepts exactly the (key, message, signature) it was armed with. contract ArmedVerifier is IMLDSAVerifier { address key; bytes32 msgHash; bytes32 sigHash; function arm(address k, bytes calldata m, bytes calldata s) external { key = k; msgHash = keccak256(m); sigHash = keccak256(s); } function verify(address k, bytes calldata m, bytes calldata s) external view returns (bool) { return k == key && keccak256(m) == msgHash && keccak256(s) == sigHash; } } contract HeldTokensPagingOverflowTest is Test { address blob = makeAddr("blob"); address dev = makeAddr("dev"); MiniToken fee; ArmedVerifier verifier; MiniWeth weth; BunkerVault vault; bytes32 id; function rep(uint8 v, uint256 n) internal pure returns (bytes memory out) { out = new bytes(n); for (uint256 i; i < n; ++i) out[i] = bytes1(v); } function setUp() public { vm.etch(blob, bytes.concat(hex"00", rep(0x01, 20544))); fee = new MiniToken(18); verifier = new ArmedVerifier(); weth = new MiniWeth(); vault = new BunkerVault(address(fee), address(fee).codehash, dev, address(verifier), address(verifier).codehash, address(weth), address(weth).codehash); fee.mint(address(this), 1e18); fee.approve(address(vault), 1e18); BunkerVault.Registration memory reg = BunkerVault.Registration(rep(0x11, 1312), blob, blob.codehash, address(this), address(this), block.timestamp + 1800, rep(0x42, 2420)); verifier.arm(blob, vault.registrationMessage(reg), reg.proofOfPossession); id = vault.createVault(reg); // Deposit two distinct tokens so the held-token list has length 2. _depositNewToken(); _depositNewToken(); assertEq(vault.heldTokenCount(id), 2); } function _depositNewToken() internal { MiniToken t = new MiniToken(18); t.mint(address(this), 100); t.approve(address(vault), 100); bytes32 h = vault.assetConfigurationHash(address(t)); BunkerVault.Account memory acc = vault.account(id); BunkerVault.Authorization memory a = BunkerVault.Authorization(acc.nonce, block.timestamp + 1800, acc.keyEpoch, rep(0x42, 2420)); bytes32 params = keccak256(abi.encode(address(t), uint256(100), h, address(this))); verifier.arm(acc.pkBlob, vault.authorizationMessage(id, vault.DEPOSIT_ERC20(), params, a.nonce, a.expiresAt, a.keyEpoch), a.signature); vault.depositERC20(id, address(t), 100, h, a); } /// heldTokens documents "Out-of-range pages are empty" and `start + count` is clamped to the list /// length, so start=1,count=type(uint256).max should return the one remaining token. /// Current code computes `start + count` with checked arithmetic and reverts with Panic(0x11). function test_HeldTokensClampsLargeCountInsteadOfOverflowing() public view { // Sanity: start=0 with max count works because 0 + max does not overflow. assertEq(vault.heldTokens(id, 0, type(uint256).max).length, 2); // start=1: 1 + max overflows -> Panic(0x11) on current code. Expected: a one-element page. address[] memory page = vault.heldTokens(id, 1, type(uint256).max); assertEq(page.length, 1, "page should hold the single remaining token"); assertEq(page[0], vault.heldTokens(id, 1, 1)[0]); } } ``` ### 2. Low: rotateKey burns arbitrary public keys for gas only: new publicKey bytes are never tied to the blob that proves possession, so key squatting costs no fee and no payer slot (extends accepted risk L1; me `src/BunkerVault.sol:287` ``` usedPublicKeys[newKeyHash] = true; // Retired keys stay burned globally. ``` MARKED: extends accepted limitation L1 (open admission lets a mempool observer deny a victim's key); the vault's custody and nonce are never affected. What goes beyond section 9: L1 and L5 describe squatting only through createVault, where each burned key costs the squatter one registration fee (90% locked forever) and the one-time payer slot of a fresh address. rotateKey writes the same global usedPublicKeys mapping with no fee and no slot. Root cause: the new-key possession proof (line 280) is verified against rotation.pkBlob, which the caller chooses and which may be the vault's CURRENT blob, while rotation.publicKey is free 1312-byte input whose keccak is only checked for prior use (line 273) and then burned globally (line 287). A registered attacker therefore signs both the ROTATE_KEY authorization and the ROTATION_POP message with the single key they already hold and burns any key hash they like, about 2.8M gas per key, repeatably, without ever losing their own vault. Attack sequence: (1) attacker registers once with key K_A and blob B_A; (2) attacker watches the mempool for a victim createVault or rotateKey carrying public key K_V; (3) attacker front-runs with rotateKey(idA, KeyRotation(K_V, B_A, codehash(B_A), attacker, PoP by K_A), Authorization(nonce, expiry, epoch, sig by K_A)); (4) the victim's transaction reverts with PublicKeyAlreadyRegistered and the victim must generate a fresh key and deploy a fresh 20,545-byte blob (about 4.5M gas) for every retry, which the attacker can repeat for each retry. A vault owner that must rotate after suspecting key exposure can be kept from rotating indefinitely; funds remain withdrawable with the old key. Preconditions: one registered vault (one fee ever) and mempool visibility of the victim's key. Minimal fix options (design decision): (a) charge the registration fee (or a dedicated rotation fee) in rotateKey so burning a key costs the same as at registration; (b) require rotation.pkBlob to be a blob never used by any vault (track usedBlobs) so each burned key costs a 20,545-byte blob deployment; (c) commit-reveal for new keys in createVault and rotateKey (commit keccak256(publicKey, salt, msg.sender) one block earlier) which removes the front-running window entirely; (d) at minimum document rotation as a second, fee-free squatting path in CONTRACT.md section 9. **Reproduction** Judge scratch test test/scratch/JudgeRepro.t.sol::test_RotateKeyBurnsVictimKeyWithoutFee (passes on current code, demonstrating the behavior; verifier is an armed mock that only accepts messages signed for the attacker's blob). State: attacker registered vault idA with key 0x11*1312 and blob B_A; victim holds key K_V = 0x22*1312 and its own blob, not yet registered. Input: attacker calls rotateKey(idA, KeyRotation(K_V, B_A, B_A.codehash, attacker, sig), Authorization(0, now+600, 0, sig)). Actual: call succeeds, usedPublicKeys[keccak256(K_V)] == true, attacker fee balance and totalLockedFees unchanged; victim's createVault(Registration(K_V, blobVictim, ...)) reverts PublicKeyAlreadyRegistered; repeating with key 0x33*1312 and the same blob burns a second key for free; a victim who already owns a vault gets PublicKeyAlreadyRegistered on rotateKey to a burned key while its nonce stays untouched. Expected per section 9: squatting a key costs the squatter a registration fee and a payer slot. ### 3. Info: withdrawERC20 accepts recipient == token (including WETH to the WETH contract), stranding the signed amount inside the token contract `src/BunkerVault.sol:349` ``` _checkRecipient(recipient); ``` Boundary inconsistency, not a permission or accounting break. withdrawETH explicitly rejects recipient == address(weth) (line 398) because value sent there is lost, and _checkRecipient rejects address(0) and the vault itself. withdrawERC20 only applies _checkRecipient, so a signed withdrawal whose recipient is the token contract itself passes every exact-delta check (reserve -amount, balanceOf(token) +amount, totalSupply unchanged) and the tokens are permanently stuck inside the token contract. For WETH this is the ERC-20 twin of the case withdrawETH guards against. Only the signing vault's own balance is affected and the recipient is part of the PQ-signed parameters, so invariants 1 to 7 hold; this is a footgun the signer can avoid. Minimal fix: in withdrawERC20 add `if (recipient == token) revert InvalidRecipient();` (optionally also reject recipient == address(weth) for any token, mirroring withdrawETH). **Reproduction** Judge scratch test test/scratch/JudgeRepro.t.sol::test_WithdrawErc20ToTokenAddressSucceeds (passes on current code, demonstrating acceptance). Vault idA deposits 100 units of token T; controller submits withdrawERC20(idA, T, recipient = T, 40, assetHash, validAuth). Expected (by analogy with withdrawETH's WETH check): revert InvalidRecipient. Actual: success, T.balanceOf(T) == 40, publicBalanceOf[idA][T] == 60. Same with token = recipient = WETH after depositETH of 1 ether: withdrawERC20(idA, weth, weth, 0.5 ether, ...) succeeds and weth.balanceOf(weth) == 0.5 ether. ### 4. Info: A 1-wei reserve shortfall of a seizable or rebasing custody token freezes every vault's balance of that token until someone donates the difference (restates accepted risk: harm confined to that token) `src/BunkerVault.sol:358` ``` if (reserveBefore < totalPublicLiability[token]) revert UnexpectedAssetBalance(); ``` MARKED: restates an accepted risk (docs/CONTRACT.md section 8 'not supported: rebasing' and section 9 'harm must stay confined to that token'). withdrawERC20 (line 358) and depositERC20 (line 328) require token.balanceOf(vault) >= totalPublicLiability[token] before acting. For a well-behaved token the vault's balance only falls through its own transfers, so the check is a sound conservation guard and is what keeps invariant 3. For a token with privileged seizure, blacklisting or negative rebase, a shortfall of even 1 wei makes all withdrawals of that token by ALL vaults revert although the reserve still covers 199 of 200 units. The freeze is confined to that token and to the vaults that chose to hold it, other tokens and ETH are unaffected, and it is not permanent: any third-party transfer of the missing amount to the vault restores reserve >= liability and withdrawals resume. Recorded because the Economics focus asks about permanent freezes. No change recommended unless the authors want graceful degradation (e.g. permit withdrawals while reserve >= amount and let the last withdrawer bear the shortfall), which would weaken invariant 3; this is a design decision, not a defect. **Reproduction** Judge scratch test test/scratch/JudgeRepro.t.sol::test_ReserveShortfallFreezesAllVaultsOfThatToken (passes on current code, demonstrating the behavior). Vaults A and B each deposit 100 of a token with an admin seize(). seize(vault, other, 1) -> reserve 199 < liability 200. Both A's and B's withdrawERC20(…, 1, …) revert UnexpectedAssetBalance. After a 1-wei transfer from a third party to the vault, B withdraws its full 100 successfully. --- Judge's submission `11b8d2766b6f83116afc388aa889926161e121df705acf2bff3462ac32262162`, accepted on the IdentityMD network. Acceptance means the report met the job's checks; it is not a guarantee that the code has no other defects.