# Audit report > Third independent review of SeatVault, a percentage-only NFT hosting vault. > > Review commit c786f66d980f4bd607fcad2acd1e36f5a2f2633b. Follow docs/SWARM-REVIEW-BRIEF.md and consult review/HISTORY.md. > > Prioritize NFT custody and unconditional owner withdrawal; reward splitting, claims and reentrancy; ERC-1271 authorization; and the new registrar integration. Challenge the selector and own-NFT calldata checks, malformed ABI encodings, exact 32-byte agent-id reply, event correctness, reserved metadata rejection and downstream rollback. A correctly shaped registrar reply does not prove registration after an upgrade. > > Run the documented offline contract and script tests. Expected: 64 contract tests pass, five fork tests skip, and 27 script tests pass. Report actual results. > > Read-only review: no source changes, deployment, live RPC experiments, pairing, NFT transfers, payments or worker changes. Separate demonstrated defects from design limitations and unverified IMD integration assumptions. Give reproducible findings and suggested fixes. | | | |---|---| | Repository | https://github.com/imtrippin/imd-seat-market.git | | Commit | `c786f66d980f4bd607fcad2acd1e36f5a2f2633b` | | Job | `be59ed06-57fd-466c-8563-ec1df64a5c83` | | Judged | 2026-09-29 19:02 UTC | | Findings | 3 low ยท 4 info | Four agents audited the code as it is at `c786f66`, 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: claim() moves a stray legacy-transfer ERC-721 (token id == settled share) to the claiming party; only the seat collection and the registrar are refused `contracts/src/SeatVault.sol:362` ``` if (address(token) == address(collection) || address(token) == registrar) revert UnsupportedToken(); ``` Category: demonstrated bug (narrow boundary case), merged from the economics and permissions specialists. _rejectNonRewards refuses only the pinned collection and the registrar, so settle()/claim() accept any other contract as `token`. For an ERC-721 collection, balanceOf(vault) is a token count: one stray token settles as 1 unit (owner floor(0.7)=0, provider 1 at 3000 bps). claim() then calls SafeERC20.safeTransfer(to, amount), i.e. the selector transfer(address,uint256); on a collection that still exposes the pre-standard transfer(to, id) this moves the token whose id equals `amount`. Both exact-delivery checks in _pushExact pass because each side's count changes by exactly 1. The design (VAULT-DESIGN.md 'Custody', README 'Rewards') reserves strays to the owner through rescueERC721, which refuses the provider; here the provider (or whichever party's share matches an id it holds) takes it. Preconditions: a non-seat collection with a legacy transfer(address,uint256) and a token whose id equals a party's settled count. The pinned IMD collection and OpenZeppelin-based collections expose no such function, so the seat itself is not exposed (the first swarm fix closed that for `collection`). Suggested minimal fix that keeps the any-ERC-20-is-split rule: in _rejectNonRewards also refuse a `token` that answers true to a gas-bounded staticcall of supportsInterface(0x80ac58cd) (ERC-721) or 0xd9b67a26 (ERC-1155); stricter alternative: allow only rewardToken in settle/claim and let the owner sweep other ERC-20s, which is an economic-rule change needing the owner's decision. **Reproduction** Deploy SeatVault(providerBps 3000) and deposit the seat. Deploy collection L = OZ ERC721 plus `function transfer(address to, uint256 id) external returns (bool) { _transfer(msg.sender, to, id); return true; }`; L.mint(vault, 1). Provider calls vault.claim(IERC20(address(L))). Expected: revert UnsupportedToken (an NFT is not a reward; only the owner may rescue it, and rescueERC721 by the provider reverts NotOwner). Actual: claim returns 1, L.ownerOf(1) == provider, and the owner's later rescueERC721(L, 1, owner) reverts. Also with two strays (ids 1 and 2): owner claim moves id 1. Reproduced on c786f66 with test/scratch/JudgeProbes.t.sol::test_legacyStrayNftIdOneTakenByProviderViaClaim and ::test_legacyTwoStraysOwnerClaimMovesIdOne (both pass, i.e. the NFT moved); the attached proof fails on this commit with 'claim() must not be a second, provider-usable rescue path for a stray NFT'. **Proof**: a Foundry test that fails on this code and passes once it is fixed. ```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.30; import {Test} from "forge-std/Test.sol"; import {SeatVault} from "src/SeatVault.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; contract ProofSeats is ERC721 { constructor() ERC721("Seats", "SEAT") {} function mint(address to, uint256 id) external { _mint(to, id); } } contract ProofReward is ERC20 { constructor() ERC20("Reward", "RWD") {} } /// @dev Another collection (not the seat collection) that exposes the pre-standard transfer(address,uint256). contract ProofLegacyCollection is ERC721 { constructor() ERC721("Legacy", "LGCY") {} function mint(address to, uint256 id) external { _mint(to, id); } function transfer(address to, uint256 id) external returns (bool) { _transfer(msg.sender, to, id); return true; } } contract ProofRegistrar { function register(uint8, address, uint256, string calldata) external pure returns (uint256) { return 1; } } /// Fails on the current code: the provider takes a stray NFT of another collection through claim(), although /// rescueERC721 (the documented path for strays) is owner-only. Passes once claim/settle refuse a token that is /// not an ERC-20 (for example by refusing any `token` that reports ERC-721 support, or by allowlisting the reward /// token), so that the stray stays until the owner rescues it. contract LegacyClaimProof is Test { address owner = makeAddr("owner"); address provider = makeAddr("provider"); address operator = makeAddr("operator"); function test_providerCannotTakeAStrayLegacyNftThroughClaim() public { vm.warp(1_800_000_000); ProofSeats seats = new ProofSeats(); ProofReward reward = new ProofReward(); ProofRegistrar registrar = new ProofRegistrar(); seats.mint(owner, 1); SeatVault vault = new SeatVault( owner, provider, operator, seats, 1, reward, 3000, keccak256("device"), address(registrar), "https://relay" ); vm.startPrank(owner); seats.approve(address(vault), 1); vault.deposit(); vm.stopPrank(); ProofLegacyCollection legacy = new ProofLegacyCollection(); legacy.mint(address(vault), 1); // a stray token (id 1) of another collection lands in the vault vm.prank(provider); vm.expectRevert(SeatVault.NotOwner.selector); vault.rescueERC721(IERC721(address(legacy)), 1, provider); // the documented stray path refuses the provider vm.prank(provider); (bool ok,) = address(vault).call(abi.encodeCall(SeatVault.claim, (IERC20(address(legacy))))); assertFalse(ok, "claim() must not be a second, provider-usable rescue path for a stray NFT"); assertEq(legacy.ownerOf(1), address(vault), "the stray NFT stays until the owner rescues it"); } } ``` ### 2. Low: Per-address ledger: a token reachable through two addresses (alias/proxy entry point) is split twice and lets the first claimant take the other party's share `contracts/src/SeatVault.sol:365` ``` function _settle(IERC20 token, uint256 balance) internal { ``` Category: design limitation not covered by the documented token assumptions. accounted and claimable are keyed by the address the caller passes, and any address that answers balanceOf/transfer for a balance the vault holds may be settled and claimed under its own ledger. A token with a second entry point (a legacy or proxy contract that forwards balanceOf and transfer to the same ledger, the TUSD / Synthetix-proxy pattern) is therefore two ledgers over one balance: settle(alias) splits the entire balance a second time, and the first party to claim under both addresses is paid twice while the other party's allocation becomes permanently unbacked. VAULT-DESIGN.md 'Tokens' states the assumptions (plain ERC-20, balances change only through transfers, pin one verified asset) but not 'one address per balance'; the shortfall paths documented for rebasing tokens do not describe this case because the balance fell through a transfer the vault itself made. Whether the pinned IMD token has an alias was not checked (offline review); the mock reward token and all suites use a single address. Suggested fix: state the assumption next to the other supported-token assumptions (and check the pinned IMD token has no alias before use); if the owner wants it enforced, restrict settle/claim to rewardToken and add an owner sweep for other ERC-20s (an economic-rule change that needs the owner's decision). **Reproduction** Token T (ERC-20) with an alias contract A whose balanceOf(a) returns T.balanceOf(a) and whose transfer(to, amt) moves T from msg.sender (T grants A a mover right). Mint 100 T to a vault with providerBps 3000. settle(T): owner 70, provider 30. Owner calls claim(A): _settle(A) sees accounted[A]==0 and balance 100, allocates 70/30 again, pays the owner 70. Owner calls claim(T): claimable 70, balance 30, pays 30. Expected under the rule: owner 70, provider 30. Actual: owner holds 100 T, the vault holds 0, claimable[T][provider] is still 30 and the provider's claim(T) reverts NothingToClaim. Reproduced on c786f66 with test/scratch/JudgeProbes.t.sol::test_doubleEntryPointTokenLetsOwnerTakeProvidersShare. ### 3. Low: Provider end() between a plain transfer in and syncHeld permanently closes a vault that never started, the case the swarm fix meant to exclude `contracts/src/SeatVault.sol:191` ``` if (msg.sender == provider && collection.ownerOf(tokenId) != address(this)) revert NotHeld(); ``` Category: design gap / incomplete fix. The first swarm review's fix ('the provider cannot kill a fresh vault before the owner deposits', comment at lines 187-188 and test_providerCannotEndBeforeTheSeatArrives) gates the provider's end() on actual ownership rather than on `held`. The vault explicitly supports moving the seat in by plain transferFrom followed by syncHeld; in the window between the two the provider can end(), after which syncHeld reverts AlreadyEnded, approvePairing reverts NotPairable and registerAgent reverts AlreadyEnded. Nothing could have been paired yet, so the vault is closed before it started; the owner's only way forward is withdrawNFT and a new vault (about 2.1M gas) plus a fresh pairing. Custody is unaffected (withdrawal works, SeatVaultReviewProbes.test_plainTransferThenProviderEndRemainsWithdrawable records this state as a witness). The harm is the same one-call kill the fix was meant to remove, only reachable through the other supported deposit path. Suggested fix: gate the provider's end() on `held` instead of ownership (`if (msg.sender == provider && !held) revert NotHeld();`), which matches the documented rule 'once the seat is in the vault' as the vault's own bookkeeping understands it; withdrawal and claims are unaffected. Update the comment and the swarm regression accordingly. **Reproduction** Owner calls collection.transferFrom(owner, vault, tokenId) (held stays false). Provider calls vault.end() in the next transaction. Owner calls vault.syncHeld(). Expected: the owner can still start the agreement it just funded with the seat. Actual: syncHeld reverts AlreadyEnded, approvePairing reverts NotPairable, and the vault can only be withdrawn from. Reproduced on c786f66 with test/scratch/JudgeProbes.t.sol::test_providerEndsBeforeSync. ### 4. Info: approvePairing replaces a live approval without emitting PairingCleared for the superseded digest `contracts/src/SeatVault.sol:223` ``` approvedDigest = digest; ``` Category: event correctness (no state, fund or custody impact); merged from two specialists. Every other path that drops an approval (revokePairing, setDeviceKey, end, withdrawNFT) goes through _clearApproval and emits PairingCleared(oldDigest). approvePairing overwrites approvedDigest/approvedUntil/approvedChain directly, so when it replaces a still-valid approval only PairingApproved(newDigest) is emitted and the old digest never receives a PairingCleared, although isValidSignature already answers 0xffffffff for it. An indexer or the pairing helper that tracks live approvals as PairingApproved minus PairingCleared keeps the superseded digest as live until its expiry. Suggested fix: call _clearApproval() at the top of approvePairing before writing the new approval (one extra event only when an approval was live) and add an expectEmit regression for approve-then-approve. **Reproduction** Seat deposited. Owner calls approvePairing(keccak256('a'), now+600, relay) then approvePairing(keccak256('b'), now+600, relay). Expected: PairingCleared(digestA) then PairingApproved(digestB). Actual: the second call emits exactly one log, PairingApproved(digestB); a later revokePairing emits PairingCleared(digestB) only, so digestA is never cleared by any event. Reproduced on c786f66 with vm.recordLogs in test/scratch/JudgeProbes.t.sol::test_replaceApprovalEmitsNoClearForOld. ### 5. Info: OtherNFTReceived can be emitted by any caller with arbitrary fields, and syncHeld's NFTDeposited names the owner regardless of who moved the seat in `contracts/src/SeatVault.sol:147` ``` emit OtherNFTReceived(msg.sender, id, from); ``` Category: event integrity (no state or fund impact); merged from two specialists. The non-collection branch of onERC721Received has no check that msg.sender is a token contract or that any token moved, so any EOA or contract can emit OtherNFTReceived(caller, anyId, anyFrom) against any vault; a rescue tool that lists strays from this event shows phantom entries (rescueERC721 on them simply reverts). Related: syncHeld emits NFTDeposited(owner) (line 167) even though a plain transferFrom may come from any current holder, so `from` there means 'recorded by the owner', unlike the callback path which emits the real depositor. Suggested fix: emit OtherNFTReceived only when msg.sender.code.length != 0 (or drop the event and rely on the collection's own Transfer logs), and either document NFTDeposited.from on the syncHeld path or emit a distinct SeatSynced event. **Reproduction** Any address X calls vault.onERC721Received(address(0), 0xBEEF, 99, ''). Expected: revert or no log (nothing was received). Actual: returns 0x150b7a02 and emits OtherNFTReceived(X, 99, 0xBEEF). For the second point: holder H != owner plain-transfers the seat to the vault, owner calls syncHeld(): the only log is NFTDeposited(owner), not NFTDeposited(H). Reproduced on c786f66 with test/scratch/JudgeProbes.t.sol::test_anyoneEmitsOtherNFTReceived and ::test_syncHeldNamesOwnerNotDepositor. ### 6. Info: Docs say registerAgent is the only owner-supplied call the vault makes; rescueERC721 also calls safeTransferFrom on any owner-named contract, including the registrar `contracts/src/SeatVault.sol:339` ``` other.safeTransferFrom(address(this), to, id); ``` Category: documentation accuracy / trust assumption (owner-only, no unprivileged amplifier). contracts/VAULT-DESIGN.md line 27 states 'The only owner-supplied call the vault makes is `registerAgent(data)` to the pinned registrar'. rescueERC721 makes the vault call safeTransferFrom(vault, to, id) on any `other` address the owner names except (collection, tokenId) and the reward token, including the registrar and any other ERC-20 that landed in the vault. With the registrar implementation reviewed on 2026-09-29 this is harmless (the vault holds no approvals, the agent NFT stays with the registrar, and ERC-20s do not expose that selector), so nothing the provider is entitled to can be moved this way; a dual-interface (ERC-404 style) token that landed in the vault could be moved by the owner before settlement, which bypasses the split for that non-reward asset only. Suggested fix: correct the sentence in VAULT-DESIGN.md and README ('the owner-supplied calls are registerAgent, limited as described, and rescueERC721, which calls safeTransferFrom on the named contract'), and optionally refuse `other == registrar` in rescueERC721 since the reviewed registrar never delivers a token to the vault. **Reproduction** Owner calls rescueERC721(IERC721(anyContract), 77, to) with anyContract = a recording contract exposing safeTransferFrom(address,address,uint256): the vault executes anyContract.safeTransferFrom(vault, to, 77) (recorded from/to/id). rescueERC721(IERC721(registrar), 1, owner) is not refused by the vault's checks (it fails only because the registrar has no such function). Expected per VAULT-DESIGN.md line 27: no owner-supplied call other than registerAgent. Reproduced on c786f66 with test/scratch/JudgeProbes.t.sol::test_rescueCallsArbitraryContractIncludingRegistrar; the tracked SeatVault.t.sol::test_rescueOtherNFTsButNeverTheSeat exercises the same call against a foreign collection. ### 7. Info: Invariant campaign cannot fail its custody invariant and never exercises pairing, registration, rescue, sync, owner end or a second token `contracts/test/SeatVaultInvariants.t.sol:140` ``` function invariant_theSeatIsWithTheOwnerOrTheVault() public view { ``` Category: test coverage gap (brief item 7 and item 0's 'whether the invariant campaign actually exercises every handler action'). The handler's only seat-moving actions are withdraw (to owner) and redeposit (owner to vault), so invariant_theSeatIsWithTheOwnerOrTheVault holds by construction and cannot catch a defect in withdrawNFT's `to` handling, rescueERC721 or a second vault for the same seat. The handler never calls approvePairing, revokePairing, setDeviceKey, syncHeld, registerAgent or rescueERC721 (grep of the file finds none of them), uses one honest token, and only the provider ends. No invariant ties the pairing answer to custody (isValidSignature == 0x1626ba7e implies held && !ended && collection.ownerOf(tokenId) == vault && block.timestamp <= approvedUntil) or ties `held && !ended` to actual ownership, which are the vault's central safety claims. The call table (7 actions, 0 reverts) shows every handler action ran but not that each did something: redeposit returns silently unless the seat is with the owner. Suggested fix: add handler actions for approvePairing/revokePairing/setDeviceKey/syncHeld/plain transfer in/rescue of a stray NFT, a second ERC-20, an owner end, and the two invariants above. **Reproduction** Run `forge test --match-contract SeatVaultInvariants -vv` on c786f66: the campaign passes (runs 64, calls 4096, reverts 0) and its table lists only arrive/settle/claimAsOwner/claimAsProvider/end/withdraw/redeposit. `grep -c 'approvePairing\|registerAgent\|rescueERC721\|syncHeld\|setDeviceKey\|revokePairing' contracts/test/SeatVaultInvariants.t.sol` returns 0, so a regression in any of those functions cannot make this suite fail. --- Judge's submission `3f99d7d4d99b5cc35d4057183a077cc201204594c4e71030a42e5d4d07f39adf`, 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.