Issue 93 - #105
Conversation
Test Coverage: 1. Guardian Role Management (3 tests) 2. Pause All Functionality (4 tests) 3. Unpause All Functionality (4 tests) 4. Paused State Enforcement (7 tests) 5. Individual Contract Pause Access Control (6 tests) 6. Integration Scenarios (5 tests)
Closes #93
…vaultType variable and inefficient contains+remove pattern and Unnecessary Zero Assignments
Reviewer's GuideThis PR integrates an emergency pause/unpause framework led by a new guardian role (alongside admin) across OrionConfig, vaults, and orchestrators, refactors vault decommissioning cleanup to use boolean-returning remove(), and removes redundant state initializations in OrionVault. Sequence diagram for protocol emergency pause flowsequenceDiagram
actor Admin
participant OrionConfig
participant InternalStatesOrchestrator
participant LiquidityOrchestrator
participant OrionVault
Admin->>OrionConfig: setGuardian(address)
Guardian->>OrionConfig: pauseAll()
OrionConfig->>InternalStatesOrchestrator: pause()
OrionConfig->>LiquidityOrchestrator: pause()
OrionConfig->>OrionVault: pause() (for each vault)
OrionConfig-->>Guardian: emit ProtocolPaused(guardian)
Admin->>OrionConfig: unpauseAll()
OrionConfig->>InternalStatesOrchestrator: unpause()
OrionConfig->>LiquidityOrchestrator: unpause()
OrionConfig->>OrionVault: unpause() (for each vault)
OrionConfig-->>Admin: emit ProtocolUnpaused(admin)
Class diagram for emergency pause integrationclassDiagram
class OrionConfig {
+address admin
+address guardian
+function setGuardian(address)
+function pauseAll()
+function unpauseAll()
}
class OrionVault {
+function pause()
+function unpause()
+requestDeposit(uint256) whenNotPaused
+cancelDepositRequest(uint256) whenNotPaused
+requestRedeem(uint256) whenNotPaused
+cancelRedeemRequest(uint256) whenNotPaused
}
class InternalStatesOrchestrator {
+function pause()
+function unpause()
+performUpkeep(bytes) whenNotPaused
}
class LiquidityOrchestrator {
+function pause()
+function unpause()
+performUpkeep(bytes) whenNotPaused
}
OrionConfig --> OrionVault : calls pause()/unpause()
OrionConfig --> InternalStatesOrchestrator : calls pause()/unpause()
OrionConfig --> LiquidityOrchestrator : calls pause()/unpause()
OrionConfig <|-- Ownable2Step
OrionVault <|-- ERC4626
OrionVault <|-- ReentrancyGuard
OrionVault <|-- Pausable
InternalStatesOrchestrator <|-- Ownable2Step
InternalStatesOrchestrator <|-- ReentrancyGuard
InternalStatesOrchestrator <|-- Pausable
LiquidityOrchestrator <|-- Ownable2Step
LiquidityOrchestrator <|-- ReentrancyGuard
LiquidityOrchestrator <|-- Pausable
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request adds emergency pause/unpause capabilities to the Orion protocol. It introduces a guardian role in OrionConfig, extends InternalStatesOrchestrator and LiquidityOrchestrator with Pausable inheritance, guards critical performUpkeep functions with whenNotPaused modifiers, and includes comprehensive test coverage for pause scenarios and access control validation. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant OrionConfig
participant InternalStatesOrch
participant LiquidityOrch
Admin->>OrionConfig: setGuardian(newGuardian)
OrionConfig-->>Admin: GuardianUpdated event
Note over Admin,LiquidityOrch: Pause Scenario
Admin->>OrionConfig: pauseAll()
OrionConfig->>InternalStatesOrch: pause()
InternalStatesOrch-->>OrionConfig: ✓
OrionConfig->>LiquidityOrch: pause()
LiquidityOrch-->>OrionConfig: ✓
OrionConfig-->>Admin: ProtocolPaused event
Note over InternalStatesOrch,LiquidityOrch: While Paused
Admin->>InternalStatesOrch: performUpkeep()
InternalStatesOrch-->>Admin: ✗ EnforcedPause
Note over Admin,LiquidityOrch: Unpause Scenario
Admin->>OrionConfig: unpauseAll()
OrionConfig->>InternalStatesOrch: unpause()
InternalStatesOrch-->>OrionConfig: ✓
OrionConfig->>LiquidityOrch: unpause()
LiquidityOrch-->>OrionConfig: ✓
OrionConfig-->>Admin: ProtocolUnpaused event
Admin->>InternalStatesOrch: performUpkeep()
InternalStatesOrch-->>Admin: ✓
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/OrionConfig.sol:426-435` </location>
<code_context>
+ function unpauseAll() external onlyAdmin {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** UnpauseAll may revert if any orchestrator or vault is already unpaused.
Consider checking the paused state before calling _unpause(), or handling already-unpaused contracts with try/catch, if idempotent behavior is preferred.
</issue_to_address>
### Comment 2
<location> `contracts/vaults/OrionVault.sol:690-692` </location>
<code_context>
+
+ /// @notice Pauses the contract
+ /// @dev Can only be called by OrionConfig for emergency situations
+ function pause() external {
+ if (msg.sender != address(config)) revert ErrorsLib.UnauthorizedAccess();
+ _pause();
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Pause and unpause functions do not restrict repeated calls.
Consider adding a check for the current paused state before calling _pause() or _unpause() to make these functions idempotent.
Suggested implementation:
```
/// @notice Pauses the contract
/// @dev Can only be called by OrionConfig for emergency situations
function pause() external {
if (msg.sender != address(config)) revert ErrorsLib.UnauthorizedAccess();
if (paused()) revert ErrorsLib.AlreadyPaused();
_pause();
}
/// @notice Unpauses the contract
/// @dev Can only be called by OrionConfig for emergency situations
function unpause() external {
if (msg.sender != address(config)) revert ErrorsLib.UnauthorizedAccess();
if (!paused()) revert ErrorsLib.NotPaused();
_unpause();
}
```
You will need to ensure that `ErrorsLib.AlreadyPaused()` and `ErrorsLib.NotPaused()` exist in your error library. If not, you should add these custom errors to `ErrorsLib`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
contracts/interfaces/ILiquidityOrchestrator.sol (1)
96-103: Pause/unpause interface additions look consistent, but note source-level breakageThe new
pause()/unpause()surface is consistent with the protocol-wide emergency controls and the docs clearly scope authority to OrionConfig. This is a source-breaking change for any existingILiquidityOrchestratorimplementations (including older mocks), so make sure all implementers are updated accordingly.If downstream consumers need to check paused state through the interface rather than concrete types, consider adding a
function paused() external view returns (bool);in a follow-up.contracts/interfaces/IInternalStateOrchestrator.sol (1)
109-115: Emergency pause controls added cleanly to internal state orchestrator interfaceAdding
pause()/unpause()here keeps InternalStateOrchestrator aligned with the protocol-wide emergency control pattern and the docs clearly state OrionConfig as the caller.This does change the interface surface, so ensure the concrete orchestrator implementation and any test doubles/mocks are updated. As with the liquidity orchestrator, a
paused()view in the interface could be useful if callers ever need to branch on paused state via the interface type.contracts/interfaces/IOrionVault.sol (1)
214-220: Pause/unpause additions align with protocol design; consider grouping under config/emergency sectionThe new
pause()/unpause()functions give OrionConfig a clear emergency control surface on each vault and match the ABI artifacts.Two minor points to keep in mind:
- This is a source-breaking change for any external
IOrionVaultimplementers; ensure all concrete vaults and mocks are updated.- For readability, these OrionConfig-only controls might be clearer if grouped under a dedicated “config/emergency” section rather than immediately after the liquidity-orchestrator-related
accrueCuratorFees, since they are not callable by the orchestrator.Function signatures and docs themselves look good.
contracts/OrionConfig.sol (1)
5-5: Consider removing unused import.The Pausable.sol import appears unnecessary since OrionConfig doesn't inherit from Pausable. The pause/unpause calls on orchestrators and vaults use their interface methods, which don't require this import.
Apply this diff to remove the unused import:
-import "@openzeppelin/contracts/utils/Pausable.sol";test/ProtocolPause.test.ts (3)
570-586: Assert the initialperformUpkeepsucceeds to isolate the pause effectIn
“should block epoch progression when paused”, the first call tointernalStatesOrchestrator.performUpkeep("0x")is executed but not asserted. If that call ever reverts for unrelated reasons, the test will fail before even exercising the pause logic, and if it silently stops doing anything, the test still passes as long as the later paused call reverts.Consider asserting the pre‑pause call explicitly so the test isolates the pause behavior:
- // Start an epoch - await internalStatesOrchestrator.connect(automationRegistry).performUpkeep("0x"); + // Start an epoch – should work before pause + await expect( + internalStatesOrchestrator.connect(automationRegistry).performUpkeep("0x"), + ).to.not.be.reverted;This keeps the intent clear and makes the failure mode more informative if the non‑paused path regresses.
271-277: Drop thevoidoperator onexpect(...)chains for claritySeveral assertions use
void expect(await contract.paused()).to.be.true/false;. Thevoidoperator is unnecessary here: the assertion side‑effects already happen when accessing the chained properties, and any failure will still throw.For readability and to match common test style, you can simplify these to:
- void expect(await internalStatesOrchestrator.paused()).to.be.true; + expect(await internalStatesOrchestrator.paused()).to.be.true; - void expect(await transparentVault.paused()).to.be.false; + expect(await transparentVault.paused()).to.be.false;(and similarly for the other paused/unpaused checks). This removes noise without changing behavior.
Also applies to: 282-286, 292-295, 303-308, 321-327, 333-336, 342-345, 352-355, 532-543
302-303: Avoid hard‑coded vault type enum literal in testsThe tests rely on
getAllOrionVaults(0)with an inline comment// VaultType.Transparent = 0. If the enum ordering inVaultTypeever changes, this will silently become incorrect.Consider at least centralizing this as a named constant in the test file:
- const allTransparentVaults = await config.getAllOrionVaults(0); // VaultType.Transparent = 0 + const VAULT_TYPE_TRANSPARENT = 0; + const allTransparentVaults = await config.getAllOrionVaults(VAULT_TYPE_TRANSPARENT);This keeps usage self‑documenting and reduces the chance of mismatches if the enum evolves.
Also applies to: 350-351
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(7 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(1 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(2 hunks)artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json(2 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(2 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(2 hunks)artifacts/contracts/libraries/EventsLib.sol/EventsLib.json(3 hunks)artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json(1 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(5 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(1 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(5 hunks)contracts/OrionConfig.sol(4 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/libraries/EventsLib.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(4 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(4 hunks)contracts/vaults/OrionVault.sol(8 hunks)test/ProtocolPause.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
contracts/orchestrators/InternalStatesOrchestrator.sol (2)
test/orchestrator/OrchestratorPerformUpkeep.test.ts (3)
it(635-744)it(746-2549)upkeepNeeded(2245-2250)test/orchestrator/Orchestrators.test.ts (5)
epochDuration(2180-2190)internalStatesOrchestrator(2467-2476)it(2456-2540)expect(2498-2502)upkeepNeeded(2150-2155)
test/ProtocolPause.test.ts (2)
test/orchestrator/Orchestrators.test.ts (1)
it(2456-2540)test/orchestrator/OrchestratorPerformUpkeep.test.ts (2)
it(746-2549)it(635-744)
contracts/vaults/OrionVault.sol (2)
test/orchestrator/OrchestratorPerformUpkeep.test.ts (1)
ABSOLUTE_VAULT_DEPOSIT(90-2550)test/OrionConfigVault.test.ts (2)
it(556-566)it(337-373)
contracts/orchestrators/LiquidityOrchestrator.sol (2)
test/orchestrator/OrchestratorPerformUpkeep.test.ts (6)
liquidityUpkeepNeeded(2323-2332)liquidityUpkeepNeeded(2313-2321)_liquidityUpkeepNeeded(2334-2342)it(635-744)liquidityUpkeepNeeded(2301-2311)it(746-2549)test/orchestrator/Orchestrators.test.ts (3)
liquidityUpkeepNeeded(2218-2226)liquidityUpkeepNeeded(2228-2237)_liquidityUpkeepNeeded(2239-2247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (27)
artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
252-253: Expected artifact recompilation with maintained backward compatibility.The bytecode and deployedBytecode have been updated, which is expected when contracts are recompiled. The ABI (lines 5–251) remains unchanged, confirming that the public interface is unaffected. This maintains backward compatibility for all contract callers and existing integrations.
artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
172-173: Artifact bytecode changes expected from source recompilation.The bytecode and deployedBytecode have been updated (lines 172–173), which is the expected result of recompiling the contract after source-level changes elsewhere in the codebase (e.g., OrionConfig guardian role integration for pause controls). The ABI definition (lines 5–171) remains unchanged, preserving the contract's public interface and ensuring backward compatibility.
artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json (1)
178-179: Bytecode regeneration expected; ABI remains stable.The bytecode has been regenerated (lines 178–179), which is expected when the Solidity compiler processes the contract following dependency updates (pause/unpause functionality additions mentioned in the PR objectives). Importantly, the contract's ABI remains unchanged across all function signatures, events, and error types, confirming that the contract's public interface is stable and backward compatible.
To confirm the recompilation is intentional and the contract behaves as expected, please verify:
- That the underlying Solidity source for
KBestTvlWeightedAverageInvalidremains functionally unchanged- That test coverage for this contract passes with the updated bytecode
artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json (1)
852-853: Bytecode regeneration is expected and requires no action.The source file
contracts/mocks/MockERC4626Asset.solwas not modified in this PR commit. The bytecode regeneration in the artifact is a normal byproduct of the project's full recompilation when other contracts were updated (specifically when Pausable was added to core protocol contracts in commit 93ca4b8).Since MockERC4626Asset is a test helper mock with no pause functionality required, no source code changes are needed. The artifact update is consistent with standard build behavior.
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
222-223: Artifact-only bytecode update; ABI remains stableOnly
bytecodeanddeployedBytecodechanged; ABI and link references are untouched, so interface-level behavior forKBestTvlWeightedAveragestays the same. Assuming this was regenerated from the updated sources with the project’s standard build, this looks good.artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
110-111: Recompiled artifact with unchanged ABIThe diff only updates
bytecodeanddeployedBytecode; the ABI and structure are identical. This is consistent with recompilingOrionAssetERC4626PriceAdapterafter upstream contract/library changes and is safe from an integration standpoint.contracts/libraries/EventsLib.sol (1)
49-59: New guardian/protocol pause events are well-scoped
GuardianUpdated,ProtocolPaused, andProtocolUnpausedare clearly documented and give a clean, centralized event surface for monitoring protocol-wide emergency actions. Signatures (single indexed address) are reasonable and align with the new OrionConfig pause/guardian controls.artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (1)
787-793: Vault ABI updated with pause/unpause; matches interface changesThe ABI additions for
pauseandunpauseare additive and align with the new functions incontracts/interfaces/IOrionVault.sol(no params, nonpayable). This is safe for integrators at the ABI level, but make sure all deployed/compiled vault implementations and test mocks now implement these functions.Also applies to: 1069-1075
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
823-829: Transparent vault ABI gains pause/unpause in line with protocol controlsThe new
pause/unpauseentries are additive ABI changes and align with the emergency controls you’re rolling out across vaults. As withIOrionVault, this is fine for callers but requires allIOrionTransparentVaultimplementations/mocks to include these functions.Also applies to: 1130-1136
artifacts/contracts/interfaces/ILiquidityOrchestrator.sol/ILiquidityOrchestrator.json (1)
69-75: LGTM! Interface additions align with emergency pause pattern.The pause() and unpause() function signatures are correctly defined in the ABI, enabling external callers to invoke emergency pause controls on the LiquidityOrchestrator.
Also applies to: 195-201
artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (1)
203-209: LGTM! Interface additions align with emergency pause pattern.The pause() and unpause() function signatures are correctly defined in the ABI, enabling external callers to invoke emergency pause controls on the InternalStateOrchestrator.
Also applies to: 249-255
artifacts/contracts/libraries/EventsLib.sol/EventsLib.json (1)
51-63: LGTM! Event definitions support protocol pause observability.The three new events (GuardianUpdated, ProtocolPaused, ProtocolUnpaused) provide proper observability for protocol-wide pause operations. The indexed parameters enable efficient filtering by guardian/pauser address.
Also applies to: 234-259
contracts/vaults/OrionVault.sol (3)
8-8: LGTM! Pausable inheritance follows OpenZeppelin patterns.The Pausable contract is correctly imported and added to the inheritance chain, enabling emergency pause controls for the vault.
Also applies to: 41-41
347-347: LGTM! User-facing LP functions are properly protected.The whenNotPaused modifier is correctly applied to all user-facing deposit and redemption request functions (requestDeposit, cancelDepositRequest, requestRedeem, cancelRedeemRequest). This prevents users from initiating or canceling requests during an emergency pause while preserving the system's ability to complete existing requests via orchestrator operations.
Also applies to: 370-370, 396-396, 419-419
688-700: LGTM! Pause/unpause access control is correctly restricted.The pause() and unpause() functions correctly restrict access to the OrionConfig contract (via
msg.sender != address(config)check), establishing a centralized emergency control point. The revert withErrorsLib.UnauthorizedAccess()provides clear error messaging.contracts/orchestrators/InternalStatesOrchestrator.sol (3)
6-6: LGTM! Pausable inheritance follows OpenZeppelin patterns.The Pausable contract is correctly imported and added to the inheritance chain, enabling emergency pause controls for the orchestrator.
Also applies to: 30-30
273-273: LGTM! performUpkeep correctly protected with whenNotPaused.The whenNotPaused modifier is appropriately added to performUpkeep, preventing state processing operations during an emergency pause. The modifier is correctly combined with existing access control (onlyAuthorizedTrigger) and reentrancy protection (nonReentrant).
730-742: LGTM! Pause/unpause access control is correctly restricted.The pause() and unpause() functions correctly restrict access to the OrionConfig contract (via
msg.sender != address(config)check), establishing a centralized emergency control point consistent with the vault implementation.contracts/orchestrators/LiquidityOrchestrator.sol (3)
6-6: LGTM! Pausable inheritance follows OpenZeppelin patterns.The Pausable contract is correctly imported and added to the inheritance chain, enabling emergency pause controls for the liquidity orchestrator.
Also applies to: 29-29
327-327: LGTM! performUpkeep correctly protected with whenNotPaused.The whenNotPaused modifier is appropriately added to performUpkeep, preventing liquidity operations (buy/sell orders, deposit/redeem fulfillment) during an emergency pause. The modifier is correctly combined with existing access control (onlyAuthorizedTrigger) and reentrancy protection (nonReentrant).
514-526: LGTM! Pause/unpause access control is correctly restricted.The pause() and unpause() functions correctly restrict access to the OrionConfig contract (via
msg.sender != address(config)check), establishing a centralized emergency control point consistent with other orchestrator and vault implementations.contracts/OrionConfig.sol (3)
36-37: LGTM! Guardian role properly implemented.The guardian address state variable and setGuardian function are correctly implemented with admin-only access control. The GuardianUpdated event provides proper observability for guardian changes.
Also applies to: 393-396
356-363: LGTM! Improved decommissioning logic.The refactored logic uses the return value from
remove()directly instead of callingcontains()followed byremove(), improving gas efficiency. The logic correctly handles cases where the vault is in either the encrypted or transparent vault list.
401-402: Access control asymmetry is intentional and aligns with security best practices.Verification confirms the pauseAll/unpauseAll asymmetry is correct: guardian and admin roles have narrowly-scoped pause/freeze rights with no unilateral unpause ability, while admin holds full-power recovery actions gated by governance oversight. The implementation matches established DeFi security patterns (Aave, Compound) where emergency actors retain only the minimal abilities needed to stop loss, without recovery authority.
artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (1)
224-233: LGTM! Artifact correctly reflects Pausable integration.The OrionVault artifact correctly includes:
- EnforcedPause and ExpectedPause error types from OpenZeppelin Pausable
- Paused and Unpaused events
- pause(), unpause(), and paused() function signatures
These additions align with the implementation changes in OrionVault.sol.
Also applies to: 489-501, 602-614, 1281-1300
artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
108-120: Guardian + global pause/unpause ABI looks consistent with intended designThe added events (
GuardianUpdated,ProtocolPaused,ProtocolUnpaused),guardian()accessor, andsetGuardian,pauseAll,unpauseAllfunctions are coherently wired in the ABI and line up with how the tests use them (event args and call signatures). I don’t see ABI‑level issues here; assuming these artifacts are compiler‑generated, they look good to ship.Also applies to: 198-223, 453-465, 658-664, 789-801, 951-957
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
48-57: Pause/unpause ABI and errors align with Pausable semantics and testsThe additions of
EnforcedPause/ExpectedPauseerrors,Paused/Unpausedevents, andpause(),paused(),unpause()functions giveLiquidityOrchestratora clean Pausable surface. These match how the new tests interact with the contract (checkingpaused()and expectingEnforcedPauseon upkeep when paused), so from an ABI perspective this looks correct and consistent.Also applies to: 196-208, 215-227, 556-575, 798-804
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/OrionConfig.sol (1)
35-37: Guardian + pauseAll/unpauseAll wiring is coherent; consider idempotency and setup guardsAccess control is consistent with the intended model (admin sets guardian; guardian or admin can pause; only admin can unpause; orchestrators restrict pause/unpause to
OrionConfigviaonlyConfig). Two follow‑ups to consider:
pauseAll/unpauseAllwill revert once orchestrators are already in the target state because OpenZeppelin’sPausablereverts on redundant_pause()/_unpause(). If you expect scripts or operators to call these functions idempotently, consider tracking a protocol‑level paused flag inOrionConfig, or checking orchestrator state (and skipping calls) instead of always invoking pause/unpause.- If
pauseAll/unpauseAllare called beforeinternalStatesOrchestratororliquidityOrchestratorare configured, the external calls will revert againstaddress(0). A simplerequire(internalStatesOrchestrator != address(0) && liquidityOrchestrator != address(0), ...)would make the failure mode clearer.Also applies to: 386-414
🧹 Nitpick comments (3)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
6-7: Pausable integration is sound; consider makingcheckUpkeeppause‑awareThe LiquidityOrchestrator side of the pause mechanism is wired cleanly:
- Inheriting
Pausable, guardingperformUpkeepwithwhenNotPaused, and restrictingpause()/unpause()toonlyConfigaligns with theOrionConfig.pauseAll/unpauseAllentry points and prevents EOAs from pausing directly.One behavioral refinement to consider:
checkUpkeepignores the paused state, so while the protocol is paused it can still returnupkeepNeeded = true, but any subsequentperformUpkeepfrom an authorized caller will revert withEnforcedPause. For smoother Chainlink Automation behavior and less wasted gas during an emergency pause, you could short‑circuit at the top ofcheckUpkeepwith apaused()check (e.g., return(false, "")when paused).Also decide explicitly whether admin‑only operations like
depositLiquidity,withdrawLiquidity, andclaimProtocolFeesshould remain callable during a protocol pause (current behavior) or also be gated bywhenNotPaused.Also applies to: 29-30, 324-337, 512-520
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
6-7: Pause wiring matches LiquidityOrchestrator; consider aligningcheckUpkeepwith paused stateThe InternalStatesOrchestrator’s pause integration looks consistent:
- Inheriting
Pausable, addingpause()/unpause()restricted toonlyConfig, and gatingperformUpkeepwithwhenNotPausedall align with the protocol‑widepauseAll/unpauseAllflow and centralize control inOrionConfig.As with the liquidity side,
checkUpkeepdoes not consider the paused state, so while paused it can still reportupkeepNeeded = true(based on timing or phase), but anyperformUpkeepattempt by an authorized trigger will revert withEnforcedPause. If you want Chainlink Automation to back off cleanly while the protocol is paused, consider an early:if (paused()) { return (false, ""); }in
checkUpkeep.You may also want to document how
paused()interacts withcurrentPhaseandconfig.isSystemIdle()so operators understand that pausing mid‑epoch freezes the phase without resetting it.Also applies to: 30-31, 151-155, 271-284, 729-737
test/ProtocolPause.test.ts (1)
302-334: Consider adding explicit vault behavior checks while pausedThese tests thoroughly verify:
- Guardian/admin access to
pauseAll/unpauseAll- Orchestrators’
paused()flagsperformUpkeepreverting under pause and resuming after unpause- Access control on orchestrators’ direct
pause/unpausecalls- Basic integration flows across multiple pause/unpause cycles
Given the PR’s goal of enforcing the pause across vault deposit/redeem flows as well, it would be useful to add a couple of cases that:
- Assert user‑facing vault operations such as
requestDeposit/requestRedeem(and possibly fulfill paths) revert while the protocol is paused, and- Verify that the same operations succeed again after
unpauseAll.That will catch any regressions in how vault‑level pause hooks are wired into the global emergency pause.
Also applies to: 382-461
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(7 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(5 hunks)artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json(1 hunks)artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json(1 hunks)contracts/OrionConfig.sol(3 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(5 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(4 hunks)contracts/vaults/OrionVault.sol(1 hunks)test/ProtocolPause.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/vaults/OrionVault.sol
- artifacts/contracts/test/KBestTvlWeightedAverageInvalid.sol/KBestTvlWeightedAverageInvalid.json
- artifacts/contracts/OrionConfig.sol/OrionConfig.json
🧰 Additional context used
🧬 Code graph analysis (2)
test/ProtocolPause.test.ts (2)
test/orchestrator/Orchestrators.test.ts (1)
it(2456-2540)test/orchestrator/OrchestratorPerformUpkeep.test.ts (2)
it(746-2549)it(635-744)
contracts/orchestrators/LiquidityOrchestrator.sol (2)
test/orchestrator/OrchestratorPerformUpkeep.test.ts (5)
liquidityUpkeepNeeded(2323-2332)liquidityUpkeepNeeded(2313-2321)_liquidityUpkeepNeeded(2334-2342)it(635-744)liquidityUpkeepNeeded(2301-2311)test/orchestrator/Orchestrators.test.ts (3)
liquidityUpkeepNeeded(2218-2226)liquidityUpkeepNeeded(2228-2237)_liquidityUpkeepNeeded(2239-2247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Build, Lint and Test
🔇 Additional comments (4)
artifacts/contracts/strategies/KBestTvlWeightedAverage.sol/KBestTvlWeightedAverage.json (1)
222-223: Artifact ABI unchanged; bytecode refresh looks fineOnly
bytecodeanddeployedBytecodechanged while the ABI and metadata stayed the same, which is consistent with a normal recompile. No interface-level concerns here.contracts/OrionConfig.sol (1)
352-370: Simplified vault decommissioning removal logic looks correctSwitching to
encryptedVaults.remove(vault)and falling back totransparentVaults.remove(vault)(reverting if both return false) preserves the previous behavior while being simpler and slightly cheaper. GivenremoveOrionVaultonly queues vaults that are already recognized as Orion vaults, the invariants still hold.test/ProtocolPause.test.ts (1)
85-218: Realistic protocol fixture inbeforeEachlooks solidThe test setup builds a full environment (config, both orchestrators, price adapter registry, ERC4626 asset, adapters, vault factory + vault, and seeded liquidity) in the intended wiring order (config → liquidity orchestrator → internal states orchestrator), which is exactly what you want for exercising protocol‑level pause flows. This should give good confidence that the pause behavior matches real deployments.
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
1-892: Focus code review on source files rather than compiled artifacts.This is a compiled contract artifact—the ABI, bytecode, and deployedBytecode are auto-generated outputs. While the additions (EnforcedPause/ExpectedPause errors, Paused/Unpaused events, pause/paused/unpause functions) correctly reflect the emergency pause mechanism, the substantive review should focus on the source Solidity files:
LiquidityOrchestrator.sol,InternalStatesOrchestrator.sol, andOrionConfig.sol.The artifact itself is valid; the real concerns are in the implementation: access control validation, whenNotPaused modifier placement, orchestrator coordination, and integration with the guardian role. These can only be assessed in the source code.
2 participants
Summary by Sourcery
Introduce a protocol-wide emergency pause mechanism, integrate OpenZeppelin’s Pausable into core contracts, and simplify vault decommissioning logic
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Tests