Skip to content

Commit 0d81dab

Browse files
committed
fix: reconcile the two identity registries, and cap the faucet globally
Three defects found on a pass over the parts where one component mirrors another. **subTier disagreed across the two IIdentityRegistry implementations.** `CleanverseIdentityRegistry._toUint8` folds anything above 99 to 0 and blocks pricing; the devnet mirror returned it verbatim and the relayer clamped to 255. So an out-of-spec subTier of 150 was refused on Monad and priced at the top band in tests — a devnet suite able to prove something production does not do. Both now fold, pinned by testFuzz_outOfRangeSubTierReadsAsZero. 75 tests. **Jurisdiction had three representations of one country.** The contract derives uint16(bytes2(group)) — 21843 for "US" — while the relayer's lookup table answered 1. The table is gone; the mirror now derives what the contract derives, so any two-letter group works and there is no map left to drift. Dormant today, since Cleanverse leaves the group empty and Covenant reads 0 as unconstrained. **The faucet had no global limit.** Its per-address cooldown was not a cap: cycling fresh addresses walks past it, and each pass mints 250,000 cvaUSD. No contract is at risk — every cash leg pulls only from msg.sender — but the settlement token could be inflated without bound. A rolling-hour cap now sits alongside the cooldown, counted only after a mint actually lands so a Cleanverse timeout does not consume the budget. Also corrects STATIC-ANALYSIS.md. It claimed the reentrancy ordering was safe only because cvaUSD has no transfer callback. It is safe regardless: a reentrant settle burns the note, so fund's trailing _transfer then reverts on a token that no longer exists and unwinds everything, and the mirror case in settle behaves the same way including when obligor == supplier. The trailing writes are the guard, not a residual risk.
1 parent ad45a72 commit 0d81dab

8 files changed

Lines changed: 109 additions & 35 deletions

File tree

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# Covenant
22

