feat: add Emergency Module Recovery and granular withdrawal circuit b…

[kalrashivam]

Summary
Implements our response to the Multyr Core Upgradeability, Emergency Recovery & Incident Response Architecture Review (snapshot 0bab749) — developer-response-recovery-architecture.pdf for the point-by-point response this PR follows through on.

The review rejected a general-purpose post-seal RecoveryController and approved a narrower Emergency Module Recovery mechanism instead, plus several P0 pre-launch hardening items. This PR implements both, in the P0 → P1 order the review's own priority list (§39) lays out.

What changed
Withdrawal pause matrix. FLAG_PAUSED_WITHDRAWALS was too coarse and, on inspection, wrong in two ways: it wrongly gated force-exit (forceWithdraw/forceWithdrawAll), which the review says must never be blockable by a generic emergency flag, while EpochedQueueModule had no pause protection at all — guardianPause() never reached it. Replaced with 5 dedicated breakers (instant settlement, queued-request creation, epoch close/fund, funded claims, force exit), each with the access level (owner-only vs. owner-or-guardian) the review's §20/§21 circuit-breaker table specifies.

SystemSealer single verifier. canSeal() and verifyAndSeal() maintained two independently-written invariant lists — canSeal() was missing the strategy-role and deployer-retains-no-roles checks, so it could return true for a config verifyAndSeal() would still revert on. Unified into one _verifyLiveState(). Also added the chain-ID binding the review calls for (§24/§42).

Emergency Module Recovery. New RecoveryGate contract + CoreVault.recoverModuleGroup(): ROOT_TIMELOCK proposes → SECURITY_APPROVER approves an exact digest → CoreVault.vetoer() can cancel any time → open execution after a 14-day-minimum delay. Covers 4 pre-approved module groups (queue, ERC4626, liquidity, fixed-maturity) sourced directly from the existing SelectorLib — no new selector registry. Role relaxation is structurally impossible: recoverModuleGroup takes no role parameter at all.

Docs. Corrected several stale pre-existing references (wrong function names, wrong access-control claims) while documenting the new mechanisms; new docs/recovery.md.

Tests. 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 lifecycle, ROOT_TIMELOCK/Guardian compromise blast-radius scenarios).

[stefanobotticelli]

Reviewed at 236bb14. The engineering underneath is solid: no regressions against ab51167, 938 tests green across 101 suites, the sealer binding now covers all five components, and force exit is correctly isolated on its own owner-only breaker. But two P0s block the merge, and one of them is the central decision of this PR.

All findings below are reproducible. I have passing PoCs and can push them to test/review-poc/ if you want to run them yourself.

---

P0 — The Guardian can still lift restrictions

pauseInstantWithdrawalOnly (CoreVault.sol:577) and pauseEpochCloseFundOnly (:591) are still onlyOwnerOrGuardian and still take bool, so the Guardian can clear a pause the owner set. This is the incoherence you flagged in your own document, and the agreed ruling is restrict-only: the Guardian restricts, the owner restores.

Breaker	Restrict	Restore
Instant settlement	Owner or Guardian	Owner or Guardian ❌
Epoch close/fund	Owner or Guardian	Owner or Guardian ❌
Queued request	Owner only	Owner only ✅
Funded claim	Owner only	Owner only ✅
Force exit	Owner only	Owner only ✅

Three PoCs pass. The operationally worst one: a compromised Guardian trips guardianPause(), then calls both setters with false and reopens instant settlement and epoch close/fund. The 7-day guardianPause cooldown does not apply to the granular setters, so the emergency brake self-reverses.

The part that concerns me more than the code. Three doc files assert the opposite of what the code does:

• docs/governance.md §4.2: "the guardian can restrict … but can never unpause anything — every clearing operation requires the owner"
• docs/architecture.md §11.2 repeats it
• docs/access-control.md:270 states it as invariant AC5

And test_guardian_canTripAndClearInstantWithdrawalBreaker (Withdrawal_PauseMatrix_Invariants.t.sol:108) asserts the opposite and passes.

So the table in governance.md is accurate, the prose asserts the agreed model, the code implements the opposite, and the tests lock the code in. Anyone reading those docs — us, an auditor, an investor — comes away believing a security property that does not exist. This has to be reconciled in the same pass, whichever way the code lands.

