Skip to content

Commit e893edf

Browse files
committed
feat(operation rules): add resetApproval and clearMintAllowances for stale-state cleanup on rebind
1 parent e79af46 commit e893edf

25 files changed

Lines changed: 586 additions & 57 deletions

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ Custom changelog tag: `Dependencies`, `Documentation`, `Testing`
4747

4848
## Unreleased
4949

50+
### Added
51+
52+
- `RuleConditionalTransferLight` / `RuleConditionalTransferLightMultiToken`: new `resetApproval(...)` operator function that discards **every** outstanding approval for a transfer key in one call (returns the cleared count, emits `TransferApprovalReset`). It deliberately does **not** require a bound token, so it can clean up approvals that survived an `unbindToken` — and, for the multi-token rule, approvals stranded under a key that can never be consumed.
53+
- `RuleMintAllowance`: new `clearMintAllowances(address[] calldata minters)` operator function that zeroes the listed minters' quotas (non-reverting batch), for discarding stale quotas before rebinding.
54+
5055
### Fixed
5156

5257
- `RuleMaxTotalSupply`: `detectTransferRestriction` / `canTransfer` / `detectTransferRestrictionFrom` no longer revert with an arithmetic panic when `currentSupply + value` would overflow `uint256`. The mint check now compares against the remaining headroom (`value > maxTotalSupply - currentSupply`), so these ERC-1404 / ERC-3643 views always return a restriction code as required. Enforcement (`transferred`) is unchanged.
@@ -63,6 +68,7 @@ Custom changelog tag: `Dependencies`, `Documentation`, `Testing`
6368
### Documentation
6469

6570
- `RuleConditionalTransferLightMultiToken`: document that the rule is **direct-binding-only** and **must not be added to a `RuleEngine`**. Approvals are recorded under the `token` argument but consumed under `msg.sender`, so behind an engine every wiring either reverts or silently loses per-token isolation. Added a "Deployment topology" section with the exhaustive case analysis to `doc/technical/RuleConditionalTransferLightMultiToken.md`, documented the caller-dependent `detectTransferRestriction`, and propagated the constraint to the README binding-model table, `RULE_SEMANTICS.md` and the project guide.
71+
- Add `doc/technical/INVARIANT_TESTS.md` — documents the stateful invariant suite: handler architecture and ghost variables, each of the four invariants and what it proves, the mutation-testing negative controls, the coverage map against the threat-model invariants, and how to add a new invariant. Linked from a new "Invariant testing" section in the README.
6672
- Add `doc/technical/RULE_SEMANTICS.md` — a per-rule comparison table (who each rule screens for `from` / `to` / spender on `transferFrom` / mint / burn, behaviour when the oracle/registry is unset, stateful?, and which pre-flight view is authoritative), with a highlights summary and link added to the README.
6773
- `RuleMintAllowance`: document that `canTransfer` / `detectTransferRestriction` are **not authoritative** (hardcoded to "allowed" because the 3-arg path has no minter identity) and that a mint pre-flight must use the spender-aware `canTransferFrom(minter, address(0), to, value)` / `detectTransferRestrictionFrom`. Added a bold callout and an eligibility-views table to `doc/technical/RuleMintAllowance.md` and a warning to the README rule section.
6874
- Add `THREAT_MODEL.md`, `RESULT.md` and `TEST_IMPROVEMENT.md` — manual security review of `src/` (0 High/Medium, 2 Low, 8 Info). Slither call-graph / inheritance / function-summary artifacts in `AUDIT/slither-graph/`.

