From e81787b07aebf5ac09ceb64860148da0edfd2b02 Mon Sep 17 00:00:00 2001 From: shivam kalra Date: Mon, 17 Aug 2026 17:15:57 +0530 Subject: [PATCH 1/4] feat: add Emergency Module Recovery and granular withdrawal circuit breakers Implements the developer response to the architecture review (0bab749): narrow, immutable module recovery in place of generic post-seal upgradeability, plus the P0 hardening items it depends on. - Split the coarse FLAG_PAUSED_WITHDRAWALS into 5 dedicated breakers (instant settlement, queued-request, epoch close/fund, funded claims, force exit). Fixes two real bugs this uncovered: force exit was wrongly gated by the generic pause flags, while EpochedQueueModule had zero pause protection at all. - Unify SystemSealer's canSeal()/verifyAndSeal() into a single _verifyLiveState() verifier and add the missing chainId binding; closes a gap where canSeal() could return true for a config verifyAndSeal() would still reject. - Add RecoveryGate (src/governance/RecoveryGate.sol) and CoreVault.recoverModuleGroup(): propose/approve/veto/execute lifecycle over four SelectorLib-derived module groups, 14-day minimum delay enforced in the constructor, role relaxation structurally impossible (no role parameter on recoverModuleGroup). - Wire recoveryGate/recoveryManifestVersion into SystemSealer's seal manifest. - Correct stale pause/access-control documentation and document the new mechanisms (architecture.md, governance.md, access-control.md, modules.md, force-exit.md, new recovery.md). - Add Withdrawal_PauseMatrix_Invariants.t.sol, Recovery_Invariants.t.sol, SystemSealer_CanSealAgreement.t.sol, and test/incident-sim/ (shell- defect unreachability, end-to-end queue incident, governance/guardian compromise blast-radius scenarios). --- docs/access-control.md | 21 +- docs/architecture.md | 42 +- docs/force-exit.md | 8 +- docs/governance.md | 64 ++- docs/modules.md | 23 +- docs/recovery.md | 174 +++++++ src/core/CoreVault.sol | 197 ++++++- src/core/SystemSealer.sol | 385 +++++++------- src/core/libraries/Events.sol | 16 + src/core/modules/ERC4626Module.sol | 20 +- src/core/modules/EpochedQueueModule.sol | 79 ++- src/core/storage/CoreStorage.sol | 13 + src/governance/RecoveryGate.sol | 415 +++++++++++++++ .../CoreVaultShellDefect_Unreachable.t.sol | 212 ++++++++ .../GovernanceCompromise_BlastRadius.t.sol | 289 ++++++++++ .../QueueModuleIncident_EndToEnd.t.sol | 194 +++++++ test/integration/DeploymentEquivalence.t.sol | 6 + test/invariants/Recovery_Invariants.t.sol | 493 ++++++++++++++++++ .../Withdrawal_PauseMatrix_Invariants.t.sol | 411 +++++++++++++++ .../SystemSealer_CanSealAgreement.t.sol | 342 ++++++++++++ .../SystemSealer_DecimalsGuard.t.sol | 3 + .../SystemSealer_TimestampHash_POC.t.sol | 8 +- 22 files changed, 3138 insertions(+), 277 deletions(-) create mode 100644 docs/recovery.md create mode 100644 src/governance/RecoveryGate.sol create mode 100644 test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol create mode 100644 test/incident-sim/GovernanceCompromise_BlastRadius.t.sol create mode 100644 test/incident-sim/QueueModuleIncident_EndToEnd.t.sol create mode 100644 test/invariants/Recovery_Invariants.t.sol create mode 100644 test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol create mode 100644 test/sprint-test/SystemSealer_CanSealAgreement.t.sol diff --git a/docs/access-control.md b/docs/access-control.md index 93a69b0..854bc51 100644 --- a/docs/access-control.md +++ b/docs/access-control.md @@ -168,14 +168,9 @@ No mechanism exists to "lock" a principal address permanently (except `sealBySea | Function | Module | Description | |----------|--------|-------------| -| `pause` | AdminModule | Set FLAG_PAUSED | -| `unpause` | AdminModule | Clear FLAG_PAUSED | -| `pauseDeposits` | AdminModule | Set FLAG_PAUSED_DEPOSITS | -| `unpauseDeposits` | AdminModule | Clear FLAG_PAUSED_DEPOSITS | -| `pauseWithdrawals` | AdminModule | Set FLAG_PAUSED_WITHDRAWALS | | `deployToStrategiesWithPlan` | LiquidityOpsModule | Deploy with a caller-supplied allocation plan. Was `ROLE_PUBLIC`; moved to OWNER_OR_GUARDIAN because a public caller could otherwise steer which registered strategies receive capital and in what proportion. | -> Note: `unpauseWithdrawals` is OWNER-only (roleOf = 1). The guardian can pause withdrawals but not unpause them. +> **Correction**: pause functions are NOT `moduleOf`/`roleOf`-routed and do not belong in this table — they are direct functions on `CoreVault` itself (like `setModule`/`freezeRouting`), gated by CoreVault's own `onlyOwner`/`onlyGuardian`/`onlyOwnerOrGuardian` modifiers, not the fallback dispatcher's role system. The full, current pause/breaker table (`pauseAll`, `pauseDepositsOnly`, `pauseWithdrawalsOnly`, `pauseInstantWithdrawalOnly`, `pauseEpochCloseFundOnly`, `pauseQueuedRequestOnly`, `pauseFundedClaimOnly`, `pauseForceExitOnly`, `guardianPause`, `unpauseAll`) with per-function access levels is authoritative in [architecture.md §11.3](architecture.md#113-pause-granularity) and [governance.md §4.2](governance.md#42-pause-functions) — not duplicated here to avoid a third copy drifting out of sync. ### 4.4 PUBLIC Functions (roleOf = 0) @@ -184,13 +179,13 @@ Key permissionless functions: | Function | Module | Notes | |----------|--------|-------| | `deposit` | ERC4626Module | Subject to pause checks | -| `requestEpochWithdrawal` | EpochedQueueModule | Subject to pause + `minClaimAmount` floor | -| `requestInstantWithdrawal` | EpochedQueueModule | Same floor; falls back to the queue when the cap or free liquidity blocks it | -| `cancelEpochWithdrawal` | EpochedQueueModule | Claim owner only, and only while the epoch is `Open` | -| `closeCurrentEpoch` | EpochedQueueModule | Callable by anyone once the epoch duration has elapsed (keeper pattern) | -| `fundEpoch` | EpochedQueueModule | Callable by anyone; no-op when the epoch is already funded | -| `claimEpochAssets` / `batchClaimEpochAssets` | EpochedQueueModule | Claim owner only, self-service, no keeper required | -| `syncOldestUnfundedEpoch` | EpochedQueueModule | Cursor maintenance, always callable | +| `requestEpochWithdrawal` | EpochedQueueModule | Subject to `FLAG_QUEUED_REQUEST_PAUSED` (new-request breaker, owner-only) + `minClaimAmount` floor | +| `requestInstantWithdrawal` | EpochedQueueModule | Same floor; falls back to the queue (subject to the same `FLAG_QUEUED_REQUEST_PAUSED` breaker) when the cap, free liquidity, or `FLAG_INSTANT_WITHDRAWAL_PAUSED` blocks instant settlement — never reverts outright for a paused instant breaker alone | +| `cancelEpochWithdrawal` | EpochedQueueModule | Claim owner only, and only while the epoch is `Open` — never pause-gated, by design (review §20) | +| `closeCurrentEpoch` | EpochedQueueModule | Callable by anyone once the epoch duration has elapsed (keeper pattern); subject to `FLAG_EPOCH_CLOSE_FUND_PAUSED` | +| `fundEpoch` | EpochedQueueModule | Callable by anyone; no-op when the epoch is already funded; subject to `FLAG_EPOCH_CLOSE_FUND_PAUSED` | +| `claimEpochAssets` / `batchClaimEpochAssets` | EpochedQueueModule | Claim owner only, self-service, no keeper required; subject to `FLAG_FUNDED_CLAIM_PAUSED` (owner-only breaker, never Guardian — review §20) | +| `syncOldestUnfundedEpoch` | EpochedQueueModule | Cursor maintenance; subject to `FLAG_EPOCH_CLOSE_FUND_PAUSED` | | `acceptOwnership` | AdminModule | Must be `pendingOwner` (checked internally) | | `markMatured` | FixedMaturityModule | Any address, once maturityTs reached | | `markFundingFailed` | FixedMaturityModule | Any address, once deadline + net < min | diff --git a/docs/architecture.md b/docs/architecture.md index 3a7ff3e..51adfd2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -243,8 +243,15 @@ Each library exposes a `layout()` function that returns a storage pointer at the | 10 | `FLAG_DEAD_DEPOSIT_DONE` | Dead deposit seeded (inflation attack hardening) | | 11 | `FLAG_FEES_INITIALIZED` | Fees initialized via `setInitialFees()` | | 12 | `FLAG_PERF_INITIALIZED` | Performance fee initialized via `setInitialPerfParams()` | +| 13 | `FLAG_INSTANT_WITHDRAWAL_PAUSED` | Instant settlement only paused (queued exits unaffected) | +| 14 | `FLAG_QUEUED_REQUEST_PAUSED` | New queued-exit requests only paused (cancelling existing ones unaffected) | +| 15 | `FLAG_EPOCH_CLOSE_FUND_PAUSED` | Epoch close/fund/crystallize only paused | +| 16 | `FLAG_FUNDED_CLAIM_PAUSED` | Funded-claim settlement only paused | +| 17 | `FLAG_FORCE_EXIT_PAUSED` | Force exit only paused — the only flag that can gate it; see §4.3 | -Source: `src/core/storage/CoreStorage.sol:24-36`. +Source: `src/core/storage/CoreStorage.sol:24-41`. + +Bits 13-17 (review §20/§21, "Recommended Withdrawal Circuit Breakers" / "Required Pause Matrix") replace the previous all-or-nothing behavior of `FLAG_PAUSED_WITHDRAWALS` on `EpochedQueueModule` — before their introduction, that flag was not read anywhere in `EpochedQueueModule.sol` at all, so `guardianPause()`/`pauseWithdrawalsOnly()` had zero effect on the queue. See §11.3. --- @@ -635,22 +642,34 @@ Source: `src/core/CoreVault.sol:394-407`. ### 11.2 Guardian -The guardian is a separate address with limited pause capability. The guardian can pause the vault via `guardianPause()`, subject to a cooldown enforced by `IParamsProvider.guardianPauseCooldown()`. Default cooldown: 7 days (`src/core/CoreVault.sol:459-468`). +The guardian is a separate address with limited pause capability, matching review §3.3 ("fast to restrict, slow to restore"): guardians cannot unpause, modify routing, or reach the exceptional owner-only breakers (§11.3) — these require owner. + +`guardianPause()` is the guardian's single rapid action, subject to a cooldown enforced by `IParamsProvider.guardianPauseCooldown()` (default 7 days). It sets three flags together: `FLAG_PAUSED` (deposits), `FLAG_INSTANT_WITHDRAWAL_PAUSED`, and `FLAG_EPOCH_CLOSE_FUND_PAUSED` — the two withdrawal breakers review §20 approves for Guardian use. It deliberately does **not** reach queued-request creation, funded claims, or force exit (`src/core/CoreVault.sol:453-469`). -Guardians cannot unpause or modify routing — these require owner. +The guardian may also trip `pauseInstantWithdrawalOnly()` and `pauseEpochCloseFundOnly()` individually (both `onlyOwnerOrGuardian`), for a narrower response than the combined `guardianPause()`. ### 11.3 Pause Granularity -Three pause levels are available (all set by owner, all stored in `packedFlags`): +`pauseAll()`/`unpauseAll()`/`pauseDepositsOnly()`/`pauseWithdrawalsOnly()` remain owner-only. Five additional breakers (review §20/§21) give targeted, owner- or guardian-scoped control over the withdrawal surface instead of one all-or-nothing flag: -| Function | Pauses | Flag | -|---|---|---| -| `pauseAll()` | All operations | `FLAG_PAUSED` | -| `pauseDepositsOnly(true)` | Deposits only | `FLAG_PAUSED_DEPOSITS` | -| `pauseWithdrawalsOnly(true)` | Withdrawals only | `FLAG_PAUSED_WITHDRAWALS` | -| `guardianPause()` | All (emergency) | `FLAG_PAUSED` | +| Function | Pauses | Flag | Who | +|---|---|---|---| +| `pauseAll()` | Deposits (only — see §11.2) | `FLAG_PAUSED` | Owner | +| `pauseDepositsOnly(true)` | Deposits only | `FLAG_PAUSED_DEPOSITS` | Owner | +| `pauseWithdrawalsOnly(true)` | Instant settlement + epoch close/fund (aggregate) | `FLAG_PAUSED_WITHDRAWALS` | Owner | +| `pauseInstantWithdrawalOnly(true)` | Instant settlement only (falls back to queue, does not revert) | `FLAG_INSTANT_WITHDRAWAL_PAUSED` | Owner or Guardian | +| `pauseEpochCloseFundOnly(true)` | `closeCurrentEpoch`/`fundEpoch`/`endEpochCrystallize`/`syncOldestUnfundedEpoch` | `FLAG_EPOCH_CLOSE_FUND_PAUSED` | Owner or Guardian | +| `pauseQueuedRequestOnly(true)` | NEW queued-exit requests only (cancelling an existing one is unaffected) | `FLAG_QUEUED_REQUEST_PAUSED` | Owner only, exceptional | +| `pauseFundedClaimOnly(true)` | `claimEpochAssets`/`batchClaimEpochAssets` only | `FLAG_FUNDED_CLAIM_PAUSED` | Owner only, exceptional | +| `pauseForceExitOnly(true)` | `forceWithdraw`/`forceWithdrawAll` only | `FLAG_FORCE_EXIT_PAUSED` | Owner only, dedicated | +| `guardianPause()` | Deposits + instant settlement + epoch close/fund | `FLAG_PAUSED` + `FLAG_INSTANT_WITHDRAWAL_PAUSED` + `FLAG_EPOCH_CLOSE_FUND_PAUSED` | Guardian | + +Two rules follow directly from review §20 and are enforced structurally, not just by convention: + +- **Force exit is read by exactly one flag, `FLAG_FORCE_EXIT_PAUSED`.** It ignores `FLAG_PAUSED`, `FLAG_PAUSED_WITHDRAWALS`, and every other breaker — `pauseAll()`/`guardianPause()`/`pauseWithdrawalsOnly()` can never block it as a side effect. +- **Queued-request creation and funded claims ignore `FLAG_PAUSED_WITHDRAWALS`.** Exit intent must stay recordable while settlement is paused (review §19), and funded claims must never be blockable by any general administrative flag (review §20) — only their own dedicated, owner-only breaker reaches them. -`unpauseAll()` clears all three flags atomically. Source: `src/core/CoreVault.sol:425-454`. +`unpauseAll()` clears all eight flags atomically. Source: `src/core/CoreVault.sol:410-540`, `src/core/modules/EpochedQueueModule.sol` (queue-side checks), `src/core/modules/ERC4626Module.sol` (force-exit check). Full per-breaker test coverage: `test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol`. --- @@ -839,6 +858,7 @@ In `FixedMaturity/Active` state, `markMatured()` is callable by anyone once `blo - Exit engine + fee policy: cluster 01a.2 (pending) - Queue mechanics: cluster 01a.2 (pending) - Deployment guide: [deployment.md](deployment.md) +- Emergency Module Recovery (post-seal): [recovery.md](recovery.md) --- diff --git a/docs/force-exit.md b/docs/force-exit.md index 6ad0d9e..62d5d8a 100644 --- a/docs/force-exit.md +++ b/docs/force-exit.md @@ -115,7 +115,9 @@ struct Pull { ``` 1. _checkForceExitAllowed() - 2. _requireNotPaused() + 2. _notPausedForceExit() — checks ONLY FLAG_FORCE_EXIT_PAUSED (its own dedicated + breaker; never FLAG_PAUSED/FLAG_PAUSED_WITHDRAWALS — review §20, force exit must + never be blocked as a side effect of a generic emergency pause) 3. _ensureFreshWarmNav() — mandatory NAV freshness (hard revert if stale + refresh fails) 4. baseShares = _previewWithdraw(assets) — convertToShares(assets) 5. feeShares = mulBpsUp(baseShares, witBps + forceExitPenaltyBps [+ preMaturityBps]) @@ -200,7 +202,9 @@ No plan — `forceWithdrawAll` pulls all caller shares and sources liquidity aut ``` 1. _checkForceExitAllowed() - 2. _requireNotPaused() + 2. _notPausedForceExit() — checks ONLY FLAG_FORCE_EXIT_PAUSED (its own dedicated + breaker; never FLAG_PAUSED/FLAG_PAUSED_WITHDRAWALS — review §20, force exit must + never be blocked as a side effect of a generic emergency pause) 3. _ensureFreshWarmNav() 4. shares = balanceOf(msg.sender) — ALL caller shares 5. if shares == 0: revert ZeroShares() diff --git a/docs/governance.md b/docs/governance.md index c4432e8..1e8d176 100644 --- a/docs/governance.md +++ b/docs/governance.md @@ -99,13 +99,12 @@ See `src/core/modules/AdminModule.sol:141` for the revoke functions. ### 2.4 Guardian -The guardian (`CoreStorage.Layout.guardian`) is a third privileged address with a narrow remit: emergency pause. The guardian can: +The guardian (`CoreStorage.Layout.guardian`) is a third privileged address with a narrow remit: emergency restriction, never restoration (review §3.3, "fast to restrict, slow to restore"). The guardian can: -- Call `pause()` / `unpause()` — set/clear `FLAG_PAUSED` -- Call `pauseDeposits()` / `unpauseDeposits()` — set/clear `FLAG_PAUSED_DEPOSITS` -- Call `pauseWithdrawals()` — set `FLAG_PAUSED_WITHDRAWALS` (but NOT `unpauseWithdrawals()`) +- Call `guardianPause()` — a single rate-limited action (cooldown via `IParamsProvider.guardianPauseCooldown()`, default 7 days) that sets `FLAG_PAUSED` (deposits), `FLAG_INSTANT_WITHDRAWAL_PAUSED`, and `FLAG_EPOCH_CLOSE_FUND_PAUSED` together. +- Call `pauseInstantWithdrawalOnly(true)` / `pauseEpochCloseFundOnly(true)` individually — the same two breakers `guardianPause()` sets together, available separately for a narrower response (`onlyOwnerOrGuardian`). -The asymmetry in withdrawals (pause yes, unpause no) is intentional: a compromised guardian can halt withdrawals temporarily but cannot unilaterally lift the hold. The owner must call `unpauseWithdrawals()`. +The guardian can restrict but never restore: there is no guardian-callable unpause for anything. Only the owner can call `unpauseAll()` or any of the individual `pause*Only(false)` setters. The guardian also cannot reach `pauseQueuedRequestOnly()`, `pauseFundedClaimOnly()`, or `pauseForceExitOnly()` at all — these are owner-only, exceptional levers by design (review §20): new queued-exit requests, funded claims, and force exit must never be blockable by the guardian's single rapid action. See [architecture.md §11.3](architecture.md#113-pause-granularity) for the full breaker table. `CoreStorage.Layout.lastGuardianPause` is updated each time the guardian pauses to enable off-chain monitoring. @@ -233,26 +232,35 @@ Pause and freeze state are encoded as bit flags in `CoreStorage.Layout.packedFla | Flag | Bit | Effect | |------|-----|--------| -| `FLAG_PAUSED` | 0 | All user-facing operations blocked | +| `FLAG_PAUSED` | 0 | Deposits blocked (also set by `guardianPause()`) | | `FLAG_PAUSED_DEPOSITS` | 1 | Deposits only blocked | -| `FLAG_PAUSED_WITHDRAWALS` | 2 | Withdrawals/claims only blocked | +| `FLAG_PAUSED_WITHDRAWALS` | 2 | Instant settlement + epoch close/fund blocked (aggregate — see §4.2) | | `FLAG_PARAMS_FROZEN` | 3 | All timelocked param changes permanently blocked | | `FLAG_SYSTEM_SEALED` | 9 | Component changes permanently blocked; min delay floor active | +| `FLAG_INSTANT_WITHDRAWAL_PAUSED` | 13 | Instant settlement only blocked (queue fallback still works) | +| `FLAG_QUEUED_REQUEST_PAUSED` | 14 | New queued-exit requests only blocked | +| `FLAG_EPOCH_CLOSE_FUND_PAUSED` | 15 | Epoch close/fund/crystallize only blocked | +| `FLAG_FUNDED_CLAIM_PAUSED` | 16 | Funded-claim settlement only blocked | +| `FLAG_FORCE_EXIT_PAUSED` | 17 | Force exit only blocked — the only flag it ever reads | -See `src/core/storage/CoreStorage.sol:24` for the full flag constant list. +See `src/core/storage/CoreStorage.sol:24-41` for the full flag constant list. ### 4.2 Pause Functions | Function | Who | Effect | |----------|-----|--------| -| `pause()` | GUARDIAN or OWNER | Sets `FLAG_PAUSED` | -| `unpause()` | GUARDIAN or OWNER | Clears `FLAG_PAUSED` | -| `pauseDeposits()` | GUARDIAN or OWNER | Sets `FLAG_PAUSED_DEPOSITS` | -| `unpauseDeposits()` | OWNER | Clears `FLAG_PAUSED_DEPOSITS` | -| `pauseWithdrawals()` | GUARDIAN or OWNER | Sets `FLAG_PAUSED_WITHDRAWALS` | -| `unpauseWithdrawals()` | OWNER | Clears `FLAG_PAUSED_WITHDRAWALS` | - -The guardian can pause quickly via any pause function. Unpausing withdrawals requires the owner (higher trust level — cannot be panic-unpaused unilaterally). +| `pauseAll()` | OWNER | Sets `FLAG_PAUSED` | +| `unpauseAll()` | OWNER | Clears all 8 pause flags (`FLAG_PAUSED` through `FLAG_FORCE_EXIT_PAUSED`) | +| `pauseDepositsOnly(bool)` | OWNER | Sets/clears `FLAG_PAUSED_DEPOSITS` | +| `pauseWithdrawalsOnly(bool)` | OWNER | Sets/clears `FLAG_PAUSED_WITHDRAWALS` | +| `pauseInstantWithdrawalOnly(bool)` | OWNER or GUARDIAN | Sets/clears `FLAG_INSTANT_WITHDRAWAL_PAUSED` | +| `pauseEpochCloseFundOnly(bool)` | OWNER or GUARDIAN | Sets/clears `FLAG_EPOCH_CLOSE_FUND_PAUSED` | +| `pauseQueuedRequestOnly(bool)` | OWNER only | Sets/clears `FLAG_QUEUED_REQUEST_PAUSED` — never Guardian (review §20) | +| `pauseFundedClaimOnly(bool)` | OWNER only | Sets/clears `FLAG_FUNDED_CLAIM_PAUSED` — never Guardian (review §20) | +| `pauseForceExitOnly(bool)` | OWNER only | Sets/clears `FLAG_FORCE_EXIT_PAUSED` — the only breaker force exit ever reads | +| `guardianPause()` | GUARDIAN | Sets `FLAG_PAUSED` + `FLAG_INSTANT_WITHDRAWAL_PAUSED` + `FLAG_EPOCH_CLOSE_FUND_PAUSED` together, subject to a cooldown | + +The guardian can restrict (via `guardianPause()` or the two `onlyOwnerOrGuardian` breakers above) but can never unpause anything — every clearing operation, and every owner-only breaker, requires the owner. This is a stronger, structurally-enforced version of the old "guardian can pause, only owner can unpause withdrawals" asymmetry: it now also holds per-breaker, not just for one flag. Full breaker table with rationale: [architecture.md §11.3](architecture.md#113-pause-granularity). **`CoreStorage.Layout.lastGuardianPause`** records the timestamp of the last guardian pause for audit trails. @@ -506,17 +514,25 @@ Attack is blocked — paramMinDelay cannot be zeroed post-seal. ``` Scenario: oracle reports anomalous NAV spike; guardian acts before owner is reachable. -Guardian calls pauseWithdrawals() - → FLAG_PAUSED_WITHDRAWALS set - → All requestClaim() + settleFeesAndProcessQueue() revert with Paused() - → Deposits still work (FLAG_PAUSED_DEPOSITS not set) +Guardian calls guardianPause() + → FLAG_PAUSED set (deposits blocked) + → FLAG_INSTANT_WITHDRAWAL_PAUSED set (requestInstantWithdrawal() falls back to + the queue instead of settling immediately — + does NOT revert; review §19 exit-intent rule) + → FLAG_EPOCH_CLOSE_FUND_PAUSED set (closeCurrentEpoch()/fundEpoch() revert + with EpochCloseFundPaused()) + → New queued exit requests still work (FLAG_QUEUED_REQUEST_PAUSED not set — + Guardian cannot reach this breaker at all, review §20) + → Funded claims still work (FLAG_FUNDED_CLAIM_PAUSED not set — same reason) + → forceWithdraw()/forceWithdrawAll() still work (force exit reads ONLY + FLAG_FORCE_EXIT_PAUSED, never FLAG_PAUSED — review §20) Investigation completes: oracle data verified normal. -Owner calls unpauseWithdrawals() - → FLAG_PAUSED_WITHDRAWALS cleared - → Queue processing resumes +Owner calls unpauseAll() + → All eight pause flags cleared + → Instant settlement and epoch close/fund resume -Guardian cannot call unpauseWithdrawals() directly — owner-only action. +Guardian cannot call unpauseAll() or any pause*Only(false) directly — owner-only. ``` ### 10.6 Ownership Handoff to Multi-Sig diff --git a/docs/modules.md b/docs/modules.md index 852ad75..e72c736 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -778,12 +778,26 @@ Source: `src/core/modules/StrategyRouter.sol:235-380`. Source: `src/core/modules/StrategyRouter.sol:390-1164`. -### 14.6 Key Invariants +### 14.6 Incident-Response / Emergency Recovery (review Alternative 1, §27) + +`withdrawAllToCore(address)` and `emergencyRedeemBatch(Pull[])` are the concrete implementation of the architecture review's recommended incident-response sequence for a compromised strategy — "Guardian quarantine + existing timelocked recovery" — without any new privileged asset-moving contract: + +1. Guardian calls `StrategyHealthRegistry.setStrategyState(strategy, BROKEN, reason)` (§16.4) — immediate, stops new deposits into the strategy via `_isHealthy`/`planDeposit` filtering. +2. Operations stop routing new allocations through the affected strategy. +3. `ROOT_TIMELOCK` (as `owner`) schedules `withdrawAllToCore(strategy)` (single strategy) or `emergencyRedeemBatch(plan)` (multiple strategies / partial amounts) through the normal governance delay. +4. Recovered assets land in `core` (the vault); accounting is reconciled before the strategy is re-enabled or removed. + +**Both recovery functions are `onlyOwner`-gated only — neither checks `StrategyState` at all.** This is deliberate, not an oversight: review §33 requires that `BROKEN` mean "no new exposure," not "no possible capital recovery" — recovery must never be blocked by the same state flag that stops new deposits. The consequence is that the four-step sequence above is a **procedural** incident-response runbook enforced by governance discipline and the existing timelock delay, not an on-chain precondition chaining `StrategyHealthRegistry` state to `StrategyRouter`'s recovery functions. `emergencyRedeemBatch` additionally bypasses `lossCap`/`navDelta`/cooldown/oracle-freshness checks for exactly this reason — see the code comment at `src/core/modules/StrategyRouter.sol:1128-1131` referencing the v6 recovery incident this path was built for. + +The architecture review rejected a generic `EmergencyExecutor` (§26) specifically because these two functions already cover the recommended first-release incident-response model; a narrower exit-only mechanism (review §28) remains a possible future addition if incident drills show the existing timelock delay is too slow in practice, not a current gap. + +### 14.7 Key Invariants - `planSum ≤ availableSurplus` before any deposit transfer — `planSum > available` reverts with `InvalidPlanSum` (`src/core/modules/StrategyRouter.sol:679-680`). - Per-strategy allocation cap uses the `navBefore` snapshot (not live NAV inside the loop), preventing double-counting when `fundsAlreadyTransferred=true` (`src/core/modules/StrategyRouter.sol:712`). - `emergencyRedeemBatch` and `forceRedeemForWithdraw` intentionally bypass loss cap — W2 policy: forced exit must never be blocked by loss accounting. - `_isHealthy` is FAIL-CLOSED: if `healthRegistry.isHealthyForDeposit()` reverts, the strategy is excluded from the batch (`src/core/modules/StrategyRouter.sol:992-996`). +- `withdrawAllToCore`/`emergencyRedeemBatch` are intentionally NOT gated by `StrategyState` — see §14.6. --- @@ -896,14 +910,14 @@ Source: `src/core/modules/StrategyScorer.sol:86-266`. | `guardian` | Constructor; hot EOA | DEGRADED, BROKEN only (`GuardianCannotMarkOK`) | | `authorizedCallers` | Added by owner | NAV updates only (`updateLastKnownNAV`) | -The guardian restriction enforces that recovery to `OK` always requires owner (Timelock) action — a guardian can quarantine but cannot unquarantine. Source: `src/core/modules/StrategyHealthRegistry.sol:36` (error declaration), `src/core/modules/StrategyHealthRegistry.sol:115` (revert in `setHealthy`), `src/core/modules/StrategyHealthRegistry.sol:140` (revert in `markHealthy`). +The guardian restriction enforces that recovery to `OK` always requires owner (Timelock) action — a guardian can quarantine but cannot unquarantine (review §3.3 "fast to restrict, slow to restore" / §33 BROKEN semantics). Source: `src/core/modules/StrategyHealthRegistry.sol:36` (`GuardianCannotMarkOK` error declaration), `src/core/modules/StrategyHealthRegistry.sol:113-115` (revert in `setStrategyState`), `src/core/modules/StrategyHealthRegistry.sol:138-140` (revert per-iteration in `batchSetStrategyState`). ### 16.4 Key Functions | Function | Access | Description | |---|---|---| | `setStrategyState(address,StrategyState,string)` | `onlyOwnerOrGuardian` | Set health state with reason string | -| `batchSetStrategyState(address[],StrategyState[],string[])` | `onlyOwnerOrGuardian` | Batch version | +| `batchSetStrategyState(address[],StrategyState[],string)` | `onlyOwnerOrGuardian` | Batch version — one reason string applied to every strategy in the batch, not a per-index array | | `updateLastKnownNAV(address,uint256)` | `onlyAuthorizedCaller` | Cache last known NAV (called post-deposit/redeem by `StrategyRouter`) | | `isHealthyForDeposit(address)` | `view` | Returns `state == OK` | | `getStrategyState(address)` | `view` | Returns raw `StrategyState` enum value | @@ -913,8 +927,9 @@ Source: `src/core/modules/StrategyHealthRegistry.sol:95-185`. ### 16.5 Key Invariants -- Guardian cannot set `OK` — `GuardianCannotMarkOK` is a hard revert (`src/core/modules/StrategyHealthRegistry.sol:36,115,140`). +- Guardian cannot set `OK` — `GuardianCannotMarkOK` is a hard revert (`src/core/modules/StrategyHealthRegistry.sol:36,113-115,138-140`). - Absence of registry (`address(0)`) in `StrategyRouter` defaults to all strategies healthy — permissive path for bootstrap phase. +- **`BROKEN`/`DEGRADED` marks inflow prohibition only, never outflow/recovery prohibition** (review §33): marking a strategy `BROKEN` here has no on-chain effect on whether `StrategyRouter.withdrawAllToCore`/`emergencyRedeemBatch` can act on it — see §14.6 below. The state machine in this contract and the recovery functions in `StrategyRouter` are deliberately independent; the link between them is procedural (an incident-response runbook), not enforced by a code-level precondition. --- diff --git a/docs/recovery.md b/docs/recovery.md new file mode 100644 index 0000000..a9e11c1 --- /dev/null +++ b/docs/recovery.md @@ -0,0 +1,174 @@ +# recovery.md — Multyr Core: Emergency Module Recovery + +**Version**: 1.0.0 | **Status**: implemented (kpi4/epochedqueue-cutover) + +--- + +## Table of Contents + +1. [Overview](#1-overview) +2. [Why Not Generic Upgradeability](#2-why-not-generic-upgradeability) +3. [Architecture](#3-architecture) +4. [Recoverable Groups](#4-recoverable-groups) +5. [Immutable Recovery Policy](#5-immutable-recovery-policy) +6. [Lifecycle](#6-lifecycle) +7. [Proposal Digest](#7-proposal-digest) +8. [Security Approver Rotation](#8-security-approver-rotation) +9. [What Recovery Cannot Do](#9-what-recovery-cannot-do) +10. [Relationship to Migration](#10-relationship-to-migration) +11. [Critical Caveat](#11-critical-caveat) +12. [Events](#12-events) +13. [Testing](#13-testing) +14. [Deployment Checklist](#14-deployment-checklist) + +--- + +## 1. Overview + +Emergency Module Recovery is a narrow, immutable mechanism that lets `ROOT_TIMELOCK` replace the module implementation behind one of four pre-approved, economically-isolated selector groups after a sealed vault's normal routing (`setModule`/`setModulesBatch`) has been permanently disabled by `freezeRouting()`. + +It exists because a software defect discovered after routing is frozen would otherwise require migrating the entire vault, even when the defect is confined to a single module. It does **not** exist to let governance continuously evolve the protocol after sealing — see [§2](#2-why-not-generic-upgradeability). + +This document, and the implementation it describes, is Multyr's response to the *Multyr Core Upgradeability, Emergency Recovery & Incident Response Architecture Review* (snapshot `0bab749`), which rejected a general-purpose post-seal `RecoveryController` in favor of exactly this narrower design. See `docs/developer-response-recovery-architecture.{html,pdf}` for the full point-by-point response. + +## 2. Why Not Generic Upgradeability + +> Multyr should support Emergency Module Recovery, not permanent protocol upgradeability. — architecture review §1 + +The review's concern with a general `RecoveryController` was that it risks converting Multyr from a *progressively immutable* protocol into a *permanently governance-upgradeable* one — materially changing the trust model users and allocators rely on. `RecoveryGate` is designed so that, structurally, it cannot do this: + +- It cannot add selectors, relax roles, or touch anything outside `moduleOf` for one pre-approved group. +- Its own policy (delay, cooldown, vault, root timelock) is immutable — set once at construction, no setters exist. +- The recoverable groups themselves are fixed at compile time (read from `SelectorLib`), not configurable post-deployment. +- `CoreVault`'s constitutional surface — the shell, governance addresses, sealing logic, and the recovery policy binding itself — is entirely outside `RecoveryGate`'s reach by construction (see [§9](#9-what-recovery-cannot-do)). + +## 3. Architecture + +``` +ROOT_TIMELOCK + | schedule(RecoveryGate.propose(groupId, newModules, reasonRef)) + v +RecoveryGate (immutable, no proxy) + | propose() — only ROOT_TIMELOCK + | approve() — only SECURITY_APPROVER, bound to an exact digest + | vetoCancel() — only CoreVault.vetoer(), read live + | execute() — open caller, once approved + matured + not vetoed + v +CoreVault.recoverModuleGroup(groupId, newModules) + | onlyRecoveryGate + | selectors derived from SelectorLib, not trusted from the caller + | never writes roleOf — role relaxation is structurally impossible + v +Atomic replacement of every selector in the group +``` + +Source: `src/governance/RecoveryGate.sol`, `src/core/CoreVault.sol` (`recoverModuleGroup`, `setRecoveryGate`, `onlyRecoveryGate`). + +`RecoveryGate.propose()` is `onlyRootTimelock`-gated the same way `SystemSealer.verifyAndSeal()` is scheduled today — through `rootTimelock.scheduleBatch([...])`. The recovery-specific delay (`minDelay`, [§5](#5-immutable-recovery-policy)) is `RecoveryGate`'s own clock, layered **on top of**, not instead of, the timelock's own scheduling delay. + +## 4. Recoverable Groups + +Selector sets are read directly from `src/core/libraries/SelectorLib.sol` — the same source of truth `CoreVault`'s own deployment wiring uses. There is no second, independently-maintained selector registry to drift out of sync (review §9's whitelist requirement, satisfied by reuse rather than a new contract). + +| Group ID | Constant | Module | Selectors | +|---|---|---|---| +| 0 | `EPOCH_QUEUE_GROUP` | `EpochedQueueModule` | `getQueueModuleSelectors()` + `getQueueModuleViewSelectors()` | +| 1 | `ERC4626_GROUP` | `ERC4626Module` | `getERC4626ModuleSelectors()` | +| 2 | `LIQUIDITY_GROUP` | `LiquidityOpsModule` | `getLiquidityOpsModuleSelectors()` | +| 3 | `FIXED_MATURITY_GROUP` | `FixedMaturityModule` | `getFixedMaturityModuleSelectors()` | + +**Permanently excluded, not merely unlisted:** `AdminModule`'s 26 owner selectors (governance/sealing/authorization surface — timelock submit/accept/revoke, vetoer rotation, component setters, `freezeParams()`, `setEcosystem()`) have no group ID at all. Every direct `CoreVault` function (`setModule*`, `freezeRouting`, `pause*`, `setSelectorRegistry`, `setRecoveryGate`, `authorizeModule`) is not `moduleOf`-routed in the first place — there is no selector for `recoverModuleGroup` to touch even if a group ID were mis-specified. `StrategyRouter`, `BufferManager`, `StrategyHealthRegistry`, `FeeCollector`, `GlobalConfig` are satellite contracts referenced by address, not routed selectors — they remain governed exclusively by `AdminModule`'s existing timelocked `submit*/accept*/revoke*` component-setter pattern, untouched by this mechanism. + +## 5. Immutable Recovery Policy + +Fixed at `RecoveryGate` construction — no setters exist for any of these: + +| Field | Value | Rationale | +|---|---|---| +| `vault` | constructor arg | The one `CoreVault` this gate serves | +| `rootTimelock` | constructor arg | The one address that may `propose()` | +| `minDelay` | constructor arg, `>= 14 days` enforced by the constructor itself | Review §12 — recovery is remediation, not same-block containment | +| `cooldown` | constructor arg | Minimum gap between two completed recoveries of the *same* group — prevents salami-slicing continuous evolution through repeated individually-reviewable recoveries | +| Recoverable groups | compile-time, via `SelectorLib` | Not configurable post-deployment at all | +| `securityApprover` | constructor arg, **the one rotatable field** | See [§8](#8-security-approver-rotation) | + +A misconfigured deployment cannot exist: `RecoveryGate`'s constructor reverts with `DelayTooShort()` if `minDelay < 14 days`, so there is no way to deploy a gate with a shorter delay than the review's floor. + +## 6. Lifecycle + +1. **Propose** — `ROOT_TIMELOCK` calls `propose(groupId, newModules, reasonRef)`. Reverts if `newModules.length` doesn't match the group's selector count, if a proposal for that group is already pending, or if the group's cooldown hasn't elapsed since its last completed recovery. Computes and stores a digest ([§7](#7-proposal-digest)), starts the `minDelay` clock. +2. **Approve** — `SECURITY_APPROVER` calls `approve(groupId, digest)`, supplying the exact digest being approved. If `propose()` is called again for the same group before this executes, the digest changes and any prior approval is silently invalidated. +3. **Veto (optional, any time before execution)** — `CoreVault.vetoer()` (read live, not cached) calls `vetoCancel(groupId)`. Cancellation-only — the vetoer has no other capability on `RecoveryGate` and cannot propose, approve, or execute. +4. **Execute** — anyone calls `execute(groupId)` once `block.timestamp >= eta`, within a 7-day execution window, once approved and not vetoed. Calls `CoreVault.recoverModuleGroup(groupId, newModules)`, which atomically rewrites every selector in the group — the whole group replaces together or the call reverts, never a partial mix of old and new implementations (review §10). + +## 7. Proposal Digest + +Per review §13, the digest committed at `propose()` time binds: + +- `vault`, `block.chainid`, `groupId` +- the group's exact selector set +- every selector's **current** `moduleOf` address and codehash +- the **proposed** module address(es) and their codehash(es) +- `MANIFEST_VERSION` (a `RecoveryGate` constant) +- `reasonRef` — an off-chain reference identifier (e.g. an incident report hash) + +"Unchanged role mapping" (also required by review §13) is not a digest field because it cannot vary: `recoverModuleGroup()` never writes `roleOf` at all ([§9](#9-what-recovery-cannot-do)), so there is nothing about roles for the approver to review or for the digest to commit to. + +## 8. Security Approver Rotation + +`securityApprover` is the one field in an otherwise fully immutable policy that can change — resolving the open question raised in the developer response (`docs/developer-response-recovery-architecture` §6): a permanently fixed approver address is itself an operational risk (signer key loss over a multi-year sealed deployment with no recourse). + +Rotation uses the same propose/execute/veto shape as a recovery itself: + +- `proposeApproverChange(newApprover)` — `onlyRootTimelock`, starts the same `minDelay` clock. +- `executeApproverChange()` — open caller, after the delay. +- `vetoApproverChange()` — `CoreVault.vetoer()` only. + +Because rotation is subject to the same delay as a recovery, a compromised `ROOT_TIMELOCK` cannot install a friendly approver in time to affect any recovery already in flight — the earliest a new approver could take effect is no sooner than a recovery proposed at the same time would mature. + +## 9. What Recovery Cannot Do + +Enforced structurally, not by convention (review §11): + +- **Cannot add new selectors** — `recoverModuleGroup` only ever rewrites `moduleOf` for the group's fixed, `SelectorLib`-derived selector set. +- **Cannot relax or change roles** — `recoverModuleGroup` takes no role parameter at all and never writes `roleOf`. +- **Cannot expose previously privileged selectors** — same reason. +- **Cannot touch CoreVault's shell, ownership, guardian, or vetoer** — none of these are `moduleOf`-routed selectors. +- **Cannot modify its own policy** — no setters exist on `vault`, `rootTimelock`, `minDelay`, `cooldown`, or the recoverable group definitions. +- **Cannot reach `AdminModule`'s governance/sealing selectors, or any satellite component** ([§4](#4-recoverable-groups)). +- **The Guardian cannot call anything on `RecoveryGate`** — it has no role in the recovery lifecycle at all, consistent with review §3.3 (Guardian is fast-restrict, never constructive). + +## 10. Relationship to Migration + +Emergency Module Recovery does not solve every defect. If the flaw is in `CoreVault`'s direct functions, the fallback routing dispatcher, core storage architecture, the recovery entry point itself, or a critical immutable governance binding, the correct solution remains **migration** — deploying a new vault and moving user funds, not attempting to repair the sealed shell in place. This is a deliberate boundary: it prevents the recovery mechanism from becoming capable of rewriting the entire system (review §36). + +## 11. Critical Caveat + +Restricting recovery to existing selectors, unchanged roles, pre-approved groups, and expected codehashes does **not** mathematically prove that a replacement module only repairs a bug. A replacement module executed through `delegatecall` can still materially alter economic behavior while using exactly the same selectors and authorization roles, and it can interact with vault storage and assets. + +The correct claim is therefore: *Emergency recovery cannot expand the sealed authorization and selector topology, and is procedurally restricted to remediation.* It is not, and should not be represented as, cryptographic proof that every future replacement contains only bug fixes. That guarantee comes from the combination of on-chain constraints (this contract) with governance delay, independent security approval, transparent codehash commitment, and pre-execution review and testing — not from any one of those alone (review §4). + +## 12. Events + +| Event | Emitted by | When | +|---|---|---| +| `RecoveryProposed(groupId, digest, eta, reasonRef)` | `RecoveryGate` | `propose()` | +| `RecoveryApproved(groupId, digest)` | `RecoveryGate` | `approve()` | +| `RecoveryVetoed(groupId, digest)` | `RecoveryGate` | `vetoCancel()` | +| `RecoveryExecuted(groupId, digest, newModules)` | `RecoveryGate` | `execute()` | +| `ApproverChangeProposed/Executed/Vetoed(...)` | `RecoveryGate` | approver rotation | +| `RecoveryGateSet(gate)` | `CoreVault` | `setRecoveryGate()` | +| `ModuleGroupRecovered(groupId, newModules)` | `CoreVault` | `recoverModuleGroup()` | + +## 13. Testing + +Acceptance tests: `test/invariants/Recovery_Invariants.t.sol`. Incident simulations: `test/incident-sim/`. See [architecture review §40](#) for the full acceptance-test list this suite is built against. + +## 14. Deployment Checklist + +Before sealing a vault that wires recovery: + +1. Deploy `RecoveryGate` with the vault's address, `ROOT_TIMELOCK`, the chosen `SECURITY_APPROVER`, `minDelay >= 14 days`, and the chosen `cooldown`. +2. Call `CoreVault.setRecoveryGate(gate)` — set-once, before `freezeRouting()`/sealing. +3. Include `recoveryGate` and `recoveryManifestVersion` (`RecoveryGate.MANIFEST_VERSION()`) in the `SystemSealer.SealConfig` passed to `verifyAndSeal()` — the seal will reject a mismatch between the manifest and what's actually wired into the vault. +4. If a deployment deliberately does not wire recovery, pass `recoveryGate: address(0)` and `recoveryManifestVersion: 0` — `SystemSealer` treats this as a valid, explicit "no recovery" configuration, not an error. diff --git a/src/core/CoreVault.sol b/src/core/CoreVault.sol index b7882f8..fc85a38 100644 --- a/src/core/CoreVault.sol +++ b/src/core/CoreVault.sol @@ -18,6 +18,7 @@ import { FeeStorage } from "./storage/FeeStorage.sol"; import { Events } from "./libraries/Events.sol"; import { Percentage } from "../libs/Percentage.sol"; import { SelectorRegistry } from "./libraries/SelectorRegistry.sol"; +import { SelectorLib } from "./libraries/SelectorLib.sol"; /// @title CoreVault v8 (Diamond-lite Thin Proxy) /// @notice ERC-4626 vault that delegates ALL economic logic to modules via delegatecall. @@ -45,6 +46,10 @@ contract CoreVault is ERC4626, ICoreVault { error Paused(); error DepositsPaused(); error WithdrawalsPaused(); + error QueuedRequestPaused(); + error EpochCloseFundPaused(); + error FundedClaimPaused(); + error ForceExitPaused(); error ZeroAmount(); error ZeroAddress(); error RoutingFrozen(); @@ -55,6 +60,10 @@ contract CoreVault is ERC4626, ICoreVault { error ReentrancyGuardLocked(); error InvalidRoleForSelector(bytes4 selector, uint8 attemptedRole, uint8 requiredRole); error SelectorRegistryAlreadySet(); + error RecoveryGateAlreadySet(); + error NotRecoveryGate(); + error InvalidRecoveryGroup(); + error WrongRecoveryModuleCount(); error SystemSealed(); error SealerAlreadySet(); error NotAuthorizedSealer(); @@ -119,6 +128,20 @@ contract CoreVault is ERC4626, ICoreVault { _; } + modifier onlyOwnerOrGuardian() { + CoreStorage.Layout storage core = CoreStorage.layout(); + if (msg.sender != core.owner && msg.sender != core.guardian) revert NotOwnerOrGuardian(); + _; + } + + /// @dev Emergency Module Recovery (review §7) — the sole caller of + /// recoverModuleGroup(). Set once via setRecoveryGate(), never + /// changed afterward (review §8: recovery policy is immutable). + modifier onlyRecoveryGate() { + if (msg.sender != CoreStorage.layout().recoveryGate) revert NotRecoveryGate(); + _; + } + modifier nonReentrant() { CoreStorage.Layout storage core = CoreStorage.layout(); if (core.packedFlags & CoreStorage.FLAG_REENTRANCY_LOCKED != 0) { @@ -257,6 +280,18 @@ contract CoreVault is ERC4626, ICoreVault { emit Events.SelectorRegistrySet(registry); } + /// @notice Bind the Emergency Module Recovery gate. Set-once, immutable + /// thereafter — same pattern as setSelectorRegistry() (review §8: + /// the recovery policy, including which contract enforces it, + /// must not be administratively changeable post-seal). + function setRecoveryGate(address gate) external onlyOwner { + CoreStorage.Layout storage core = CoreStorage.layout(); + if (core.recoveryGate != address(0)) revert RecoveryGateAlreadySet(); + if (gate == address(0)) revert ZeroAddress(); + core.recoveryGate = gate; + emit Events.RecoveryGateSet(gate); + } + function setModule(bytes4 selector, address module, uint8 role) external onlyOwner { CoreStorage.Layout storage core = CoreStorage.layout(); if (core.packedFlags & CoreStorage.FLAG_ROUTING_FROZEN != 0) revert RoutingFrozen(); @@ -321,6 +356,58 @@ contract CoreVault is ERC4626, ICoreVault { return CoreStorage.layout().selectorRegistry; } + function recoveryGate() external view returns (address) { + return CoreStorage.layout().recoveryGate; + } + + /// @notice Emergency Module Recovery entry point (review §7) — completely + /// separate from setModule()/setModulesBatch(), which remain + /// permanently disabled once FLAG_ROUTING_FROZEN is set. Only + /// callable by the immutable recoveryGate. + /// @dev Deliberately takes no role parameter: it only ever rewrites + /// moduleOf[selector] for the group's fixed, SelectorLib-derived + /// selector set and never touches roleOf, which structurally + /// forecloses role relaxation (review §11) rather than relying on a + /// runtime check. Selectors are derived here from SelectorLib + /// directly — the same source of truth CoreVault's own deployment + /// wiring uses — not trusted from the caller, so RecoveryGate + /// cannot mis-specify which selectors a group covers. All-or- + /// nothing: `newModules.length` must match the group's selector + /// count exactly or the whole call reverts (review §10, atomic + /// module-group replacement). + function recoverModuleGroup(uint8 groupId, address[] calldata newModules) + external + onlyRecoveryGate + { + bytes4[] memory selectors = _recoverySelectorsForGroup(groupId); + if (newModules.length != selectors.length) revert WrongRecoveryModuleCount(); + + CoreStorage.Layout storage core = CoreStorage.layout(); + for (uint256 i; i < selectors.length; ++i) { + core.moduleOf[selectors[i]] = newModules[i]; + } + + emit Events.ModuleGroupRecovered(groupId, newModules); + } + + /// @dev Mirrors RecoveryGate._selectorsForGroup() exactly — kept in sync + /// because both read the same underlying SelectorLib getters, not + /// because either trusts the other's definition. + function _recoverySelectorsForGroup(uint8 groupId) internal pure returns (bytes4[] memory) { + if (groupId == 0) { + bytes4[] memory writeSel = SelectorLib.getQueueModuleSelectors(); + bytes4[] memory viewSel = SelectorLib.getQueueModuleViewSelectors(); + bytes4[] memory combined = new bytes4[](writeSel.length + viewSel.length); + for (uint256 i; i < writeSel.length; ++i) combined[i] = writeSel[i]; + for (uint256 i; i < viewSel.length; ++i) combined[writeSel.length + i] = viewSel[i]; + return combined; + } + if (groupId == 1) return SelectorLib.getERC4626ModuleSelectors(); + if (groupId == 2) return SelectorLib.getLiquidityOpsModuleSelectors(); + if (groupId == 3) return SelectorLib.getFixedMaturityModuleSelectors(); + revert InvalidRecoveryGroup(); + } + function authorizedSealer() external view returns (address) { return CoreStorage.layout().authorizedSealer; } @@ -419,17 +506,117 @@ contract CoreVault is ERC4626, ICoreVault { return CoreStorage.layout().packedFlags & CoreStorage.FLAG_PAUSED_WITHDRAWALS != 0; } + // --- Granular withdrawal circuit breakers (review §20/§21) --- + // FLAG_PAUSED_WITHDRAWALS (above) is honored, in addition to the specific + // flag below, ONLY by instant settlement and epoch close/fund — the two + // breakers Guardian may also trip via guardianPause(). It deliberately does + // NOT reach queued-request creation or funded claims: exit intent must stay + // recordable even while settlement is paused (review §19), and funded + // claims must never be blockable by any general administrative flag + // (review §20 — only the dedicated pauseFundedClaimOnly()/ + // pauseQueuedRequestOnly() can reach those). Force exit reads neither this + // flag nor FLAG_PAUSED: it has its own dedicated breaker so it can never be + // blocked as a side effect of a generic emergency pause (review §20). + function pausedInstantWithdrawal() external view returns (bool) { + return CoreStorage.layout().packedFlags & CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED != 0; + } + + function pausedQueuedRequest() external view returns (bool) { + return CoreStorage.layout().packedFlags & CoreStorage.FLAG_QUEUED_REQUEST_PAUSED != 0; + } + + function pausedEpochCloseFund() external view returns (bool) { + return CoreStorage.layout().packedFlags & CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED != 0; + } + + function pausedFundedClaim() external view returns (bool) { + return CoreStorage.layout().packedFlags & CoreStorage.FLAG_FUNDED_CLAIM_PAUSED != 0; + } + + function pausedForceExit() external view returns (bool) { + return CoreStorage.layout().packedFlags & CoreStorage.FLAG_FORCE_EXIT_PAUSED != 0; + } + function pauseAll() external onlyOwner { CoreStorage.layout().packedFlags |= CoreStorage.FLAG_PAUSED; emit Events.AllPaused(); } function unpauseAll() external onlyOwner { - CoreStorage.layout().packedFlags &= - ~(CoreStorage.FLAG_PAUSED | CoreStorage.FLAG_PAUSED_DEPOSITS | CoreStorage.FLAG_PAUSED_WITHDRAWALS); + CoreStorage.layout().packedFlags &= ~( + CoreStorage.FLAG_PAUSED | CoreStorage.FLAG_PAUSED_DEPOSITS + | CoreStorage.FLAG_PAUSED_WITHDRAWALS | CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED + | CoreStorage.FLAG_QUEUED_REQUEST_PAUSED | CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED + | CoreStorage.FLAG_FUNDED_CLAIM_PAUSED | CoreStorage.FLAG_FORCE_EXIT_PAUSED + ); emit Events.AllUnpaused(); } + /// @notice Guardian-eligible instant-settlement breaker (review §20: approved + /// as a narrow circuit breaker Guardian may trip immediately). + function pauseInstantWithdrawalOnly(bool p) external onlyOwnerOrGuardian { + if (p) { + CoreStorage.layout().packedFlags |= CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; + emit Events.InstantWithdrawalPaused(); + } else { + CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; + emit Events.InstantWithdrawalUnpaused(); + } + } + + /// @notice Guardian-eligible epoch close/fund breaker (review §20: "may be + /// temporarily restricted where the affected accounting or + /// settlement path is implicated"; separate from exit-intent + /// recording, which is pauseQueuedRequestOnly below). + function pauseEpochCloseFundOnly(bool p) external onlyOwnerOrGuardian { + if (p) { + CoreStorage.layout().packedFlags |= CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; + emit Events.EpochCloseFundPaused(); + } else { + CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; + emit Events.EpochCloseFundUnpaused(); + } + } + + /// @notice Owner-only, exceptional: blocking NEW queued-exit requests is + /// explicitly NOT approved as a routine Guardian tool (review §20). + /// Existing requests can still be cancelled regardless of this flag. + function pauseQueuedRequestOnly(bool p) external onlyOwner { + if (p) { + CoreStorage.layout().packedFlags |= CoreStorage.FLAG_QUEUED_REQUEST_PAUSED; + emit Events.QueuedRequestPaused(); + } else { + CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_QUEUED_REQUEST_PAUSED; + emit Events.QueuedRequestUnpaused(); + } + } + + /// @notice Owner-only, exceptional: funded claims are permissionless by + /// default and must never be stoppable by ordinary/Guardian action + /// (review §20 — only if the claim execution path itself is unsafe). + function pauseFundedClaimOnly(bool p) external onlyOwner { + if (p) { + CoreStorage.layout().packedFlags |= CoreStorage.FLAG_FUNDED_CLAIM_PAUSED; + emit Events.FundedClaimPaused(); + } else { + CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_FUNDED_CLAIM_PAUSED; + emit Events.FundedClaimUnpaused(); + } + } + + /// @notice Owner-only, exceptional: force exit's own dedicated breaker. + /// Never touched by pauseAll()/guardianPause() — its behavior must + /// be a deliberate, separate governance action (review §20). + function pauseForceExitOnly(bool p) external onlyOwner { + if (p) { + CoreStorage.layout().packedFlags |= CoreStorage.FLAG_FORCE_EXIT_PAUSED; + emit Events.ForceExitPaused(); + } else { + CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_FORCE_EXIT_PAUSED; + emit Events.ForceExitUnpaused(); + } + } + function pauseDepositsOnly(bool p) external onlyOwner { if (p) { CoreStorage.layout().packedFlags |= CoreStorage.FLAG_PAUSED_DEPOSITS; @@ -460,7 +647,11 @@ contract CoreVault is ERC4626, ICoreVault { revert GuardianCooldownActive(); } core.lastGuardianPause = uint64(block.timestamp); - core.packedFlags |= CoreStorage.FLAG_PAUSED; + // Deposits (unchanged), plus the two withdrawal breakers review §20 + // approves for Guardian's immediate use. Queued-request, funded-claim, + // and force-exit stay owner-only/exceptional and are never touched here. + core.packedFlags |= CoreStorage.FLAG_PAUSED | CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED + | CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; emit Events.GuardianPauseActivated(msg.sender, block.timestamp); } diff --git a/src/core/SystemSealer.sol b/src/core/SystemSealer.sol index 56496b1..7aaa97e 100644 --- a/src/core/SystemSealer.sol +++ b/src/core/SystemSealer.sol @@ -12,6 +12,7 @@ import { Incentives } from "./modules/Incentives.sol"; import { IncentivesEngine } from "./modules/IncentivesEngine.sol"; import { IRewardsPayoutManager } from "../interfaces/IRewardsPayoutManager.sol"; import { SelectorRegistry } from "./libraries/SelectorRegistry.sol"; +import { RecoveryGate } from "../governance/RecoveryGate.sol"; import { AccessControl } from "@openzeppelin/contracts/access/AccessControl.sol"; import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; @@ -40,7 +41,17 @@ import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/I * - Only authorized SystemSealer can call vault.sealBySealer() * - configHash binds the verified config to the seal event for auditability * - * PRE-SEAL CHECKLIST (verified by verifyAndSeal()): + * SINGLE VERIFICATION ENGINE (review §25): + * canSeal() and verifyAndSeal() both derive their result from the same + * internal _verifyLiveState(). Before this, the two functions maintained + * independent invariant lists that had drifted apart: canSeal() never + * checked strategy role assignments or the deployer-retains-no-roles + * invariant, so it could return (true, "") for a config verifyAndSeal() + * would still revert on. verifyAndSeal() now adds only the authorization + * check, the state transition, and seal-digest storage on top of the + * shared verifier — it does not maintain a second independent list. + * + * PRE-SEAL CHECKLIST (verified by _verifyLiveState(), shared by both entry points): * [x] CoreVault.owner == ROOT_TIMELOCK * [x] CoreVault.guardian == SAFE_GUARDIAN * [x] CoreVault.vetoer == SAFE_VETO @@ -55,6 +66,8 @@ import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/I * [x] StrategyHealthRegistry.owner == ROOT_TIMELOCK * [x] StrategyHealthRegistry.guardian == SAFE_GUARDIAN * [x] Incentives.owner == ROOT_TIMELOCK (if deployed) + * [x] CoreVault.recoveryGate == config.recoveryGate (if deployed) and its + * MANIFEST_VERSION matches config.recoveryManifestVersion * [x] Strategy: DEFAULT_ADMIN_ROLE -> ROOT_TIMELOCK * [x] Strategy: PARAM_ROLE -> ROOT_TIMELOCK * [x] Strategy: CORE_ROLE -> CoreVault @@ -64,6 +77,8 @@ import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/I * provider the vault actually resolves at runtime) * [x] Non-6dp vault asset has VAULT_CAP/WITHDRAWAL/GOV_CAPS overrides set * (GlobalConfig defaults are 6dp/USDC-shaped and would brick the vault) + * [x] config.chainId == block.chainid (review §24/§42 — a manifest built for + * one chain must not be sealable against a vault on another) */ contract SystemSealer { // ═══════════════════════════════════════════════════════════════════════════════ @@ -71,7 +86,6 @@ contract SystemSealer { // ═══════════════════════════════════════════════════════════════════════════════ error NotRootTimelock(); error InvariantViolation(string reason); - error SelectorRoleMismatch(bytes4 selector, uint8 actual, uint8 expected); // ═══════════════════════════════════════════════════════════════════════════════ // EVENTS @@ -91,6 +105,9 @@ contract SystemSealer { // ═══════════════════════════════════════════════════════════════════════════════ struct SealConfig { + // Chain binding — the manifest identity this config was authored for. + uint256 chainId; + // Core addresses address vault; address strategyRouter; @@ -116,6 +133,14 @@ contract SystemSealer { // RewardsPayoutManager (optional) address rewardsPayoutManager; + // Emergency Module Recovery gate (optional — address(0) if this + // deployment does not wire recovery). If set, must match + // vault.recoveryGate() exactly and its MANIFEST_VERSION must match + // recoveryManifestVersion below, so sealing also commits to the + // identity of the recovery policy, not just its address. + address recoveryGate; + uint256 recoveryManifestVersion; + // RewardsTreasury: NOT validated — recorded in configHash for audit purposes // only. The actual invariant (non-zero if rewardsPayoutManager is deployed) // is checked against vault.rewardsTreasury() (see INVARIANT 8d), never @@ -145,25 +170,99 @@ contract SystemSealer { function verifyAndSeal(SealConfig calldata config) external { if (msg.sender != config.rootTimelock) revert NotRootTimelock(); + (bool ok, string memory reason) = _verifyLiveState(config); + if (!ok) revert InvariantViolation(reason); + + CoreVault vault = CoreVault(payable(config.vault)); + + // block.timestamp is intentionally excluded. Including it would make the + // hash non-deterministic between scheduleBatch() and executeBatch() (they + // run at different blocks separated by the full timelock delay). The TOCTOU + // protection comes from the address binding alone — every field here is a + // deploy-time constant that cannot change between schedule and execute. + bytes32 configHash = keccak256( + abi.encode( + config.chainId, + config.vault, + config.rootTimelock, + config.guardian, + config.vetoer, + config.feeCollector, + config.strategy, + config.incentives, + config.incentivesEngine, + config.rewardsPayoutManager, + config.rewardsTreasury, + config.recoveryGate, + config.recoveryManifestVersion + ) + ); + + vault.sealBySealer(configHash); + + emit SystemSealedEvent(config.vault, msg.sender, configHash, block.timestamp); + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // VIEW FUNCTIONS + // ═══════════════════════════════════════════════════════════════════════════════ + + /** + * @notice Check if system would pass seal verification (dry run) + * @param config Configuration to verify + * @return valid True if all invariants pass + * @return reason Error message if validation fails + */ + function canSeal(SealConfig calldata config) + external + view + returns (bool valid, string memory reason) + { + return _verifyLiveState(config); + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // SINGLE VERIFICATION ENGINE (review §25) + // ═══════════════════════════════════════════════════════════════════════════════ + + /// @dev The sole source of truth for every pre-seal invariant. Both + /// canSeal() and verifyAndSeal() call this and only this — there is + /// no second, independently-maintained invariant list. A positive + /// result here must imply verifyAndSeal() succeeds against unchanged + /// state (review §42). + function _verifyLiveState(SealConfig calldata config) + internal + view + returns (bool ok, string memory reason) + { CoreVault vault = CoreVault(payable(config.vault)); + // ───────────────────────────────────────────────────────────────────────── + // Chain binding + // ───────────────────────────────────────────────────────────────────────── + if (config.chainId != block.chainid) { + return (false, "chainId mismatch"); + } + + if (vault.isSystemSealed()) return (false, "Already sealed"); + // ───────────────────────────────────────────────────────────────────────── // INVARIANT 1: CoreVault ownership and state // ───────────────────────────────────────────────────────────────────────── if (vault.owner() != config.rootTimelock) { - revert InvariantViolation("CoreVault.owner != ROOT_TIMELOCK"); + return (false, "CoreVault.owner != ROOT_TIMELOCK"); } if (vault.guardian() != config.guardian) { - revert InvariantViolation("CoreVault.guardian != SAFE_GUARDIAN"); + return (false, "CoreVault.guardian != SAFE_GUARDIAN"); } if (vault.vetoer() != config.vetoer) { - revert InvariantViolation("CoreVault.vetoer != SAFE_VETO"); + return (false, "CoreVault.vetoer != SAFE_VETO"); } if (!vault.isRoutingFrozen()) { - revert InvariantViolation("CoreVault.isRoutingFrozen != true"); + return (false, "CoreVault.isRoutingFrozen != true"); } if (!IAdminModule(config.vault).isComponentsTimelocked()) { - revert InvariantViolation("CoreVault.isComponentsTimelocked != true"); + return (false, "CoreVault.isComponentsTimelocked != true"); } // ───────────────────────────────────────────────────────────────────────── @@ -171,17 +270,14 @@ contract SystemSealer { // ───────────────────────────────────────────────────────────────────────── address registryAddr = vault.selectorRegistry(); if (registryAddr == address(0)) { - revert InvariantViolation("SelectorRegistry not set"); + return (false, "SelectorRegistry not set"); } SelectorRegistry registry = SelectorRegistry(registryAddr); bytes4[] memory ownerSelectors = registry.getOwnerSelectors(); - for (uint256 i = 0; i < ownerSelectors.length; i++) { - bytes4 sel = ownerSelectors[i]; - uint8 actualRole = vault.roleOf(sel); - if (actualRole != ROLE_OWNER) { - revert SelectorRoleMismatch(sel, actualRole, ROLE_OWNER); + if (vault.roleOf(ownerSelectors[i]) != ROLE_OWNER) { + return (false, "Selector role mismatch"); } } @@ -190,7 +286,7 @@ contract SystemSealer { // ───────────────────────────────────────────────────────────────────────── FeeCollector fc = FeeCollector(config.feeCollector); if (fc.governor() != config.rootTimelock) { - revert InvariantViolation("FeeCollector.governor != ROOT_TIMELOCK (IMMUTABLE!)"); + return (false, "FeeCollector.governor != ROOT_TIMELOCK (IMMUTABLE!)"); } // ───────────────────────────────────────────────────────────────────────── @@ -198,23 +294,21 @@ contract SystemSealer { // ───────────────────────────────────────────────────────────────────────── GlobalConfig gc = GlobalConfig(config.globalConfig); if (gc.governor() != config.rootTimelock) { - revert InvariantViolation("GlobalConfig.governor != ROOT_TIMELOCK"); + return (false, "GlobalConfig.governor != ROOT_TIMELOCK"); } // ───────────────────────────────────────────────────────────────────────── // INVARIANT 5: StrategyRouter ownership // ───────────────────────────────────────────────────────────────────────── - StrategyRouter router = StrategyRouter(config.strategyRouter); - if (router.owner() != config.rootTimelock) { - revert InvariantViolation("StrategyRouter.owner != ROOT_TIMELOCK"); + if (StrategyRouter(config.strategyRouter).owner() != config.rootTimelock) { + return (false, "StrategyRouter.owner != ROOT_TIMELOCK"); } // ───────────────────────────────────────────────────────────────────────── // INVARIANT 6: BufferManager ownership // ───────────────────────────────────────────────────────────────────────── - BufferManager buffer = BufferManager(config.bufferManager); - if (buffer.owner() != config.rootTimelock) { - revert InvariantViolation("BufferManager.owner != ROOT_TIMELOCK"); + if (BufferManager(config.bufferManager).owner() != config.rootTimelock) { + return (false, "BufferManager.owner != ROOT_TIMELOCK"); } // ───────────────────────────────────────────────────────────────────────── @@ -222,33 +316,35 @@ contract SystemSealer { // ───────────────────────────────────────────────────────────────────────── StrategyHealthRegistry hr = StrategyHealthRegistry(config.healthRegistry); if (hr.owner() != config.rootTimelock) { - revert InvariantViolation("HealthRegistry.owner != ROOT_TIMELOCK"); + return (false, "HealthRegistry.owner != ROOT_TIMELOCK"); } if (hr.guardian() != config.guardian) { - revert InvariantViolation("HealthRegistry.guardian != SAFE_GUARDIAN"); + return (false, "HealthRegistry.guardian != SAFE_GUARDIAN"); } // ───────────────────────────────────────────────────────────────────────── // INVARIANT 8: Incentives ownership (if deployed) // ───────────────────────────────────────────────────────────────────────── if (config.incentives != address(0)) { - Incentives inc = Incentives(config.incentives); - if (inc.owner() != config.rootTimelock) { - revert InvariantViolation("Incentives.owner != ROOT_TIMELOCK"); + if (Incentives(config.incentives).owner() != config.rootTimelock) { + return (false, "Incentives.owner != ROOT_TIMELOCK"); } } // INVARIANT 8b: IncentivesEngine v2 governance (if deployed) if (config.incentivesEngine != address(0)) { if (IncentivesEngine(config.incentivesEngine).governance() != config.rootTimelock) { - revert InvariantViolation("IncentivesEngine.governance != ROOT_TIMELOCK"); + return (false, "IncentivesEngine.governance != ROOT_TIMELOCK"); } } // INVARIANT 8c: RewardsPayoutManager governance (if deployed) if (config.rewardsPayoutManager != address(0)) { - if (IRewardsPayoutManager(config.rewardsPayoutManager).governance() != config.rootTimelock) { - revert InvariantViolation("RewardsPayoutManager.governance != ROOT_TIMELOCK"); + if ( + IRewardsPayoutManager(config.rewardsPayoutManager).governance() + != config.rootTimelock + ) { + return (false, "RewardsPayoutManager.governance != ROOT_TIMELOCK"); } } @@ -260,14 +356,32 @@ contract SystemSealer { // address here while storage is still address(0), sealing in a permanently broken // payout path. if (config.rewardsPayoutManager != address(0) && vault.rewardsTreasury() == address(0)) { - revert InvariantViolation("RewardsTreasury not set but RewardsPayoutManager is deployed"); + return (false, "RewardsTreasury not set but RewardsPayoutManager is deployed"); + } + + // INVARIANT 8e: Emergency Module Recovery gate binding (optional — a + // deployment may seal without wiring recovery at all). Reads the + // vault's actual on-chain value, exactly like every other component + // check above, so a caller cannot claim a recovery gate is wired + // when it is not (or vice versa). When set, the manifest's declared + // version must match the gate's own MANIFEST_VERSION — sealing + // commits to the identity of the recovery policy, not just its + // address (review §23 full seal manifest). + if (vault.recoveryGate() != config.recoveryGate) { + return (false, "CoreVault.recoveryGate != config.recoveryGate"); + } + if (config.recoveryGate != address(0)) { + if (RecoveryGate(config.recoveryGate).MANIFEST_VERSION() != config.recoveryManifestVersion) { + return (false, "RecoveryGate.MANIFEST_VERSION != config.recoveryManifestVersion"); + } } // ───────────────────────────────────────────────────────────────────────── // INVARIANT 9: Strategy roles (if strategy is deployed) // ───────────────────────────────────────────────────────────────────────── if (config.strategy != address(0)) { - _verifyStrategyRoles(config); + (bool strategyOk, string memory strategyReason) = _verifyStrategyRoles(config); + if (!strategyOk) return (false, strategyReason); } // ───────────────────────────────────────────────────────────────────────── @@ -276,14 +390,14 @@ contract SystemSealer { if (config.deployer != address(0) && config.deployer != config.rootTimelock) { // Vault owner should not be deployer if (vault.owner() == config.deployer) { - revert InvariantViolation("Deployer still owns CoreVault"); + return (false, "Deployer still owns CoreVault"); } // Strategy admin should not be deployer if (config.strategy != address(0)) { AccessControl strategy = AccessControl(config.strategy); bytes32 adminRole = strategy.DEFAULT_ADMIN_ROLE(); if (strategy.hasRole(adminRole, config.deployer)) { - revert InvariantViolation("Deployer still has strategy DEFAULT_ADMIN_ROLE"); + return (false, "Deployer still has strategy DEFAULT_ADMIN_ROLE"); } } } @@ -292,7 +406,7 @@ contract SystemSealer { // INVARIANT 11: Dead deposit seeded (inflation attack hardening) // ───────────────────────────────────────────────────────────────────────── if (!IAdminModule(config.vault).isDeadDepositDone()) { - revert InvariantViolation("Dead deposit not seeded - inflation attack risk"); + return (false, "Dead deposit not seeded - inflation attack risk"); } // ───────────────────────────────────────────────────────────────────────── @@ -303,7 +417,7 @@ contract SystemSealer { // override check below proves nothing about the configuration the vault // actually reads at runtime. if (address(vault.params()) != config.globalConfig) { - revert InvariantViolation("GlobalConfig not bound to vault"); + return (false, "GlobalConfig not bound to vault"); } // ───────────────────────────────────────────────────────────────────────── @@ -314,41 +428,16 @@ contract SystemSealer { // overrides gets a deposit cap of ~0.00001 WETH and a near-zero minDeployAmount — // a bricked configuration. The override setters exist but nothing else enforces // their use, so require them here before the vault becomes unconfigurable. - _checkDecimalsOverrides(vault, gc, config.vault); - - // ───────────────────────────────────────────────────────────────────────── - // ALL INVARIANTS PASSED - COMPUTE CONFIG HASH AND SEAL ATOMICALLY - // ───────────────────────────────────────────────────────────────────────── - - // block.timestamp is intentionally excluded. Including it would make the - // hash non-deterministic between scheduleBatch() and executeBatch() (they - // run at different blocks separated by the full timelock delay). The TOCTOU - // protection comes from the address binding alone — every field here is a - // deploy-time constant that cannot change between schedule and execute. - bytes32 configHash = keccak256( - abi.encode( - config.vault, - config.rootTimelock, - config.guardian, - config.vetoer, - config.feeCollector, - config.strategy, - config.incentives, - config.incentivesEngine, - config.rewardsPayoutManager, - config.rewardsTreasury - ) - ); - - vault.sealBySealer(configHash); - - emit SystemSealedEvent(config.vault, msg.sender, configHash, block.timestamp); + return _verifyDecimalsOverrides(vault, gc, config.vault); } - /** - * @dev Verify strategy role assignments - */ - function _verifyStrategyRoles(SealConfig calldata config) internal view { + /// @dev Verify strategy role assignments. Non-reverting so both canSeal() + /// and verifyAndSeal() can share it via _verifyLiveState(). + function _verifyStrategyRoles(SealConfig calldata config) + internal + view + returns (bool ok, string memory reason) + { AccessControl strategy = AccessControl(config.strategy); bytes32 adminRole = strategy.DEFAULT_ADMIN_ROLE(); @@ -358,176 +447,50 @@ contract SystemSealer { // ROOT_TIMELOCK must have DEFAULT_ADMIN_ROLE if (!strategy.hasRole(adminRole, config.rootTimelock)) { - revert InvariantViolation("Strategy: ROOT_TIMELOCK missing DEFAULT_ADMIN_ROLE"); + return (false, "Strategy: ROOT_TIMELOCK missing DEFAULT_ADMIN_ROLE"); } // ROOT_TIMELOCK must have PARAM_ROLE if (!strategy.hasRole(paramRole, config.rootTimelock)) { - revert InvariantViolation("Strategy: ROOT_TIMELOCK missing PARAM_ROLE"); + return (false, "Strategy: ROOT_TIMELOCK missing PARAM_ROLE"); } // CoreVault must have CORE_ROLE if (!strategy.hasRole(coreRole, config.vault)) { - revert InvariantViolation("Strategy: CoreVault missing CORE_ROLE"); + return (false, "Strategy: CoreVault missing CORE_ROLE"); } // Guardian should have KEEPER_ROLE (backup) if (!strategy.hasRole(keeperRole, config.guardian)) { - revert InvariantViolation("Strategy: Guardian missing KEEPER_ROLE (backup)"); + return (false, "Strategy: Guardian missing KEEPER_ROLE (backup)"); } + + return (true, ""); } - /// @dev Reverts unless a non-6dp vault has all three decimals-sensitive GlobalConfig - /// overrides (VAULT_CAP, WITHDRAWAL, GOV_CAPS) set. 6dp vaults match the - /// GlobalConfig defaults and are exempt. - function _checkDecimalsOverrides(CoreVault vault, GlobalConfig gc, address vaultAddr) + /// @dev True unless a non-6dp vault is missing any of the three + /// decimals-sensitive GlobalConfig overrides (VAULT_CAP, WITHDRAWAL, + /// GOV_CAPS). 6dp vaults match the GlobalConfig defaults and are + /// exempt. Non-reverting so both canSeal() and verifyAndSeal() can + /// share it via _verifyLiveState(). + function _verifyDecimalsOverrides(CoreVault vault, GlobalConfig gc, address vaultAddr) internal view + returns (bool ok, string memory reason) { uint8 assetDecimals = IERC20Metadata(vault.asset()).decimals(); - if (assetDecimals == 6) return; + if (assetDecimals == 6) return (true, ""); if (!gc.hasOverride(vaultAddr, GlobalConfig.ParamType.VAULT_CAP)) { - revert InvariantViolation("Non-6dp vault missing VAULT_CAP override"); + return (false, "Non-6dp vault missing VAULT_CAP override"); } if (!gc.hasOverride(vaultAddr, GlobalConfig.ParamType.WITHDRAWAL)) { - revert InvariantViolation("Non-6dp vault missing WITHDRAWAL override"); + return (false, "Non-6dp vault missing WITHDRAWAL override"); } if (!gc.hasOverride(vaultAddr, GlobalConfig.ParamType.GOV_CAPS)) { - revert InvariantViolation("Non-6dp vault missing GOV_CAPS override"); - } - } - - // ═══════════════════════════════════════════════════════════════════════════════ - // VIEW FUNCTIONS - // ═══════════════════════════════════════════════════════════════════════════════ - - /** - * @notice Check if system would pass seal verification (dry run) - * @param config Configuration to verify - * @return valid True if all invariants pass - * @return reason Error message if validation fails - */ - function canSeal(SealConfig calldata config) - external - view - returns (bool valid, string memory reason) - { - CoreVault vault = CoreVault(payable(config.vault)); - - // Basic checks - if (vault.isSystemSealed()) return (false, "Already sealed"); - if (vault.owner() != config.rootTimelock) { - return (false, "CoreVault.owner != ROOT_TIMELOCK"); - } - if (vault.guardian() != config.guardian) { - return (false, "CoreVault.guardian != SAFE_GUARDIAN"); - } - if (vault.vetoer() != config.vetoer) return (false, "CoreVault.vetoer != SAFE_VETO"); - if (!vault.isRoutingFrozen()) return (false, "CoreVault.isRoutingFrozen != true"); - - // SelectorRegistry - address registryAddr = vault.selectorRegistry(); - if (registryAddr == address(0)) return (false, "SelectorRegistry not set"); - - // Check owner selectors - SelectorRegistry registry = SelectorRegistry(registryAddr); - bytes4[] memory ownerSelectors = registry.getOwnerSelectors(); - for (uint256 i = 0; i < ownerSelectors.length; i++) { - if (vault.roleOf(ownerSelectors[i]) != ROLE_OWNER) { - return (false, "Selector role mismatch"); - } - } - - // ComponentsTimelocked check - if (!IAdminModule(config.vault).isComponentsTimelocked()) { - return (false, "CoreVault.isComponentsTimelocked != true"); - } - - // FeeCollector (most critical - immutable) - if (FeeCollector(config.feeCollector).governor() != config.rootTimelock) { - return (false, "FeeCollector.governor != ROOT_TIMELOCK"); - } - - // GlobalConfig - if (GlobalConfig(config.globalConfig).governor() != config.rootTimelock) { - return (false, "GlobalConfig.governor != ROOT_TIMELOCK"); - } - - // StrategyRouter - if (StrategyRouter(config.strategyRouter).owner() != config.rootTimelock) { - return (false, "StrategyRouter.owner != ROOT_TIMELOCK"); - } - - // BufferManager - if (BufferManager(config.bufferManager).owner() != config.rootTimelock) { - return (false, "BufferManager.owner != ROOT_TIMELOCK"); - } - - // StrategyHealthRegistry - StrategyHealthRegistry hr = StrategyHealthRegistry(config.healthRegistry); - if (hr.owner() != config.rootTimelock) { - return (false, "HealthRegistry.owner != ROOT_TIMELOCK"); - } - if (hr.guardian() != config.guardian) { - return (false, "HealthRegistry.guardian != SAFE_GUARDIAN"); - } - - // Incentives (legacy, if deployed) - if (config.incentives != address(0)) { - if (Incentives(config.incentives).owner() != config.rootTimelock) { - return (false, "Incentives.owner != ROOT_TIMELOCK"); - } - } - - // IncentivesEngine v2 (if deployed) - if (config.incentivesEngine != address(0)) { - if (IncentivesEngine(config.incentivesEngine).governance() != config.rootTimelock) { - return (false, "IncentivesEngine.governance != ROOT_TIMELOCK"); - } - } - - // RewardsPayoutManager (if deployed) - if (config.rewardsPayoutManager != address(0)) { - if (IRewardsPayoutManager(config.rewardsPayoutManager).governance() != config.rootTimelock) { - return (false, "RewardsPayoutManager.governance != ROOT_TIMELOCK"); - } - } - - // RewardsTreasury must be funded before a live RewardsPayoutManager is sealed in. - // Reads the vault's actual on-chain value rather than trusting config.rewardsTreasury. - if (config.rewardsPayoutManager != address(0) && vault.rewardsTreasury() == address(0)) { - return (false, "RewardsTreasury not set but RewardsPayoutManager is deployed"); - } - - // Dead deposit (inflation attack hardening) - if (!IAdminModule(config.vault).isDeadDepositDone()) { - return (false, "Dead deposit not seeded"); - } - - // config.globalConfig must be the vault's live params provider, otherwise the - // override check below reads a config the vault never consults. - if (address(vault.params()) != config.globalConfig) { - return (false, "GlobalConfig not bound to vault"); - } - - // Non-6dp vault asset must have VAULT_CAP/WITHDRAWAL/GOV_CAPS overrides set — - // see _checkDecimalsOverrides in verifyAndSeal for why. - GlobalConfig gcView = GlobalConfig(config.globalConfig); - uint8 assetDecimals = IERC20Metadata(vault.asset()).decimals(); - if (assetDecimals != 6) { - if (!gcView.hasOverride(config.vault, GlobalConfig.ParamType.VAULT_CAP)) { - return (false, "Non-6dp vault missing VAULT_CAP override"); - } - if (!gcView.hasOverride(config.vault, GlobalConfig.ParamType.WITHDRAWAL)) { - return (false, "Non-6dp vault missing WITHDRAWAL override"); - } - if (!gcView.hasOverride(config.vault, GlobalConfig.ParamType.GOV_CAPS)) { - return (false, "Non-6dp vault missing GOV_CAPS override"); - } + return (false, "Non-6dp vault missing GOV_CAPS override"); } - // All checks passed return (true, ""); } } diff --git a/src/core/libraries/Events.sol b/src/core/libraries/Events.sol index 7bf8a5a..37ccfbe 100644 --- a/src/core/libraries/Events.sol +++ b/src/core/libraries/Events.sol @@ -63,6 +63,18 @@ library Events { event WithdrawalsPaused(); event WithdrawalsUnpaused(); + // --- Granular withdrawal circuit breakers (review §20/§21) --- + event InstantWithdrawalPaused(); + event InstantWithdrawalUnpaused(); + event QueuedRequestPaused(); + event QueuedRequestUnpaused(); + event EpochCloseFundPaused(); + event EpochCloseFundUnpaused(); + event FundedClaimPaused(); + event FundedClaimUnpaused(); + event ForceExitPaused(); + event ForceExitUnpaused(); + // --- Withdrawal rate limiting --- event MaxWithdrawalPerBlockUpdated(uint256 limit); event MaxWithdrawalPerTxUpdated(uint256 limit); @@ -207,6 +219,10 @@ library Events { event AuthorizedSealerSet(address indexed sealer); event SystemSealed(address indexed sealer, bytes32 configHash, uint256 timestamp); + // --- Emergency Module Recovery events (review §7-§14) --- + event RecoveryGateSet(address indexed gate); + event ModuleGroupRecovered(uint8 indexed groupId, address[] newModules); + // --- Module Authorization events --- event ModuleAuthorized(address indexed module, bool authorized); diff --git a/src/core/modules/ERC4626Module.sol b/src/core/modules/ERC4626Module.sol index 968a108..e7d6668 100644 --- a/src/core/modules/ERC4626Module.sol +++ b/src/core/modules/ERC4626Module.sol @@ -56,6 +56,10 @@ contract ERC4626Module { error Paused(); error DepositsPaused(); error WithdrawalsPaused(); + /// @dev Dedicated force-exit breaker (review §20: force exit must never be + /// blocked as a side effect of a generic emergency flag — it has its + /// own flag and is never touched by pauseAll()/guardianPause()). + error ForceExitPaused(); error ZeroAmount(); error ZeroAddress(); error DepositBelowMinimum(uint256 assets, uint256 minimum); @@ -190,7 +194,7 @@ contract ERC4626Module { // Blocked in: Funding, Starting, Matured, Closed, FundingFailed. _checkForceExitAllowed(FixedMaturityStorage.layout()); - _notPausedWithdrawals(); + _notPausedForceExit(); _enterNonReentrant(); if (assets == 0) revert ZeroAmount(); @@ -303,7 +307,7 @@ contract ERC4626Module { // FixedMaturity gate: same as forceWithdraw — only Active state or OpenEnded. _checkForceExitAllowed(FixedMaturityStorage.layout()); - _notPausedWithdrawals(); + _notPausedForceExit(); _enterNonReentrant(); if (receiver == address(0)) revert ZeroAddress(); @@ -757,10 +761,14 @@ contract ERC4626Module { if (flags & CoreStorage.FLAG_PAUSED_DEPOSITS != 0) revert DepositsPaused(); } - function _notPausedWithdrawals() internal view { - uint256 flags = CoreStorage.layout().packedFlags; - if (flags & CoreStorage.FLAG_PAUSED != 0) revert Paused(); - if (flags & CoreStorage.FLAG_PAUSED_WITHDRAWALS != 0) revert WithdrawalsPaused(); + /// @dev Force exit has its own dedicated breaker and is deliberately NOT + /// gated by FLAG_PAUSED or FLAG_PAUSED_WITHDRAWALS — review §20: + /// "Force exit ... should therefore not automatically disappear + /// simply because a generic emergency flag is active." + function _notPausedForceExit() internal view { + if (CoreStorage.layout().packedFlags & CoreStorage.FLAG_FORCE_EXIT_PAUSED != 0) { + revert ForceExitPaused(); + } } function _enterNonReentrant() internal { diff --git a/src/core/modules/EpochedQueueModule.sol b/src/core/modules/EpochedQueueModule.sol index 4103286..8c7fb1d 100644 --- a/src/core/modules/EpochedQueueModule.sol +++ b/src/core/modules/EpochedQueueModule.sol @@ -187,6 +187,18 @@ contract EpochedQueueModule { error EpochAlreadyFunded(); error InsufficientEscrow(); error ClaimTooSmall(); + // Granular withdrawal circuit breakers (review §20/§21) — previously this + // module had zero pause protection at all; guardianPause()/pauseAll() + // never reached it. FLAG_PAUSED_WITHDRAWALS is honored, in addition to the + // specific flag, only by instant settlement and epoch close/fund — NOT by + // queued-request creation or funded claims (see _notPausedQueuedRequest / + // _notPausedFundedClaim below for why). + // No InstantWithdrawalPaused error: requestInstantWithdrawal() treats a + // paused instant breaker as "instant unavailable" and silently falls back + // to the queue (see the instantAllowed check below), never reverts for it. + error QueuedRequestPaused(); + error EpochCloseFundPaused(); + error FundedClaimPaused(); // ========================================================================= // EVENTS (epoch lifecycle + claims) @@ -283,6 +295,7 @@ contract EpochedQueueModule { external returns (uint256 epochId, uint256 claimId) { + _notPausedQueuedRequest(); _enterNonReentrant(); (epochId, claimId) = _requestEpochWithdrawal(msg.sender, shares); _exitNonReentrant(); @@ -427,6 +440,7 @@ contract EpochedQueueModule { /// feeCollector in a single call (vs. per-claim in QueueModule). /// Opens a fresh epoch immediately so new submissions are not blocked. function closeCurrentEpoch() external { + _notPausedEpochCloseFund(); // Guarded for the same reason as the rest: it refreshes warm NAV and // batch-transfers fee shares out, both external calls, before writing // the epoch's locked price. @@ -500,6 +514,7 @@ contract EpochedQueueModule { /// Core improvement over QueueModule: a single external-call pull /// covers ALL claims in the epoch, not one pull per settled claim. function fundEpoch(uint256 epochId) external { + _notPausedEpochCloseFund(); // Guarded like every other state-changing entry point on this module. // This one calls out to the buffer manager and the strategy router // mid-body and then re-reads the hot balance to decide whether to mark @@ -626,6 +641,7 @@ contract EpochedQueueModule { /// too instead of reverting. A cursor pointing at a FUNDED epoch is /// therefore always recoverable without governance. function syncOldestUnfundedEpoch() external { + _notPausedEpochCloseFund(); _syncOldestUnfunded(EpochQueueStorage.layout()); } @@ -651,6 +667,7 @@ contract EpochedQueueModule { external returns (uint256 assets) { + _notPausedFundedClaim(); _enterNonReentrant(); EpochQueueStorage.Layout storage eq = EpochQueueStorage.layout(); @@ -698,6 +715,7 @@ contract EpochedQueueModule { external returns (uint256 totalAssets) { + _notPausedFundedClaim(); _enterNonReentrant(); EpochQueueStorage.Layout storage eq = EpochQueueStorage.layout(); @@ -748,6 +766,7 @@ contract EpochedQueueModule { /// @notice End epoch and crystallize performance fee. Permissionless. function endEpochCrystallize() external { + _notPausedEpochCloseFund(); _crystallize(); _updateNavSmooth(); } @@ -909,7 +928,16 @@ contract EpochedQueueModule { // checks pass) and hands it back so a successful settlement reuses it // below instead of a second _asset() call; failing fast still costs // zero extra calls. - (bool instantOk, address assetAddr) = _canInstant(gross, wp, core); + // + // When the instant-settlement breaker is tripped, treat instant as + // unavailable rather than reverting the whole call — this function's + // documented contract is "falls back to epoch queue if [instant] + // check fails", and review §19/§20 require exit *intent* to remain + // recordable even when a specific settlement mechanism is paused. + bool instantAllowed = CoreStorage.layout().packedFlags + & (CoreStorage.FLAG_PAUSED_WITHDRAWALS | CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED) == 0; + (bool instantOk, address assetAddr) = + instantAllowed ? _canInstant(gross, wp, core) : (false, address(0)); if (instantOk) { // Settle now @@ -934,7 +962,12 @@ contract EpochedQueueModule { epochId = 0; claimId = 0; } else { - // Fallback: enqueue in current epoch as standard claim. + // Fallback: enqueue in current epoch as standard claim. Gated + // separately by the queued-request breaker (owner-only, + // exceptional — review §20) so pausing instant settlement alone + // never blocks this fallback, and so this entry point can't be + // used to route around pauseQueuedRequestOnly() either. + _notPausedQueuedRequest(); // Calls the internal helper directly (never `this.foo()`) so the // claim is correctly attributed to msg.sender, not to the vault. (epochId, claimId) = _requestEpochWithdrawal(msg.sender, shares); @@ -1123,6 +1156,48 @@ contract EpochedQueueModule { CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_REENTRANCY_LOCKED; } + // ========================================================================= + // INTERNAL: GRANULAR WITHDRAWAL BREAKERS (review §20/§21) + // ========================================================================= + // Note: there is no _notPausedInstantWithdrawal() revert-style helper — + // requestInstantWithdrawal() checks FLAG_INSTANT_WITHDRAWAL_PAUSED inline + // and treats "paused" as "instant unavailable" (forcing its existing + // queue-fallback branch) rather than reverting the whole call. See the + // instantAllowed check at the _canInstant() call site. + + /// @dev Gates only the creation of NEW queued-exit requests. Cancelling an + /// existing request is never gated by this — review §20 does not + /// approve blocking new requests as a routine tool, but a user's + /// ability to withdraw an already-submitted request must stay open. + /// Deliberately NOT included under FLAG_PAUSED_WITHDRAWALS: exit + /// *intent* must remain recordable even while settlement is paused + /// (review §19) — only the dedicated, exceptional + /// pauseQueuedRequestOnly()/pauseAll() can reach this. + function _notPausedQueuedRequest() internal view { + if (CoreStorage.layout().packedFlags & CoreStorage.FLAG_QUEUED_REQUEST_PAUSED != 0) { + revert QueuedRequestPaused(); + } + } + + function _notPausedEpochCloseFund() internal view { + uint256 flags = CoreStorage.layout().packedFlags; + if (flags & (CoreStorage.FLAG_PAUSED_WITHDRAWALS | CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED) != 0) { + revert EpochCloseFundPaused(); + } + } + + /// @dev Funded claims are permissionless by default (review §20: "no + /// general administrative capability to arbitrarily prevent funded + /// users from claiming"). Deliberately NOT included under + /// FLAG_PAUSED_WITHDRAWALS or FLAG_PAUSED — only the dedicated, + /// exceptional pauseFundedClaimOnly() can reach this, never + /// pauseAll()/guardianPause()/pauseWithdrawalsOnly(). + function _notPausedFundedClaim() internal view { + if (CoreStorage.layout().packedFlags & CoreStorage.FLAG_FUNDED_CLAIM_PAUSED != 0) { + revert FundedClaimPaused(); + } + } + // ========================================================================= // INTERNAL: INSTANT SETTLEMENT CHECK // ========================================================================= diff --git a/src/core/storage/CoreStorage.sol b/src/core/storage/CoreStorage.sol index 637f3ad..4a31b69 100644 --- a/src/core/storage/CoreStorage.sol +++ b/src/core/storage/CoreStorage.sol @@ -35,6 +35,15 @@ library CoreStorage { uint256 internal constant FLAG_FEES_INITIALIZED = 1 << 11; uint256 internal constant FLAG_PERF_INITIALIZED = 1 << 12; + // Granular withdrawal circuit breakers (review §20/§21: FLAG_PAUSED_WITHDRAWALS is too + // coarse — it must not be able to block funded claims or new queued-exit requests, and + // force exit must never be gated by a generic emergency flag at all). + uint256 internal constant FLAG_INSTANT_WITHDRAWAL_PAUSED = 1 << 13; + uint256 internal constant FLAG_QUEUED_REQUEST_PAUSED = 1 << 14; + uint256 internal constant FLAG_EPOCH_CLOSE_FUND_PAUSED = 1 << 15; + uint256 internal constant FLAG_FUNDED_CLAIM_PAUSED = 1 << 16; + uint256 internal constant FLAG_FORCE_EXIT_PAUSED = 1 << 17; + struct Layout { // Addresses (each 20 bytes, separate slots for simplicity) IParamsProvider params; @@ -84,6 +93,10 @@ library CoreStorage { // Selector registry for role validation (set once, immutable) address selectorRegistry; + // Emergency Module Recovery gate (set once, immutable) — the sole + // authorized caller of recoverModuleGroup(). Review §7/§8. + address recoveryGate; + // System sealer binding - set atomically by sealBySealer() (called from // SystemSealer.verifyAndSeal()) alongside FLAG_SYSTEM_SEALED, in the same call // that verifies the config hash. Retained post-seal as an audit record. diff --git a/src/governance/RecoveryGate.sol b/src/governance/RecoveryGate.sol new file mode 100644 index 0000000..437bb54 --- /dev/null +++ b/src/governance/RecoveryGate.sol @@ -0,0 +1,415 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +import { CoreVault } from "../core/CoreVault.sol"; +import { SelectorLib } from "../core/libraries/SelectorLib.sol"; + +/** + * @title RecoveryGate + * @notice Emergency Module Recovery — the narrow, immutable alternative to + * generic post-seal upgradeability approved by the architecture + * review (Multyr Core Upgradeability, Emergency Recovery & Incident + * Response Architecture Review, snapshot 0bab749, §§5-14). + * + * WHAT THIS IS NOT: + * Not a general-purpose upgrade mechanism. Not callable by anyone but + * ROOT_TIMELOCK to propose, SECURITY_APPROVER to approve, and CoreVault's + * own vetoer to cancel. Cannot add selectors, cannot relax roles, cannot + * touch CoreVault's shell, governance, or this contract's own policy. + * + * WHAT THIS IS: + * A dedicated, separate entry point (review §7) that lets ROOT_TIMELOCK + * replace the module implementation behind one of four pre-approved, + * economically-isolated selector groups (review §9), after an immutable + * minimum delay (review §12), independent security approval bound to an + * exact digest (review §13), and subject to cancellation by CoreVault's + * vetoer at any point before execution (review §14). + * + * RECOVERABLE GROUPS (review §9 — economically isolated execution modules; + * AdminModule's governance/sealing/authorization surface and every direct + * CoreVault function are permanently out of scope, not merely excluded from + * a whitelist — they are not moduleOf-routed selectors and this contract has + * no path to reach them): + * 0 = EPOCH_QUEUE_GROUP — EpochedQueueModule (write + view selectors) + * 1 = ERC4626_GROUP — ERC4626Module + * 2 = LIQUIDITY_GROUP — LiquidityOpsModule + * 3 = FIXED_MATURITY_GROUP — FixedMaturityModule + * Selector sets are read directly from SelectorLib — the same source of + * truth CoreVault's own deployment wiring uses — so there is no second, + * independently-maintained selector registry to drift out of sync. + * + * IMMUTABLE RECOVERY POLICY (review §8 — fixed at construction, no setters + * of any kind on delay, cooldown, vault, or root timelock): + * - minDelay: hard floor of 14 days, enforced in the constructor itself + * (a misconfigured deployment cannot even be deployed with less). + * - cooldown: minimum gap between two completed recoveries of the same + * group, preventing a group from being salami-sliced through + * repeated recoveries that each individually pass review but + * cumulatively amount to continuous evolution. + * - securityApprover is the one field that IS rotatable — but only by + * ROOT_TIMELOCK, and only subject to the same minDelay as a recovery + * itself, so a compromised timelock cannot fast-track a friendly + * approver into place in time to matter. This resolves the open + * question raised in docs/developer-response-recovery-architecture + * §6 in favor of rotatability over permanent key-loss risk. + * + * CAVEAT (review §4): restricting recovery to existing selectors, unchanged + * roles, and pre-approved groups does not mathematically prove a replacement + * module only repairs a bug — a delegatecall-executed module can still alter + * economic behavior while using identical selectors and roles. This contract + * enforces the procedural restrictions; it is not cryptographic proof of + * semantic equivalence. See docs/recovery.md. + */ +contract RecoveryGate { + // ═══════════════════════════════════════════════════════════════════════════════ + // GROUP IDS + // ═══════════════════════════════════════════════════════════════════════════════ + uint8 public constant EPOCH_QUEUE_GROUP = 0; + uint8 public constant ERC4626_GROUP = 1; + uint8 public constant LIQUIDITY_GROUP = 2; + uint8 public constant FIXED_MATURITY_GROUP = 3; + uint8 public constant GROUP_COUNT = 4; + + /// @dev Committed into every proposal digest so an approval can never be + /// replayed against a policy the approver did not actually review. + uint256 public constant MANIFEST_VERSION = 1; + + /// @dev Matches the execution-grace-window idiom already used by + /// TimelockLib/AdminModule elsewhere in this codebase: a proposal + /// that becomes executable but is never executed expires rather + /// than remaining eternally executable. + uint64 public constant EXECUTION_WINDOW = 7 days; + + // ═══════════════════════════════════════════════════════════════════════════════ + // IMMUTABLE POLICY + // ═══════════════════════════════════════════════════════════════════════════════ + address public immutable vault; + address public immutable rootTimelock; + uint64 public immutable minDelay; + uint64 public immutable cooldown; + + // ═══════════════════════════════════════════════════════════════════════════════ + // STORAGE + // ═══════════════════════════════════════════════════════════════════════════════ + address public securityApprover; + + struct Proposal { + bytes32 digest; + uint64 eta; + bool approved; + bool exists; + address[] newModules; + } + + mapping(uint8 => Proposal) public proposals; + mapping(uint8 => uint64) public lastRecoveryCompletedAt; + + struct PendingApprover { + address newApprover; + uint64 eta; + bool exists; + } + + PendingApprover public pendingApprover; + + // ═══════════════════════════════════════════════════════════════════════════════ + // ERRORS + // ═══════════════════════════════════════════════════════════════════════════════ + error DelayTooShort(); + error ZeroAddress(); + error InvalidGroup(); + error NotRootTimelock(); + error NotSecurityApprover(); + error NotVetoer(); + error WrongSelectorCount(); + error PendingProposalExists(); + error NoPendingProposal(); + error DigestMismatch(); + error EtaNotReached(); + error EtaExpired(); + error NotApproved(); + error CooldownActive(); + + // ═══════════════════════════════════════════════════════════════════════════════ + // EVENTS + // ═══════════════════════════════════════════════════════════════════════════════ + event RecoveryProposed(uint8 indexed groupId, bytes32 digest, uint64 eta, bytes32 reasonRef); + event RecoveryApproved(uint8 indexed groupId, bytes32 digest); + event RecoveryVetoed(uint8 indexed groupId, bytes32 digest); + event RecoveryExecuted(uint8 indexed groupId, bytes32 digest, address[] newModules); + event ApproverChangeProposed(address indexed newApprover, uint64 eta); + event ApproverChangeExecuted(address indexed newApprover); + event ApproverChangeVetoed(address indexed newApprover); + + // ═══════════════════════════════════════════════════════════════════════════════ + // CONSTRUCTOR + // ═══════════════════════════════════════════════════════════════════════════════ + + /// @param _minDelay Minimum recovery delay. Reverts if under 14 days — + /// review §12's recommended floor, enforced structurally rather + /// than by policy so a misconfigured deployment cannot exist. + constructor( + address _vault, + address _rootTimelock, + address _securityApprover, + uint64 _minDelay, + uint64 _cooldown + ) { + if (_vault == address(0) || _rootTimelock == address(0) || _securityApprover == address(0)) { + revert ZeroAddress(); + } + if (_minDelay < 14 days) revert DelayTooShort(); + + vault = _vault; + rootTimelock = _rootTimelock; + securityApprover = _securityApprover; + minDelay = _minDelay; + cooldown = _cooldown; + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // MODIFIERS + // ═══════════════════════════════════════════════════════════════════════════════ + + modifier onlyRootTimelock() { + if (msg.sender != rootTimelock) revert NotRootTimelock(); + _; + } + + modifier onlySecurityApprover() { + if (msg.sender != securityApprover) revert NotSecurityApprover(); + _; + } + + /// @dev Reads CoreVault.vetoer() live on every call rather than caching + /// it — CoreVault is the single source of truth for who the vetoer + /// is, so this can never drift from it. + modifier onlyVetoer() { + if (msg.sender != CoreVault(payable(vault)).vetoer()) revert NotVetoer(); + _; + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // RECOVERY LIFECYCLE + // ═══════════════════════════════════════════════════════════════════════════════ + + /// @notice Propose replacing the module backing every selector in + /// `groupId` with `newModules` (one address per selector, same + /// order as `_selectorsForGroup`). Only ROOT_TIMELOCK — schedule + /// this through the timelock's own delay exactly as + /// SystemSealer.verifyAndSeal() is scheduled; this contract's + /// minDelay clock then runs on top of, not instead of, that. + function propose(uint8 groupId, address[] calldata newModules, bytes32 reasonRef) + external + onlyRootTimelock + { + bytes4[] memory selectors = _selectorsForGroup(groupId); + if (newModules.length != selectors.length) revert WrongSelectorCount(); + + Proposal storage p = proposals[groupId]; + if (p.exists && !_isExpired(p)) revert PendingProposalExists(); + + if ( + lastRecoveryCompletedAt[groupId] != 0 + && block.timestamp < uint256(lastRecoveryCompletedAt[groupId]) + cooldown + ) { + revert CooldownActive(); + } + + bytes32 digest = _computeDigest(groupId, selectors, newModules, reasonRef); + uint64 eta = uint64(block.timestamp) + minDelay; + + p.digest = digest; + p.eta = eta; + p.approved = false; + p.exists = true; + p.newModules = newModules; + + emit RecoveryProposed(groupId, digest, eta, reasonRef); + } + + /// @notice Approve the currently pending proposal for `groupId`. Must be + /// called with the exact digest being approved — if `propose()` + /// is called again for the same group before this executes, the + /// digest changes and this approval is silently invalidated + /// (review §13: "any modification invalidates previous + /// approval"). + function approve(uint8 groupId, bytes32 digest) external onlySecurityApprover { + Proposal storage p = proposals[groupId]; + if (!p.exists || _isExpired(p)) revert NoPendingProposal(); + if (p.digest != digest) revert DigestMismatch(); + + p.approved = true; + emit RecoveryApproved(groupId, digest); + } + + /// @notice Cancel the pending proposal for `groupId`. Cancellation-only — + /// the vetoer has no other capability on this contract, so it + /// structurally cannot propose, approve, or execute (review §14). + function vetoCancel(uint8 groupId) external onlyVetoer { + Proposal storage p = proposals[groupId]; + if (!p.exists) revert NoPendingProposal(); + + bytes32 digest = p.digest; + delete proposals[groupId]; + emit RecoveryVetoed(groupId, digest); + } + + /// @notice Execute an approved, matured, non-vetoed proposal. Open + /// caller — mirrors acceptRouter()/acceptFeeParams() already + /// being open in AdminModule once their own conditions are met. + function execute(uint8 groupId) external { + Proposal storage p = proposals[groupId]; + if (!p.exists) revert NoPendingProposal(); + if (block.timestamp < p.eta) revert EtaNotReached(); + if (block.timestamp > uint256(p.eta) + EXECUTION_WINDOW) revert EtaExpired(); + if (!p.approved) revert NotApproved(); + + bytes32 digest = p.digest; + address[] memory newModules = p.newModules; + + delete proposals[groupId]; + lastRecoveryCompletedAt[groupId] = uint64(block.timestamp); + + CoreVault(payable(vault)).recoverModuleGroup(groupId, newModules); + + emit RecoveryExecuted(groupId, digest, newModules); + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // SECURITY APPROVER ROTATION + // ═══════════════════════════════════════════════════════════════════════════════ + // The one rotatable field in an otherwise immutable policy (see contract + // NatSpec). Subject to the same minDelay and the same vetoer as a + // recovery itself, so compromising ROOT_TIMELOCK cannot install a + // friendly approver in time to matter for any recovery already in flight. + + function proposeApproverChange(address newApprover) external onlyRootTimelock { + if (newApprover == address(0)) revert ZeroAddress(); + if (pendingApprover.exists && !_isApproverChangeExpired()) revert PendingProposalExists(); + + uint64 eta = uint64(block.timestamp) + minDelay; + pendingApprover = PendingApprover({ newApprover: newApprover, eta: eta, exists: true }); + + emit ApproverChangeProposed(newApprover, eta); + } + + function executeApproverChange() external { + if (!pendingApprover.exists) revert NoPendingProposal(); + if (block.timestamp < pendingApprover.eta) revert EtaNotReached(); + if (block.timestamp > uint256(pendingApprover.eta) + EXECUTION_WINDOW) revert EtaExpired(); + + address newApprover = pendingApprover.newApprover; + delete pendingApprover; + securityApprover = newApprover; + + emit ApproverChangeExecuted(newApprover); + } + + function vetoApproverChange() external onlyVetoer { + if (!pendingApprover.exists) revert NoPendingProposal(); + address rejected = pendingApprover.newApprover; + delete pendingApprover; + emit ApproverChangeVetoed(rejected); + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // VIEWS + // ═══════════════════════════════════════════════════════════════════════════════ + + /// @notice The exact selector set a given group covers, in the same + /// order `propose()`/`execute()` expect `newModules` to match. + /// Sourced directly from SelectorLib — no second registry. + function selectorsForGroup(uint8 groupId) external pure returns (bytes4[] memory) { + return _selectorsForGroup(groupId); + } + + function pendingProposal(uint8 groupId) + external + view + returns (bytes32 digest, uint64 eta, bool approved, bool exists) + { + Proposal storage p = proposals[groupId]; + return (p.digest, p.eta, p.approved, p.exists); + } + + // ═══════════════════════════════════════════════════════════════════════════════ + // INTERNAL + // ═══════════════════════════════════════════════════════════════════════════════ + + function _isExpired(Proposal storage p) internal view returns (bool) { + return block.timestamp > uint256(p.eta) + EXECUTION_WINDOW; + } + + function _isApproverChangeExpired() internal view returns (bool) { + return block.timestamp > uint256(pendingApprover.eta) + EXECUTION_WINDOW; + } + + function _selectorsForGroup(uint8 groupId) internal pure returns (bytes4[] memory) { + if (groupId == EPOCH_QUEUE_GROUP) { + return _concat(SelectorLib.getQueueModuleSelectors(), SelectorLib.getQueueModuleViewSelectors()); + } + if (groupId == ERC4626_GROUP) { + return SelectorLib.getERC4626ModuleSelectors(); + } + if (groupId == LIQUIDITY_GROUP) { + return SelectorLib.getLiquidityOpsModuleSelectors(); + } + if (groupId == FIXED_MATURITY_GROUP) { + return SelectorLib.getFixedMaturityModuleSelectors(); + } + revert InvalidGroup(); + } + + function _concat(bytes4[] memory a, bytes4[] memory b) internal pure returns (bytes4[] memory out) { + out = new bytes4[](a.length + b.length); + for (uint256 i; i < a.length; ++i) out[i] = a[i]; + for (uint256 i; i < b.length; ++i) out[a.length + i] = b[i]; + } + + /// @dev Digest fields per review §13: vault, chain ID, module group, + /// selector set, current implementation codehash, proposed + /// implementation address + codehash, manifest version, + /// reason/reference identifier. "Unchanged role mapping" is not a + /// digest field because it is structurally impossible for a + /// recovery to change roles at all — recoverModuleGroup() never + /// writes roleOf (see CoreVault.sol), so there is nothing here that + /// could vary. + function _computeDigest( + uint8 groupId, + bytes4[] memory selectors, + address[] calldata newModules, + bytes32 reasonRef + ) internal view returns (bytes32) { + CoreVault v = CoreVault(payable(vault)); + uint256 len = selectors.length; + + address[] memory currentModules = new address[](len); + bytes32[] memory currentCodehashes = new bytes32[](len); + for (uint256 i; i < len; ++i) { + address cur = v.moduleOf(selectors[i]); + currentModules[i] = cur; + currentCodehashes[i] = cur.codehash; + } + + bytes32[] memory newCodehashes = new bytes32[](newModules.length); + for (uint256 i; i < newModules.length; ++i) { + newCodehashes[i] = newModules[i].codehash; + } + + return keccak256( + abi.encode( + vault, + block.chainid, + groupId, + selectors, + currentModules, + currentCodehashes, + newModules, + newCodehashes, + MANIFEST_VERSION, + reasonRef + ) + ); + } +} diff --git a/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol b/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol new file mode 100644 index 0000000..e8b331d --- /dev/null +++ b/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// INCIDENT SIMULATION — CoreVault shell defect +// +// Review §36: "Migration Remains Mandatory for Shell Defects". Emergency +// Module Recovery must NOT be capable of repairing a defect in CoreVault's +// direct functions, the fallback dispatcher, core storage, the recovery +// entry point itself, or a critical immutable governance binding — the +// correct (and only) remedy for those is migration to a new vault. +// +// This is a NEGATIVE test suite: every scenario below is an attempt to reach +// shell-level state through the recovery path, and every one must fail — +// not because of a policy check that could theoretically be misconfigured, +// but because there is structurally no path from RecoveryGate/ +// recoverModuleGroup to any of this state at all. +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { EpochedQueueModule } from "../../src/core/modules/EpochedQueueModule.sol"; +import { AdminModule } from "../../src/core/modules/AdminModule.sol"; +import { FeeCollector } from "../../src/core/modules/FeeCollector.sol"; +import { GlobalConfig } from "../../src/core/config/GlobalConfig.sol"; +import { SelectorRegistry } from "../../src/core/libraries/SelectorRegistry.sol"; +import { SelectorLib } from "../../src/core/libraries/SelectorLib.sol"; +import { RecoveryGate } from "../../src/governance/RecoveryGate.sol"; +import { IAdminModule } from "../../src/interfaces/IAdminModule.sol"; +import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; +import { ERC20Mock } from "../../src/mocks/ERC20Mock.sol"; + +contract CoreVaultShellDefect_Unreachable is Test { + uint256 constant TIMELOCK_DELAY = 2 days; + uint64 constant MIN_DELAY = 14 days; + uint64 constant COOLDOWN = 30 days; + + address internal deployer; + address internal guardian; + address internal vetoer; + address internal treasury; + address internal securityApprover; + address internal attacker; + + ERC20Mock internal usdc; + IncentivesTimelock internal rootTimelock; + CoreVault internal vault; + RecoveryGate internal gate; + + function setUp() public { + deployer = makeAddr("deployer"); + guardian = makeAddr("guardian"); + vetoer = makeAddr("vetoer"); + treasury = makeAddr("treasury"); + securityApprover = makeAddr("securityApprover"); + attacker = makeAddr("attacker"); + + vm.startPrank(deployer); + + usdc = new ERC20Mock("USD Coin", "USDC", 6); + + address[] memory proposers = new address[](1); + address[] memory executors = new address[](1); + proposers[0] = deployer; + executors[0] = deployer; + rootTimelock = new IncentivesTimelock(TIMELOCK_DELAY, proposers, executors, deployer); + + FeeCollector feeCollector = + new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000); + GlobalConfig globalConfig = + new GlobalConfig(address(rootTimelock), 50, 100, 2000, 0, 10, 500, 3600, 3600); + + vault = new CoreVault( + IERC20Metadata(address(usdc)), "Vault", "V", deployer, address(feeCollector), address(globalConfig) + ); + + SelectorRegistry selectorRegistry = new SelectorRegistry(); + vault.setSelectorRegistry(address(selectorRegistry)); + + AdminModule am = new AdminModule(); + bytes4[] memory s = SelectorLib.getAdminModuleOwnerSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_OWNER); + + gate = new RecoveryGate(address(vault), address(rootTimelock), securityApprover, MIN_DELAY, COOLDOWN); + vault.setRecoveryGate(address(gate)); + vault.freezeRouting(); + + vault.beginOwnerTransfer(address(rootTimelock)); + vm.stopPrank(); + + vm.prank(address(rootTimelock)); + vault.acceptOwnerTransfer(); + } + + // ══════════════════════════════════════════════════════════════════════════ + // No group ID reaches AdminModule's governance/sealing surface + // ══════════════════════════════════════════════════════════════════════════ + + function test_noRecoveryGroup_coversAnyAdminModuleOwnerSelector() public view { + bytes4[] memory adminSelectors = SelectorLib.getAdminModuleOwnerSelectors(); + + for (uint8 groupId; groupId < gate.GROUP_COUNT(); ++groupId) { + bytes4[] memory groupSelectors = gate.selectorsForGroup(groupId); + for (uint256 i; i < adminSelectors.length; ++i) { + for (uint256 j; j < groupSelectors.length; ++j) { + assertTrue( + adminSelectors[i] != groupSelectors[j], + "no recoverable group may contain an AdminModule owner selector" + ); + } + } + } + } + + /// @dev Confirms the negative directly, not just by absence-of-overlap: + /// the vault's actual live AdminModule routing is completely + /// unaffected by recovering every other group. + function test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup() public { + bytes4[] memory adminSelectors = SelectorLib.getAdminModuleOwnerSelectors(); + address adminModuleBefore = vault.moduleOf(adminSelectors[0]); + + // Recover EPOCH_QUEUE_GROUP (unwired in this minimal fixture — + // recovering an unwired group is still valid: it just sets moduleOf + // from address(0) to the new address, same as normal) and confirm + // AdminModule's routing is completely untouched. + EpochedQueueModule qm = new EpochedQueueModule(); + bytes4[] memory queueSelectors = gate.selectorsForGroup(0); + address[] memory queueModules = new address[](queueSelectors.length); + for (uint256 i; i < queueModules.length; ++i) queueModules[i] = address(qm); + + vm.prank(address(rootTimelock)); + gate.propose(0, queueModules, "shell-defect-sim"); + (bytes32 digest,,,) = gate.pendingProposal(0); + vm.prank(securityApprover); + gate.approve(0, digest); + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.execute(0); + + assertEq( + vault.moduleOf(adminSelectors[0]), + adminModuleBefore, + "AdminModule routing untouched by recovering an unrelated group" + ); + } + + // ══════════════════════════════════════════════════════════════════════════ + // recoverModuleGroup rejects any group ID outside the four defined ones — + // there is no "escape hatch" group that reaches CoreVault's own functions. + // ══════════════════════════════════════════════════════════════════════════ + + function test_recoverModuleGroup_rejectsOutOfRangeGroupId() public { + address[] memory newModules = new address[](1); + newModules[0] = makeAddr("maliciousModule"); + + vm.prank(address(gate)); + vm.expectRevert(CoreVault.InvalidRecoveryGroup.selector); + vault.recoverModuleGroup(4, newModules); + + vm.prank(address(gate)); + vm.expectRevert(CoreVault.InvalidRecoveryGroup.selector); + vault.recoverModuleGroup(255, newModules); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Direct CoreVault functions are not moduleOf-routed at all — recovery + // has no selector to target them with, regardless of group ID. + // ══════════════════════════════════════════════════════════════════════════ + + function test_directCoreVaultFunctions_areNeverModuleOfRouted() public view { + // setModule, freezeRouting, pauseAll, setRecoveryGate, and + // setSelectorRegistry are defined directly on CoreVault, dispatched + // by explicit function selectors in the contract itself, not via the + // fallback()/moduleOf() mechanism recoverModuleGroup writes to. + // Confirm none of their selectors appear in any recoverable group. + bytes4[] memory shellSelectors = new bytes4[](5); + shellSelectors[0] = CoreVault.setModule.selector; + shellSelectors[1] = CoreVault.freezeRouting.selector; + shellSelectors[2] = CoreVault.pauseAll.selector; + shellSelectors[3] = CoreVault.setRecoveryGate.selector; + shellSelectors[4] = CoreVault.setSelectorRegistry.selector; + + for (uint8 groupId; groupId < gate.GROUP_COUNT(); ++groupId) { + bytes4[] memory groupSelectors = gate.selectorsForGroup(groupId); + for (uint256 i; i < shellSelectors.length; ++i) { + for (uint256 j; j < groupSelectors.length; ++j) { + assertTrue( + shellSelectors[i] != groupSelectors[j], + "no recoverable group may contain a direct CoreVault shell function" + ); + } + } + } + } + + // ══════════════════════════════════════════════════════════════════════════ + // The recovery policy itself (RecoveryGate's own immutable fields) cannot + // be reached through recovery — there is no group that recovers the + // recovery mechanism. + // ══════════════════════════════════════════════════════════════════════════ + + function test_recoveryGate_hasNoSelfModificationPath() public { + // RecoveryGate has no setter for vault/rootTimelock/minDelay/cooldown + // at all (immutable), and setRecoveryGate() on CoreVault is set-once. + // An attacker in control of ROOT_TIMELOCK cannot even attempt to + // rewire recovery to a hostile gate post-seal. + vm.prank(address(rootTimelock)); + vm.expectRevert(CoreVault.RecoveryGateAlreadySet.selector); + vault.setRecoveryGate(attacker); + } +} diff --git a/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol b/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol new file mode 100644 index 0000000..43dc7e4 --- /dev/null +++ b/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// INCIDENT SIMULATION — ROOT_TIMELOCK compromise and Guardian compromise +// +// Neither RecoveryGate nor CoreVault's pause matrix assume any single +// privileged actor is fully trustworthy forever. This suite proves the +// actual bound on what a compromise of each one can do, rather than +// asserting it in prose: +// +// - A compromised ROOT_TIMELOCK can propose a malicious recovery, but +// cannot single-handedly execute it — independent approval AND the +// minDelay window both stand between proposal and effect, and the +// vetoer (a distinct key) can cancel it at any point in that window. +// - A compromised Guardian can trip exactly two withdrawal breakers plus +// deposits, rate-limited by a cooldown, and can reach nothing on +// RecoveryGate at all (review §3.3 — fast to restrict, never +// constructive). +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { EpochedQueueModule } from "../../src/core/modules/EpochedQueueModule.sol"; +import { AdminModule } from "../../src/core/modules/AdminModule.sol"; +import { ERC4626Module } from "../../src/core/modules/ERC4626Module.sol"; +import { LiquidityOpsModule } from "../../src/core/modules/LiquidityOpsModule.sol"; +import { FeeCollector } from "../../src/core/modules/FeeCollector.sol"; +import { GlobalConfig } from "../../src/core/config/GlobalConfig.sol"; +import { SelectorRegistry } from "../../src/core/libraries/SelectorRegistry.sol"; +import { SelectorLib } from "../../src/core/libraries/SelectorLib.sol"; +import { RecoveryGate } from "../../src/governance/RecoveryGate.sol"; +import { IAdminModule } from "../../src/interfaces/IAdminModule.sol"; +import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; +import { ERC20Mock } from "../../src/mocks/ERC20Mock.sol"; + +contract GovernanceCompromise_BlastRadius is Test { + uint256 constant TIMELOCK_DELAY = 2 days; + uint64 constant MIN_DELAY = 14 days; + uint64 constant COOLDOWN = 30 days; + uint8 constant EPOCH_QUEUE_GROUP = 0; + + address internal deployer; + address internal guardian; + address internal vetoer; + address internal treasury; + address internal securityApprover; + address internal randomAttacker; + + ERC20Mock internal usdc; + IncentivesTimelock internal rootTimelock; + CoreVault internal vault; + RecoveryGate internal gate; + EpochedQueueModule internal queueModuleV1; + + function setUp() public { + deployer = makeAddr("deployer"); + guardian = makeAddr("guardian"); + vetoer = makeAddr("vetoer"); + treasury = makeAddr("treasury"); + securityApprover = makeAddr("securityApprover"); + randomAttacker = makeAddr("randomAttacker"); + + vm.startPrank(deployer); + + usdc = new ERC20Mock("USD Coin", "USDC", 6); + + address[] memory proposers = new address[](1); + address[] memory executors = new address[](1); + proposers[0] = deployer; + executors[0] = deployer; + rootTimelock = new IncentivesTimelock(TIMELOCK_DELAY, proposers, executors, deployer); + + FeeCollector feeCollector = + new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000); + GlobalConfig globalConfig = + new GlobalConfig(address(rootTimelock), 50, 100, 2000, 0, 10, 500, 3600, 3600); + + vault = new CoreVault( + IERC20Metadata(address(usdc)), "Vault", "V", deployer, address(feeCollector), address(globalConfig) + ); + + SelectorRegistry selectorRegistry = new SelectorRegistry(); + vault.setSelectorRegistry(address(selectorRegistry)); + + queueModuleV1 = new EpochedQueueModule(); + AdminModule am = new AdminModule(); + ERC4626Module e4626 = new ERC4626Module(); + LiquidityOpsModule lo = new LiquidityOpsModule(); + + bytes4[] memory s; + s = SelectorLib.getQueueModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(queueModuleV1), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getQueueModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(queueModuleV1), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getAdminModuleOwnerSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_OWNER); + s = SelectorLib.getAdminModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getERC4626ModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(e4626), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getLiquidityOpsModuleSelectors(); + for (uint256 i; i < s.length; ++i) { + uint8 role = s[i] == LiquidityOpsModule.deployToStrategiesWithPlan.selector + ? SelectorLib.ROLE_OWNER_OR_GUARDIAN + : SelectorLib.ROLE_PUBLIC; + vault.setModule(s[i], address(lo), role); + } + + IAdminModule(address(vault)).setEcosystem(IAdminModule.EcosystemConfig({ + bufferManager: makeAddr("bufferManager"), + strategyRouter: makeAddr("strategyRouter"), + healthRegistry: address(0), + incentives: address(0), + guardian: guardian, + vetoer: vetoer + })); + + gate = new RecoveryGate(address(vault), address(rootTimelock), securityApprover, MIN_DELAY, COOLDOWN); + vault.setRecoveryGate(address(gate)); + vault.freezeRouting(); + + vault.beginOwnerTransfer(address(rootTimelock)); + vm.stopPrank(); + + vm.prank(address(rootTimelock)); + vault.acceptOwnerTransfer(); + } + + // ══════════════════════════════════════════════════════════════════════════ + // ROOT_TIMELOCK compromise: a malicious proposal alone cannot execute + // ══════════════════════════════════════════════════════════════════════════ + + /// @dev Simulates a compromised ROOT_TIMELOCK (e.g. a multisig with a + /// stolen signer threshold) proposing a hostile module. Two + /// independent controls — the security approver and the vetoer — + /// each individually stop it from ever taking effect. + function test_compromisedRootTimelock_cannotSingleHandedlyExecuteRecovery() public { + address hostileModule = makeAddr("hostileModule"); + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory hostileModules = new address[](groupSelectors.length); + for (uint256 i; i < hostileModules.length; ++i) hostileModules[i] = hostileModule; + + // The compromised ROOT_TIMELOCK can propose... + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, hostileModules, "compromised-proposal"); + + // ...but cannot approve its own proposal (SECURITY_APPROVER is a + // distinct key it does not control) or bypass the delay. + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(RecoveryGate.NotApproved.selector); + gate.execute(EPOCH_QUEUE_GROUP); + + // Even if it also somehow controlled SECURITY_APPROVER (a strictly + // stronger compromise), the vetoer — a third, independent key — can + // still cancel at any point up to execution. Clear the first + // (unapproved, now-moot) proposal first — only one may be pending + // per group at a time. + vm.prank(vetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, hostileModules, "compromised-proposal-2"); + (bytes32 digest,,,) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + + vm.prank(securityApprover); // worst case: approver also compromised + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.prank(vetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(RecoveryGate.NoPendingProposal.selector); + gate.execute(EPOCH_QUEUE_GROUP); + + for (uint256 i; i < groupSelectors.length; ++i) { + assertTrue(vault.moduleOf(groupSelectors[i]) != hostileModule); + } + } + + /// @dev A compromised ROOT_TIMELOCK also cannot rotate the security + /// approver fast enough to matter — that rotation is subject to + /// the identical minDelay and is itself vetoable. + function test_compromisedRootTimelock_cannotFastTrackAFriendlyApprover() public { + address friendlyApprover = makeAddr("friendlyApprover"); + + vm.prank(address(rootTimelock)); + gate.proposeApproverChange(friendlyApprover); + + // Meanwhile it also proposes a malicious recovery. + address hostileModule = makeAddr("hostileModule"); + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory hostileModules = new address[](groupSelectors.length); + for (uint256 i; i < hostileModules.length; ++i) hostileModules[i] = hostileModule; + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, hostileModules, "racing-approver-swap"); + + // The approver swap matures no sooner than the recovery itself — + // by the time a "friendly" approver could act, the original + // recovery proposal's own approval window has already been open + // for the same duration, giving defenders (the vetoer) an equal + // window to react to either. + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.executeApproverChange(); + assertEq(gate.securityApprover(), friendlyApprover); + + // The still-unapproved recovery proposal is untouched by the swap + // and remains blocked until *someone* approves it. + vm.expectRevert(RecoveryGate.NotApproved.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Guardian compromise: bounded, rate-limited, never constructive + // ══════════════════════════════════════════════════════════════════════════ + + function test_compromisedGuardian_canOnlyReachDepositsAndTwoWithdrawalBreakers() public { + vm.prank(guardian); + vault.guardianPause(); + + assertTrue(vault.paused()); + assertTrue(vault.pausedInstantWithdrawal()); + assertTrue(vault.pausedEpochCloseFund()); + // Everything else is out of reach: + assertFalse(vault.pausedQueuedRequest()); + assertFalse(vault.pausedFundedClaim()); + assertFalse(vault.pausedForceExit()); + } + + function test_compromisedGuardian_cannotReachOwnerOnlyBreakers() public { + vm.startPrank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + vault.pauseQueuedRequestOnly(true); + + vm.expectRevert(CoreVault.NotOwner.selector); + vault.pauseFundedClaimOnly(true); + + vm.expectRevert(CoreVault.NotOwner.selector); + vault.pauseForceExitOnly(true); + + vm.expectRevert(CoreVault.NotOwner.selector); + vault.unpauseAll(); + vm.stopPrank(); + } + + function test_compromisedGuardian_isRateLimitedByCooldown() public { + vm.prank(guardian); + vault.guardianPause(); + + vm.prank(guardian); + vm.expectRevert(CoreVault.GuardianCooldownActive.selector); + vault.guardianPause(); + } + + function test_compromisedGuardian_hasNoPathToModuleReplacement() public { + vm.startPrank(guardian); + vm.expectRevert(RecoveryGate.NotRootTimelock.selector); + gate.propose(EPOCH_QUEUE_GROUP, new address[](0), "guardian-attempt"); + + vm.expectRevert(CoreVault.NotOwner.selector); + vault.setModule(SelectorLib.getQueueModuleSelectors()[0], randomAttacker, 0); + + vm.expectRevert(CoreVault.RoutingFrozen.selector); + vm.stopPrank(); + vm.prank(address(rootTimelock)); + vault.setModule(SelectorLib.getQueueModuleSelectors()[0], randomAttacker, 0); + } + + function test_randomAttacker_hasNoCapabilityAnywhereInTheRecoverySystem() public { + vm.startPrank(randomAttacker); + vm.expectRevert(CoreVault.NotOwnerOrGuardian.selector); + vault.pauseInstantWithdrawalOnly(true); + + vm.expectRevert(); + vault.pauseAll(); + + vm.expectRevert(); + gate.propose(EPOCH_QUEUE_GROUP, new address[](0), "attacker"); + + vm.expectRevert(); + gate.approve(EPOCH_QUEUE_GROUP, bytes32(0)); + + vm.expectRevert(); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + vm.stopPrank(); + } +} diff --git a/test/incident-sim/QueueModuleIncident_EndToEnd.t.sol b/test/incident-sim/QueueModuleIncident_EndToEnd.t.sol new file mode 100644 index 0000000..148af37 --- /dev/null +++ b/test/incident-sim/QueueModuleIncident_EndToEnd.t.sol @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// INCIDENT SIMULATION — EpochedQueueModule exploit, full lifecycle +// +// Ties together Phase 1 (withdrawal pause matrix) and the Emergency Module +// Recovery mechanism into one incident-response narrative matching review +// §47's three phases: +// +// Phase A — Containment (seconds/minutes): Guardian trips the two +// withdrawal breakers it may reach. No module replacement. +// Phase B — Stabilization (hours/days): governance schedules recovery. +// Phase C — Remediation (days/weeks): Emergency Module Recovery executes; +// normal operation resumes on the patched module. +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + +import { CoreHarness } from "../helpers/CoreHarness.sol"; +import { MockUSDC } from "../helpers/MockUSDC.sol"; +import { MockParamsProvider } from "../helpers/MockParamsProvider.sol"; +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { ERC4626Module } from "../../src/core/modules/ERC4626Module.sol"; +import { EpochedQueueModule } from "../../src/core/modules/EpochedQueueModule.sol"; +import { SelectorLib } from "../../src/core/libraries/SelectorLib.sol"; +import { RecoveryGate } from "../../src/governance/RecoveryGate.sol"; +import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; + +contract QueueModuleIncident_EndToEnd is Test { + uint256 constant TIMELOCK_DELAY = 2 days; + uint64 constant MIN_DELAY = 14 days; + uint64 constant COOLDOWN = 30 days; + uint8 constant EPOCH_QUEUE_GROUP = 0; + + address internal owner; + address internal guardian; + address internal vetoer; + address internal user; + address internal securityApprover; + + CoreHarness internal core; + MockUSDC internal usdc; + MockParamsProvider internal params; + IncentivesTimelock internal rootTimelock; + RecoveryGate internal gate; + EpochedQueueModule internal patchedQueueModule; + + function setUp() public { + guardian = makeAddr("guardian"); + vetoer = makeAddr("vetoer"); + user = makeAddr("user"); + securityApprover = makeAddr("securityApprover"); + + address[] memory proposers = new address[](1); + address[] memory executors = new address[](1); + proposers[0] = address(this); + executors[0] = address(this); + rootTimelock = new IncentivesTimelock(TIMELOCK_DELAY, proposers, executors, address(this)); + owner = address(rootTimelock); + + usdc = new MockUSDC(); + params = new MockParamsProvider(); + + // CoreHarness's constructor wires EpochedQueueModule/ERC4626Module/ + // etc for us and starts unpaused — deploy with rootTimelock as owner + // directly so we don't need a separate ownership-transfer step. + core = new CoreHarness( + IERC20Metadata(address(usdc)), "Vault", "V", owner, owner, address(params) + ); + + vm.prank(owner); + core.setGuardian(guardian); + vm.prank(owner); + // AdminModule.setVetoer is ROLE_OWNER-routed through the fallback in + // production; CoreHarness wires AdminModule for exactly this. + (bool ok,) = address(core).call(abi.encodeWithSignature("setVetoer(address)", vetoer)); + require(ok, "setVetoer failed"); + + gate = new RecoveryGate(address(core), owner, securityApprover, MIN_DELAY, COOLDOWN); + vm.prank(owner); + core.setRecoveryGate(address(gate)); + + vm.prank(owner); + core.freezeRouting(); + + patchedQueueModule = new EpochedQueueModule(); + + usdc.mint(user, 10_000_000e6); + vm.prank(user); + IERC20(address(usdc)).approve(address(core), type(uint256).max); + } + + function test_fullIncidentLifecycle_containment_recovery_resolution() public { + // ── Normal operation before the incident ────────────────────────── + vm.prank(user); + uint256 shares = ERC4626Module(address(core)).deposit(1_000_000e6, user); + assertGt(shares, 0); + + // ── PHASE A: Containment ─────────────────────────────────────────── + // Guardian detects anomalous behavior in EpochedQueueModule and + // contains it immediately. No module replacement at this stage. + vm.prank(guardian); + core.guardianPause(); + + assertTrue(core.pausedInstantWithdrawal(), "instant settlement contained"); + assertTrue(core.pausedEpochCloseFund(), "epoch close/fund contained"); + // Queued-request creation and funded claims remain open throughout — + // exit intent is never blocked by containment (review §19/§20). + assertFalse(core.pausedQueuedRequest()); + assertFalse(core.pausedFundedClaim()); + + // Users can still register exit intent during containment. + vm.prank(user); + (uint256 epochId, uint256 claimId) = + EpochedQueueModule(address(core)).requestEpochWithdrawal(shares / 2); + assertGt(claimId + 1, 0); + + // ── PHASE B: Stabilization — governance schedules recovery ───────── + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory newModules = new address[](groupSelectors.length); + for (uint256 i; i < newModules.length; ++i) newModules[i] = address(patchedQueueModule); + + vm.prank(owner); + gate.propose(EPOCH_QUEUE_GROUP, newModules, "incident-2026-08-queue-exploit"); + + (bytes32 digest,,,) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + // ── PHASE C: Remediation ─────────────────────────────────────────── + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.execute(EPOCH_QUEUE_GROUP); + + for (uint256 i; i < groupSelectors.length; ++i) { + assertEq(core.moduleOf(groupSelectors[i]), address(patchedQueueModule)); + } + + // Governance lifts containment now that the patched module is live. + vm.prank(owner); + core.unpauseAll(); + assertFalse(core.pausedInstantWithdrawal()); + assertFalse(core.pausedEpochCloseFund()); + + // Normal operation resumes on the patched module: the pre-incident + // claim is still valid (module swap does not touch queue storage) + // and new activity works end-to-end. + vm.warp(block.timestamp + 7 days + 1); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + EpochedQueueModule(address(core)).fundEpoch(epochId); + + vm.prank(user); + EpochedQueueModule(address(core)).claimEpochAssets(epochId, claimId); + } + + function test_vetoBlocksAMaliciousRecoveryProposal_evenUnderContainment() public { + vm.prank(user); + ERC4626Module(address(core)).deposit(1_000_000e6, user); + + vm.prank(guardian); + core.guardianPause(); + + // A malicious or mistaken proposal is submitted (e.g. a compromised + // ROOT_TIMELOCK signer set, or a rushed patch that turns out wrong). + address suspiciousModule = makeAddr("suspiciousModule"); + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory newModules = new address[](groupSelectors.length); + for (uint256 i; i < newModules.length; ++i) newModules[i] = suspiciousModule; + + vm.prank(owner); + gate.propose(EPOCH_QUEUE_GROUP, newModules, "suspicious-proposal"); + + // The security approver never approves it. + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(RecoveryGate.NotApproved.selector); + gate.execute(EPOCH_QUEUE_GROUP); + + // Independently, the vetoer can cancel it outright at any point. + vm.prank(vetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + (, , , bool exists) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + assertFalse(exists, "malicious proposal cancelled before it could ever execute"); + + for (uint256 i; i < groupSelectors.length; ++i) { + assertTrue( + core.moduleOf(groupSelectors[i]) != suspiciousModule, + "vault never routed to the suspicious module" + ); + } + } +} diff --git a/test/integration/DeploymentEquivalence.t.sol b/test/integration/DeploymentEquivalence.t.sol index 4c6984b..d44441c 100644 --- a/test/integration/DeploymentEquivalence.t.sol +++ b/test/integration/DeploymentEquivalence.t.sol @@ -240,6 +240,7 @@ contract DeploymentEquivalence_Test is Test { vm.startPrank(rootTimelock); result.systemSealer.verifyAndSeal( SystemSealer.SealConfig({ + chainId: block.chainid, vault: address(result.vault), strategyRouter: address(result.strategyRouter), bufferManager: address(result.bufferManager), @@ -253,6 +254,8 @@ contract DeploymentEquivalence_Test is Test { incentives: address(0), incentivesEngine: address(0), rewardsPayoutManager: address(0), + recoveryGate: address(0), + recoveryManifestVersion: 0, rewardsTreasury: address(0), deployer: deployer }) @@ -399,6 +402,7 @@ contract DeploymentEquivalence_Test is Test { vm.startPrank(rootTimelock); result.systemSealer.verifyAndSeal( SystemSealer.SealConfig({ + chainId: block.chainid, vault: address(result.vault), strategyRouter: address(result.strategyRouter), bufferManager: address(result.bufferManager), @@ -412,6 +416,8 @@ contract DeploymentEquivalence_Test is Test { incentives: address(0), incentivesEngine: address(0), rewardsPayoutManager: address(0), + recoveryGate: address(0), + recoveryManifestVersion: 0, rewardsTreasury: address(0), deployer: deployer }) diff --git a/test/invariants/Recovery_Invariants.t.sol b/test/invariants/Recovery_Invariants.t.sol new file mode 100644 index 0000000..48dd36c --- /dev/null +++ b/test/invariants/Recovery_Invariants.t.sol @@ -0,0 +1,493 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// Emergency Module Recovery acceptance tests (review §40, adapted to the +// actual RecoveryGate/CoreVault.recoverModuleGroup API). See docs/recovery.md +// for the full design and docs/developer-response-recovery-architecture for +// the review this implements. +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { EpochedQueueModule } from "../../src/core/modules/EpochedQueueModule.sol"; +import { AdminModule } from "../../src/core/modules/AdminModule.sol"; +import { ERC4626Module } from "../../src/core/modules/ERC4626Module.sol"; +import { LiquidityOpsModule } from "../../src/core/modules/LiquidityOpsModule.sol"; +import { FeeCollector } from "../../src/core/modules/FeeCollector.sol"; +import { GlobalConfig } from "../../src/core/config/GlobalConfig.sol"; +import { SelectorRegistry } from "../../src/core/libraries/SelectorRegistry.sol"; +import { SelectorLib } from "../../src/core/libraries/SelectorLib.sol"; +import { RecoveryGate } from "../../src/governance/RecoveryGate.sol"; +import { IAdminModule } from "../../src/interfaces/IAdminModule.sol"; +import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; +import { ERC20Mock } from "../../src/mocks/ERC20Mock.sol"; + +contract Recovery_Invariants is Test { + uint256 constant TIMELOCK_DELAY = 2 days; + uint64 constant MIN_DELAY = 14 days; + uint64 constant COOLDOWN = 30 days; + uint8 constant EPOCH_QUEUE_GROUP = 0; + uint8 constant ERC4626_GROUP = 1; + + address internal deployer; + address internal guardian; + address internal vetoer; + address internal treasury; + address internal securityApprover; + + ERC20Mock internal usdc; + IncentivesTimelock internal rootTimelock; + CoreVault internal vault; + FeeCollector internal feeCollector; + GlobalConfig internal globalConfig; + RecoveryGate internal gate; + + EpochedQueueModule internal queueModuleV1; + EpochedQueueModule internal queueModuleV2; + address[] internal queueGroupV2Modules; + + function setUp() public { + deployer = makeAddr("deployer"); + guardian = makeAddr("guardian"); + vetoer = makeAddr("vetoer"); + treasury = makeAddr("treasury"); + securityApprover = makeAddr("securityApprover"); + + vm.startPrank(deployer); + + usdc = new ERC20Mock("USD Coin", "USDC", 6); + + address[] memory proposers = new address[](1); + address[] memory executors = new address[](1); + proposers[0] = deployer; + executors[0] = deployer; + rootTimelock = new IncentivesTimelock(TIMELOCK_DELAY, proposers, executors, deployer); + + feeCollector = + new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000); + globalConfig = new GlobalConfig(address(rootTimelock), 50, 100, 2000, 0, 10, 500, 3600, 3600); + + vault = new CoreVault( + IERC20Metadata(address(usdc)), "Vault", "V", deployer, address(feeCollector), address(globalConfig) + ); + + SelectorRegistry selectorRegistry = new SelectorRegistry(); + vault.setSelectorRegistry(address(selectorRegistry)); + + queueModuleV1 = new EpochedQueueModule(); + queueModuleV2 = new EpochedQueueModule(); + AdminModule am = new AdminModule(); + ERC4626Module e4626 = new ERC4626Module(); + LiquidityOpsModule lo = new LiquidityOpsModule(); + + bytes4[] memory s; + s = SelectorLib.getQueueModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(queueModuleV1), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getQueueModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(queueModuleV1), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getAdminModuleOwnerSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_OWNER); + s = SelectorLib.getAdminModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getERC4626ModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(e4626), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getLiquidityOpsModuleSelectors(); + for (uint256 i; i < s.length; ++i) { + uint8 role = s[i] == LiquidityOpsModule.deployToStrategiesWithPlan.selector + ? SelectorLib.ROLE_OWNER_OR_GUARDIAN + : SelectorLib.ROLE_PUBLIC; + vault.setModule(s[i], address(lo), role); + } + + // setEcosystem requires non-zero bufferManager/strategyRouter; these + // recovery tests never exercise deposit/withdraw flows, so + // placeholder addresses are sufficient. + IAdminModule(address(vault)).setEcosystem(IAdminModule.EcosystemConfig({ + bufferManager: makeAddr("bufferManager"), + strategyRouter: makeAddr("strategyRouter"), + healthRegistry: address(0), + incentives: address(0), + guardian: guardian, + vetoer: vetoer + })); + + gate = new RecoveryGate(address(vault), address(rootTimelock), securityApprover, MIN_DELAY, COOLDOWN); + vault.setRecoveryGate(address(gate)); + + vault.freezeRouting(); + + vault.beginOwnerTransfer(address(rootTimelock)); + vm.stopPrank(); + + vm.prank(address(rootTimelock)); + vault.acceptOwnerTransfer(); + + // The replacement group: same new module address for every selector + // in EPOCH_QUEUE_GROUP (write + view), the realistic shape of a + // recovery — one logical implementation owns the whole group. + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + queueGroupV2Modules = new address[](groupSelectors.length); + for (uint256 i; i < groupSelectors.length; ++i) { + queueGroupV2Modules[i] = address(queueModuleV2); + } + } + + // ══════════════════════════════════════════════════════════════════════════ + // Constructor / immutable policy + // ══════════════════════════════════════════════════════════════════════════ + + function test_constructor_reverts_belowFourteenDayFloor() public { + vm.expectRevert(RecoveryGate.DelayTooShort.selector); + new RecoveryGate(address(vault), address(rootTimelock), securityApprover, 13 days, COOLDOWN); + } + + function test_constructor_reverts_zeroAddresses() public { + vm.expectRevert(RecoveryGate.ZeroAddress.selector); + new RecoveryGate(address(0), address(rootTimelock), securityApprover, MIN_DELAY, COOLDOWN); + } + + function test_recoveryGate_immutableOnceSet() public { + vm.prank(address(rootTimelock)); + vm.expectRevert(CoreVault.RecoveryGateAlreadySet.selector); + vault.setRecoveryGate(makeAddr("otherGate")); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Access control + // ══════════════════════════════════════════════════════════════════════════ + + function test_onlyRecoveryGate_canCall_recoverModuleGroup() public { + vm.prank(address(rootTimelock)); + vm.expectRevert(CoreVault.NotRecoveryGate.selector); + vault.recoverModuleGroup(EPOCH_QUEUE_GROUP, queueGroupV2Modules); + } + + function test_onlyRootTimelock_canPropose() public { + vm.prank(deployer); + vm.expectRevert(RecoveryGate.NotRootTimelock.selector); + gate.propose(EPOCH_QUEUE_GROUP, queueGroupV2Modules, "reason"); + } + + function test_onlySecurityApprover_canApprove() public { + bytes32 digest = _proposeQueueRecovery(); + vm.prank(deployer); + vm.expectRevert(RecoveryGate.NotSecurityApprover.selector); + gate.approve(EPOCH_QUEUE_GROUP, digest); + } + + function test_onlyVetoer_canVetoCancel() public { + _proposeQueueRecovery(); + vm.prank(deployer); + vm.expectRevert(RecoveryGate.NotVetoer.selector); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + } + + function test_guardian_cannotCallAnythingOnRecoveryGate() public { + vm.startPrank(guardian); + vm.expectRevert(RecoveryGate.NotRootTimelock.selector); + gate.propose(EPOCH_QUEUE_GROUP, queueGroupV2Modules, "reason"); + + vm.expectRevert(RecoveryGate.NotSecurityApprover.selector); + gate.approve(EPOCH_QUEUE_GROUP, bytes32(0)); + + vm.expectRevert(RecoveryGate.NotVetoer.selector); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + vm.stopPrank(); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Proposal validation + // ══════════════════════════════════════════════════════════════════════════ + + function test_propose_reverts_wrongSelectorCount() public { + address[] memory tooFew = new address[](1); + tooFew[0] = address(queueModuleV2); + + vm.prank(address(rootTimelock)); + vm.expectRevert(RecoveryGate.WrongSelectorCount.selector); + gate.propose(EPOCH_QUEUE_GROUP, tooFew, "reason"); + } + + function test_propose_reverts_invalidGroup() public { + vm.prank(address(rootTimelock)); + vm.expectRevert(RecoveryGate.InvalidGroup.selector); + gate.propose(99, queueGroupV2Modules, "reason"); + } + + function test_propose_reverts_whilePendingProposalExists() public { + _proposeQueueRecovery(); + + vm.prank(address(rootTimelock)); + vm.expectRevert(RecoveryGate.PendingProposalExists.selector); + gate.propose(EPOCH_QUEUE_GROUP, queueGroupV2Modules, "reason-2"); + } + + function test_approve_reverts_onDigestMismatch() public { + _proposeQueueRecovery(); + + vm.prank(securityApprover); + vm.expectRevert(RecoveryGate.DigestMismatch.selector); + gate.approve(EPOCH_QUEUE_GROUP, bytes32(uint256(1234))); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Execution timing + // ══════════════════════════════════════════════════════════════════════════ + + function test_execute_reverts_beforeDelayElapsed() public { + bytes32 digest = _proposeQueueRecovery(); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.expectRevert(RecoveryGate.EtaNotReached.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + function test_execute_reverts_withoutApproval() public { + _proposeQueueRecovery(); + vm.warp(block.timestamp + MIN_DELAY + 1); + + vm.expectRevert(RecoveryGate.NotApproved.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + function test_execute_reverts_afterExecutionWindowExpires() public { + bytes32 digest = _proposeQueueRecovery(); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.warp(block.timestamp + MIN_DELAY + gate.EXECUTION_WINDOW() + 1); + + vm.expectRevert(RecoveryGate.EtaExpired.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + function test_execute_reverts_onReplay() public { + _executeQueueRecoveryHappyPath(); + + vm.expectRevert(RecoveryGate.NoPendingProposal.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Veto + // ══════════════════════════════════════════════════════════════════════════ + + function test_vetoCancel_blocksExecution() public { + bytes32 digest = _proposeQueueRecovery(); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.prank(vetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(RecoveryGate.NoPendingProposal.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + function test_vetoCancel_readsVetoerLiveFromCoreVault() public { + // Rotate the vault's vetoer, then confirm the OLD vetoer can no + // longer veto and the NEW one can — proves RecoveryGate never + // caches the address. + address newVetoer = makeAddr("newVetoer"); + vm.prank(address(rootTimelock)); + IAdminModule(address(vault)).setVetoer(newVetoer); + + _proposeQueueRecovery(); + + vm.prank(vetoer); + vm.expectRevert(RecoveryGate.NotVetoer.selector); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + vm.prank(newVetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Re-proposal invalidates prior approval + // ══════════════════════════════════════════════════════════════════════════ + + function test_repropose_invalidatesPriorApproval() public { + bytes32 digest1 = _proposeQueueRecovery(); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest1); + + // Vetoing clears the slot so we can propose a different plan for the + // same group without waiting out the (non-existent yet) cooldown. + vm.prank(vetoer); + gate.vetoCancel(EPOCH_QUEUE_GROUP); + + EpochedQueueModule queueModuleV3 = new EpochedQueueModule(); + address[] memory v3Modules = new address[](queueGroupV2Modules.length); + for (uint256 i; i < v3Modules.length; ++i) v3Modules[i] = address(queueModuleV3); + + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, v3Modules, "reason-3"); + + vm.warp(block.timestamp + MIN_DELAY + 1); + // Never approved under the NEW digest -> must still revert. + vm.expectRevert(RecoveryGate.NotApproved.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Concurrent proposals across different groups don't interfere + // ══════════════════════════════════════════════════════════════════════════ + + function test_concurrentProposals_differentGroups_doNotInterfere() public { + _proposeQueueRecovery(); + + ERC4626Module erc4626V2 = new ERC4626Module(); + bytes4[] memory erc4626Selectors = gate.selectorsForGroup(ERC4626_GROUP); + address[] memory erc4626Modules = new address[](erc4626Selectors.length); + for (uint256 i; i < erc4626Modules.length; ++i) erc4626Modules[i] = address(erc4626V2); + + vm.prank(address(rootTimelock)); + gate.propose(ERC4626_GROUP, erc4626Modules, "erc4626-reason"); + + (bytes32 erc4626Digest,,, bool exists) = gate.pendingProposal(ERC4626_GROUP); + assertTrue(exists); + + vm.prank(securityApprover); + gate.approve(ERC4626_GROUP, erc4626Digest); + + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.execute(ERC4626_GROUP); + + // The EPOCH_QUEUE_GROUP proposal is untouched and still pending, + // unapproved. + (, , bool queueApproved, bool queueExists) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + assertTrue(queueExists, "queue group proposal untouched by the ERC4626 group's execution"); + assertFalse(queueApproved); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Cooldown + // ══════════════════════════════════════════════════════════════════════════ + + function test_cooldown_blocksImmediateReRecoveryOfSameGroup() public { + _executeQueueRecoveryHappyPath(); + + EpochedQueueModule queueModuleV3 = new EpochedQueueModule(); + address[] memory v3Modules = new address[](queueGroupV2Modules.length); + for (uint256 i; i < v3Modules.length; ++i) v3Modules[i] = address(queueModuleV3); + + vm.prank(address(rootTimelock)); + vm.expectRevert(RecoveryGate.CooldownActive.selector); + gate.propose(EPOCH_QUEUE_GROUP, v3Modules, "too-soon"); + } + + function test_cooldown_clearsAfterElapsing() public { + _executeQueueRecoveryHappyPath(); + + vm.warp(block.timestamp + COOLDOWN + 1); + + EpochedQueueModule queueModuleV3 = new EpochedQueueModule(); + address[] memory v3Modules = new address[](queueGroupV2Modules.length); + for (uint256 i; i < v3Modules.length; ++i) v3Modules[i] = address(queueModuleV3); + + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, v3Modules, "cooldown-elapsed"); + (, , , bool exists) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + assertTrue(exists); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Happy path — end to end, and the structural role-relaxation guarantee + // ══════════════════════════════════════════════════════════════════════════ + + function test_happyPath_recoversEntireGroupAtomically() public { + bytes4[] memory selectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + + // Capture every selector's role before recovery. + uint8[] memory rolesBefore = new uint8[](selectors.length); + for (uint256 i; i < selectors.length; ++i) { + rolesBefore[i] = vault.roleOf(selectors[i]); + assertEq(vault.moduleOf(selectors[i]), address(queueModuleV1), "starts on V1"); + } + + _executeQueueRecoveryHappyPath(); + + for (uint256 i; i < selectors.length; ++i) { + assertEq(vault.moduleOf(selectors[i]), address(queueModuleV2), "every selector moved to V2"); + assertEq(vault.roleOf(selectors[i]), rolesBefore[i], "role unchanged by recovery (review section 11)"); + } + } + + function test_happyPath_doesNotTouchOtherGroups() public { + address erc4626ModuleBefore = vault.moduleOf(bytes4(keccak256("deposit(uint256,address)"))); + + _executeQueueRecoveryHappyPath(); + + assertEq( + vault.moduleOf(bytes4(keccak256("deposit(uint256,address)"))), + erc4626ModuleBefore, + "ERC4626_GROUP untouched by an EPOCH_QUEUE_GROUP recovery" + ); + } + + function test_setModule_stillPermanentlyDisabled_afterFreeze() public { + vm.prank(address(rootTimelock)); + vm.expectRevert(CoreVault.RoutingFrozen.selector); + vault.setModule(SelectorLib.getQueueModuleSelectors()[0], address(queueModuleV2), 0); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Security approver rotation + // ══════════════════════════════════════════════════════════════════════════ + + function test_approverRotation_happyPath() public { + address newApprover = makeAddr("newApprover"); + + vm.prank(address(rootTimelock)); + gate.proposeApproverChange(newApprover); + + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.executeApproverChange(); + + assertEq(gate.securityApprover(), newApprover); + } + + function test_approverRotation_vetoable() public { + address newApprover = makeAddr("newApprover"); + + vm.prank(address(rootTimelock)); + gate.proposeApproverChange(newApprover); + + vm.prank(vetoer); + gate.vetoApproverChange(); + + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(RecoveryGate.NoPendingProposal.selector); + gate.executeApproverChange(); + + assertEq(gate.securityApprover(), securityApprover, "unchanged after veto"); + } + + function test_approverRotation_onlyRootTimelockCanPropose() public { + vm.prank(deployer); + vm.expectRevert(RecoveryGate.NotRootTimelock.selector); + gate.proposeApproverChange(makeAddr("newApprover")); + } + + // ── helpers ─────────────────────────────────────────────────────────────── + + function _proposeQueueRecovery() internal returns (bytes32 digest) { + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, queueGroupV2Modules, "queue-incident-001"); + (digest,,,) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + } + + function _executeQueueRecoveryHappyPath() internal { + bytes32 digest = _proposeQueueRecovery(); + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.execute(EPOCH_QUEUE_GROUP); + } +} diff --git a/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol b/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol new file mode 100644 index 0000000..023ddd5 --- /dev/null +++ b/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol @@ -0,0 +1,411 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// Withdrawal / pause matrix — review §20/§21 (Multyr Core Architecture Review, +// snapshot 0bab749) and docs/developer-response-recovery-architecture.{html,pdf} +// §9. +// +// Before this change, FLAG_PAUSED_WITHDRAWALS never gated EpochedQueueModule at +// all (requestInstantWithdrawal/requestEpochWithdrawal/closeCurrentEpoch/ +// fundEpoch/claimEpochAssets had zero pause protection), while it DID gate +// ERC4626Module's forceWithdraw/forceWithdrawAll — the exact opposite of what +// the review requires (force exit must never be blocked by a generic emergency +// flag; the queue needs real breakers). +// +// This suite encodes the review §21 pause matrix directly: each of the five new +// breakers blocks only its own surface, guardianPause() reaches exactly the two +// breakers §20 approves for Guardian (instant settlement, epoch close/fund) and +// nothing else, and force exit is unaffected by pauseAll()/guardianPause() under +// any combination. +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + +import { CoreHarness } from "../helpers/CoreHarness.sol"; +import { MockUSDC } from "../helpers/MockUSDC.sol"; +import { MockParamsProvider } from "../helpers/MockParamsProvider.sol"; +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { ERC4626Module } from "../../src/core/modules/ERC4626Module.sol"; +import { EpochedQueueModule, EpochQueueStorage } from "../../src/core/modules/EpochedQueueModule.sol"; + +contract Withdrawal_PauseMatrix_Invariants is Test { + // MockParamsProvider.getQueueParams() default epoch duration. + uint256 internal constant EPOCH_DURATION = 7 days; + + address internal owner; + address internal guardian; + address internal user; + + CoreHarness internal core; + MockUSDC internal usdc; + MockParamsProvider internal params; + + function setUp() public { + owner = address(this); + guardian = makeAddr("guardian"); + user = makeAddr("user"); + + usdc = new MockUSDC(); + params = new MockParamsProvider(); + + core = new CoreHarness( + IERC20Metadata(address(usdc)), "Vault", "V", owner, owner, address(params) + ); + core.setGuardian(guardian); + + usdc.mint(user, 10_000_000e6); + vm.prank(user); + IERC20(address(usdc)).approve(address(core), type(uint256).max); + } + + function _deposit(uint256 assets) internal returns (uint256 shares) { + vm.prank(user); + shares = ERC4626Module(address(core)).deposit(assets, user); + } + + function _depositAndQueue(uint256 assets) + internal + returns (uint256 epochId, uint256 claimId) + { + uint256 shares = _deposit(assets); + vm.prank(user); + (epochId, claimId) = EpochedQueueModule(address(core)).requestEpochWithdrawal(shares); + } + + function _depositQueueCloseFund(uint256 assets) + internal + returns (uint256 epochId, uint256 claimId) + { + (epochId, claimId) = _depositAndQueue(assets); + vm.warp(block.timestamp + EPOCH_DURATION + 1); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + EpochedQueueModule(address(core)).fundEpoch(epochId); + } + + // ═══════════════════════════════════════════════════════════════════════ + // Instant-settlement breaker — Guardian-eligible (review §20) + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseInstantWithdrawalOnly_forcesQueueFallback_doesNotRevert() public { + uint256 shares = _deposit(1_000_000e6); + + core.pauseInstantWithdrawalOnly(true); + assertTrue(core.pausedInstantWithdrawal()); + + vm.prank(user); + (bool settledImmediately, uint256 epochId, uint256 claimId) = + EpochedQueueModule(address(core)).requestInstantWithdrawal(shares); + + assertFalse(settledImmediately, "instant paused -> exit intent still recorded via queue fallback"); + EpochQueueStorage.EpochClaim memory claim = + EpochedQueueModule(address(core)).epochClaim(epochId, claimId); + assertEq(claim.user, user, "fallback claim correctly attributed to the real user"); + } + + function test_guardian_canTripAndClearInstantWithdrawalBreaker() public { + vm.prank(guardian); + core.pauseInstantWithdrawalOnly(true); + assertTrue(core.pausedInstantWithdrawal()); + + vm.prank(guardian); + core.pauseInstantWithdrawalOnly(false); + assertFalse(core.pausedInstantWithdrawal()); + } + + function test_owner_canTripInstantWithdrawalBreaker() public { + core.pauseInstantWithdrawalOnly(true); + assertTrue(core.pausedInstantWithdrawal()); + } + + function test_randomAddress_cannotTripInstantWithdrawalBreaker() public { + vm.prank(user); + vm.expectRevert(CoreVault.NotOwnerOrGuardian.selector); + core.pauseInstantWithdrawalOnly(true); + } + + function test_guardianPause_tripsInstantWithdrawalBreaker() public { + uint256 shares = _deposit(1_000_000e6); + + vm.prank(guardian); + core.guardianPause(); + assertTrue(core.pausedInstantWithdrawal(), "guardianPause must reach instant settlement (review section 20)"); + + vm.prank(user); + (bool settledImmediately,,) = + EpochedQueueModule(address(core)).requestInstantWithdrawal(shares); + assertFalse(settledImmediately, "instant settlement suppressed while guardianPause is active"); + } + + // ═══════════════════════════════════════════════════════════════════════ + // Queued-request breaker — owner-only, exceptional (review §20) + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseQueuedRequestOnly_blocksNewQueuedRequests() public { + uint256 shares = _deposit(1_000_000e6); + + core.pauseQueuedRequestOnly(true); + + vm.prank(user); + vm.expectRevert(EpochedQueueModule.QueuedRequestPaused.selector); + EpochedQueueModule(address(core)).requestEpochWithdrawal(shares); + } + + function test_pauseQueuedRequestOnly_alsoBlocksTheInstantFallbackPath() public { + // requestInstantWithdrawal()'s queue-fallback must not be a bypass for + // pauseQueuedRequestOnly() — same underlying risk (accepting a new + // queued request during an active incident), same breaker. Force the + // fallback branch deterministically via the lock period so this + // exercises the fallback path rather than instant settlement. + params.setLockPeriod(1 days); + uint256 shares = _deposit(1_000_000e6); + core.pauseQueuedRequestOnly(true); + + vm.prank(user); + vm.expectRevert(EpochedQueueModule.QueuedRequestPaused.selector); + EpochedQueueModule(address(core)).requestInstantWithdrawal(shares); + } + + function test_guardian_cannotTripQueuedRequestBreaker() public { + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + core.pauseQueuedRequestOnly(true); + } + + function test_guardianPause_doesNotBlockNewQueuedRequests() public { + uint256 shares = _deposit(1_000_000e6); + + vm.prank(guardian); + core.guardianPause(); + assertFalse(core.pausedQueuedRequest(), "guardianPause must never reach queued-request creation (review section 20)"); + + vm.prank(user); + EpochedQueueModule(address(core)).requestEpochWithdrawal(shares); // must not revert + } + + function test_cancelEpochWithdrawal_remainsOpen_whileQueuedRequestPaused() public { + (uint256 epochId, uint256 claimId) = _depositAndQueue(1_000_000e6); + + core.pauseQueuedRequestOnly(true); + + vm.prank(user); + EpochedQueueModule(address(core)).cancelEpochWithdrawal(epochId, claimId); // must not revert + } + + // ═══════════════════════════════════════════════════════════════════════ + // Epoch close/fund breaker — Guardian-eligible (review §20) + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseEpochCloseFundOnly_blocksCloseAndFundAndSync() public { + (uint256 epochId,) = _depositAndQueue(1_000_000e6); + vm.warp(block.timestamp + EPOCH_DURATION + 1); + + vm.prank(guardian); + core.pauseEpochCloseFundOnly(true); + + vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + + vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); + EpochedQueueModule(address(core)).syncOldestUnfundedEpoch(); + + vm.prank(guardian); + core.pauseEpochCloseFundOnly(false); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + + vm.prank(guardian); + core.pauseEpochCloseFundOnly(true); + vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); + EpochedQueueModule(address(core)).fundEpoch(epochId); + } + + function test_guardianPause_blocksEpochCloseFund() public { + _depositAndQueue(1_000_000e6); + vm.warp(block.timestamp + EPOCH_DURATION + 1); + + vm.prank(guardian); + core.guardianPause(); + + vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + } + + function test_pauseEpochCloseFundOnly_doesNotBlockNewQueuedRequestsOrCancel() public { + (uint256 epochId, uint256 claimId) = _depositAndQueue(1_000_000e6); + + vm.prank(guardian); + core.pauseEpochCloseFundOnly(true); + + vm.prank(user); + EpochedQueueModule(address(core)).cancelEpochWithdrawal(epochId, claimId); // must not revert + + uint256 moreShares = _deposit(500_000e6); + vm.prank(user); + EpochedQueueModule(address(core)).requestEpochWithdrawal(moreShares); // must not revert + } + + // ═══════════════════════════════════════════════════════════════════════ + // Funded-claim breaker — owner-only, exceptional (review §20) + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseFundedClaimOnly_blocksClaimAndBatchClaim() public { + (uint256 epochId, uint256 claimId) = _depositQueueCloseFund(1_000_000e6); + + core.pauseFundedClaimOnly(true); + + vm.prank(user); + vm.expectRevert(EpochedQueueModule.FundedClaimPaused.selector); + EpochedQueueModule(address(core)).claimEpochAssets(epochId, claimId); + + uint256[] memory ids = new uint256[](1); + ids[0] = claimId; + vm.prank(user); + vm.expectRevert(EpochedQueueModule.FundedClaimPaused.selector); + EpochedQueueModule(address(core)).batchClaimEpochAssets(epochId, ids); + } + + function test_guardian_cannotTripFundedClaimBreaker() public { + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + core.pauseFundedClaimOnly(true); + } + + function test_guardianPause_doesNotBlockFundedClaims() public { + (uint256 epochId, uint256 claimId) = _depositQueueCloseFund(1_000_000e6); + + vm.prank(guardian); + core.guardianPause(); + assertFalse(core.pausedFundedClaim(), "guardianPause must never reach funded claims (review section 20)"); + + vm.prank(user); + EpochedQueueModule(address(core)).claimEpochAssets(epochId, claimId); // must not revert + } + + // ═══════════════════════════════════════════════════════════════════════ + // Force-exit breaker — owner-only, dedicated, never generic (review §20) + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseForceExitOnly_blocksForceWithdrawAll() public { + _deposit(1_000_000e6); + + core.pauseForceExitOnly(true); + + vm.prank(user); + vm.expectRevert(ERC4626Module.ForceExitPaused.selector); + ERC4626Module(address(core)).forceWithdrawAll(user, 0); + } + + function test_guardian_cannotTripForceExitBreaker() public { + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + core.pauseForceExitOnly(true); + } + + function test_guardianPause_doesNotBlockForceExit() public { + _deposit(1_000_000e6); + + vm.prank(guardian); + core.guardianPause(); + assertFalse(core.pausedForceExit(), "force exit must never be reachable from guardianPause (review section 20)"); + + vm.prank(user); + uint256 got = ERC4626Module(address(core)).forceWithdrawAll(user, 0); + assertGt(got, 0, "force exit succeeds despite an active guardianPause"); + } + + function test_pauseAll_doesNotBlockForceExit() public { + _deposit(1_000_000e6); + + core.pauseAll(); + assertTrue(core.paused()); + + vm.prank(user); + uint256 got = ERC4626Module(address(core)).forceWithdrawAll(user, 0); + assertGt(got, 0, "force exit succeeds despite pauseAll (review section 20, must never be a side effect)"); + } + + function test_pauseWithdrawalsOnly_doesNotBlockForceExit() public { + _deposit(1_000_000e6); + + core.pauseWithdrawalsOnly(true); + assertTrue(core.pausedWithdrawals()); + + vm.prank(user); + uint256 got = ERC4626Module(address(core)).forceWithdrawAll(user, 0); + assertGt(got, 0, "force exit has its own dedicated breaker, independent of FLAG_PAUSED_WITHDRAWALS"); + } + + // ═══════════════════════════════════════════════════════════════════════ + // pauseWithdrawalsOnly() as an owner-only aggregate — reaches exactly the + // same two breakers as guardianPause() (instant settlement, epoch + // close/fund) and nothing else. It must NOT reach queued-request creation + // (review §19: exit intent stays recordable while settlement is paused) + // or funded claims (review §20: no general administrative flag may ever + // block them) — confirmed by a pre-existing test + // (test_F2_pause_withdrawals_does_not_block_deposits in + // CoreEngine_Integration_Hardening.t.sol) that already asserted queuing a + // claim during pauseWithdrawalsOnly must succeed. + // ═══════════════════════════════════════════════════════════════════════ + + function test_pauseWithdrawalsOnly_blocksInstantSettlementAndEpochCloseFund_only() public { + (uint256 epochId, uint256 claimId) = _depositAndQueue(1_000_000e6); + uint256 moreShares = _deposit(1_000e6); + + core.pauseWithdrawalsOnly(true); + + // Instant settlement becomes unavailable (silently) -> falls back to + // the (unblocked) queue path instead of reverting. + vm.prank(user); + (bool settledImmediately,,) = + EpochedQueueModule(address(core)).requestInstantWithdrawal(moreShares); + assertFalse(settledImmediately, "instant unavailable under pauseWithdrawalsOnly -> queue fallback"); + + vm.warp(block.timestamp + EPOCH_DURATION + 1); + vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); + EpochedQueueModule(address(core)).closeCurrentEpoch(); + + // Cancelling an already-submitted request must still work. + vm.prank(user); + EpochedQueueModule(address(core)).cancelEpochWithdrawal(epochId, claimId); + } + + function test_pauseWithdrawalsOnly_doesNotBlockNewQueuedRequests() public { + uint256 shares = _deposit(1_000_000e6); + + core.pauseWithdrawalsOnly(true); + + vm.prank(user); + EpochedQueueModule(address(core)).requestEpochWithdrawal(shares); // must not revert + } + + function test_pauseWithdrawalsOnly_doesNotBlockFundedClaims() public { + (uint256 epochId, uint256 claimId) = _depositQueueCloseFund(1_000_000e6); + + core.pauseWithdrawalsOnly(true); + + vm.prank(user); + EpochedQueueModule(address(core)).claimEpochAssets(epochId, claimId); // must not revert + } + + // ═══════════════════════════════════════════════════════════════════════ + // unpauseAll() clears every granular flag alongside the legacy ones + // ═══════════════════════════════════════════════════════════════════════ + + function test_unpauseAll_clearsAllGranularFlags() public { + core.pauseInstantWithdrawalOnly(true); + core.pauseQueuedRequestOnly(true); + core.pauseEpochCloseFundOnly(true); + core.pauseFundedClaimOnly(true); + core.pauseForceExitOnly(true); + + core.unpauseAll(); + + assertFalse(core.pausedInstantWithdrawal()); + assertFalse(core.pausedQueuedRequest()); + assertFalse(core.pausedEpochCloseFund()); + assertFalse(core.pausedFundedClaim()); + assertFalse(core.pausedForceExit()); + } +} diff --git a/test/sprint-test/SystemSealer_CanSealAgreement.t.sol b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol new file mode 100644 index 0000000..e6fbcdc --- /dev/null +++ b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol @@ -0,0 +1,342 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// SPRINT SECURITY TEST — SystemSealer single verification engine (review §25) +// +// BUG (0bab749 architecture review, 2026-08-14): +// canSeal() and verifyAndSeal() maintained two independently-written +// invariant lists. Two invariants existed ONLY in verifyAndSeal(): the +// strategy role checks (_verifyStrategyRoles) and the deployer-retains-no- +// roles check. canSeal() could therefore return (true, "") for a config +// that verifyAndSeal() would still revert on — exactly the drift risk +// review §25 warns about. Neither function checked block.chainid at all +// (review §24/§42). +// +// FIX: +// Both entry points now call one shared _verifyLiveState(). This suite +// proves the closing requirement of review §42 directly: a positive result +// from canSeal() must imply verifyAndSeal() succeeds against unchanged +// state — including the two cases that used to be checked in one function +// but not the other, plus the new chainId binding. +// ───────────────────────────────────────────────────────────────────────────── + +import { Test } from "forge-std/Test.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; +import { AccessControl } from "@openzeppelin/contracts/access/AccessControl.sol"; + +import { CoreVault } from "../../src/core/CoreVault.sol"; +import { EpochedQueueModule } from "../../src/core/modules/EpochedQueueModule.sol"; +import { AdminModule } from "../../src/core/modules/AdminModule.sol"; +import { ERC4626Module } from "../../src/core/modules/ERC4626Module.sol"; +import { LiquidityOpsModule } from "../../src/core/modules/LiquidityOpsModule.sol"; +import { FeeCollector } from "../../src/core/modules/FeeCollector.sol"; +import { BufferManager } from "../../src/core/modules/BufferManager.sol"; +import { StrategyRouter } from "../../src/core/modules/StrategyRouter.sol"; +import { StrategyHealthRegistry } from "../../src/core/modules/StrategyHealthRegistry.sol"; +import { GlobalConfig } from "../../src/core/config/GlobalConfig.sol"; +import { SelectorRegistry } from "../../src/core/libraries/SelectorRegistry.sol"; +import { SelectorLib } from "../../src/core/libraries/SelectorLib.sol"; +import { SystemSealer } from "../../src/core/SystemSealer.sol"; +import { IAdminModule } from "../../src/interfaces/IAdminModule.sol"; +import { IBufferManager } from "../../src/interfaces/IBufferManager.sol"; +import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; +import { ERC20Mock } from "../../src/mocks/ERC20Mock.sol"; + +/// @dev Minimal AccessControl strategy stand-in — only used to exercise +/// SystemSealer's strategy-role invariant (INVARIANT 9). Whoever holds +/// DEFAULT_ADMIN_ROLE can grant/revoke every other role by default. +contract MinimalStrategyMock is AccessControl { + bytes32 public constant PARAM_ROLE = keccak256("PARAM_ROLE"); + bytes32 public constant CORE_ROLE = keccak256("CORE_ROLE"); + bytes32 public constant KEEPER_ROLE = keccak256("KEEPER_ROLE"); + + constructor(address admin) { + _grantRole(DEFAULT_ADMIN_ROLE, admin); + } +} + +contract SystemSealer_CanSealAgreement_Test is Test { + uint256 constant TIMELOCK_DELAY = 2 days; + + address internal deployer; + address internal guardian; + address internal vetoer; + address internal treasury; + + ERC20Mock internal usdc; // 6dp — decimals overrides are exempt + IncentivesTimelock internal rootTimelock; + CoreVault internal vault; + FeeCollector internal feeCollector; + GlobalConfig internal globalConfig; + BufferManager internal bufferManager; + StrategyRouter internal strategyRouter; + StrategyHealthRegistry internal healthRegistry; + SystemSealer internal systemSealer; + MinimalStrategyMock internal strategy; + + SystemSealer.SealConfig internal sealConfig; + + function setUp() public { + deployer = makeAddr("deployer"); + guardian = makeAddr("guardian"); + vetoer = makeAddr("vetoer"); + treasury = makeAddr("treasury"); + + vm.startPrank(deployer); + + usdc = new ERC20Mock("USD Coin", "USDC", 6); + + address[] memory proposers = new address[](1); + address[] memory executors = new address[](1); + proposers[0] = deployer; + executors[0] = deployer; + rootTimelock = new IncentivesTimelock(TIMELOCK_DELAY, proposers, executors, deployer); + + feeCollector = + new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000); + globalConfig = new GlobalConfig(address(rootTimelock), 50, 100, 2000, 0, 10, 500, 3600, 3600); + + systemSealer = new SystemSealer(); + SelectorRegistry selectorRegistry = new SelectorRegistry(); + + vault = new CoreVault( + IERC20Metadata(address(usdc)), "Vault", "V", deployer, address(feeCollector), address(globalConfig) + ); + + _wireModules(address(selectorRegistry)); + + IBufferManager.BufferConfig memory bufCfg = IBufferManager.BufferConfig({ + targetHotBps: 1000, minHotBps: 500, targetWarmBps: 1000, maxWarmBps: 2000, + opsReserveTargetBps: 100, maxWarmSlippageBps: 50, asset: address(usdc), + warmAdapter: address(0), twapWindowSec: 0, paused: true + }); + bufferManager = new BufferManager(deployer, address(vault), bufCfg); + strategyRouter = new StrategyRouter(deployer, address(vault), address(globalConfig)); + healthRegistry = new StrategyHealthRegistry(deployer, guardian); + + // Strategy correctly wired from the start: ROOT_TIMELOCK holds + // DEFAULT_ADMIN_ROLE/PARAM_ROLE, CoreVault holds CORE_ROLE, guardian + // holds KEEPER_ROLE, deployer holds nothing. Individual tests break + // one of these to prove canSeal() now catches it. + strategy = new MinimalStrategyMock(deployer); + strategy.grantRole(strategy.DEFAULT_ADMIN_ROLE(), address(rootTimelock)); + strategy.grantRole(strategy.PARAM_ROLE(), address(rootTimelock)); + strategy.grantRole(strategy.CORE_ROLE(), address(vault)); + strategy.grantRole(strategy.KEEPER_ROLE(), guardian); + strategy.renounceRole(strategy.DEFAULT_ADMIN_ROLE(), deployer); + + IAdminModule(address(vault)).setEcosystem(IAdminModule.EcosystemConfig({ + bufferManager: address(bufferManager), + strategyRouter: address(strategyRouter), + healthRegistry: address(healthRegistry), + incentives: address(0), + guardian: guardian, + vetoer: vetoer + })); + + uint256 deadAmt = 10_000_000; + usdc._mint(deployer, deadAmt); + usdc.approve(address(vault), deadAmt); + IAdminModule(address(vault)).seedDeadDeposit(deadAmt); + IAdminModule(address(vault)).enableComponentsTimelock(); + vault.freezeRouting(); + vault.setAuthorizedSealer(address(systemSealer)); + + bufferManager.transferOwnership(address(rootTimelock)); + strategyRouter.transferOwnership(address(rootTimelock)); + healthRegistry.transferOwnership(address(rootTimelock)); + vault.beginOwnerTransfer(address(rootTimelock)); + + vm.stopPrank(); + + vm.prank(address(rootTimelock)); + vault.acceptOwnerTransfer(); + + sealConfig = SystemSealer.SealConfig({ + chainId: block.chainid, + vault: address(vault), + strategyRouter: address(strategyRouter), + bufferManager: address(bufferManager), + healthRegistry: address(healthRegistry), + globalConfig: address(globalConfig), + feeCollector: address(feeCollector), + rootTimelock: address(rootTimelock), + guardian: guardian, + vetoer: vetoer, + strategy: address(strategy), + incentives: address(0), + incentivesEngine: address(0), + rewardsPayoutManager: address(0), + recoveryGate: address(0), + recoveryManifestVersion: 0, + rewardsTreasury: address(0), + deployer: deployer + }); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Happy path — the property review §42 requires: canSeal() true implies + // verifyAndSeal() succeeds against unchanged state. Exercises the strategy + // and chainId dimensions that were previously untested end-to-end. + // ══════════════════════════════════════════════════════════════════════════ + + function test_canSealTrue_impliesVerifyAndSealSucceeds() public { + (bool ok, string memory reason) = systemSealer.canSeal(sealConfig); + assertTrue(ok, string.concat("canSeal should pass on a fully correct config: ", reason)); + + _scheduleAndExecute(sealConfig, "happy-path-salt"); + assertTrue(vault.isSystemSealed(), "vault must seal when canSeal() already agreed it could"); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Strategy role invariant — previously checked ONLY in verifyAndSeal(). + // canSeal() used to return (true, "") here; it must now agree with + // verifyAndSeal() and reject it too. + // ══════════════════════════════════════════════════════════════════════════ + + function test_canSeal_and_verifyAndSeal_agree_whenRootTimelockMissingParamRole() public { + // Fetch the role constant BEFORE pranking — it's itself an external + // call, and vm.prank only applies to the single next call. + bytes32 paramRole = strategy.PARAM_ROLE(); + vm.prank(address(rootTimelock)); + strategy.revokeRole(paramRole, address(rootTimelock)); + + (bool ok, string memory reason) = systemSealer.canSeal(sealConfig); + assertFalse(ok, "canSeal must now catch a missing strategy PARAM_ROLE, not just verifyAndSeal"); + assertEq(reason, "Strategy: ROOT_TIMELOCK missing PARAM_ROLE"); + + _scheduleAndExpectRevert(sealConfig, "missing-param-role-salt"); + assertFalse(vault.isSystemSealed()); + } + + function test_canSeal_and_verifyAndSeal_agree_whenGuardianMissingKeeperRole() public { + bytes32 keeperRole = strategy.KEEPER_ROLE(); + vm.prank(address(rootTimelock)); + strategy.revokeRole(keeperRole, guardian); + + (bool ok, string memory reason) = systemSealer.canSeal(sealConfig); + assertFalse(ok, "canSeal must now catch a missing Guardian KEEPER_ROLE"); + assertEq(reason, "Strategy: Guardian missing KEEPER_ROLE (backup)"); + + _scheduleAndExpectRevert(sealConfig, "missing-keeper-role-salt"); + assertFalse(vault.isSystemSealed()); + } + + function test_canSeal_and_verifyAndSeal_agree_whenCoreVaultMissingCoreRole() public { + bytes32 coreRole = strategy.CORE_ROLE(); + vm.prank(address(rootTimelock)); + strategy.revokeRole(coreRole, address(vault)); + + (bool ok, string memory reason) = systemSealer.canSeal(sealConfig); + assertFalse(ok, "canSeal must now catch a missing CoreVault CORE_ROLE"); + assertEq(reason, "Strategy: CoreVault missing CORE_ROLE"); + + _scheduleAndExpectRevert(sealConfig, "missing-core-role-salt"); + assertFalse(vault.isSystemSealed()); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Deployer-retains-no-roles invariant — previously checked ONLY in + // verifyAndSeal(). canSeal() used to return (true, "") here too. + // ══════════════════════════════════════════════════════════════════════════ + + function test_canSeal_and_verifyAndSeal_agree_whenDeployerStillHasStrategyAdminRole() public { + // Deployer forgot to renounce DEFAULT_ADMIN_ROLE on the strategy after + // handing governance to ROOT_TIMELOCK — OZ AccessControl allows + // multiple simultaneous admins, so this is a real, reachable + // misconfiguration distinct from vault ownership (which invariant 1 + // already forces to ROOT_TIMELOCK before this check runs). + bytes32 adminRole = strategy.DEFAULT_ADMIN_ROLE(); + vm.prank(address(rootTimelock)); + strategy.grantRole(adminRole, deployer); + + (bool ok, string memory reason) = systemSealer.canSeal(sealConfig); + assertFalse(ok, "canSeal must catch a deployer that still holds strategy DEFAULT_ADMIN_ROLE"); + assertEq(reason, "Deployer still has strategy DEFAULT_ADMIN_ROLE"); + + _scheduleAndExpectRevert(sealConfig, "deployer-strategy-admin-salt"); + assertFalse(vault.isSystemSealed()); + } + + // ══════════════════════════════════════════════════════════════════════════ + // Chain binding — neither function checked this before (review §24/§42). + // ══════════════════════════════════════════════════════════════════════════ + + function test_canSeal_and_verifyAndSeal_agree_whenChainIdMismatched() public { + SystemSealer.SealConfig memory badConfig = sealConfig; + badConfig.chainId = sealConfig.chainId + 1; + + (bool ok, string memory reason) = systemSealer.canSeal(badConfig); + assertFalse(ok, "canSeal must reject a manifest built for a different chain"); + assertEq(reason, "chainId mismatch"); + + _scheduleAndExpectRevert(badConfig, "wrong-chain-salt"); + assertFalse(vault.isSystemSealed()); + } + + // ── helpers ─────────────────────────────────────────────────────────────── + + function _scheduleAndExecute(SystemSealer.SealConfig memory config, bytes32 saltSeed) internal { + address[] memory targets = new address[](1); + uint256[] memory values = new uint256[](1); + bytes[] memory payloads = new bytes[](1); + targets[0] = address(systemSealer); + payloads[0] = abi.encodeCall(SystemSealer.verifyAndSeal, (config)); + + bytes32 salt = keccak256(abi.encode(saltSeed)); + + vm.prank(deployer); + rootTimelock.scheduleBatch(targets, values, payloads, bytes32(0), salt, TIMELOCK_DELAY); + vm.warp(block.timestamp + TIMELOCK_DELAY + 1); + + vm.prank(deployer); + rootTimelock.executeBatch(targets, values, payloads, bytes32(0), salt); + } + + function _scheduleAndExpectRevert(SystemSealer.SealConfig memory config, bytes32 saltSeed) internal { + address[] memory targets = new address[](1); + uint256[] memory values = new uint256[](1); + bytes[] memory payloads = new bytes[](1); + targets[0] = address(systemSealer); + payloads[0] = abi.encodeCall(SystemSealer.verifyAndSeal, (config)); + + bytes32 salt = keccak256(abi.encode(saltSeed)); + + vm.prank(deployer); + rootTimelock.scheduleBatch(targets, values, payloads, bytes32(0), salt, TIMELOCK_DELAY); + vm.warp(block.timestamp + TIMELOCK_DELAY + 1); + + vm.prank(deployer); + vm.expectRevert(); + rootTimelock.executeBatch(targets, values, payloads, bytes32(0), salt); + } + + function _wireModules(address selectorRegistry) internal { + vault.setSelectorRegistry(selectorRegistry); + + EpochedQueueModule qm = new EpochedQueueModule(); + AdminModule am = new AdminModule(); + ERC4626Module e4626 = new ERC4626Module(); + LiquidityOpsModule lo = new LiquidityOpsModule(); + + bytes4[] memory s; + s = SelectorLib.getQueueModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(qm), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getQueueModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(qm), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getAdminModuleOwnerSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_OWNER); + s = SelectorLib.getAdminModuleViewSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(am), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getERC4626ModuleSelectors(); + for (uint256 i; i < s.length; ++i) vault.setModule(s[i], address(e4626), SelectorLib.ROLE_PUBLIC); + s = SelectorLib.getLiquidityOpsModuleSelectors(); + for (uint256 i; i < s.length; ++i) { + uint8 role = s[i] == LiquidityOpsModule.deployToStrategiesWithPlan.selector + ? SelectorLib.ROLE_OWNER_OR_GUARDIAN + : SelectorLib.ROLE_PUBLIC; + vault.setModule(s[i], address(lo), role); + } + } +} diff --git a/test/sprint-test/SystemSealer_DecimalsGuard.t.sol b/test/sprint-test/SystemSealer_DecimalsGuard.t.sol index 8fcf36b..9800502 100644 --- a/test/sprint-test/SystemSealer_DecimalsGuard.t.sol +++ b/test/sprint-test/SystemSealer_DecimalsGuard.t.sol @@ -129,6 +129,7 @@ contract SystemSealer_DecimalsGuard_Test is Test { vault.acceptOwnerTransfer(); sealConfig = SystemSealer.SealConfig({ + chainId: block.chainid, vault: address(vault), strategyRouter: address(strategyRouter), bufferManager: address(bufferManager), @@ -142,6 +143,8 @@ contract SystemSealer_DecimalsGuard_Test is Test { incentives: address(0), incentivesEngine: address(0), rewardsPayoutManager: address(0), + recoveryGate: address(0), + recoveryManifestVersion: 0, rewardsTreasury: address(0), deployer: deployer }); diff --git a/test/sprint-test/SystemSealer_TimestampHash_POC.t.sol b/test/sprint-test/SystemSealer_TimestampHash_POC.t.sol index 854fb8c..1717569 100644 --- a/test/sprint-test/SystemSealer_TimestampHash_POC.t.sol +++ b/test/sprint-test/SystemSealer_TimestampHash_POC.t.sol @@ -129,6 +129,7 @@ contract SystemSealer_TimestampHash_POC is Test { vault.acceptOwnerTransfer(); sealConfig = SystemSealer.SealConfig({ + chainId: block.chainid, vault: address(vault), strategyRouter: address(strategyRouter), bufferManager: address(bufferManager), @@ -142,6 +143,8 @@ contract SystemSealer_TimestampHash_POC is Test { incentives: address(0), incentivesEngine: address(0), rewardsPayoutManager: address(0), + recoveryGate: address(0), + recoveryManifestVersion: 0, rewardsTreasury: address(0), deployer: deployer }); @@ -203,6 +206,7 @@ contract SystemSealer_TimestampHash_POC is Test { */ function test_configHash_is_deterministic_across_timelock_delay() public { bytes32 hashAtSchedule = keccak256(abi.encode( + sealConfig.chainId, sealConfig.vault, sealConfig.rootTimelock, sealConfig.guardian, @@ -212,7 +216,9 @@ contract SystemSealer_TimestampHash_POC is Test { sealConfig.incentives, sealConfig.incentivesEngine, sealConfig.rewardsPayoutManager, - sealConfig.rewardsTreasury + sealConfig.rewardsTreasury, + sealConfig.recoveryGate, + sealConfig.recoveryManifestVersion )); // Advance past the timelock delay and seal From aaf07294d11e34851ab7b16eebcc317f44e220c5 Mon Sep 17 00:00:00 2001 From: shivam kalra Date: Mon, 17 Aug 2026 17:57:39 +0530 Subject: [PATCH 2/4] fix minor missings --- docs/architecture.md | 17 ++++++++++------- docs/modules.md | 5 +++-- docs/storage-layout.md | 5 +++-- src/core/CoreVault.sol | 32 +++++++++++++++++++++++++++++++- 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 51adfd2..b1362b9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -117,17 +117,20 @@ The following functions are NOT routed via the module dispatch — they are impl | `maxRedeem(address)` | Always returns 0 (queued protocol) | | `previewDeposit(uint256)` | Fee-aware shares preview | | `previewMint(uint256)` | Fee-aware assets-in preview | -| `owner()`, `guardian()` | Role reads | -| `paused()`, `pausedDeposits()`, `pausedWithdrawals()` | State reads | +| `owner()`, `guardian()`, `recoveryGate()` | Role reads | +| `paused()`, `pausedDeposits()`, `pausedWithdrawals()`, `pausedInstantWithdrawal()`, `pausedQueuedRequest()`, `pausedEpochCloseFund()`, `pausedFundedClaim()`, `pausedForceExit()` | State reads — see §11.3 | | `moduleOf(bytes4)`, `roleOf(bytes4)` | Routing table reads | -| `setModule()`, `setModulesBatch()` | Routing table writes (onlyOwner) | +| `setModule()`, `setModulesBatch()` | Routing table writes (onlyOwner, blocked once `FLAG_ROUTING_FROZEN` is set) | | `freezeRouting()` | Freeze routing table (irreversible) | -| `setSelectorRegistry()` | One-shot registry binding | -| `pauseAll()`, `unpauseAll()`, `guardianPause()` | Pause control | +| `setSelectorRegistry()` | One-shot registry binding (onlyOwner, blocked post-seal) | +| `setGuardian()` | Update guardian address (onlyOwner, blocked post-seal — matches `AdminModule.setVetoer()`'s existing `_requireNotSealed()` guard; **not** the `AdminModule.setEcosystem()`-era table entry that used to appear under §4 AdminModule in modules.md — this is a direct CoreVault function) | +| `setRecoveryGate()` | One-shot Emergency Module Recovery gate binding (onlyOwner, blocked post-seal — see [recovery.md](recovery.md)) | +| `recoverModuleGroup()` | Emergency Module Recovery entry point (onlyRecoveryGate only — see [recovery.md](recovery.md)) | +| `pauseAll()`, `unpauseAll()`, `pauseDepositsOnly()`, `pauseWithdrawalsOnly()`, `pauseInstantWithdrawalOnly()`, `pauseEpochCloseFundOnly()`, `pauseQueuedRequestOnly()`, `pauseFundedClaimOnly()`, `pauseForceExitOnly()`, `guardianPause()` | Pause control — full breaker table in §11.3 | | `beginOwnerTransfer()`, `acceptOwnerTransfer()` | Ownership transfer | | `processorMint()`, `processorBurn()`, `processorTransfer()`, `processorSpendAllowance()` | Module callbacks for share accounting | -| `authorizeModule()`, `isModuleAuthorized()` | Module authorization for processor functions | -| `setAuthorizedSealer()`, `sealBySealer()` | System sealing | +| `authorizeModule()`, `isModuleAuthorized()` | Module authorization for processor functions (onlyOwner, blocked post-seal) | +| `setAuthorizedSealer()`, `sealBySealer()` | System sealing (onlyOwner, blocked post-seal) | | `payRewardShares()` | Dedicated reward payout (via RewardsPayoutManager) — transfers from a pre-funded rewards treasury, never mints; non-dilutive by construction | --- diff --git a/docs/modules.md b/docs/modules.md index e72c736..bf105da 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -329,6 +329,8 @@ revoke*(): ### 4.3 Key Functions +> **Correction**: `setGuardian(address)` previously appeared in this table but is not an `AdminModule` function — it's implemented directly on `CoreVault` (see [architecture.md §2.3](architecture.md#23-functions-implemented-directly-on-corevault)). It is now `onlyOwner` and blocked post-seal, matching `setVetoer()` below. + | Function | Role | Timelock | Description | |---|---|---|---| | `submitFeeParams(dep,wit,immExit,forceExit,treasury)` | OWNER | yes | Queue fee change | @@ -342,8 +344,7 @@ revoke*(): | `setBufferManager(address)` | OWNER | conditionally | Set BufferManager | | `setRouter(address)` | OWNER | conditionally | Set StrategyRouter | | `setFeeCollector(address)` | OWNER | — | Update fee recipient | -| `setGuardian(address)` | OWNER | — | Update guardian | -| `setVetoer(address)` | OWNER | — | Update vetoer | +| `setVetoer(address)` | OWNER | — | Update vetoer (blocked post-seal via `_requireNotSealed()`) | | `enableComponentsTimelock()` | OWNER | — | Enable timelock for setParams/setRouter/setBM | | `submitBufferManager(address)` | OWNER | yes | Queue BM change (if componentsTl) | | `acceptBufferManager()` | OWNER | ETA check | Apply queued BM | diff --git a/docs/storage-layout.md b/docs/storage-layout.md index 7cff657..f787cc9 100644 --- a/docs/storage-layout.md +++ b/docs/storage-layout.md @@ -152,7 +152,7 @@ This is the primary namespace. All modules share this storage. Access via `CoreS | `incentives` | `IIncentives` | Legacy incentives (v1) | Updatable via AdminModule | | `feeCollector` | `address` | Fee recipient | Updatable via AdminModule | | `vetoer` | `address` | Can veto pending param changes | Updatable via AdminModule | -| `guardian` | `address` | Limited pause authority | Updatable via `setGuardian()` (onlyOwner) | +| `guardian` | `address` | Limited pause authority | Updatable via `setGuardian()` (onlyOwner, blocked post-seal) | | `owner` | `address` | Full admin authority | Updatable via 2-step transfer | | `pendingOwner` | `address` | Transfer target | Set by `beginOwnerTransfer()` | @@ -218,7 +218,8 @@ Source: `src/core/storage/CoreStorage.sol:79-80`. Populated by `setModule()` / ` | Field | Type | Purpose | |---|---|---| -| `selectorRegistry` | `address` | SelectorRegistry address (set once, immutable) | +| `selectorRegistry` | `address` | SelectorRegistry address (set once, blocked post-seal — see `setSelectorRegistry()`) | +| `recoveryGate` | `address` | Emergency Module Recovery gate (set once, blocked post-seal — see `setRecoveryGate()`, [recovery.md](recovery.md)) | | `pendingSealHash` | `bytes32` | Hash commitment for system sealing | | `authorizedSealer` | `address` | SystemSealer contract address (set once) | | `isAuthorizedModule[address]` | `mapping(address => bool)` | Module → processorMint/Burn/Transfer authorized | diff --git a/src/core/CoreVault.sol b/src/core/CoreVault.sol index fc85a38..d545fb0 100644 --- a/src/core/CoreVault.sol +++ b/src/core/CoreVault.sol @@ -273,8 +273,17 @@ contract CoreVault is ERC4626, ICoreVault { // MODULE ROUTING ADMIN // ═══════════════════════════════════════════════════════════════════════════════ + /// @dev Set-once, and explicitly blocked post-seal — matching + /// authorizeModule()/setAuthorizedSealer()/setRecoveryGate(). The + /// set-once check alone already makes this unreachable in practice + /// today (SystemSealer's INVARIANT 2 requires selectorRegistry != + /// address(0) to seal at all, so it can never be zero post-seal) — + /// the seal check is added for defense-in-depth consistency with + /// the other set-once admin functions, not because a live gap was + /// found here. function setSelectorRegistry(address registry) external onlyOwner { CoreStorage.Layout storage core = CoreStorage.layout(); + if (core.packedFlags & CoreStorage.FLAG_SYSTEM_SEALED != 0) revert SystemSealed(); if (core.selectorRegistry != address(0)) revert SelectorRegistryAlreadySet(); core.selectorRegistry = registry; emit Events.SelectorRegistrySet(registry); @@ -284,8 +293,19 @@ contract CoreVault is ERC4626, ICoreVault { /// thereafter — same pattern as setSelectorRegistry() (review §8: /// the recovery policy, including which contract enforces it, /// must not be administratively changeable post-seal). + /// @dev Explicitly blocked post-seal (matching authorizeModule() / + /// setAuthorizedSealer()): SystemSealer's INVARIANT 8e allows a + /// vault to seal with no recovery gate wired at all + /// (config.recoveryGate == address(0) is a valid, documented "no + /// recovery" configuration). Without this check, the owner could + /// seal without recovery, then call this afterward to install an + /// arbitrary contract with immediate, unrestricted + /// recoverModuleGroup() access — no delay, no independent + /// approval, no veto — defeating the entire point of the recovery + /// mechanism's immutable, pre-seal-only policy. function setRecoveryGate(address gate) external onlyOwner { CoreStorage.Layout storage core = CoreStorage.layout(); + if (core.packedFlags & CoreStorage.FLAG_SYSTEM_SEALED != 0) revert SystemSealed(); if (core.recoveryGate != address(0)) revert RecoveryGateAlreadySet(); if (gate == address(0)) revert ZeroAddress(); core.recoveryGate = gate; @@ -659,8 +679,18 @@ contract CoreVault is ERC4626, ICoreVault { return CoreStorage.layout().guardian; } + /// @dev Blocked post-seal, matching AdminModule.setVetoer()'s existing + /// _requireNotSealed() guard. Unlike setSelectorRegistry() above, + /// this one had no protection at all pre-fix — SystemSealer locks + /// in and verifies vault.guardian() == config.guardian at seal + /// time, and review §11 explicitly lists "Guardian modification" + /// as something the system must never permit, but nothing in code + /// stopped the owner from calling this freely at any time, + /// including after sealing. function setGuardian(address newGuardian) external onlyOwner { - CoreStorage.layout().guardian = newGuardian; + CoreStorage.Layout storage core = CoreStorage.layout(); + if (core.packedFlags & CoreStorage.FLAG_SYSTEM_SEALED != 0) revert SystemSealed(); + core.guardian = newGuardian; emit Events.GuardianUpdated(newGuardian); } From 236bb14e468da774e4bd6e802eae2f0e9db49dbb Mon Sep 17 00:00:00 2001 From: shivam kalra Date: Mon, 17 Aug 2026 19:34:53 +0530 Subject: [PATCH 3/4] fixes as per stefanos comments --- src/core/SystemSealer.sol | 33 +++++++- .../SystemSealer_CanSealAgreement.t.sol | 75 +++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) diff --git a/src/core/SystemSealer.sol b/src/core/SystemSealer.sol index 7aaa97e..4b6fcb4 100644 --- a/src/core/SystemSealer.sol +++ b/src/core/SystemSealer.sol @@ -59,10 +59,16 @@ import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/I * [x] CoreVault.isComponentsTimelocked == true * [x] CoreVault.selectorRegistry is set * [x] All AdminModule owner selectors have roleOf == ROLE_OWNER + * [x] CoreVault.feeCollector == config.feeCollector (live-wiring bind — a + * correctly governed FeeCollector the vault does not read from must not + * satisfy the seal) * [x] FeeCollector.governor == ROOT_TIMELOCK (immutable) * [x] GlobalConfig.governor == ROOT_TIMELOCK + * [x] CoreVault.router() == config.strategyRouter (live-wiring bind) * [x] StrategyRouter.owner == ROOT_TIMELOCK + * [x] CoreVault.bufferManager() == config.bufferManager (live-wiring bind) * [x] BufferManager.owner == ROOT_TIMELOCK + * [x] CoreVault.healthRegistry() == config.healthRegistry (live-wiring bind) * [x] StrategyHealthRegistry.owner == ROOT_TIMELOCK * [x] StrategyHealthRegistry.guardian == SAFE_GUARDIAN * [x] Incentives.owner == ROOT_TIMELOCK (if deployed) @@ -282,8 +288,17 @@ contract SystemSealer { } // ───────────────────────────────────────────────────────────────────────── - // INVARIANT 3: FeeCollector governor (IMMUTABLE - most critical check) + // INVARIANT 3: FeeCollector is bound to the vault, and its governor // ───────────────────────────────────────────────────────────────────────── + // Live-wiring bind (review §24) — same narrow-fix pattern already + // applied to GlobalConfig on PR #11: without this, a correctly + // governed FeeCollector that the vault does NOT actually read from + // still satisfies the governance check below, so a decoy address + // passes the seal even though CoreVault.feeCollector() points + // somewhere else entirely. + if (vault.feeCollector() != config.feeCollector) { + return (false, "FeeCollector not bound to vault"); + } FeeCollector fc = FeeCollector(config.feeCollector); if (fc.governor() != config.rootTimelock) { return (false, "FeeCollector.governor != ROOT_TIMELOCK (IMMUTABLE!)"); @@ -298,22 +313,32 @@ contract SystemSealer { } // ───────────────────────────────────────────────────────────────────────── - // INVARIANT 5: StrategyRouter ownership + // INVARIANT 5: StrategyRouter is bound to the vault, and its ownership // ───────────────────────────────────────────────────────────────────────── + if (address(vault.router()) != config.strategyRouter) { + return (false, "StrategyRouter not bound to vault"); + } if (StrategyRouter(config.strategyRouter).owner() != config.rootTimelock) { return (false, "StrategyRouter.owner != ROOT_TIMELOCK"); } // ───────────────────────────────────────────────────────────────────────── - // INVARIANT 6: BufferManager ownership + // INVARIANT 6: BufferManager is bound to the vault, and its ownership // ───────────────────────────────────────────────────────────────────────── + if (address(vault.bufferManager()) != config.bufferManager) { + return (false, "BufferManager not bound to vault"); + } if (BufferManager(config.bufferManager).owner() != config.rootTimelock) { return (false, "BufferManager.owner != ROOT_TIMELOCK"); } // ───────────────────────────────────────────────────────────────────────── - // INVARIANT 7: StrategyHealthRegistry ownership and guardian + // INVARIANT 7: StrategyHealthRegistry is bound to the vault, its + // ownership, and its guardian // ───────────────────────────────────────────────────────────────────────── + if (address(vault.healthRegistry()) != config.healthRegistry) { + return (false, "HealthRegistry not bound to vault"); + } StrategyHealthRegistry hr = StrategyHealthRegistry(config.healthRegistry); if (hr.owner() != config.rootTimelock) { return (false, "HealthRegistry.owner != ROOT_TIMELOCK"); diff --git a/test/sprint-test/SystemSealer_CanSealAgreement.t.sol b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol index e6fbcdc..b85661f 100644 --- a/test/sprint-test/SystemSealer_CanSealAgreement.t.sol +++ b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol @@ -189,6 +189,81 @@ contract SystemSealer_CanSealAgreement_Test is Test { assertTrue(vault.isSystemSealed(), "vault must seal when canSeal() already agreed it could"); } + // ══════════════════════════════════════════════════════════════════════════ + // Live-wiring bind checks (review §24) — a correctly governed decoy + // component must not be able to satisfy the seal if it is not the + // component actually wired into the vault. Previously only globalConfig + // had this check (fixed on PR #11); feeCollector/strategyRouter/ + // bufferManager/healthRegistry did not. + // ══════════════════════════════════════════════════════════════════════════ + + function test_canSeal_and_verifyAndSeal_agree_whenFeeCollectorIsADecoy() public { + FeeCollector decoy = + new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000); + SystemSealer.SealConfig memory decoyConfig = sealConfig; + decoyConfig.feeCollector = address(decoy); + + (bool ok, string memory reason) = systemSealer.canSeal(decoyConfig); + assertFalse(ok, "canSeal must reject a correctly-governed FeeCollector the vault does not read from"); + assertEq(reason, "FeeCollector not bound to vault"); + + _scheduleAndExpectRevert(decoyConfig, "feecollector-decoy-salt"); + assertFalse(vault.isSystemSealed()); + } + + function test_canSeal_and_verifyAndSeal_agree_whenStrategyRouterIsADecoy() public { + StrategyRouter decoy = new StrategyRouter(deployer, address(vault), address(globalConfig)); + vm.prank(deployer); + decoy.transferOwnership(address(rootTimelock)); + + SystemSealer.SealConfig memory decoyConfig = sealConfig; + decoyConfig.strategyRouter = address(decoy); + + (bool ok, string memory reason) = systemSealer.canSeal(decoyConfig); + assertFalse(ok, "canSeal must reject a correctly-governed StrategyRouter the vault does not read from"); + assertEq(reason, "StrategyRouter not bound to vault"); + + _scheduleAndExpectRevert(decoyConfig, "router-decoy-salt"); + assertFalse(vault.isSystemSealed()); + } + + function test_canSeal_and_verifyAndSeal_agree_whenBufferManagerIsADecoy() public { + IBufferManager.BufferConfig memory bufCfg = IBufferManager.BufferConfig({ + targetHotBps: 1000, minHotBps: 500, targetWarmBps: 1000, maxWarmBps: 2000, + opsReserveTargetBps: 100, maxWarmSlippageBps: 50, asset: address(usdc), + warmAdapter: address(0), twapWindowSec: 0, paused: true + }); + BufferManager decoy = new BufferManager(deployer, address(vault), bufCfg); + vm.prank(deployer); + decoy.transferOwnership(address(rootTimelock)); + + SystemSealer.SealConfig memory decoyConfig = sealConfig; + decoyConfig.bufferManager = address(decoy); + + (bool ok, string memory reason) = systemSealer.canSeal(decoyConfig); + assertFalse(ok, "canSeal must reject a correctly-governed BufferManager the vault does not read from"); + assertEq(reason, "BufferManager not bound to vault"); + + _scheduleAndExpectRevert(decoyConfig, "buffermanager-decoy-salt"); + assertFalse(vault.isSystemSealed()); + } + + function test_canSeal_and_verifyAndSeal_agree_whenHealthRegistryIsADecoy() public { + StrategyHealthRegistry decoy = new StrategyHealthRegistry(deployer, guardian); + vm.prank(deployer); + decoy.transferOwnership(address(rootTimelock)); + + SystemSealer.SealConfig memory decoyConfig = sealConfig; + decoyConfig.healthRegistry = address(decoy); + + (bool ok, string memory reason) = systemSealer.canSeal(decoyConfig); + assertFalse(ok, "canSeal must reject a correctly-governed HealthRegistry the vault does not read from"); + assertEq(reason, "HealthRegistry not bound to vault"); + + _scheduleAndExpectRevert(decoyConfig, "healthregistry-decoy-salt"); + assertFalse(vault.isSystemSealed()); + } + // ══════════════════════════════════════════════════════════════════════════ // Strategy role invariant — previously checked ONLY in verifyAndSeal(). // canSeal() used to return (true, "") here; it must now agree with From 65cbbb6466722fe4e7969a71acdb833f7e5ebe05 Mon Sep 17 00:00:00 2001 From: shivam kalra Date: Tue, 18 Aug 2026 17:33:24 +0530 Subject: [PATCH 4/4] fix: address both merge-blocking P0s and all P2s from 236bb14 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guardian breaker restrict-only enforcement, RecoveryGate deploy wiring, and five quick hardening fixes flagged in review of 236bb14. - pauseInstantWithdrawalOnly/pauseEpochCloseFundOnly: Guardian could clear a pause it (or the owner) had set, since both take a bool and were onlyOwnerOrGuardian unconditionally. Now restrict-only: tripping stays Owner-or-Guardian, clearing reverts NotOwner() unless called by the owner. Fixes the doc/code mismatch across governance.md §4.2, architecture.md §11.2, and access-control.md AC5, all of which already asserted the restrict-only model the code didn't implement. - DeployCoreSystem.s.sol: RecoveryGate was never deployed or wired by any script, so a real deploy could seal with recoveryGate == address(0) and no way to add it later (setRecoveryGate is post-seal-blocked). Now deployed and wired in Phase 5 with the approved 21-day/30-day minDelay/cooldown, gated on a new required SECURITY_APPROVER_ADDRESS env var (no silent fallback), plus a pre-seal inline assertion. - SystemSealer.canSeal() didn't check vault.authorizedSealer(), so it could return (true, "") for a SystemSealer instance the vault would reject at verifyAndSeal() time via NotAuthorizedSealer. - recoverModuleGroup() accepted codeless replacement addresses, silently bricking a group into no-op delegatecalls instead of reverting. - CoreStorage.Layout.recoveryGate moved from mid-struct to the true end, matching the file's own append-only convention. - Added a test proving RecoveryGate.selectorsForGroup() and CoreVault's internal selector derivation agree index-for-index, not just by length. - Corrected stale access-control.md claims that guardian/vetoer rotation works "at any time"/"indefinitely" — both revert post-seal. - Corrected the factually-wrong AdminModule-exclusion rationale in RecoveryGate.sol's NatSpec, and the misleading docstring on test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup so it can't be cited as disproving the (still-open) role-relaxation P1 finding. 938 -> 945 tests, all passing. P1 findings from the same review (approver rotation clock, role-relaxation caveat, unenforced RecoveryGate constructor floors, unbounded funded-claim breaker) are deliberately not included — reviewer scoped them before-mainnet, not before-merge, and two of them need a design call before implementation. --- docs/access-control.md | 17 +++-- docs/storage-layout.md | 4 +- script/DeployCoreSystem.s.sol | 36 +++++++++- src/core/CoreVault.sol | 25 +++++-- src/core/SystemSealer.sol | 10 +++ src/core/storage/CoreStorage.sol | 8 +-- src/governance/RecoveryGate.sol | 16 +++-- .../CoreVaultShellDefect_Unreachable.t.sol | 23 +++++-- .../GovernanceCompromise_BlastRadius.t.sol | 22 +++++++ test/invariants/Recovery_Invariants.t.sol | 65 +++++++++++++++++++ .../Withdrawal_PauseMatrix_Invariants.t.sol | 37 +++++++++-- .../SystemSealer_CanSealAgreement.t.sol | 31 +++++++++ 12 files changed, 265 insertions(+), 29 deletions(-) diff --git a/docs/access-control.md b/docs/access-control.md index 854bc51..e8b0937 100644 --- a/docs/access-control.md +++ b/docs/access-control.md @@ -99,7 +99,7 @@ All three named principals are set during deployment: | `vetoer` | Owner | `setVetoer(addr)` | Immediate setter; no timelock | | `guardian` | Owner via batch | `setEcosystem(cfg)` or `setHealthRegistry`-area setters | `cfg.guardian` required non-zero in `setEcosystem` | -The `guardian` field in `CoreStorage.Layout` is set either directly (owner-only setter) or via `setEcosystem`. It can be updated by the owner at any time post-deploy (no timelock). +The `guardian` field in `CoreStorage.Layout` is set either directly (owner-only setter) or via `setEcosystem`. It can be updated by the owner at any time pre-seal (no timelock) — but `setGuardian()` reverts with `SystemSealed()` once `FLAG_SYSTEM_SEALED` is set, same as `setVetoer()`. Post-seal, neither principal is rotatable at all. ### 3.2 How to Change Roles @@ -109,7 +109,7 @@ The `guardian` field in `CoreStorage.Layout` is set either directly (owner-only | `vetoer` | `setVetoer(addr)` (owner-only, immediate) | None | `NotOwner()` if non-owner | | `guardian` | Direct setter or `setEcosystem` (owner-only, immediate) | None | `NotOwner()` if non-owner | -No mechanism exists to "lock" a principal address permanently (except `sealBySealer` which locks `bufferManager`/`router` only). The owner retains ability to rotate vetoer and guardian indefinitely. +Both `setVetoer()` (`AdminModule.sol`) and `setGuardian()` (`CoreVault.sol`) revert with `SystemSealed()` once the vault is sealed — sealing does lock these principals permanently, alongside `bufferManager`/`router`. Pre-seal, the owner can rotate vetoer and guardian at will. ### 3.3 `roleOf[selector]` Management @@ -328,12 +328,21 @@ Risk: if newFixedMaturityModule has a reentrancy bug or is malicious, ``` Scenario: Current guardian key suspected compromised. +PRE-SEAL ONLY — both setters revert with SystemSealed() once sealed: + owner calls setVetoer(0xNewSecure) ← same call for vetoer rotation -owner calls (guardian setter)(0xNewGuardian) ← replace guardian +owner calls setGuardian(0xNewGuardian) ← replace guardian -Both changes are immediate (no timelock). +Both changes are immediate (no timelock) while unsealed. Old guardian key immediately loses all pause authority. No delay — enables fast rotation in incident response. + +POST-SEAL: rotation is permanently unavailable. A compromised guardian +post-seal can still be neutralized operationally (it can only ever trip +pauseInstantWithdrawalOnly/pauseEpochCloseFundOnly/guardianPause — see +§9.4/architecture.md §11.3 — and cannot clear its own breakers), but the +key itself cannot be replaced. This is the tradeoff sealing makes deliberately: +see docs/architecture.md §10 for the immutability rationale. ``` ### 9.4 Per-Function Role Inspection at Deploy diff --git a/docs/storage-layout.md b/docs/storage-layout.md index f787cc9..9a44f81 100644 --- a/docs/storage-layout.md +++ b/docs/storage-layout.md @@ -219,7 +219,6 @@ Source: `src/core/storage/CoreStorage.sol:79-80`. Populated by `setModule()` / ` | Field | Type | Purpose | |---|---|---| | `selectorRegistry` | `address` | SelectorRegistry address (set once, blocked post-seal — see `setSelectorRegistry()`) | -| `recoveryGate` | `address` | Emergency Module Recovery gate (set once, blocked post-seal — see `setRecoveryGate()`, [recovery.md](recovery.md)) | | `pendingSealHash` | `bytes32` | Hash commitment for system sealing | | `authorizedSealer` | `address` | SystemSealer contract address (set once) | | `isAuthorizedModule[address]` | `mapping(address => bool)` | Module → processorMint/Burn/Transfer authorized | @@ -237,8 +236,9 @@ Source: `src/core/storage/CoreStorage.sol:83-93`. | `executionMemory` | `address` | Allocation cost tracking | V10 allocation engine | | `strictExecutionMemory` | `bool` | Enforce ExecutionMemory on every deploy | V10 allocation engine | | `rewardsTreasury` | `address` | Pre-funded share balance `payRewardShares()` transfers from | Post-initial deploy | +| `recoveryGate` | `address` | Emergency Module Recovery gate (set once, blocked post-seal — see `setRecoveryGate()`, [recovery.md](recovery.md)) | Emergency Module Recovery | -Source: `src/core/storage/CoreStorage.sol:97-114`. These fields were appended to the Layout struct. Appending to EIP-7201 layout structs is safe — the namespace slot is a hash, and struct fields are allocated sequentially from that slot. No collision with earlier fields. +Source: `src/core/storage/CoreStorage.sol:97-118`. These fields were appended to the Layout struct, in declaration order, after every pre-existing field. Appending to EIP-7201 layout structs is safe — the namespace slot is a hash, and struct fields are allocated sequentially from that slot. No collision with earlier fields. --- diff --git a/script/DeployCoreSystem.s.sol b/script/DeployCoreSystem.s.sol index 77da102..246db9a 100644 --- a/script/DeployCoreSystem.s.sol +++ b/script/DeployCoreSystem.s.sol @@ -37,6 +37,7 @@ import { IIncentives } from "@multyr-core/interfaces/IIncentives.sol"; // Security import { SelectorRegistry } from "@multyr-core/core/libraries/SelectorRegistry.sol"; import { SystemSealer } from "@multyr-core/core/SystemSealer.sol"; +import { RecoveryGate } from "@multyr-core/governance/RecoveryGate.sol"; // Factory import { VaultFactory } from "@multyr-core/factory/VaultFactory.sol"; @@ -53,7 +54,8 @@ import { DeployTypes } from "@multyr-core/libs/DeployTypes.sol"; /// have dedicated standalone scripts: DeployWarmAdapters.s.sol, DeployVaultUpkeep.s.sol. /// @custom:chain-id 42161 (Arbitrum One -- enforced at runtime) /// @custom:env-vars DEPLOYER_PRIVATE_KEY, GOVERNOR_ADDRESS, GUARDIAN_ADDRESS, TREASURY_ADDRESS, -/// OPS_ADDRESS, SAFETY_RESERVE_ADDRESS, TIMELOCK_ADDRESS (opt), VETOER_ADDRESS (opt), +/// OPS_ADDRESS, SAFETY_RESERVE_ADDRESS, SECURITY_APPROVER_ADDRESS, +/// TIMELOCK_ADDRESS (opt), VETOER_ADDRESS (opt), /// DEPLOY_INCENTIVES (opt), DEPLOY_UPKEEP (opt), DEPLOY_WARM_ADAPTERS (opt), /// CHAINLINK_USDC_FEED (opt), OUTPUT_JSON (opt) /// @custom:post-deploy 1) Run DeployUsdcLendingStrategy.s.sol with vault+ecosystem addresses @@ -70,6 +72,12 @@ contract DeployCoreSystem is Script { address constant USDC = 0xaf88d065e77c8cC2239327C5EDb3A432268e5831; address constant MORPHO_GAUNTLET_CORE = 0x7e97fa6893871A2751B5fE961978DCCb2c201E65; + // RecoveryGate policy (approved values — see docs/recovery.md). The contract's + // own constructor only enforces a 14-day floor on minDelay; these are the + // actual configured values for this deployment. + uint64 constant RECOVERY_MIN_DELAY = 21 days; + uint64 constant RECOVERY_COOLDOWN = 30 days; + // ═══════════════════════════════════════════════════════════════════════════════ // DEPLOYMENT RESULT // ═══════════════════════════════════════════════════════════════════════════════ @@ -85,6 +93,7 @@ contract DeployCoreSystem is Script { // Phase 2: Security SelectorRegistry selectorRegistry; SystemSealer systemSealer; + RecoveryGate recoveryGate; // deployed/wired in Phase 5 (needs `vault` to exist first) // Phase 3: Core + Modules CoreVault vault; @@ -118,6 +127,7 @@ contract DeployCoreSystem is Script { address safetyReserve; address timelock; // Final owner (usually same as governor) address vetoer; // SAFE_VETO + address securityApprover; // RecoveryGate's independent approver (should be distinct from governor/guardian/vetoer) address chainlinkUsdcFeed; bool deployIncentives; bool deployUpkeep; @@ -152,6 +162,7 @@ contract DeployCoreSystem is Script { console.log("Guardian (SAFE_GUARDIAN):", cfg.guardian); console.log("Vetoer (SAFE_VETO):", cfg.vetoer); console.log("Timelock:", cfg.timelock); + console.log("Security Approver:", cfg.securityApprover); console.log(""); console.log("Feature Flags:"); console.log(" Deploy Warm Adapters:", cfg.deployWarmAdapters); @@ -197,6 +208,7 @@ contract DeployCoreSystem is Script { require(IAdminModule(address(result.vault)).getImmediateExitPenalty() == 100, "FINAL: immediateExitPenalty != 100"); require(!result.vault.isRoutingFrozen(), "FINAL: routing should NOT be frozen yet"); require(result.vault.paused(), "FINAL: vault must still be paused"); + require(result.vault.recoveryGate() != address(0), "FINAL: recoveryGate not wired, sealing without it makes recovery permanently unavailable"); console.log(" [OK] All critical assertions passed"); vm.stopBroadcast(); @@ -527,6 +539,21 @@ contract DeployCoreSystem is Script { result.vault.setSelectorRegistry(address(result.selectorRegistry)); console.log(" SelectorRegistry set (guardrail NOW active)"); + // 5.8b RecoveryGate (Emergency Module Recovery — must be wired before seal, + // since setRecoveryGate() is blocked post-seal and address(0) is otherwise + // accepted by SystemSealer as a silent "no recovery" configuration) + console.log("[5.8b] Deploying and wiring RecoveryGate..."); + result.recoveryGate = new RecoveryGate( + address(result.vault), + cfg.timelock, // ROOT_TIMELOCK + cfg.securityApprover, + RECOVERY_MIN_DELAY, + RECOVERY_COOLDOWN + ); + result.vault.setRecoveryGate(address(result.recoveryGate)); + require(result.vault.recoveryGate() == address(result.recoveryGate), "DEPLOY_BUG: recoveryGate not wired"); + console.log(" RecoveryGate:", address(result.recoveryGate)); + // 5.9 Initial fees console.log("[5.9] Setting initial fees..."); if (!IAdminModule(address(result.vault)).isFeesInitialized()) { @@ -675,6 +702,7 @@ contract DeployCoreSystem is Script { cfg.treasury = vm.envAddress("TREASURY_ADDRESS"); cfg.ops = vm.envAddress("OPS_ADDRESS"); cfg.safetyReserve = vm.envAddress("SAFETY_RESERVE_ADDRESS"); + cfg.securityApprover = vm.envAddress("SECURITY_APPROVER_ADDRESS"); try vm.envAddress("TIMELOCK_ADDRESS") returns (address t) { cfg.timelock = t; } catch { cfg.timelock = cfg.governor; } @@ -720,6 +748,7 @@ contract DeployCoreSystem is Script { vm.serializeAddress(json, "selectorRegistry", address(result.selectorRegistry)); vm.serializeAddress(json, "systemSealer", address(result.systemSealer)); + vm.serializeAddress(json, "recoveryGate", address(result.recoveryGate)); vm.serializeAddress(json, "vault", address(result.vault)); vm.serializeAddress(json, "queueModule", address(result.queueModule)); @@ -740,7 +769,8 @@ contract DeployCoreSystem is Script { vm.serializeAddress(json, "ops", cfg.ops); vm.serializeAddress(json, "safetyReserve", cfg.safetyReserve); vm.serializeAddress(json, "timelock", cfg.timelock); - string memory finalJson = vm.serializeAddress(json, "vetoer", cfg.vetoer); + vm.serializeAddress(json, "vetoer", cfg.vetoer); + string memory finalJson = vm.serializeAddress(json, "securityApprover", cfg.securityApprover); vm.writeJson(finalJson, cfg.outputJsonPath); console.log("Address book written to:", cfg.outputJsonPath); @@ -758,6 +788,7 @@ contract DeployCoreSystem is Script { console.log("Security:"); console.log(" SelectorRegistry: ", address(result.selectorRegistry)); console.log(" SystemSealer: ", address(result.systemSealer)); + console.log(" RecoveryGate: ", address(result.recoveryGate)); console.log("Core:"); console.log(" CoreVault: ", address(result.vault)); console.log(" EpochedQueueModule: ", address(result.queueModule)); @@ -797,6 +828,7 @@ contract DeployCoreSystem is Script { console.log(" FEE_COLLECTOR_ADDRESS=", address(result.feeCollector)); console.log(" SELECTOR_REGISTRY_ADDRESS=", address(result.selectorRegistry)); console.log(" SYSTEM_SEALER_ADDRESS=", address(result.systemSealer)); + console.log(" RECOVERY_GATE_ADDRESS=", address(result.recoveryGate)); console.log(" GUARDIAN_ADDRESS=", cfg.guardian); console.log(" TIMELOCK_ADDRESS=", cfg.timelock); console.log("2. Timelock: acceptOwnerTransfer + setAuthorizedSealer + systemSealer.verifyAndSeal(config)"); diff --git a/src/core/CoreVault.sol b/src/core/CoreVault.sol index d545fb0..59013ef 100644 --- a/src/core/CoreVault.sol +++ b/src/core/CoreVault.sol @@ -64,6 +64,7 @@ contract CoreVault is ERC4626, ICoreVault { error NotRecoveryGate(); error InvalidRecoveryGroup(); error WrongRecoveryModuleCount(); + error RecoveryModuleHasNoCode(); error SystemSealed(); error SealerAlreadySet(); error NotAuthorizedSealer(); @@ -404,6 +405,11 @@ contract CoreVault is ERC4626, ICoreVault { CoreStorage.Layout storage core = CoreStorage.layout(); for (uint256 i; i < selectors.length; ++i) { + // A codeless address (typo, or an undeployed CREATE2 address) would + // otherwise wire in silently: the fallback's delegatecall to an + // empty address succeeds and returns empty data, bricking the + // group into silent no-ops instead of reverting. + if (newModules[i].code.length == 0) revert RecoveryModuleHasNoCode(); core.moduleOf[selectors[i]] = newModules[i]; } @@ -574,12 +580,18 @@ contract CoreVault is ERC4626, ICoreVault { /// @notice Guardian-eligible instant-settlement breaker (review §20: approved /// as a narrow circuit breaker Guardian may trip immediately). + /// @dev Restrict-only for the Guardian: tripping (p == true) is Owner-or- + /// Guardian, but clearing (p == false) is Owner-only. The Guardian must + /// never be able to reverse its own emergency action — every clearing + /// operation requires the Owner (docs/governance.md §4.2, AC5). function pauseInstantWithdrawalOnly(bool p) external onlyOwnerOrGuardian { + CoreStorage.Layout storage core = CoreStorage.layout(); if (p) { - CoreStorage.layout().packedFlags |= CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; + core.packedFlags |= CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; emit Events.InstantWithdrawalPaused(); } else { - CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; + if (msg.sender != core.owner) revert NotOwner(); + core.packedFlags &= ~CoreStorage.FLAG_INSTANT_WITHDRAWAL_PAUSED; emit Events.InstantWithdrawalUnpaused(); } } @@ -588,12 +600,17 @@ contract CoreVault is ERC4626, ICoreVault { /// temporarily restricted where the affected accounting or /// settlement path is implicated"; separate from exit-intent /// recording, which is pauseQueuedRequestOnly below). + /// @dev Restrict-only for the Guardian: tripping (p == true) is Owner-or- + /// Guardian, but clearing (p == false) is Owner-only. Same rationale + /// as pauseInstantWithdrawalOnly above. function pauseEpochCloseFundOnly(bool p) external onlyOwnerOrGuardian { + CoreStorage.Layout storage core = CoreStorage.layout(); if (p) { - CoreStorage.layout().packedFlags |= CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; + core.packedFlags |= CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; emit Events.EpochCloseFundPaused(); } else { - CoreStorage.layout().packedFlags &= ~CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; + if (msg.sender != core.owner) revert NotOwner(); + core.packedFlags &= ~CoreStorage.FLAG_EPOCH_CLOSE_FUND_PAUSED; emit Events.EpochCloseFundUnpaused(); } } diff --git a/src/core/SystemSealer.sol b/src/core/SystemSealer.sol index 4b6fcb4..7e5bec0 100644 --- a/src/core/SystemSealer.sol +++ b/src/core/SystemSealer.sol @@ -252,6 +252,16 @@ contract SystemSealer { if (vault.isSystemSealed()) return (false, "Already sealed"); + // This specific SystemSealer instance must be the one CoreVault will + // actually accept a seal from — otherwise canSeal() can return + // (true, "") for a config where verifyAndSeal() would still revert + // with NotAuthorizedSealer inside vault.sealBySealer(), reintroducing + // exactly the canSeal()/verifyAndSeal() divergence this file exists + // to eliminate (review §25/§42). + if (vault.authorizedSealer() != address(this)) { + return (false, "SystemSealer not authorized on vault"); + } + // ───────────────────────────────────────────────────────────────────────── // INVARIANT 1: CoreVault ownership and state // ───────────────────────────────────────────────────────────────────────── diff --git a/src/core/storage/CoreStorage.sol b/src/core/storage/CoreStorage.sol index 4a31b69..59d2283 100644 --- a/src/core/storage/CoreStorage.sol +++ b/src/core/storage/CoreStorage.sol @@ -93,10 +93,6 @@ library CoreStorage { // Selector registry for role validation (set once, immutable) address selectorRegistry; - // Emergency Module Recovery gate (set once, immutable) — the sole - // authorized caller of recoverModuleGroup(). Review §7/§8. - address recoveryGate; - // System sealer binding - set atomically by sealBySealer() (called from // SystemSealer.verifyAndSeal()) alongside FLAG_SYSTEM_SEALED, in the same call // that verifies the config hash. Retained post-seal as an audit record. @@ -125,6 +121,10 @@ library CoreStorage { // shares are minted, so PPS is unaffected. Funding this address is a // treasury-ops decision outside the vault's scope. address rewardsTreasury; + + // Emergency Module Recovery gate (appended, set once, immutable) — the + // sole authorized caller of recoverModuleGroup(). Review §7/§8. + address recoveryGate; } function layout() internal pure returns (Layout storage l) { diff --git a/src/governance/RecoveryGate.sol b/src/governance/RecoveryGate.sol index 437bb54..dc18171 100644 --- a/src/governance/RecoveryGate.sol +++ b/src/governance/RecoveryGate.sol @@ -25,11 +25,17 @@ import { SelectorLib } from "../core/libraries/SelectorLib.sol"; * exact digest (review §13), and subject to cancellation by CoreVault's * vetoer at any point before execution (review §14). * - * RECOVERABLE GROUPS (review §9 — economically isolated execution modules; - * AdminModule's governance/sealing/authorization surface and every direct - * CoreVault function are permanently out of scope, not merely excluded from - * a whitelist — they are not moduleOf-routed selectors and this contract has - * no path to reach them): + * RECOVERABLE GROUPS (review §9 — economically isolated execution modules). + * Two different reasons keep everything else permanently out of scope: + * - AdminModule's governance/sealing/authorization selectors ARE + * moduleOf-routed (same dispatcher as every recoverable group), but no + * group here enumerates them — exclusion is definitional (a fixed, + * closed whitelist), not structural. A fifth group could theoretically + * be added in a future contract; this one has none. + * - CoreVault's own direct functions (setModule*, freezeRouting, pause*, + * setSelectorRegistry, authorizeModule, etc.) are NOT moduleOf-routed at + * all — there is no selector for recoverModuleGroup() to touch them + * with, regardless of group ID. This exclusion IS structural. * 0 = EPOCH_QUEUE_GROUP — EpochedQueueModule (write + view selectors) * 1 = ERC4626_GROUP — ERC4626Module * 2 = LIQUIDITY_GROUP — LiquidityOpsModule diff --git a/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol b/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol index e8b331d..9fb056b 100644 --- a/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol +++ b/test/incident-sim/CoreVaultShellDefect_Unreachable.t.sol @@ -114,9 +114,21 @@ contract CoreVaultShellDefect_Unreachable is Test { } } - /// @dev Confirms the negative directly, not just by absence-of-overlap: - /// the vault's actual live AdminModule routing is completely - /// unaffected by recovering every other group. + /// @dev NOTE ON SCOPE: this only re-confirms, end-to-end for one group + /// instead of by selector-set comparison, that recoverModuleGroup() + /// never WRITES to an AdminModule selector's moduleOf entry — the + /// same fact test_noRecoveryGroup_coversAnyAdminModuleOwnerSelector + /// above already establishes by construction. It is NOT evidence + /// that AdminModule's governance/sealing surface is unreachable + /// through a *malicious recovered module's code* — a module + /// installed via recoverModuleGroup runs via delegatecall and + /// therefore has full read/write access to all of CoreStorage, + /// including roleOf and owner, regardless of which selectors it was + /// recovered under. Whether that constitutes a live "role relaxation" + /// gap is tracked separately (see docs/recovery.md and the caveat in + /// RecoveryGate.sol's own NatSpec: "restricting recovery to existing + /// selectors ... does not mathematically prove a replacement module + /// only repairs a bug"). Do not cite this test as disproving that. function test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup() public { bytes4[] memory adminSelectors = SelectorLib.getAdminModuleOwnerSelectors(); address adminModuleBefore = vault.moduleOf(adminSelectors[0]); @@ -124,7 +136,10 @@ contract CoreVaultShellDefect_Unreachable is Test { // Recover EPOCH_QUEUE_GROUP (unwired in this minimal fixture — // recovering an unwired group is still valid: it just sets moduleOf // from address(0) to the new address, same as normal) and confirm - // AdminModule's routing is completely untouched. + // AdminModule's *routing table entry* is untouched. The replacement + // module here (EpochedQueueModule) is never actually invoked through + // the recovered selectors in this test, so this says nothing about + // what a malicious module's code could do once installed. EpochedQueueModule qm = new EpochedQueueModule(); bytes4[] memory queueSelectors = gate.selectorsForGroup(0); address[] memory queueModules = new address[](queueSelectors.length); diff --git a/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol b/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol index 43dc7e4..e0cf359 100644 --- a/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol +++ b/test/incident-sim/GovernanceCompromise_BlastRadius.t.sol @@ -245,6 +245,28 @@ contract GovernanceCompromise_BlastRadius is Test { vm.stopPrank(); } + function test_compromisedGuardian_cannotSelfReverseItsOwnEmergencyBrake() public { + // Worst-case scenario a compromised Guardian could attempt: trip + // guardianPause(), then immediately try to clear the two breakers it + // reaches, reopening instant settlement and epoch close/fund with no + // owner involvement at all. Must be impossible — restrict-only. + vm.prank(guardian); + vault.guardianPause(); + assertTrue(vault.pausedInstantWithdrawal()); + assertTrue(vault.pausedEpochCloseFund()); + + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + vault.pauseInstantWithdrawalOnly(false); + + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + vault.pauseEpochCloseFundOnly(false); + + assertTrue(vault.pausedInstantWithdrawal(), "guardian must not be able to self-reverse the brake"); + assertTrue(vault.pausedEpochCloseFund(), "guardian must not be able to self-reverse the brake"); + } + function test_compromisedGuardian_isRateLimitedByCooldown() public { vm.prank(guardian); vault.guardianPause(); diff --git a/test/invariants/Recovery_Invariants.t.sol b/test/invariants/Recovery_Invariants.t.sol index 48dd36c..3193e10 100644 --- a/test/invariants/Recovery_Invariants.t.sol +++ b/test/invariants/Recovery_Invariants.t.sol @@ -25,6 +25,11 @@ import { IAdminModule } from "../../src/interfaces/IAdminModule.sol"; import { IncentivesTimelock } from "../../src/governance/IncentivesTimelock.sol"; import { ERC20Mock } from "../../src/mocks/ERC20Mock.sol"; +/// @dev Trivial code-bearing placeholder — recoverModuleGroup() only requires +/// code.length > 0, so any deployed contract qualifies as a "module" for +/// ordering-agreement tests that never actually call into it. +contract _RecoveryOrderingDummy {} + contract Recovery_Invariants is Test { uint256 constant TIMELOCK_DELAY = 2 days; uint64 constant MIN_DELAY = 14 days; @@ -400,6 +405,66 @@ contract Recovery_Invariants is Test { // Happy path — end to end, and the structural role-relaxation guarantee // ══════════════════════════════════════════════════════════════════════════ + function test_execute_reverts_whenReplacementModuleHasNoCode() public { + // A codeless address (typo, or a CREATE2 address that was never + // actually deployed to) must not be silently wired in — the + // fallback's delegatecall to an empty address succeeds and returns + // empty data, which would brick the group into silent no-ops instead + // of a loud revert. + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory badModules = new address[](groupSelectors.length); + for (uint256 i; i < groupSelectors.length; ++i) { + badModules[i] = address(queueModuleV2); + } + badModules[0] = makeAddr("codelessReplacement"); // no code deployed here + + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, badModules, "codeless-module-attempt"); + (bytes32 digest,,,) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.warp(block.timestamp + MIN_DELAY + 1); + vm.expectRevert(CoreVault.RecoveryModuleHasNoCode.selector); + gate.execute(EPOCH_QUEUE_GROUP); + } + + function test_recoverModuleGroup_selectorOrdering_matchesRecoveryGate() public { + // RecoveryGate.selectorsForGroup() and CoreVault's own internal + // _recoverySelectorsForGroup() are two independently-maintained + // definitions of the same group (CoreVault.sol's own comment: "kept + // in sync because both read the same underlying SelectorLib getters, + // not because either trusts the other's definition"). A distinct + // module address per selector must land on that exact selector after + // recovery — proving index-for-index agreement, not just matching + // lengths, which the "same module for every selector" happy-path + // test above cannot distinguish from a silently reordered group. + bytes4[] memory groupSelectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); + address[] memory distinctModules = new address[](groupSelectors.length); + for (uint256 i; i < groupSelectors.length; ++i) { + distinctModules[i] = address(new _RecoveryOrderingDummy()); + } + + vm.prank(address(rootTimelock)); + gate.propose(EPOCH_QUEUE_GROUP, distinctModules, "ordering-check"); + (bytes32 digest,,,) = gate.pendingProposal(EPOCH_QUEUE_GROUP); + + vm.prank(securityApprover); + gate.approve(EPOCH_QUEUE_GROUP, digest); + + vm.warp(block.timestamp + MIN_DELAY + 1); + gate.execute(EPOCH_QUEUE_GROUP); + + for (uint256 i; i < groupSelectors.length; ++i) { + assertEq( + vault.moduleOf(groupSelectors[i]), + distinctModules[i], + "CoreVault's internal selector ordering must match RecoveryGate.selectorsForGroup() index-for-index" + ); + } + } + function test_happyPath_recoversEntireGroupAtomically() public { bytes4[] memory selectors = gate.selectorsForGroup(EPOCH_QUEUE_GROUP); diff --git a/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol b/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol index 023ddd5..43bd2f8 100644 --- a/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol +++ b/test/invariants/Withdrawal_PauseMatrix_Invariants.t.sol @@ -105,14 +105,15 @@ contract Withdrawal_PauseMatrix_Invariants is Test { assertEq(claim.user, user, "fallback claim correctly attributed to the real user"); } - function test_guardian_canTripAndClearInstantWithdrawalBreaker() public { + function test_guardian_canTripButNotClearInstantWithdrawalBreaker() public { vm.prank(guardian); core.pauseInstantWithdrawalOnly(true); assertTrue(core.pausedInstantWithdrawal()); vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); core.pauseInstantWithdrawalOnly(false); - assertFalse(core.pausedInstantWithdrawal()); + assertTrue(core.pausedInstantWithdrawal(), "guardian must not be able to clear its own breaker"); } function test_owner_canTripInstantWithdrawalBreaker() public { @@ -120,6 +121,15 @@ contract Withdrawal_PauseMatrix_Invariants is Test { assertTrue(core.pausedInstantWithdrawal()); } + function test_owner_canClearInstantWithdrawalBreaker_evenIfGuardianTrippedIt() public { + vm.prank(guardian); + core.pauseInstantWithdrawalOnly(true); + assertTrue(core.pausedInstantWithdrawal()); + + core.pauseInstantWithdrawalOnly(false); // owner, no prank + assertFalse(core.pausedInstantWithdrawal()); + } + function test_randomAddress_cannotTripInstantWithdrawalBreaker() public { vm.prank(user); vm.expectRevert(CoreVault.NotOwnerOrGuardian.selector); @@ -211,8 +221,7 @@ contract Withdrawal_PauseMatrix_Invariants is Test { vm.expectRevert(EpochedQueueModule.EpochCloseFundPaused.selector); EpochedQueueModule(address(core)).syncOldestUnfundedEpoch(); - vm.prank(guardian); - core.pauseEpochCloseFundOnly(false); + core.pauseEpochCloseFundOnly(false); // owner, no prank — guardian cannot clear (see below) EpochedQueueModule(address(core)).closeCurrentEpoch(); vm.prank(guardian); @@ -221,6 +230,26 @@ contract Withdrawal_PauseMatrix_Invariants is Test { EpochedQueueModule(address(core)).fundEpoch(epochId); } + function test_guardian_canTripButNotClearEpochCloseFundBreaker() public { + vm.prank(guardian); + core.pauseEpochCloseFundOnly(true); + assertTrue(core.pausedEpochCloseFund()); + + vm.prank(guardian); + vm.expectRevert(CoreVault.NotOwner.selector); + core.pauseEpochCloseFundOnly(false); + assertTrue(core.pausedEpochCloseFund(), "guardian must not be able to clear its own breaker"); + } + + function test_owner_canClearEpochCloseFundBreaker_evenIfGuardianTrippedIt() public { + vm.prank(guardian); + core.pauseEpochCloseFundOnly(true); + assertTrue(core.pausedEpochCloseFund()); + + core.pauseEpochCloseFundOnly(false); // owner, no prank + assertFalse(core.pausedEpochCloseFund()); + } + function test_guardianPause_blocksEpochCloseFund() public { _depositAndQueue(1_000_000e6); vm.warp(block.timestamp + EPOCH_DURATION + 1); diff --git a/test/sprint-test/SystemSealer_CanSealAgreement.t.sol b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol index b85661f..cd85942 100644 --- a/test/sprint-test/SystemSealer_CanSealAgreement.t.sol +++ b/test/sprint-test/SystemSealer_CanSealAgreement.t.sol @@ -197,6 +197,37 @@ contract SystemSealer_CanSealAgreement_Test is Test { // bufferManager/healthRegistry did not. // ══════════════════════════════════════════════════════════════════════════ + function test_canSeal_and_verifyAndSeal_agree_whenSystemSealerIsNotAuthorizedOnVault() public { + // A second, otherwise-identical SystemSealer that the vault never + // authorized via setAuthorizedSealer(). canSeal() on THIS instance + // must reject, even though every other invariant in sealConfig is + // satisfied — otherwise a deploy-day dry run against the wrong + // SystemSealer address could report "ready to seal" when + // verifyAndSeal() would actually revert with NotAuthorizedSealer. + SystemSealer unauthorizedSealer = new SystemSealer(); + + (bool ok, string memory reason) = unauthorizedSealer.canSeal(sealConfig); + assertFalse(ok, "canSeal must reject a SystemSealer the vault did not authorize"); + assertEq(reason, "SystemSealer not authorized on vault"); + + address[] memory targets = new address[](1); + uint256[] memory values = new uint256[](1); + bytes[] memory payloads = new bytes[](1); + targets[0] = address(unauthorizedSealer); + payloads[0] = abi.encodeCall(SystemSealer.verifyAndSeal, (sealConfig)); + + bytes32 salt = keccak256(abi.encode("unauthorized-sealer-salt")); + + vm.prank(deployer); + rootTimelock.scheduleBatch(targets, values, payloads, bytes32(0), salt, TIMELOCK_DELAY); + vm.warp(block.timestamp + TIMELOCK_DELAY + 1); + + vm.prank(deployer); + vm.expectRevert(); + rootTimelock.executeBatch(targets, values, payloads, bytes32(0), salt); + assertFalse(vault.isSystemSealed()); + } + function test_canSeal_and_verifyAndSeal_agree_whenFeeCollectorIsADecoy() public { FeeCollector decoy = new FeeCollector(address(rootTimelock), treasury, treasury, treasury, 7000, 200, 3000);