Fix: split into a Guardian-callable restrict-only entry point with clearing behind onlyOwner, or reject p == false when msg.sender != owner. Then fix the test asserting the opposite and add the missing negative cells.

---

P0 — RecoveryGate is not wired by any deploy script

Zero references to RecoveryGate or setRecoveryGate anywhere under script/. setRecoveryGate is blocked post-seal (CoreVault.sol:~300), and address(0) is an accepted "no recovery" configuration in invariant 8e, so sealing succeeds without it.

Net effect: a 415-line recovery contract, 28 invariant tests and a design doc for a mechanism that no deploy path installs — and if we ever seal without wiring it, recovery becomes permanently unavailable with no way back.

Fix: wire it in DeployCoreSystem.s.sol before verifyAndSeal, and add a pre-seal assertion that vault.recoveryGate() != address(0). If a no-recovery deployment is ever legitimate, it should be an explicit flag rather than the silent default.

---

P1 — before mainnet, not before merge

1. Approver rotation runs on the same clock as recovery. proposeApproverChange and propose in the same block, then at t+minDelay: executeApproverChange (open caller), approve, execute, all in one block. A compromised ROOT_TIMELOCK installs a hostile module group with no independent approval and no additional delay. PoC passes.

The NatSpec at RecoveryGate.sol:50-52 claims a compromised timelock "cannot fast-track a friendly approver into place in time to matter". test_compromisedRootTimelock_cannotFastTrackAFriendlyApprover (:186) stops exactly one call short of disproving it: it rotates the approver, asserts the still-unapproved proposal reverts, and never has the new approver approve it.

Fix: rotation delay strictly greater than minDelay, or invalidate every pending proposal when the approver changes.

2. "Role relaxation is structurally impossible" is false. Stated at RecoveryGate.sol:17, CoreVault.sol:389-391 and RecoveryGate.sol:370-377. A replacement module runs in delegatecall (CoreVault.sol:186) and therefore owns CoreStorage, including roleOf, moduleOf and owner. PoC: one permissionless call into a recovered group flips all 35 AdminModule owner selectors to PUBLIC and reassigns owner. That recoverModuleGroup takes no role parameter constrains the gate, not the module.

This matters beyond wording: that premise is why "unchanged role mapping" was dropped from the approval digest at :374-377, which the architecture review §13 required. Either put it back, or state the caveat plainly.

Related: the reason given for excluding AdminModule (:29-32, "not moduleOf-routed selectors") is factually wrong — AdminModule is moduleOf-routed (DeployCoreSystem.s.sol:607). The exclusion holds because no group enumerates it; the justification doesn't.

3. Approved parameters are not in the code. Floor is 14 days hardcoded at RecoveryGate.sol:161; approved was 21. Cooldown has no floor validation at all and accepts 0; approved was 30. Both are constructor arguments and no script constructs a RecoveryGate, so there is no configured value to inspect anywhere.

4. The funded-claim breaker has no bound. Shares escrowed, epoch closed at a locked price, reservedForClaims incremented, then pauseFundedClaimOnly(true): claimEpochAssets reverts (:667, :715), cancelEpochWithdrawal requires an Open epoch (:408), forceWithdraw needs shares the user no longer holds. Unbounded duration, no expiry, no timelock to set it, owner-only to clear. A lost or hostile owner key makes those assets permanently unclaimable and permanently undeployable, since reservedForClaims keeps them out of canDeploy.

This is also the exact guarantee we want to state publicly about funded claims, so it needs a bound: auto-expiry, or a timelock on setting the flag.

---

P2 — before merge, quick

• canSeal does not check vault.authorizedSealer(), so it can return (true, "") on a vault where verifyAndSeal then reverts NotAuthorizedSealer. Same divergence class this PR set out to eliminate, and exactly what a deploy-day dry run depends on. test_canSealTrue_impliesVerifyAndSealSucceeds misses it because its fixture wires the sealer.
• recoverModuleGroup accepts codeless addresses. Codehash is snapshotted at propose time, so a codeless address encodes 0 and passes; the fallback then delegatecalls it and succeeds silently returning empty data. A typo or an undeployed CREATE2 address bricks the group into silent no-ops instead of reverting. PoC passes.
• recoveryGate is inserted mid-struct at CoreStorage.sol:98 rather than appended, shifting every subsequent field including the isAuthorizedModule mapping base. Harmless for a fresh deploy, but against the file's own append-only convention. Move it to the end.
• _selectorsForGroup is duplicated in RecoveryGate and CoreVault with no test asserting the two orderings agree. Silent misinstall risk, currently masked because all entries are the same address.
• docs/access-control.md §9.3 still documents Guardian rotation as the compromise playbook, and lines 102/112 still say the owner can rotate the guardian "at any time" / "indefinitely". Both false post-seal.
• test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup asserts moduleOf right after execute() without ever invoking the recovered group, and is presented as evidence AdminModule is out of reach. The role-rewrite PoC disproves that.