doc/technical/INVARIANT_TESTS.md

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
# Invariant Tests
2+
3+
[TOC]
4+
5+
This document describes the **stateful invariant suite** in [`test/invariant/`](../../test/invariant/) — what each invariant asserts, why it matters, how the handlers are built, and how the suite was verified to actually catch bugs.
6+
7+
Invariant tests differ from the unit and fuzz tests elsewhere in `test/`: instead of exercising a fixed call sequence, Foundry drives a **handler** contract with long, randomly-ordered sequences of calls and re-checks every `invariant_*` function after each step. They are the right tool for the two **stateful (operation) rules**, whose storage evolves across calls.
8+
9+
Validation rules are read-only and hold no per-transfer state, so they have nothing to conserve across a call sequence — they are covered by unit and fuzz tests instead.
10+
11+
---
12+
13+
## 1. Running the suite
14+
15+
```bash
16+
forge test --match-path "test/invariant/*" # the invariant suite only
17+
forge test --match-contract ConditionalTransferInvariants
18+
forge test --match-contract MintAllowanceInvariants
19+
forge test # everything, invariants included
20+
```
21+
22+
Configuration lives in `foundry.toml`:
23+
24+
```toml
25+
[invariant]
26+
runs = 64 # independent random sequences
27+
depth = 128 # calls per sequence
28+
fail_on_revert = true # a reverting handler call fails the run
29+
```
30+
31+
`runs × depth`**8 192 handler calls per invariant**. `fail_on_revert = true` is deliberate: the handlers are written to only ever make calls the rule will accept, so **any** revert means the handler (or the rule) is wrong, and we want to know. The suite currently reports **0 reverts**.
32+
33+
---
34+
35+
## 2. Architecture — the handler pattern
36+
37+
Foundry cannot usefully fuzz a rule directly: `approveTransfer` needs `OPERATOR_ROLE`, `transferred` may only be called by the bound entity, and random inputs would mostly revert. So each rule gets a **handler** that:
38+
39+
1. **Holds the required roles and is itself the bound entity.** The handler is passed to `bindToken(address(handler))` and granted the operator role, so `msg.sender` inside the rule is the handler and every call is authorized.
40+
2. **Bounds the inputs.** A small actor set (3 addresses) and a small value range make the fuzzer *collide* on the same keys repeatedly, which is what actually exercises the accounting.
41+
3. **Skips calls that would revert** (e.g. cancelling a non-existent approval), so `fail_on_revert = true` stays meaningful.
42+
4. **Maintains ghost variables** — an independent, off-chain-style mirror of what the rule's state *should* be. The invariant then compares the rule against the ghost.
43+
44+
```
45+
┌──────────────────────┐ randomly-ordered calls ┌──────────────────┐
46+
│ Foundry invariant │ ──────────────────────────▶ │ Handler │
47+
│ fuzzer │ │ (bound entity, │
48+
└──────────────────────┘ │ role holder) │
49+
│ └────────┬─────────┘
50+
│ after every call real call │ ghost update
51+
▼ ▼
52+
┌──────────────────────┐ ┌────────┐ ┌────────────┐
53+
│ invariant_*() │ compares ─────────────▶│ Rule │ │ ghosts │
54+
└──────────────────────┘ └────────┘ └────────────┘
55+
```
56+
57+
`targetSelector` restricts the fuzzer to the handler's own action functions. Without it, Foundry would also call the public functions the handler inherits from forge-std's `Test`, wasting the call budget.
58+
59+
| File | Role |
60+
|---|---|
61+
| [`test/invariant/ConditionalTransferHandler.sol`](../../test/invariant/ConditionalTransferHandler.sol) | Drives `RuleConditionalTransferLight`'s approval state machine |
62+
| [`test/invariant/MintAllowanceHandler.sol`](../../test/invariant/MintAllowanceHandler.sol) | Drives `RuleMintAllowance`'s quota accounting |
63+
| [`test/invariant/RuleInvariants.t.sol`](../../test/invariant/RuleInvariants.t.sol) | The two invariant test contracts and their `setUp` |
64+
65+
---
66+
67+
## 3. The invariants
68+
69+
### 3.1 `RuleConditionalTransferLight` — approval conservation
70+
71+
Handler actions: `approve` · `cancel` · `execute` · `executeMintOrBurn`.
72+
Ghosts: `totalApproved`, `totalCancelled`, `totalExecuted`, `mintBurnCalls`.
73+
74+
#### `invariant_approvalConservation`
75+
76+
```
77+
totalApproved − totalCancelled − totalExecuted == Σ approvalCounts
78+
```
79+
80+
Every approval ever recorded is, at any moment, in exactly one of three states: **still outstanding**, **cancelled**, or **consumed by a transfer**. The equality says approvals are neither **double-spent** (one `approveTransfer` consumed twice) nor **lost** (an approval that vanishes without being cancelled or used).
81+
82+
The invariant additionally asserts `totalApproved >= totalCancelled + totalExecuted`, which fails loudly if the rule ever lets more approvals be consumed than were recorded — i.e. an underflow of `approvalCounts` (`INV-5`).
83+
84+
#### `invariant_noApprovalExceedsTotalRecorded`
85+
86+
```
87+
Σ approvalCounts ≤ totalApproved
88+
```
89+
90+
A weaker but independent bound: no tuple can ever hold more outstanding approvals than were granted in total. It catches a class of bug (spurious increments) that conservation alone could mask if a matching spurious decrement existed.
91+
92+
#### Emergent property — mint/burn never consume an approval
93+
94+
The handler calls `executeMintOrBurn`, firing `transferred(address(0), to, v)` and `transferred(from, address(0), v)`, but **deliberately does not** count these in `totalExecuted`. If a mint or burn ever consumed an approval, `Σ approvalCounts` would drop while `totalExecuted` stayed put, and `invariant_approvalConservation` would break.
95+
96+
So the mint/burn exemption is proved by the conservation invariant itself — no separate test needed.
97+
98+
### 3.2 `RuleMintAllowance` — exact quota accounting
99+
100+
Handler actions: `setAllowance` · `increase` · `decrease` · `mint` · `regularTransfer`.
101+
Ghosts: `ghostAllowance[minter]` (a full mirror), `totalCredited`, `totalMinted`.
102+
103+
#### `invariant_allowanceMatchesGhost`
104+
105+
```
106+
for every minter m: rule.mintAllowance(m) == ghostAllowance[m]
107+
```
108+
109+
The strongest of the four. The handler recomputes the expected allowance independently after every accepted operation, so this asserts the rule's arithmetic is **exactly** right after *any* interleaving of set / increase / decrease / mint. It subsumes "never underflows" and "monotonically non-increasing across mints" (`INV-7`).
110+
111+
#### `invariant_mintedNeverExceedsCredited`
112+
113+
```
114+
Σ minted ≤ Σ credited
115+
```
116+
117+
A cumulative safety bound independent of the mirror: across the whole run, minters can never mint more in total than was ever granted to them, regardless of how quotas were reset or adjusted along the way.
118+
119+
#### Emergent property — non-mint transfers never touch a quota
120+
121+
The handler calls `regularTransfer` (a `transferred(spender, from ≠ 0, to, v)` call) and **deliberately leaves the ghost unchanged**. If the rule ever deducted quota on a non-mint path, the mirror would diverge and `invariant_allowanceMatchesGhost` would fail.
122+
123+
---
124+
125+
## 4. Negative controls — proving the suite can fail
126+
127+
An invariant suite that cannot fail is worthless. Both invariants were **mutation-tested**: a real bug was injected into the rule, the suite was run, and the failure was confirmed. The mutations were then reverted.
128+
129+
| Mutation | Injected bug | Result |
130+
|---|---|---|
131+
| `RuleConditionalTransferLightApprovalBase._transferred` — remove `approvalCounts[hash] = count - 1` | Approval **double-spend**: one approval can be consumed forever |`invariant_approvalConservation` fails: `approval accounting drifted: 0 != 1` |
132+
| `RuleMintAllowanceBase._transferredFrom``current - value``current - value + 1` | **Off-by-one** quota deduction: minters slowly gain free quota |`invariant_allowanceMatchesGhost` fails: `mint allowance drifted from expected: 1938542 != 1938541` |
133+
134+
Re-run these yourself before trusting a change to either rule: if you mutate the accounting and the suite still passes, the suite has regressed.
135+
136+
---
137+
138+
## 5. Coverage map
139+
140+
Invariant IDs refer to [`THREAT_MODEL.md`](../../THREAT_MODEL.md) §8; verification status is tracked in [`RESULT.md`](../../RESULT.md).
141+
142+
| Invariant (threat model) | Property | Covered by |
143+
|---|---|---|
144+
| `INV-5` | `approvalCounts` never underflows; one approval ⇒ one transfer | `invariant_approvalConservation`, `invariant_noApprovalExceedsTotalRecorded` |
145+
| `INV-7` | `mintAllowance` non-increasing across mints, never underflows, `Σ minted ≤ Σ granted` | `invariant_allowanceMatchesGhost`, `invariant_mintedNeverExceedsCredited` |
146+
| `INV-12` (partial) | Mint/burn handled explicitly, never falling into the transfer path | Emergent from both suites (see §3.1, §3.2) |
147+
148+
**Not covered by invariants (by design):**
149+
150+
- `INV-1`, `INV-2`, `INV-3`, `INV-9` — properties of *stateless* view functions; unit and fuzz tests are the right tool (`test/ThreatModel/ThreatModelTests.t.sol`).
151+
- `INV-6` (`_transferHash` injectivity) — a pure function; covered by `testFuzz_HASH1_ApprovalBucketsAreDistinct`.
152+
- `INV-4`, `INV-11` (access control) — covered by the per-rule access-control suites.
153+
- `INV-10` (ERC-2771 binding identity) — currently holds by static reasoning; a live regression test is the open item **I-10a** in [`RULE_IMPROVEMENT.md`](../../RULE_IMPROVEMENT.md).
154+
155+
---
156+
157+
## 6. Adding a new invariant
158+
159+
1. Add the action to the relevant handler. **Guard it** so it can only make calls the rule accepts (`fail_on_revert = true` will otherwise fail the run).
160+
2. Update the ghost state *after* the rule call, in the same function. If the rule call reverts, the whole handler call reverts and the ghost rolls back with it — which is what keeps the mirror consistent.
161+
3. Register the new selector in the `targetSelector(...)` array in `RuleInvariants.t.sol`, otherwise it will never be fuzzed.
162+
4. Add the `invariant_*` function, with a message argument on each assertion so a failure is legible.
163+
5. **Mutation-test it.** Inject the bug it is supposed to catch and confirm it fails.
164+
165+
If you add a new stateful rule, it needs its own handler; validation rules do not.

