feat(EXSC-413): S5 — pause (instance + global-read; withdrawals-always-open)#1982
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
S5. Add VaultWrapperPausable abstract mixin with two independent instance flags (emergencyPaused via the LI.FI emergency multisig read live from the factory, integratorPaused via the vault admin) plus a live factory.globalPaused() read. The pause guard is wired into deposit/mint only; withdraw/redeem stay structurally open under every pause combination. _requireNotPaused was removed from the shared _beforeOperation (which previously gated exits too) and replaced by _requireDepositsNotPaused on inflows — fixing the latent freeze-on-exit defect. ILiFiVaultWrapperFactory gains globalPaused()/emergencyPauser() view declarations (already satisfied by the factory's public getters). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review gaps: add a real-factory end-to-end suite proving factory.globalPause() freezes a live instance's deposits (and unpause resumes), plus the documented carve-out that a deploy while globally paused yields a frozen-from-birth instance. Cover mint() under integrator and global pause, and withdraw-open under global-pause-alone. Blank line after pragma in LiFiVaultWrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Append-only upgrade gap so the pause mixin can gain state without shifting the storage of the contracts that inherit it, consistent with the wrapper's own gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gvladika
force-pushed
the
feature/exsc-413-s5-pause-instance-global-read-withdrawals-always-open
branch
from
b0e15d6 to
f83fbb7
Compare
gvladika
changed the base branch from
feature/exsc-408-s11-access-interfaces-reference-adapter
to
feature/exsc-409-s1-core-erc-4626-immutable-args-cwia
… breaker Removes the wrapper-level emergency pause (story A13): the LI.FI emergency authority now halts an instance's deposits only via the factory-level global circuit breaker (A14), never per-instance. Integrator pause (I28) and the withdrawals-always-open guarantee (U20) are unchanged. - VaultWrapperPausable: drop emergencyPaused/emergencyPause(Unpause)/ EmergencyPauseSet/NotEmergencyPauser/onlyEmergencyPauser/_emergencyPauseAuthority; depositsPaused() = integratorPaused || _globalPaused(). - Wrapper: drop the _emergencyPauseAuthority() override (no factory.emergencyPauser() read). - ILiFiVaultWrapperFactory: drop the now-unused emergencyPauser() view (keep globalPaused()); the factory's own NotEmergencyPauser error is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iew 1+3) - maxDeposit/maxMint return 0 while any pause source is engaged, so EIP-4626 consumers see the vault as closed and don't build deposits that revert. - Move the pause gate from the deposit/mint entrypoints into the single _deposit chokepoint both route through, gating every current and future inflow by construction. Consequence: with max* == 0, OZ's deposit/mint reject a non-zero amount with ERC4626ExceededMaxDeposit/Mint before _deposit runs; the _deposit guard (DepositsPaused) is the backstop for zero-amount / non-standard inflows that clear the max check. Tests updated accordingly (+ blank-line fix per CONV:BLANKLINES). 123/123 VaultWrapper tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-413-s5-pause-instance-global-read-withdrawals-always-open # Conflicts: # src/VaultWrapper/LiFiVaultWrapper.sol
…ble mixin Dissolve the VaultWrapperPausable mixin into LiFiVaultWrapper. The mixin's indirection (integrator-pause-authority hook, requireDepositsNotPaused helper, global-paused hook, custom NotIntegratorAdmin modifier/error) added no value now that the wrapper is the only consumer. - pause()/unpause() replace integratorPause()/integratorUnpause(), gated by OZ's existing onlyOwner (the authority was always owner()). - Pause check inlined at the _deposit chokepoint; global breaker read inlined into depositsPaused(). - Storage bool integratorPaused -> paused (packs with integratorShareBps); event IntegratorPauseSet -> PauseSet. Behaviour unchanged: dual gate (max*==0 + _deposit guard) on inflows, exits always open (U20). Unreleased v1.0.0, so the storage-layout shift is a no-op. 124/124 VaultWrapper tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The instance `paused` flag is now toggled by either the per-vault owner (integrator) or the factory's emergency pauser, in either direction — a single shared flag, so one authority can lift the other's pause. Adds an `onlyPauseAuthority` modifier (owner() or factory.emergencyPauser(), read live) and a `NotPauseAuthority` error; re-adds `emergencyPauser()` to the factory interface so the wrapper can read it. The factory-level global breaker is unchanged. 127/127 VaultWrapper tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hten pause bodies Move the modifier above the functions into a dedicated Modifiers section and drop the blank line before the emit in pause()/unpause(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the pause guard from the _deposit chokepoint to the deposit()/mint() entrypoints, reverting the named DepositsPaused before super runs (code-review). Previously maxDeposit/maxMint==0 made OZ revert ERC4626ExceededMaxDeposit/Mint first, so DepositsPaused was unreachable on the normal path and callers saw a misleading capacity error. maxDeposit/maxMint still return 0 for EIP-4626 consumers. Also: PauseSet NatSpec now names both pause authorities; the LiFiVaultWrapper.t.sol factory stub gained emergencyPauser(). 127/127 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
requires-types
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
Test Coverage ReportLine Coverage: 90.20% (3602 / 3993 lines) |
| instance = LiFiVaultWrapper(_deploy(0)); | ||
| } | ||
|
|
||
| function test_GlobalPauseFromFactoryFreezesInstanceDeposits() public { |
There was a problem hiding this comment.
Nit [400-solidity-tests.md]: testRevert_ is required for any test that calls vm.expectRevert (this one does, on the DepositsPaused expectation below). Rename to testRevert_GlobalPauseFromFactoryFreezesInstanceDeposits for consistency with the rest of the suite (the MockPauseFactory tests above already follow this).
There was a problem hiding this comment.
Fixed in e479d86b6 — renamed to testRevert_GlobalPauseFromFactoryFreezesInstanceDeposits.
🔍 QA Review — EXSC-413 · S5 — Pause (instance + global-read; withdrawals-always-open)Re-review · Prior What this PR doesEXSC-413 (S5) implements the per-instance and global-circuit-breaker pause layer for Resolution status — all 7 prior items
Detailed verificationItem 1 — Shared-flag design [High] → ✅ Fixed The redesign is correct and clean. Item 2 — V2 reward-injection seam [Medium] → ✅ Fixed
/// TODO(pause): any future inflow entrypoint (e.g. the V2 reward-injection path) must
/// gate on `if (depositsPaused()) revert DepositsPaused();` like `deposit`/`mint`.Placement is logical — it lives where future inflow entrypoints will be added. Satisfies AC-05. Item 3 — Role-rotation-under-pause test [Medium] → ✅ Fixed
Item 4 — Both E2E tests correctly renamed to:
Both use Item 5 —
Item 6 — Preview consistency + Preview: NatSpec block added above Emitter: vm.expectEmit(true, true, true, true, address(wrapper));Confirmed correct — fifth argument is the emitter address per Foundry convention. Item 7 — Interface gap in Deferred to an interface-cleanup ticket. Acceptable per the original finding's explicit note. New issues introduced by changesNo new blockers or high-severity issues found. Three informational observations: Observation A —
Observation B — The test correctly verifies that calling Observation C — The E2E test helper passes Acceptance criteria coverage
Security assessment
Test coverage summary
Coverage rating: Comprehensive Downstream impactEXSC-421 (S12 — Multi-actor scenario tests): S12 depends on this PR's authority model being stable. The disjoint-authority design (integrator-only instance pause, factory-only global CB) is cleaner for multi-actor scenarios — no cross-lift to model. S12 authors should adopt EXSC-417 (S8 — Factory): S8 owns the Pre-merge-to-main reminder: Process items
✅ Verdict: PassAll 7 previously flagged items are resolved (6 fixed in code, 1 accepted as deferred with explicit product justification). The design change for item 1 correctly implements the disjoint-authority model and improves the spec alignment. Three informational observations are noted — none are blocking. The implementation satisfies all 10 acceptance criteria.
Posted by |
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on 7 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🔴 High | Design | Shared-flag pause design may diverge from canonical two-primitive spec — confirm spec alignment |
| 2 | 🟠 Medium | Code | V2 reward injection entrypoint has no pause-guard seam or TODO comment |
| 3 | 🟠 Medium | Test gap | test/solidity/VaultWrapper/VaultWrapperPause.t.sol (existing) |
| 4 | 🟡 Low | Convention | Two E2E tests use vm.expectRevert but named test_ not testRevert_ |
| 5 | 🟡 Low | Test gap | test/solidity/VaultWrapper/VaultWrapperPause.t.sol (existing) |
| 6 | 🟡 Low | Code | previewDeposit/previewMint inconsistency under pause + missing vm.expectEmit emitter |
| 7 | 🟡 Low | Documentation | pause()/unpause()/depositsPaused() absent from ILiFiVaultWrapper |
1. [High] Shared-flag pause design may diverge from canonical two-primitive spec
The implementation uses a single shared bool paused flag that both the integrator (owner) and the LI.FI emergency pauser can toggle in either direction — including lifting the other authority's pause. Reviewer mirooon references a "canonical Pause Authority Matrix" and "Legal Brief Topic 3" that define the instance pause as two separate primitives with authority-mirrored unpause. The test test_IntegratorCanLiftEmergencyPause documents this cross-lift as intentional, but if the spec requires isolation between the two authority levels, the implementation needs two separate booleans, a revised modifier, a revised depositsPaused(), and updated tests. Resolution of this item directly affects EXSC-421 (S12) test design.
Required: Spec owner to confirm in a PR comment whether (a) the shared-flag model is an accepted V1 simplification or (b) two separate storage flags are required. If (b), update LiFiVaultWrapper.sol and VaultWrapperPause.t.sol accordingly.
2. [Medium] V2 reward injection seam missing
The ticket scope states "pause covers deposits (+ V2 reward injection)". No such function exists yet, and no TODO comment exists to signal that the future entrypoint must gate on depositsPaused().
Required: Add // TODO(pause): V2 reward injection entrypoint must check: if (depositsPaused()) revert DepositsPaused(); at the appropriate extension point in src/VaultWrapper/LiFiVaultWrapper.sol.
3. [Medium] Test gap — VaultWrapperPause.t.sol
The ticket specifies "bypass cases keep working (self-withdraw, fee sweep, role rotation)". Role rotation is untested.
- Missing:
test_OwnershipTransferOpenUnderPause— verifytransferOwnership+acceptOwnershipsucceed whilepaused = trueandfactory.globalPaused = true
4. [Low] testRevert_ naming violations (×2)
Two tests in VaultWrapperGlobalPauseE2ETest call vm.expectRevert but are named test_ not testRevert_, violating [CONV:TESTS] (400-solidity-tests.md):
test_GlobalPauseFromFactoryFreezesInstanceDeposits→testRevert_GlobalPauseFromFactoryFreezesInstanceDepositstest_DeployWhileGloballyPausedYieldsFrozenInstance→testRevert_DeployWhileGloballyPausedYieldsFrozenInstance
5. [Low] Test gap — VaultWrapperPause.t.sol
- Missing:
testRevert_StrangerCannotUnpause— symmetric totestRevert_StrangerCannotPause; bothpause()andunpause()carryonlyPauseAuthority, only one side is tested.
6. [Low] previewDeposit/previewMint inconsistency under pause + missing emitter
previewDeposit()/previewMint() return positive share estimates when depositsPaused() is true, while deposit()/mint() revert. EIP-4626 requires previews to reflect what the actual call would do. Add a NatSpec note (or revert when paused); add test_PreviewDepositReturnsBehaviourWhenPaused.
Also: test_PauseSettersEmitEvents calls vm.expectEmit(true, true, true, true) without the 5th emitter arg — should be vm.expectEmit(true, true, true, true, address(wrapper)).
7. [Low] Pause interface gap in ILiFiVaultWrapper
pause(), unpause(), depositsPaused(), and PauseSet are absent from ILiFiVaultWrapper. Callers holding an interface reference cannot access these without a cast. Process note for the interface-cleanup ticket — acceptable to defer if that is planned.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
…obal breaker Per review (mirooon / QA): the shared instance-pause flag diverged from the authoritative Pause Authority Matrix + Legal Topic 3. New model: the integrator (owner) is the sole authority over the instance `paused` flag; LI.FI's only lever over a clone is the factory-level global circuit breaker. This makes I28 'no LI.FI involvement' hold and removes the cross-lift. - pause()/unpause() gated by OZ onlyOwner; drop onlyPauseAuthority modifier and NotPauseAuthority error; drop emergencyPauser() from the factory interface (no longer read by the wrapper). depositsPaused() = paused || factory.globalPaused(). Also addresses review nits: - rename two E2E tests to testRevert_ (call vm.expectRevert) [400-solidity-tests] - add testRevert_StrangerCannotUnpause; add test_OwnershipTransferOpenUnderPause - add address(wrapper) emitter to test_PauseSettersEmitEvents - NatSpec: previews intentionally ignore pause per EIP-4626; TODO(pause) seam for the future V2 reward-injection inflow 126/126 VaultWrapper tests pass; solhint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Addressed in
126/126 VaultWrapper tests pass; solhint clean. Re-applying the Agent Review Request label for re-review. Note: |
github-actions
Bot
added
QA AI Reviewing
…into feature/exsc-413-s5-pause-instance-global-read-withdrawals-always-open
There was a problem hiding this comment.
Re-review complete — all 7 CHANGES_REQUESTED items resolved; EXSC-413 S5 pause module approved at HEAD e479d86.
mirooon
merged commit 8bd4bc7
into
feature/exsc-409-s1-core-erc-4626-immutable-args-cwia
mirooon
deleted the
feature/exsc-413-s5-pause-instance-global-read-withdrawals-always-open
branch
3 participants
Which Linear task belongs to this PR?
EXSC-413 — S5: Pause (instance + global-read; withdrawals-always-open)
Stacked PR. Base is
feature/exsc-409-s1-core-erc-4626-immutable-args-cwia(#1992, S1). Review the incremental diff against that base; do not merge before S1 lands. Stack: S1 → S5.Why did I implement it this way?
S5 is the clone-side pause: enforcement plus the live read of the factory-level circuit breaker. The factory's global flag and the emergency-pauser role already exist from S1 (
globalPause()/globalUnpause()gated byonlyEmergencyPauser, plusglobalPaused()); this PR makes instances honour that flag and adds the integrator's own per-clone pause.paused) or the factory's global breaker is engaged.depositsPaused() = paused || factory.globalPaused(), read live so toggling the factory breaker propagates to every instance with no clone change.pause()/unpause()are callable solely by the per-vaultowner(thevaultWrapperAdmin, i.e. the integrator), enforced by OZ'sonlyOwner. LI.FI has no per-instance pause — its authority acts only through the factory-level global circuit breaker (globalPaused), which an integrator cannot clear. This keeps the two authorities' scopes disjoint: the integrator owns its clone's pause (I28 "no LI.FI involvement" holds), and LI.FI owns the global halt (A14). (Reverts an earlier shared-flag iteration that let either authority toggle the instance flag — flagged in review as diverging from the Pause Authority Matrix / Legal Topic 3.)deposit()/mint()revertDepositsPausedbefore doing anything else. It is not in the shared_beforeOperation()hook (access + accrual), which also runs onwithdraw/redeem— gating there would freeze user exits during an emergency, the opposite of a circuit breaker. Self-withdraw therefore works under every pause combination.maxDeposit/maxMintreport 0 while paused. EIP-4626 consumers see the vault as closed and don't build deposits that would revert. The namedDepositsPausedis raised at thedeposit/mintentrypoints (before OZ'smaxcheck), so callers get the real reason rather than OZ'sERC4626ExceededMaxDeposit/Mint; themax*overrides remain purely for off-chain consumers reading the capacity views.paused), thePauseSetevent, theDepositsPausederror, thepause/unpausecontrols, and thedepositsPaused()view all live directly inLiFiVaultWrapper. An earlier iteration used an abstractVaultWrapperPausablemixin with virtual authority/global hooks; with the wrapper as the only consumer that indirection earned nothing, so it was inlined and the mixin removed.globalPaused()— a view declaration already satisfied by the factory's existing public-getter state var, so no factory code change and no version bump (the subsystem is unreleased).Tests
VaultWrapperPause.t.sol— unit tests against aMockPauseFactorythat deploys the proxy (so it is thefactorythe instance reads back) and exposes the toggleable global flag, plus 3 end-to-end tests against the realLiFiVaultWrapperFactory:DepositsPaused, including a zero-amount deposit (the entrypoint guard is unconditional);transferOwnership/acceptOwnership) also stays open under pause;pause()/unpause()— a stranger cannot (OwnableUnauthorizedAccount), tested for both pause and unpause; an instanceunpause()cannot clear the factory's global breaker;unpause()resumes deposits;maxDeposit/maxMintgo to 0 while paused and are restored after unpause; thedepositsPaused()view reflects each source;PauseSetis emitted;factory.globalPause()freezes an existing instance's deposits,globalUnpause()resumes them, and deploying while globally paused yields an instance frozen from birth (the documented carve-out).The existing
LiFiVaultWrapper.t.sol(whosefactoryis the test contract) carries aglobalPaused()shim so its deposits still resolve.Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.mdChecklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)
Maintainer note (propagation merge). This branch was updated by merging the latest S1 (#1992), which brings: the
Ownable2Stepadmin refactor, theInitialized→VaultWrapperConfiguredevent rename, theenforceLibAssetRouting+solc_floorCI carve-outs for the VaultWrapper subsystem, theIYieldAdapterversion reset to1.0.0, and the latestmain(viadev-vault-wrapper). Local/pr-readywas bypassed for this push (PR_READY_OK=1) because the diff is mechanical propagation of already-reviewed S1 and upstreammaincode; CI CodeRabbit still runs as the safety net. All 126 VaultWrapper tests pass and thesolc_floor(0.8.17) build is green.