Skip to content

Latest commit

 

History

History
254 lines (198 loc) · 14.4 KB

File metadata and controls

254 lines (198 loc) · 14.4 KB

Adversarial review — 2026-08-08

A hostile pass over the contracts written today, plus the live checks that back it up. Written down rather than summarised in a commit message because none of the six findings were fixed in the deployed source, and that decision needs its reasoning attached.

Findings 1 and 2 came from the first pass. Findings 3–5 came from a second, stricter pass over the whole submission — site, code and copy — run after the first pass had already declared the contracts clean. Finding 6 came from a third. That progression is worth stating plainly: the first review concluded "nothing here is reachable by a third party" and stopped, the finding that actually touches money was sitting one question further on, and a plain code-versus-comment mismatch survived two passes because nobody re-read a file the rework had not touched.

What was attacked

Third-party exploitability, in order of what would hurt most:

Attack Result
Mint a note with no obligor confirmation Blocked — originate reverts ConfirmationNotFound; proven live, the recorded demo opens on it
Mint a second note off one confirmation Blocked — tokenIdForKey[key] != 0
Confirm the same debt twice Blocked — AlreadyConfirmed
Fund a note twice Blocked — n.funded set before any external call
Settle someone else's note Blocked — msg.sender != n.obligor
Move a note to an unverified wallet Blocked — _update re-checks isVerified(to)
Reenter originate through _safeMint's onERC721Received Safe — tokenIdForKey, notes[] and _nextTokenId are all written before the mint, so the reentrant call hits the same-key guard
Reenter fund / settle through the asset Safe — funded / settled are set before escrow is called, and cvaUSD is a plain ERC-20 with no transfer callback
Starve a gate by making the CVI read revert Safe — _read and _ruleSatisfied catch, and every catch resolves to unverified

Nothing here is reachable by a third party. Every finding below is owner-only — but "owner-only" is not the same as "harmless", and Finding 3 is the one that touches money.

Finding 3 — the escrow owner can drain any outstanding allowance

Found on a second pass, and the most serious of the three.

FactoringEscrow.setNote is onlyOwner and stays mutable for the life of the contract, while every cash leg moves value with safeTransferFrom against an allowance the counterparty granted to the escrow. Those two facts compose:

  1. the owner calls setNote(attackerAddress), which passes onlyNote from then on;
  2. the attacker calls fundAdvance(cvaUSD, victim, attackerWallet, victimAllowance).

Both parties are checked by _requireCompliant, so the attacker's wallet must hold a live A-Pass — which the desk's own /api/onboard hands out in one click. The compliance gate is not a barrier here; it is a formality the attacker satisfies in a browser.

This is live, not hypothetical: at the time of review the financier wallet 0x85D03949…03CaD3 held an unused 3,000 cvaUSD allowance to the escrow, left over from a lifecycle run.

Two things bound the blast radius, and neither makes it a non-issue. The dashboard approves the exact advance immediately before funding rather than an unbounded amount (NotesRegister.tsx), so a wallet is only exposed between its approve and its fund. And settleRedemption is bounded the same way. The exposure is therefore one approval deep per participant, not a standing claim on their balance.

Mitigated by revoking the outstanding allowance, which removes the live exposure but not the capability. The capability itself closes in one of two ways: renouncing the escrow's ownership, which permanently removes setNote and costs the ability to ever repoint the note; or making note immutable and set once, which is a redeploy. Both are recorded here rather than done silently, because the first is irreversible and the second breaks the verification match for the reasons below.

Finding 4 — the escrow's own comment describes an architecture that was replaced

FactoringEscrow's doc comment states that the settlement asset is MockAToken, "a bare ERC-20 stand-in", because "real aUSDC's transfer gate ABI isn't public, so a pool/escrow contract can't hold real aUSDC". That was true when it was written and is now false: note.asset() is 0x01926a2F…ACF0B, a real Cleanverse A-Token, and the escrow moves it fine.