doc/technical/RuleConditionalTransferLight.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,18 @@ Increments the approval count for the `(from, to, value)` hash by 1. Restricted
5353

5454
Decrements the approval count for the `(from, to, value)` hash by 1. Reverts if no approval exists. Restricted to `OPERATOR_ROLE`. Emits `TransferApprovalCancelled`.
5555

56+
### `resetApproval(address from, address to, uint256 value) → uint256`
57+
58+
Discards **every** outstanding approval for the `(from, to, value)` hash in one call and returns the count that was cleared. Reverts if no approval exists. Restricted to `OPERATOR_ROLE`. Emits `TransferApprovalReset`.
59+
60+
Unlike `approveTransfer`, this deliberately does **not** require a token to be bound — its main use is discarding approvals that survived an `unbindToken` (see below), at which point nothing is bound.
61+
5662
### `approveAndTransferIfAllowed(address from, address to, uint256 value) → bool`
5763

5864
Approves the transfer and immediately calls `SafeERC20.safeTransferFrom` on the currently bound token, using this rule contract as the spender. Requires `from` to have previously approved this contract for at least `value` tokens. Restricted to `OPERATOR_ROLE`.
5965

66+
> ⚠️ Requires the bound entity to be the ERC-20 token itself (direct-binding mode). Under a `RuleEngine` the bound entity is the engine, so this call reverts.
67+
6068
### `approvedCount(address from, address to, uint256 value) → uint256`
6169