---

Two small things

The PR description says it is built on 0bab749. It is actually on ab51167, which is better — worth correcting so nobody reviews against the wrong base.

There is no aggregate view for the breakers: five getters, eight counting paused/pausedDeposits/pausedWithdrawals, packedFlags not exposed, and no accessor on CoreVaultLens. The frontend needs eight eth_calls to answer "why can't I withdraw". A withdrawalBreakers() struct getter would save that before I hardcode it.

Also worth knowing for the UI: a tripped instant breaker does not revert requestInstantWithdrawal, it silently falls back to the queue (EpochedQueueModule.sol:938). Good default, but the frontend has to read the flag to explain why the user got a claim instead of cash.

[kalrashivam]

Thanks for the thorough review, especially the PoCs and precise line references. I verified each finding against the implementation and addressed all P0 and P2 items.
The build is clean, and the full suite passes: 945/945 tests (938 baseline + 7 targeted additions).
P0 — Guardian could lift restrictions: fixed
pauseInstantWithdrawalOnly and pauseEpochCloseFundOnly now distinguish between tripping and clearing a breaker:
Tripping (p == true) remains available to the owner or guardian.
Clearing (p == false) is owner-only and reverts with NotOwner() for any other caller.
I used the suggested single-entry-point fix because it preserves the existing breaker API while enforcing the intended restrict-only guardian model.
Test coverage now includes:
Renaming test_guardian_canTripAndClearInstantWithdrawalBreaker to reflect that the guardian may trip, but not clear, the breaker.
Adding equivalent coverage for the epoch-close/fund breaker.
Verifying that the owner can clear either breaker, including one tripped by the guardian.
Adding test_compromisedGuardian_cannotSelfReverseItsOwnEmergencyBrake, which reproduces the worst-case scenario directly: the guardian calls guardianPause(), then both clearing attempts revert.
No documentation changes were required. governance.md §4.2, architecture.md §11.2, and access-control.md AC5 already described the correct restrict-only model; the implementation now matches them.
P0 — RecoveryGate was missing from deployment: fixed
DeployCoreSystem.s.sol now:
Deploys RecoveryGate during Phase 5, immediately after SelectorRegistry (step 5.8b).
Configures it with the approved parameters:minDelay = 21 days
cooldown = 30 days

Calls vault.setRecoveryGate() before the later fee, dead-deposit, and timelock steps.
Requires a new SECURITY_APPROVER_ADDRESS environment variable. It hard-reverts when unset, consistent with GOVERNOR_ADDRESS and GUARDIAN_ADDRESS; there is no silent fallback.
Verifies during Phase 6 that vault.recoveryGate() != address(0), preventing the script from producing a pre-seal state with recovery unwired.
Includes the RecoveryGate address in the address book, deployment summary, and next-steps output.
P2 — all five fixed
I also completed the storage-layout item that I had previously deferred:
canSeal() did not verify vault.authorizedSealer().
Added the check to _verifyLiveState, returning "SystemSealer not authorized on vault". Added test_canSeal_and_verifyAndSeal_agree_whenSystemSealerIsNotAuthorizedOnVault.

recoverModuleGroup accepted addresses without code.
Added a code.length > 0 check with the new RecoveryModuleHasNoCode error. Added test_execute_reverts_whenReplacementModuleHasNoCode.

recoveryGate was inserted mid-struct.
Moved it to the actual end of CoreStorage.Layout, after rewardsTreasury, and corrected storage-layout.md.

Selector-ordering coverage could not detect reordering.
Added test_recoverModuleGroup_selectorOrdering_matchesRecoveryGate. It assigns a distinct dummy module to each selector and verifies index-for-index agreement between RecoveryGate.selectorsForGroup() and CoreVault’s internal derivation.

