Skip to content

Commit 5b38e0b

Browse files
authored
post-audit cleanup: renames + max-nonce sentinel + revoke refactor (#1)
Renames: - Layout -> StorageLayout (struct + all references) - _isWhitelisted -> isWhitelisted (storage field) - setValidatorNonce -> setValidatorNonceFloor (ABI break, contract not yet deployed; matches the floor semantics) - ValidatorNonceSet event param newNonce -> newFloor (topic hash unchanged) Behavior: - QuorumSigLib rejects nonce == type(uint256).max as the retired-validator sentinel. Without this, _removeValidator's floor saturation still left one usable nonce (the floor itself, since the floor check is strict-<). Closes the audit-flagged hole in the "all sigs become unusable after removal" claim. - _removeValidator now returns bool (false on no-op when address isn't a validator) instead of reverting NotValidator. The strict removeValidator / removeValidatorBySig wrap and revert NotValidator themselves; revokeAllRoles ignores the return so the validator side gracefully degrades to a no-op while compliance strip still runs. revokeAllRoles becomes a thin wrapper over _removeValidator + _setComplianceOfficer. Documentation: - initialize: quorum range [1, len) (strict-less-than). - QuorumOutOfRange: "0 or >= validator count". - removeValidator: rewrite to match floor-saturation reality. - pauseFor: "sets to now + secs", not "extends by secs". - _setQuorum + README A1: drop overstated iterative-shrink prevention claim; the strict invariant is one-step anti-capture only. - README C3 + contract header: only setValidatorNonceFloor, post-renounce *BySig paths, and revokeAllRoles remain reachable while paused. Owner paths (addValidator/removeValidator/setQuorum/setComplianceOfficer) are whenNotPaused. - revokeAllRoles: document the pause-asymmetry as intentional recovery hook; document quorum-boundary partial-failure as caller-side precondition. - Various other doc references updated for the new names. Tests: - test/RequestWhitelist.t.sol: setValidatorNonce -> setValidatorNonceFloor. - Invariants: bump selector array to 12, add h_revokeAllRoles (the handler had it but it wasn't fuzzed); rename h_setValidatorNonce. forge build, forge fmt --check, and forge test (79/79) all pass.
1 parent 7b1981f commit 5b38e0b

8 files changed

Lines changed: 233 additions & 137 deletions

‎README.md‎

Lines changed: 41 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ flowchart LR
1818
V["off-chain validators<br/>sign EIP-712 WhitelistRequest"]
1919
S(["Owner (phase 1)<br/>/ anyone (phase 2)"])
2020
C(["Compliance officer"])
21-
WL["RequestWhitelist<br/>_isWhitelisted[a]<br/>pausedUntil"]
21+
WL["RequestWhitelist<br/>isWhitelisted[a]<br/>pausedUntil"]
2222
D(["downstream protocols<br/>/ dApps"])
2323
2424
V -- "quorum of sigs" --> S
@@ -42,9 +42,15 @@ run points straight at the clause that broke.
4242
**A. Validator set & quorum structure**
4343

4444
- **A1.** `1 <= quorum < validators.length <= MAX_VALIDATORS` (256). The
45-
strict inequality between `quorum` and `validators.length` guarantees
46-
that no quorum-sized coalition can brick or fully capture the registry
47-
by shrinking the set down to its own membership.
45+
strict inequality between `quorum` and `validators.length` is a
46+
one-step anti-capture rail: a quorum-sized coalition cannot in a
47+
single transaction remove every dissenting validator, because each
48+
removal still has to leave `length > quorum`. Iterative capture is
49+
*not* prevented — a coalition that controls quorum can lower
50+
`quorum` first and then remove non-coalition members one by one;
51+
the contract-level rule only buys time and emits on-chain
52+
observability (`ValidatorRemoved`, `QuorumSet`). Defense against
53+
coordinated capture is governance-level.
4854
- **A2.** The validator set never contains duplicates and never contains
4955
`address(0)`. `_addValidator` rejects both; the set itself is a
5056
solady `EnumerableSetLib.AddressSet` so duplicates are structurally
@@ -56,14 +62,15 @@ run points straight at the clause that broke.
5662
**B. Nonce discipline**
5763

5864
- **B1.** `validatorNonceFloor[v]` is monotonically non-decreasing.
59-
`setValidatorNonce` is strict-increase; `_removeValidator` and
65+
`setValidatorNonceFloor` is strict-increase; `_removeValidator` and
6066
`revokeAllRoles` saturate to `type(uint256).max`; no other writer
6167
exists.
6268
- **B2.** Once a validator has ever been removed, their nonce floor is
6369
`type(uint256).max` forever — even if the same address is later
64-
re-added. Combined with the bitmap's permanent-set property this
65-
closes the re-add replay window: no signature the retired address
66-
ever produced can pass `NonceBelowFloor` again.
70+
re-added. Combined with the bitmap's permanent-set property and
71+
`QuorumSigLib`'s explicit rejection of `nonce == type(uint256).max`,
72+
this closes the re-add replay window: no signature the retired
73+
address ever produced can pass.
6774
- **B3.** Each validator signature carries a signer-chosen nonce. The
6875
nonce is recorded in a per-validator bitmap on successful verify and
6976
permanently single-use — a bit set to `1` never returns to `0`. Sigs
@@ -79,19 +86,26 @@ run points straight at the clause that broke.
7986
- **C2.** `isPaused()` is a pure function of `pausedUntil` and
8087
`block.timestamp` — no cached pause state anywhere.
8188
- **C3.** While paused, `whitelist` and `unwhitelist` revert with
82-
`ContractPaused`. Compliance `pauseFor` / `unpause` and every
83-
rotation path (`setValidatorNonce`, all `*BySig` admin functions)
84-
remain reachable — the circuit breaker exists to buy the quorum time
85-
to rotate, not to freeze rotation itself.
89+
`ContractPaused`. The Phase-1 owner admin paths (`addValidator`,
90+
`removeValidator`, `setQuorum`, `setComplianceOfficer`) are also
91+
gated by `whenNotPaused`. The recovery surface that *does* stay
92+
reachable during pause is: compliance `pauseFor` / `unpause`, the
93+
validator-self lever `setValidatorNonceFloor`, the post-renounce
94+
`*BySig` admin twins, and the owner-only `revokeAllRoles` recovery
95+
hook (intentionally pause-callable as the only owner-side path
96+
available when the rogue actor is the compliance officer who
97+
triggered the pause). The breaker buys the quorum time to rotate
98+
via the `*BySig` paths or wait the pause out and use Phase-1
99+
rotations; it does not freeze rotation itself.
86100

87101
**D. `isWhitelisted` semantics**
88102

89103
- **D1.** While paused, `isWhitelisted(a)` never returns
90104
`Whitelisted` or `NotWhitelisted` — the return is always one of the
91105
`Paused*` variants, regardless of the underlying attestation bit.
92106
- **D2.** While live (not paused), `isWhitelisted(a)` returns exactly
93-
one of `Whitelisted` / `NotWhitelisted`, matching the `_isWhitelisted`
94-
bit.
107+
one of `Whitelisted` / `NotWhitelisted`, matching the `isWhitelisted`
108+
storage bit.
95109

96110
**E. Ownership irreversibility**
97111

@@ -164,13 +178,16 @@ Two pieces of per-validator state guard replay:
164178
relayer race.
165179
- `validatorNonceFloor[v]` — a single scalar below which every sig is
166180
rejected `NonceBelowFloor`. This is the **mass-invalidate lever**.
167-
The validator raises it themselves via `setValidatorNonce(newFloor)`
168-
(one SSTORE, no per-sig bookkeeping). The floor is strict-increase:
169-
it cannot be lowered once raised. Removal of a validator saturates
170-
the floor to `type(uint256).max`, which permanently retires that
171-
address from ever signing again — combined with the strict-increase
172-
rule, a removed validator cannot be re-admitted at the same address
173-
with a lower floor, so any pre-signed sig at any reasonable nonce is
181+
The validator raises it themselves via
182+
`setValidatorNonceFloor(newFloor)` (one SSTORE, no per-sig
183+
bookkeeping). The floor is strict-increase: it cannot be lowered
184+
once raised. Removal of a validator saturates the floor to
185+
`type(uint256).max`, which permanently retires that address from
186+
ever signing again — `QuorumSigLib` separately rejects
187+
`nonce == type(uint256).max` as the retired-validator sentinel, so
188+
the saturated floor leaves *no* usable nonce. Combined with the
189+
strict-increase rule, a removed validator cannot be re-admitted at
190+
the same address with a lower floor, so any pre-signed sig is
174191
unusable after removal (closes the re-add replay window).
175192

176193
Validators are free to allocate nonces sequentially (simplest),
@@ -179,8 +196,8 @@ consumed" rule is enforced. The suggested default for operators is
179196
sequential from 0, since that minimises bitmap storage cost: only the
180197
words actually touched by consumed nonces are written to.
181198

182-
`setValidatorNonce(newFloor)` remains reachable while the registry is
183-
paused. Pausing *is* the signal that something is wrong, so the
199+
`setValidatorNonceFloor(newFloor)` remains reachable while the registry
200+
is paused. Pausing *is* the signal that something is wrong, so the
184201
mass-invalidate lever must be usable under pause.
185202

186203
#### Trust model for ERC-1271 signers
@@ -232,7 +249,7 @@ All mutable state lives in a single ERC-7201 namespaced struct at
232249
which cannot collide.
233250

234251
**Storage upgrade rule (append-only)**: future versions must never
235-
reorder, resize, or remove fields in `Layout`. Slot 0's packing of
252+
reorder, resize, or remove fields in `StorageLayout`. Slot 0's packing of
236253
`uint16 quorum` + `uint48 pausedUntil` leaves 26 bytes for forward-
237254
compatible flags and must not be rearranged. Dynamic members (the
238255
validator set and the mappings) each anchor their own base slot, so

0 commit comments

Comments
 (0)