6270
Returns the current approval count for the `(from, to, value)` tuple.
@@ -65,6 +73,8 @@ Returns the current approval count for the `(from, to, value)` tuple.
6573

6674
Binds or unbinds a token contract. Only bound tokens are authorised to call `transferred`. Restricted to `COMPLIANCE_MANAGER_ROLE`.
6775

76+
> ⚠️ **`unbindToken` does not clear `approvalCounts`.** Approvals recorded while the previous token was bound remain in storage and become consumable by the next token that is bound. The operator who controls rebinding also controls approvals, so the trust model is preserved — but when migrating to a different token, call `resetApproval` for each affected `(from, to, value)` **before** rebinding if the old approvals must not carry over.
77+
6878
## Workflow
6979

7080
### Token holder initiates

doc/technical/RuleConditionalTransferLightMultiToken.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,12 @@ Approves one transfer for a specific token key.
9595

9696
Removes one approval for a specific token key. Reverts if none exists.
9797

98+
### `resetApproval(address token, address from, address to, uint256 value) -> uint256`
99+
100+
Discards **every** outstanding approval for the `(token, from, to, value)` key in one call and returns the count that was cleared. Reverts if none exists. Restricted to `OPERATOR_ROLE`. Emits `TransferApprovalReset`.
101+
102+
Unlike `approveTransfer`, this deliberately does **not** require the token to be bound. That is what makes it usable for the two cleanup cases: approvals left behind by an `unbindToken`, and approvals stranded under a key that can never be consumed (see finding **F-4** and the [Deployment topology](#deployment-topology--why-a-ruleengine-does-not-work) section).
103+
98104
### `approvedCount(address token, address from, address to, uint256 value) -> uint256`
99105

100106
Returns the remaining count for a specific token key.
@@ -116,4 +122,4 @@ Only bound tokens can call transfer execution hooks. Approval consumption uses t
116122
- Mints and burns are exempt from approval consumption (`from == address(0)` or `to == address(0)`).
117123
- This rule is ERC-20 operation-focused, like `RuleConditionalTransferLight`.
118124
- **Do not deploy this rule behind a `RuleEngine`.** Approvals are consumed under `msg.sender`, so token scoping is lost (or the rule breaks outright) in that topology — the full case analysis is in [Deployment topology](#deployment-topology--why-a-ruleengine-does-not-work). If you need a conditional-transfer rule for a single token behind a RuleEngine, use [`RuleConditionalTransferLight`](./RuleConditionalTransferLight.md) instead.
119-
- `unbindToken` does **not** clear `approvalCounts`. Approvals recorded for a previously bound token remain in storage and become consumable again if that token is rebound.
125+
- `unbindToken` does **not** clear `approvalCounts`. Approvals recorded for a previously bound token remain in storage and become consumable again if that token is rebound. Use `resetApproval(token, from, to, value)` to discard them before rebinding — it works on an unbound token precisely for this reason.

doc/technical/RuleMintAllowance.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,12 @@ Adds `amount` to `minter`'s current allowance. Restricted to `ALLOWANCE_OPERATOR
6464

6565
Subtracts `amount` from `minter`'s current allowance. Reverts with `RuleMintAllowance_DecreaseBelowZero` if `amount` exceeds the current allowance. Restricted to `ALLOWANCE_OPERATOR_ROLE`. Emits `MintAllowanceDecreased`.
6666

67+
### `clearMintAllowances(address[] calldata minters)`
68+
69+
Sets the allowance of every listed minter to zero. Batch operation: does not revert on minters that already have a zero allowance, on duplicates, or on an empty array. Restricted to `ALLOWANCE_OPERATOR_ROLE`. Emits `MintAllowanceSet(minter, 0)` per entry.
70+
71+
Intended for migration — see the `bindToken` warning below.
72+
6773
### `mintAllowance(address minter) → uint256`
6874

6975
Returns the remaining mint allowance for `minter`. Default is `0`.
@@ -72,6 +78,8 @@ Returns the remaining mint allowance for `minter`. Default is `0`.
7278

7379
Binds or unbinds the caller address. Only the bound address is authorised to call `transferred`. In practice, bind the RuleEngine address. Restricted to `COMPLIANCE_MANAGER_ROLE`. A second `bindToken` call reverts until the current binding is removed.
7480

81+
> ⚠️ **`unbindToken` does not clear `mintAllowance`.** Quotas granted while the previous RuleEngine/token was bound remain in storage and are spendable by the same minters as soon as a new caller is bound. The operator who controls rebinding also controls allowances, so the trust model is preserved — but when migrating, call `clearMintAllowances` **before** rebinding if the old quotas must not carry over.
82+
7583
## Workflow
7684

7785
1. Deploy `RuleMintAllowance` (or `RuleMintAllowanceOwnable2Step`).

0 commit comments

Comments
 (0)