Access-control documentation was inaccurate post-seal.
Corrected access-control.md §9.3 and the two “at any time”/“indefinitely” statements. Both setVetoer and setGuardian revert with SystemSealed() after sealing. The guardian-rotation playbook is now explicitly marked pre-seal-only and documents the remaining post-seal capabilities of a compromised guardian.

I also made two related cleanups:
Corrected the docstring on test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup; the test does not exercise the role-rewrite path and no longer implies otherwise.
Fixed the inaccurate AdminModule-exclusion rationale in RecoveryGate.sol NatSpec. The statement that AdminModule was not moduleOf-routed was incorrect.
P1 — all four verified; decisions needed
I confirmed all four P1 findings. I agree they should be resolved before mainnet, but they do not need to block this merge. Two require an explicit design decision before implementation:
Approver-rotation same-clock race
Confirmed—the existing test stops one call short. The available fixes have different operational consequences:
Require the rotation delay to be strictly greater than minDelay; or
Invalidate all pending proposals whenever the approver changes.
Which behavior do you prefer?

“Role relaxation is structurally impossible” is incorrect
Confirmed. Because the replacement module executes through delegatecall, it receives full access to CoreStorage, including roleOf and owner.
We can either:
Restore the unchanged-role-mapping check to the approval digest, as originally requested in review §13; or
Keep the current design and revise the NatSpec/docs to state clearly that recovery restricts selectors, not code behavior.
This changes the system’s guarantee, so I would like your ruling before proceeding.

Approved timing parameters are not enforced by the contract
The deployment script now uses 21 days and 30 days, but the constructor still permits a 14-day delay and imposes no minimum cooldown. The straightforward fix is to raise the delay floor and add a cooldown floor. I can include that in the next pass.

The funded-claim breaker is unbounded
Confirmed. This needs a public guarantee, but the mechanism is a design choice: automatic expiry or timelock-gated clearing. Let me know which model you prefer.

Remaining notes
The PR description will cite ab51167 as the base commit, not 0bab749.
The aggregate withdrawalBreakers() getter is not implemented yet. I can add it to avoid requiring eight separate eth_calls.
For frontend behavior: when the instant-withdrawal breaker is tripped, EpochedQueueModule.sol silently falls back to the queue; it does not revert. That behavior should be surfaced in any UI built around the aggregate getter.

[stefanobotticelli]

Re-reviewed at 65cbbb6. Both P0s are closed with executable evidence, four of five P2s are closed. Approving.

The three PoCs now fail, and they fail with NotOwner() — on the expected revert, not on a setup error:
[FAIL: NotOwner()] test_POC_guardianLiftsOwnerImposedInstantWithdrawalPause
[FAIL: NotOwner()] test_POC_guardianLiftsOwnerImposedEpochCloseFundPause
[FAIL: NotOwner()] test_POC_guardianSelfReversesItsOwnEmergencyPause

I also enumerated every writer of packedFlags in src/ rather than trusting function names: nine in CoreVault, all clearing operations owner-reachable only; the modules touch only the reentrancy, nav-smooth and one-shot init bits, never bits 0, 1, 2 or 13-17. No batch, reset or generic clear. No residual path.

The RecoveryGate deploy was verified end to end on a real Arbitrum One fork, not just compiled. SECURITY_APPROVER_ADDRESS reverts hard when unset, with no silent fallback.

Two things I want to record before the sealing, and one that affects deployment day.

---

Deployment day, and it isn't from this PR

foundry.toml has access = "read" in fs_permissions, and _writeAddressBook is called unconditionally at the end of the script. The script reverts on that last line — and with --broadcast it reverts before any transaction is sent, so the deploy doesn't land at all.

Pre-existing on main, but it will bite on 1 September. access = "read-write" needs to be in place before then. Adding it to the deploy checklist.

---

The codeless-module fix closes the symptom, not the substance

code.length > 0 landed in recoverModuleGroup (CoreVault.sol:408) but not in propose(). That covers the typo, not the case I was describing.

The chain that still passes:

1. propose() accepts a CREATE2 address that has no code yet
2. the digest encodes codehash == 0
3. the approver signs that digest — i.e. signs "empty contract"
4. the code is deployed at the predicted address
5. after 21 days execute() finds code.length > 0 and installs