33
[![CI](https://github.com/UnityNodes/Covenant/actions/workflows/ci.yml/badge.svg)](https://github.com/UnityNodes/Covenant/actions/workflows/ci.yml)
4-
[![Tests](https://img.shields.io/badge/tests-74%20passing-2ea44f)](https://github.com/UnityNodes/Covenant/actions/workflows/ci.yml)
4+
[![Tests](https://img.shields.io/badge/tests-75%20passing-2ea44f)](https://github.com/UnityNodes/Covenant/actions/workflows/ci.yml)
55
[![Coverage](https://img.shields.io/badge/coverage-97.5%25%20lines%20%C2%B7%2094.1%25%20branches-2ea44f)](docs/SCALE.md)
66
[![Sourcify](https://img.shields.io/badge/Sourcify-5%2F5%20exact__match-6E54FF)](https://repo.sourcify.dev/10143/0xAa15aBFC21424B842B5272BdeD23158D180E785A)
77

@@ -158,10 +158,10 @@ Full adversarial review, including six owner-only findings that were deliberatel
158158
forge build && forge test -vv
159159
```
160160

161-
74 tests across three kinds:
161+
75 tests across three kinds:
162162

163163
- **49 example tests** — happy-path lifecycle, subTier repricing, `AlreadyConfirmed` / `AlreadyOriginated` replay guards, escrow and token-level compliance gates, EIP-712 signed confirmation (valid / expired / wrong-signer), and the live identity adapter against a mock CVI that reproduces the real contract's revert-on-missing-pass behaviour.
164-
- **16 property tests** ([`test/Fuzz.t.sol`](test/Fuzz.t.sol)) — 256 randomised runs each. The rate curve is monotonic across all 256 subTiers and never prices above face; two different obligations never share a key; a whole lifecycle conserves value for any face value and any maturity; no unverified address anywhere in the address space can hold a note.
164+
- **17 property tests** ([`test/Fuzz.t.sol`](test/Fuzz.t.sol)) — 256 randomised runs each. The rate curve is monotonic across all 256 subTiers and never prices above face; two different obligations never share a key; a whole lifecycle conserves value for any face value and any maturity; no unverified address anywhere in the address space can hold a note.
165165
- **9 system invariants** ([`test/Invariant.t.sol`](test/Invariant.t.sol)) — 12,800 randomly ordered calls each, from nine wallets spread across every band, with identity revocation and time skips mixed in. The escrow's balance is zero at every step, value is conserved, and no note is ever priced off the published curve.
166166

167167
**Coverage: 97.5% of lines and 94.1% of branches across the five deployed contracts** (`forge coverage`). `ObligationRegistry` — the core mechanic — is at 100% on every metric.

dashboard/src/app/api/onboard/route.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,21 @@ const MINT_AMOUNT = 250_000_000_000n; // 250,000.000000 cvaUSD
2828
const COOLDOWN_MS = 60 * 60 * 1000;
2929
const lastOnboard = new Map<string, number>();
3030

31+
// The per-address cooldown alone is not a limit: cycling fresh addresses walks straight
32+
// past it, and every pass through mints 250,000 real cvaUSD. No contract is at risk —
33+
// every cash leg pulls only from msg.sender — but the settlement token can be inflated
34+
// without bound, which is worth stopping even on a testnet. So the window is global as
35+
// well as per-address.
36+
const GLOBAL_WINDOW_MS = 60 * 60 * 1000;
37+
const GLOBAL_MAX_PER_WINDOW = 25;
38+
const recentOnboards: number[] = [];
39+
40+
function globalBudgetLeft(): boolean {
41+
const cutoff = Date.now() - GLOBAL_WINDOW_MS;
42+
while (recentOnboards.length > 0 && recentOnboards[0] < cutoff) recentOnboards.shift();
43+
return recentOnboards.length < GLOBAL_MAX_PER_WINDOW;
44+
}
45+
3146
const monadTestnet = defineChain({
3247
id: CHAIN_ID,
3348
name: "Monad Testnet",
@@ -59,6 +74,13 @@ export async function POST(request: Request) {
5974
return bad(`This wallet was onboarded recently. Try again in ${minutes} minute${minutes === 1 ? "" : "s"}.`, 429);
6075
}
6176

77+
if (!globalBudgetLeft()) {
78+
return bad(
79+
`The desk has onboarded ${GLOBAL_MAX_PER_WINDOW} wallets in the past hour, which is its cap. Try again shortly, or switch to Demo mode — it needs no wallet at all.`,
80+
429,
81+
);
82+
}
83+
6284
const addresses = CHAIN_ADDRESSES[CHAIN_ID];
6385
const publicClient = createPublicClient({ chain: monadTestnet, transport: http() });
6486

@@ -99,7 +121,10 @@ export async function POST(request: Request) {
99121
const receipt = await publicClient.waitForTransactionReceipt({ hash: mintTx });
100122
if (receipt.status !== "success") return bad("The mint transaction was mined but failed. Try again.", 502);
101123

124+
// Counted only once the mint has actually landed, so a Cleanverse timeout or a
125+
// failed mint does not silently consume the hour's budget.
102126
lastOnboard.set(address.toLowerCase(), Date.now());
127+
recentOnboards.push(Date.now());
103128

104129
const apass = await queryApass(address);
105130
return Response.json({

dashboard/src/app/pitch/page.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ const CLEANVERSE_ADDRESSES = [
3333
const COMPETITORS = ["Centrifuge", "Huma Finance", "Polytrade", "Goldfinch", "Credix", "Defactor"];
3434

3535
const STATS = [
36-
{ figure: "74/74", label: "Foundry tests · fuzz + invariants · 97.5% lines, 94.1% branches" },
36+
{ figure: "75/75", label: "Foundry tests · fuzz + invariants · 97.5% lines, 94.1% branches" },
3737
{ figure: "0", label: "oracles between Cleanverse and the gate" },
3838
{ figure: "3", label: "independent compliance re-checks per cash leg" },
3939
];

docs/Covenant-pitch-deck.pdf

820 Bytes
Binary file not shown.

docs/STATIC-ANALYSIS.md

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,25 @@ The business state is written *before* the external call — `n.funded`,
6666
`fund` hits `AlreadyFunded` and a reentrant `settle` hits `AlreadySettled`. What lands
6767
after the call is ERC-721 ownership (`_owners`), which is why the detector fires.
6868

69-
The honest statement of the residual risk, which is worth more than "safe":
70-
71-
> During `fund`'s external call, the note is still owned by the supplier. If the
72-
> settlement asset could hand control back to a party able to call `settle`, that call
73-
> would pay face value to the supplier instead of the financier. It cannot, because
74-
> `settle` requires `msg.sender == n.obligor` and cvaUSD is a plain ERC-20 with no
75-
> transfer callback — verified on-chain, not assumed. **The safety of this ordering
76-
> depends on that property of the asset.** An ERC-777-style settlement asset would
77-
> require moving `_transfer` above the escrow call.
78-
79-
Named here so that swapping the settlement asset is known to be a change that has to
80-
come back to this paragraph.
69+
This section used to end with a caveat: that the ordering was safe only because cvaUSD is
70+
a plain ERC-20 with no transfer callback, and that an ERC-777-style asset would require
71+
moving `_transfer` above the escrow call.
72+
73+
**That was too cautious.** Working the reentrant cases through shows the trailing writes
74+
are not a weakness left over after the external call — they are the guard.
75+
76+
> Suppose the settlement asset *can* hand control back mid-transfer, and the obligor
77+
> re-enters `settle` during `fund`. `settle` proceeds — `funded` is already true — pays
78+
> face value to the current holder, and burns the note. Control returns to `fund`, which
79+
> then runs `_transfer(supplier, financier, tokenId)` on a token that no longer exists.
80+
> ERC-721 reverts, and the whole transaction unwinds, settlement included. The same holds
81+
> for a reentrant transfer: `_transfer` reverts because the supplier is no longer the
82+
> owner. Every reentrant path that touches note state makes the outer call fail closed,
83+
> including the multi-role case where `obligor == supplier`.
84+
85+
So the safety does not rest on a property of the asset. It rests on `fund` and `settle`
86+
each ending with a write that cannot succeed if their own state was disturbed. The
87+
detector is right that the pattern deserves a look; the look concludes it holds.
8188

8289
## Medium — `uninitialized-local` in `CleanverseIdentityRegistry._read`
8390

relayer/src/mapping.ts

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,30 +9,35 @@ export interface OnchainIdentity {
99
}
1010

1111
const ACTIVE_STATUS = 1;
12-
const MAX_UINT8 = 255;
13-
14-
const JURISDICTION_CODES: Record<string, number> = {
15-
US: 1,
16-
};
1712

13+
/// Cleanverse documents subTier as an integrator-set band in 0–99.
14+
const MAX_BAND = 99;
15+
16+
/// The same number `CleanverseIdentityRegistry.jurisdiction` derives on-chain:
17+
/// `uint16(bytes2(group))`, i.e. the first two ASCII bytes big-endian, so "US" is
18+
/// 0x5553 = 21843.
19+
///
20+
/// This used to be a lookup table that answered 1 for "US", so the same country had
21+
/// three different numbers across the production registry (21843), this mirror (1) and
22+
/// a unit test (1). The field is dormant — Cleanverse leaves `group` empty, so every
23+
/// read is 0 today and Covenant's own gate treats 0 as unconstrained — but a mirror
24+
/// that disagrees with what it mirrors is a fault waiting for the day the field is
25+
/// populated.
1826
export function mapJurisdiction(group: string | undefined): number {
1927
if (!group) return 0;
20-
const code = JURISDICTION_CODES[group.toUpperCase()];
21-
if (code === undefined) {
22-
console.warn(`covenant-relayer: unmapped Cleanverse jurisdiction group "${group}", defaulting to 0`);
23-
return 0;
24-
}
25-
return code;
28+
const bytes = Buffer.from(group.toUpperCase(), "ascii");
29+
return ((bytes[0] ?? 0) << 8) | (bytes[1] ?? 0);
2630
}
2731

2832
export function toOnchainIdentity(apass: ApassInfo | null, nowSeconds: number, ttlSeconds: number): OnchainIdentity {
2933
if (!apass) return { verified: false, tier: 0, subTier: 0, jurisdiction: 0, verifiedUntil: 0 };
3034

3135
// `tier` arrives as a string ("50") and is assigned by Cleanverse; `subTier` is the
32-
// integrator-set band Covenant actually prices on. Both are clamped rather than
33-
// truncated so an unexpected upstream value can never wrap into a higher band.
34-
const tier = clampByte(apass.tier);
35-
const subTier = clampByte(apass.subTier);
36+
// integrator-set band Covenant actually prices on. Anything outside 0–99 folds to 0
37+
// — the same rule `CleanverseIdentityRegistry._toUint8` applies on-chain, so the
38+
// mirror and the contract it mirrors refuse the same values.
39+
const tier = toBand(apass.tier);
40+
const subTier = toBand(apass.subTier);
3641

3742
const jurisdiction = mapJurisdiction(apass.group);
3843

@@ -46,8 +51,11 @@ export function toOnchainIdentity(apass: ApassInfo | null, nowSeconds: number, t
4651
return { verified, tier, subTier, jurisdiction, verifiedUntil };
4752
}
4853

49-
function clampByte(value: string | number | undefined): number {
54+
/// Folds rather than clamps: an out-of-range band must read as "unpriceable", not as the
55+
/// nearest priceable value. Clamping 150 to 99 would have quietly landed it in the top
56+
/// band; folding it to 0 blocks origination, which is what the contract does.
57+
function toBand(value: string | number | undefined): number {
5058
const n = value !== undefined ? Number(value) : NaN;
51-
if (!Number.isFinite(n)) return 0;
52-
return Math.min(Math.max(Math.trunc(n), 0), MAX_UINT8);
59+
if (!Number.isFinite(n) || n < 0 || n > MAX_BAND) return 0;
60+
return Math.trunc(n);
5361
}

src/IdentityRegistry.sol

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ contract IdentityRegistry is Ownable, IIdentityRegistry {
3838
);
3939
event RelayerChanged(address indexed relayer);
4040

41+
/// Cleanverse documents subTier as an integrator-set band in 0–99.
42+
uint8 internal constant MAX_SUBTIER = 99;
43+
4144
error NotRelayer();
4245
error ZeroRelayer();
4346

@@ -126,8 +129,18 @@ contract IdentityRegistry is Ownable, IIdentityRegistry {
126129
return _identity[subject].tier;
127130
}
128131

132+
/// Clamped exactly as `CleanverseIdentityRegistry._toUint8` clamps it, so the two
133+
/// implementations of `IIdentityRegistry` answer the same question the same way.
134+
///
135+
/// They did not agree before this: production folds anything above 99 to 0 and
136+
/// blocks pricing, while this mirror returned it verbatim — so an out-of-spec
137+
/// subTier of 150 was refused in production and priced at the top band here. Not
138+
/// exploitable, since only the trusted relayer writes, but two implementations of
139+
/// one interface disagreeing about a value is how a devnet test comes to prove
140+
/// something production does not do.
129141
function subTier(address subject) external view returns (uint8) {
130-
return _identity[subject].subTier;
142+
uint8 value = _identity[subject].subTier;
143+
return value > MAX_SUBTIER ? 0 : value;
131144
}
132145

133146
function jurisdiction(address subject) external view returns (uint16) {

test/Fuzz.t.sol

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@ contract FuzzTest is CovenantTestBase {
142142
/// buying a guaranteed loss, and the supplier would be borrowing at a negative rate.
143143
function testFuzz_advanceNeverExceedsFace(uint96 faceValue, uint8 subTier) public {
144144
faceValue = uint96(bound(faceValue, 1e6, 1_000_000e6));
145+
// 0–99 is the documented domain of subTier. Fuzzing it over the whole uint8 was
146+
// asking the registry to price a value Cleanverse cannot issue, which both
147+
// registries now answer with 0.
148+
subTier = uint8(bound(subTier, 0, 99));
145149
vm.assume(note.advanceRateBpsFor(subTier) > 0);
146150

147151
address obligor = makeAddr("fuzzObligor");
@@ -311,4 +315,21 @@ contract FuzzTest is CovenantTestBase {
311315
vm.expectRevert(IdentityRegistry.ZeroRelayer.selector);
312316
new IdentityRegistry(address(0));
313317
}
318+
319+
/// Both implementations of `IIdentityRegistry` must answer identically for a subTier
320+
/// outside Cleanverse's documented 0–99: fold it to 0, which prices at nothing and
321+
/// blocks origination. The devnet mirror used to return it verbatim while production
322+
/// clamped, so a value refused on Monad was priced at the top band in tests.
323+
function testFuzz_outOfRangeSubTierReadsAsZero(uint8 subTier) public {
324+
address subject = makeAddr("clampProbe");
325+
identityRegistry.adminSetIdentity(subject, true, 50, subTier, 1, uint64(block.timestamp + 365 days));
326+
327+
uint8 got = identityRegistry.subTier(subject);
328+
if (subTier > 99) {
329+
assertEq(got, 0, "out-of-range subTier must not be readable");
330+
assertEq(note.advanceRateBpsFor(got), 0, "an unreadable identity must not be priced");
331+
} else {
332+
assertEq(got, subTier, "in-range subTier must round-trip");
333+
}
334+
}
314335
}

0 commit comments

Comments
 (0)