It matters more than a stale comment normally would. The contract is verified at exact_match, so "source ↗" on the deck opens this text, and it contradicts the single strongest claim in the submission — that the settlement leg is genuinely Cleanverse's asset and not a mock. A reader who checks the source finds the project arguing against itself.

It is not corrected in place for the same reason as everything else in this section: the metadata hash covers the source bytes, comments included, so editing one word un-matches the deployed bytecode. Correcting it in the repository alone would be worse still — then GitHub and Sourcify would show different files, which is exactly the discrepancy the verification exists to rule out. The behaviour the comment describes badly is documented correctly in the README, and the comment is first in line for the post-hackathon change.

Finding 5 — three unguarded inputs in originate

None reachable for profit, all worth closing:

  • faceValue is not required to be non-zero. fund and settle both use n.faceValue == 0 as their existence check, so a zero-value note mints a real ERC-721 that both then treat as non-existent — a token that exists to ownerOf and not to the lifecycle. Costs the caller gas and produces a permanently inert note.
  • maturity is not validated. A note can be originated already past maturity, and settle never checks that maturity has been reached, so "at maturity" in the docs is narrative rather than enforced. Early settlement by the obligor is benign; the docs overstating the guarantee is the actual defect.
  • The obligor's current verification is not re-read at origination, only the subTier snapshotted at confirmation. An obligor revoked between confirming and originating still yields a mintable note. No money can move on it — fund re-reads the obligor and reverts — so the result is a dead note, not a loss.

Finding 6 — keyFor's own comment omits a field it hashes

ObligationRegistry.keyFor documents the confirmation key as (invoiceHash, obligor, faceValue, maturity). Two lines below, it hashes (invoiceHash, obligor, supplier, faceValue, maturity)supplier is part of the key and is missing from the sentence describing it.

It reads as a smaller claim than the code makes. Including supplier means two different suppliers cannot collide on one obligor's invoice, which is stronger than the comment implies, so the defect understates the contract rather than overstating it. The README (## How it works, step 1) states the key correctly.

Found on a third pass, in a file that had not been re-read since the subTier rework. Same constraint as Finding 4: ObligationRegistry is verified at exact_match, so the comment cannot be corrected without un-matching the deployed bytecode.

Finding 1 — setRateBands accepts an empty curve

delete _rateBands runs before the loop, so setRateBands([]) leaves the note with no bands at all. advanceRateBpsFor then returns 0 for every subTier and every origination reverts NoAdvanceRateForSubTier. The desk is bricked by an owner call that emits RateBandsChanged(0) and looks successful.

Finding 2 — a band floor of 0 would price an unreadable identity

subTier() returns 0 for three different situations: no A-Pass, a value outside Cleanverse's documented 0–99 range, and a genuine subTier of 0. A band with minSubTier: 0 would therefore quote a rate for an identity the registry could not read. The deployed curve floors at 20 so this is latent, but the ordering check does not forbid it.

Neither is exploitable without the owner key, and the owner key can already set any curve it likes.

Why none of them were fixed in the source

Every one of these fixes is a few lines. Shipping them is not.

The contracts are deployed, verified on Sourcify at exact_match, and every receipt on the pitch, in the README and in the one-pager points at those addresses. Editing the source without redeploying would break the verification match and leave the repository disagreeing with the chain — a reviewer comparing the two would find a real discrepancy where there is currently none. Editing and redeploying means new addresses, a fresh lifecycle run, re-verification, and four documents plus two PDFs updated, roughly thirty hours before a deadline, to close footguns that only the owner can trigger.

The deployed system being consistent with its own evidence is worth more than closing an owner-only footgun. What was done instead, on-chain and without touching a byte of source: the outstanding allowance behind Finding 3 was revoked in 0x9755ff3f…fe8c3a7f, verified by reading the allowance back as 0. The one concrete exposure is gone; the capability remains. Re-runnable at any time with npm run revoke.

The post-hackathon change is already specified by the findings above:

Finding Fix
1 if (bands.length == 0) revert EmptyCurve();
2 minSubTier >= 1 floor in the ordering check
3 set note once in the constructor and drop setNote, making it immutable
4 rewrite the FactoringEscrow comment around the real A-Token
5 require faceValue > 0 and maturity > block.timestamp; re-read the obligor in originate
6 correct keyFor's comment to name supplier