The module reaches production without the approver ever having seen the real codehash, and nothing re-checks the digest against live codehashes at execution time. That's a bypass of the independent approval, same family as the approver-rotation P1.

Not merge-blocking, since recovery is inert while routing stays mutable. Before sealing: either code.length > 0 in propose() too, or better, recompute the digest in execute() against live codehashes.

---

Rulings on the four P1s

1. Approver rotation — invalidate pending proposals. A longer rotation delay is quantitative: it moves the window, an attacker with ROOT_TIMELOCK waits 28 days instead of 21 and retries. Invalidation is qualitative — rotate first, propose after, 42 days total, and the approver change becomes an isolated observable event instead of something hidden inside a recovery already in flight. Three lines iterating the four groups in executeApproverChange(). Add the longer delay too if it's free, but invalidation is the one that matters.

2. Role relaxation — keep the design, fix the wording. Putting the role mapping back in the digest doesn't close anything: the problem is what the module does after installation in delegatecall, not what it declares at proposal time. A module that rewrites roleOf on first invocation produces the same digest as a benign one — the codehash carries that information, not the mapping. Add it if you like for the documentary value, it costs nothing and introduces no non-determinism.

What actually needs doing is removing the word "structurally" from the three remaining claims (RecoveryGate.sol:17, CoreVault.sol:389-391, RecoveryGate.sol:370-377). That word is what makes a reader believe the process controls are optional. Recovery is a procedural mechanism, not a cryptographic one: the real defences are the 21 days, the independent approver, the veto, and reviewing the replacement module's code. The docs should say that plainly.

3. Constructor floors — add them. No technical reason not to. Right now conformance to the approved values rests on two constants in a script that no test checks, and the constructor still accepts cooldown = 0. if (_minDelay < 21 days) revert DelayTooShort(); and if (_cooldown < 30 days) revert CooldownTooShort();. The 14-day fixtures need renaming, mechanical.

4. Funded-claim breaker — automatic expiry, not a timelock on activation. The timelock protects against the wrong thing. The risk isn't the owner tripping the breaker by mistake, it's tripping it — possibly legitimately, for a bug in the claim path — and then not clearing it: lost key, compromised owner, or simply nobody remembering. A timelock on entry helps in none of those cases, and it makes the one case where the breaker genuinely matters worse, since a live bug in the claim path needs zero seconds, not 48 hours.

Expiry attacks the real problem. pauseFundedClaimOnly(true) writes fundedClaimPausedUntil = block.timestamp + 7 days; _notPausedFundedClaim() reads the timestamp instead of the bit. The breaker clears itself, and the owner can re-trip it deliberately, with a visible transaction every week — which is exactly the signal we want during a prolonged incident. The property that matters holds without depending on anyone: a user with a funded claim waits at most 7 days, whoever holds the owner key and whatever happens to them.

That is also the guarantee we want to state publicly about funded claims, so it should be enforced rather than promised.

---

Smaller things, not blocking

• securityApprover has no validation that it differs from timelock, governor, guardian or vetoer. securityApprover == timelock is accepted, and in that case the independent approval disappears entirely. The comment at :130 says "should be distinct" but doesn't enforce it.
• VaultFactory, DeployFixedMaturityVault.s.sol, DeployLib.sol and DeployQueueModule.s.sol all deploy a CoreVault without wiring RecoveryGate and without the guard. Irrelevant for 1 September since we use DeployCoreSystem, but a vault created through the factory and later sealed loses recovery permanently, silently.
• Three doc inaccuracies left: governance.md:256-257 ("Sets/clears … OWNER or GUARDIAN") is now false and contradicts the prose seven lines below — same inconsistency as before, inverted; architecture.md:652 cites the modifier, which is no longer the whole story since the clear authorisation lives in the function body; access-control.md:270 (AC5) is true but its cited evidence refers to a selector that doesn't exist, and the pause functions aren't roleOf-routed anyway — pre-existing.
• For the frontend: a Guardian attempting to clear gets NotOwner() from an onlyOwnerOrGuardian function. Correct but counterintuitive, and it needs mapping in the error decoding.

---

Worth saying: correcting the test_adminModuleRouting_survivesRecoveryOfEveryOtherGroup docstring with "Do not cite this test as disproving that", and replacing the false AdminModule routing rationale with two accurate reasons, is the right instinct. Correcting a claim rather than defending it is what makes the rest of the review trustworthy.

Merging.