Operational dependency, verified live

Covenant's gate consults Cleanverse's rule engine on every check. That is the point, and it cuts both ways: Cleanverse can revoke, and Covenant obeys in the same transaction.

Tested end to end on a wallet holding a live, active, subTier-60 US A-Pass:

Step query_apass.status isVerified on-chain
Issued 1 true
update_status → freeze 2 false
update_status → unfreeze 1 true

No relayer, no poll, no redeploy. The same mechanism means a paused pool or a revoked pass halts the desk for that party immediately — which is correct behaviour for a compliance product, and is a dependency worth stating rather than discovering.

Country enforcement was verified the same way: a wallet holding a valid subTier-60 pass issued against a Singapore passport is refused by isVerified on country alone, while four US demo wallets and a freshly onboarded visitor pass.

Scale, checked rather than asserted

The CVI validator is deployed at the same address on Monad testnet, Base Sepolia and BSC testnet — confirmed by calling isRegistered on all three and getting identical behaviour. It was not present at that address on Arbitrum Sepolia or Polygon Amoy, so "every chain" would be an overstatement; three is what was checked.

The band floor, checked on both rule surfaces

Covenant's cheapest band opens at subTier 20, and two independent Cleanverse rules stand between a wallet at exactly 20 and a funded trade: the validator's RuleV2 on the identity registry, and the A-Token's own rule on cvaUSD. Both spell the field min_sub_tier, and they do not mean the same thing — the validator's is strictly greater-than, the token's is inclusive.

That is a one-off waiting to happen at the precise value the pricing argument depends on, so it was measured. A throwaway wallet was issued a real A-Pass at exactly 20:

Check Result
subTier(wallet) on the registry 20
isVerified(wallet) — validator RuleV2 admits it true
advanceRateBpsFor(20) 8800
cvaUSD accepts a mint to it yes — 0x5d6152f5…2b8fd88b, block 52,207,261, balance 0 → 1.000000
atoken/rules reports the token's own floor min_sub_tier: 20, inclusive

The floor holds on every surface. The mint row is a real transaction, not an estimate — the wallet's balance went from 0 to 1.000000 cvaUSD in block 52,207,261, which is the A-Token's own policy admitting a subTier-20 holder on-chain.

npm run boundary (relayer/src/verifyBoundary.ts) re-checks all five and fails loudly if any stops agreeing. Its last step is an eth_call rather than a send — the same bytecode against the same state, just not committed — so the script can be run repeatedly without spending gas or needing the minter key. The committed proof is the hash above.

Monad mainnet, checked directly

Prompted by another team reporting the production registry looked empty, both Cleanverse addresses were probed on Monad mainnet (chainId 143, block 94,408,422):

Mainnet (143) Testnet (10143)
A-Pass registry 0xbA82D1…C58B9 122-byte proxy, impl 0x18914244… 122-byte proxy, impl 0x9406f5d4…
hasAPass(wallet) false — answers, holds nothing true
getAPassData(wallet) reverts 0xfb524a44 (no pass) returns the record
Compliance validator 0xaC7e51…61792 0 bytes — not deployed 122 bytes, answers

The registry on mainnet is not broken, it is empty: it responds correctly and no A-Pass has been issued against it. The validator simply is not there yet.

Two design choices are load-bearing because of this, and both were made before it was checked. validator is owner-settable rather than a constructor argument, so a mainnet deployment works with the rule layer inactive until Cleanverse deploys the validator. And _read catches the revert instead of propagating it, so an empty registry denies every wallet rather than bricking the desk. A Covenant deployed to Monad mainnet today reads the real production registry, verifies nobody, and blocks every action — which is the correct behaviour, reached with no code change.

This also bounds the claim honestly: Covenant is testnet-proven, not mainnet-proven. Nothing has been transacted on Monad mainnet, and nothing could be until the first mainnet A-Pass exists.