Upgradability - #119
Conversation
…egistry - Converted OrionConfig to OrionConfigUpgradeable using UUPS pattern - Converted PriceAdapterRegistry to PriceAdapterRegistryUpgradeable - Changed immutable ADMIN to regular storage variable - Replaced constructors with initialize() functions - Added _authorizeUpgrade() for owner-controlled upgrades - Added 50-slot storage gaps for future upgrades - Used standard EnumerableSet (no upgradeable version in OZ v5) and Added @openzeppelin/contracts-upgradeable@^5.4.0 and @openzeppelin/hardhat-upgrades for UUPS and Beacon proxy patterns
- Converted InternalStatesOrchestrator to upgradeable version - Converted LiquidityOrchestrator to upgradeable version - Changed all base contracts to upgradeable versions: - Ownable2Step → Ownable2StepUpgradeable - ReentrancyGuard → ReentrancyGuardUpgradeable - Pausable → PausableUpgradeable - Added UUPS pattern with _authorizeUpgrade() - Replaced constructors with initialize() functions - Added 50-slot storage gaps
- Created OrionVaultUpgradeable abstract base contract - Created OrionTransparentVaultUpgradeable concrete implementation - Converted from ERC4626 to ERC4626Upgradeable - Changed ReentrancyGuard to ReentrancyGuardUpgradeable - Replaced constructors with initialize() functions - Used __OrionVault_init() for internal initialization - NO UUPS logic - upgrades handled by Beacon pattern - Added 50-slot storage gaps This enables all vault instances to share a single upgradeable implementation via UpgradeableBeacon, allowing protocol-wide vault upgrades with a single transaction.
- Converted TransparentVaultFactory to UUPS upgradeable pattern - Added vaultBeacon state variable for UpgradeableBeacon reference - Modified createVault() to deploy BeaconProxy instances instead of direct contracts - Encodes initialization data and passes to BeaconProxy constructor - Added setVaultBeacon() function for updating beacon address - Storage gap: 49 slots (50 - 1 for vaultBeacon) Factory now deploys all vaults as BeaconProxy instances pointing to a shared UpgradeableBeacon, enabling single-transaction upgrades for all vaults.
Demonstrates complete upgrade lifecycle: - Deploy all UUPS contracts (Config, Registry, Orchestrators, Factory) - Deploy Beacon + vault implementation - Create vault via factory (BeaconProxy) - Test V1 behavior - Upgrade vault implementation via Beacon - Verify state preservation after vault upgrade - Upgrade OrionConfig via UUPS - Verify state preservation after UUPS upgrade
Mock V2 implementations: - OrionConfigUpgradeableV2: adds newV2Variable state and setV2Variable() - OrionTransparentVaultUpgradeableV2: adds vaultDescription and setVaultDescription() - Both include version() function returning "v2" Test scripts: 1. testUpgradeability.ts - Full upgrade lifecycle test - Deploy all UUPS contracts (Config, Registry, Orchestrators, Factory) - Deploy Beacon + vault implementation - Create vault via factory (BeaconProxy) - Upgrade vault via beacon (V1 → V2) - Verify implementation address changes - Test V2 new features work - Upgrade OrionConfig via UUPS (V1 → V2) - Verify state preservation and V2 features 2. verifyUpgradeRequiresNewAddress.ts - Verification test - Proves implementation address MUST change for real upgrades - Tests both UUPS and Beacon patterns - Demonstrates same-address "upgrades" are no-ops
now uses a Deploy complete upgradeable protocol infrastructure
What the Tests Cover: pendingDeposit Batch Limit (3 tests) Returns exact amount when requests < maxFulfillBatchSize Returns ONLY first maxFulfillBatchSize when requests exceed limit Handles edge case when requests = maxFulfillBatchSize exactly pendingRedeem Batch Limit (3 tests) Returns exact shares when requests < maxFulfillBatchSize Returns ONLY first maxFulfillBatchSize when requests exceed limit Handles edge case when requests = maxFulfillBatchSize exactly Verify Fix Prevents Double-Counting (2 tests) Verifies pendingDeposit limiting prevents totalAssets overcounting Verifies pendingRedeem limiting prevents double-subtraction from totalAssets
This commit addresses 4 critical code review action items:
1. **cancelRedeemRequest Edge-Case Tests** (OrionConfigVault.test.ts)
- Add test for zero amount (should revert with AmountMustBeGreaterThanZero)
- Add test for excessive amount (should revert with InsufficientAmount)
- Add test for successful full cancellation
- Add test for partial cancellation
- Mirrors existing cancelDepositRequest test pattern
- Uses LiquidityOrchestrator impersonation to mint shares for testing
2. **USDC Decimals On-Chain Fetch** (erc4626VaultCompatibility.test.ts)
- Replace hardcoded USDC_DECIMALS = 6 with on-chain contract call
- Prevents configuration drift detection issues
- Fetches decimals via USDC contract interface in before() hook
3. **Real Immutability Test** (erc4626VaultCompatibility.test.ts)
- Replace basic "call 3 times" test with comprehensive checks:
* Bytecode immutability verification across blocks
* EIP-1967 implementation slot immutability (proxy detection)
* Cross-block property validation for asset() and decimals()
* Deterministic read verification within same block
- Detects upgradeable contracts and validates implementation slots
4. **Adapter Decimals Assertion** (erc4626VaultCompatibility.test.ts)
- Add assertion that adapter decimals match underlying asset decimals
- Critical for ensuring consistent price calculations
- Prevents potential price calculation bugs
**Bonus Fix:**
- Execution adapter compatibility test now whitelists vaults before validation
- Ensures OrionConfig has vault decimals registered before adapter validates
- Prevents InvalidAdapter errors during validation
…s, eliminating code duplication
Test a) Different implementations after setVaultBeacon Deploys first vault with V1 implementation via original beacon Creates new beacon pointing to V2 implementation Calls setVaultBeacon() to switch factory to new beacon Deploys second vault with V2 implementation Asserts: Vault 1 uses V1, Vault 2 uses V2 Test b) Same new implementation after vaultBeacon.upgradeTo Deploys first vault with V1 implementation Calls vaultBeacon.upgradeTo(newImpl) to upgrade existing beacon Deploys second vault Asserts: Both old and new vaults now use V2 implementation Test c) Factory UUPS upgrade maintains beacon functionality Deploys vault with original factory + V1 beacon Upgrades factory itself via UUPS Creates new V2 beacon and calls setVaultBeacon() on upgraded factory Deploys vault with upgraded factory + V2 beacon Asserts: First vault still V1, second vault uses V2, factory functions correctly after UUPS upgrade
…of deployProtocol
🛡️ 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. |
Reviewer's GuideIntroduce full upgradability to the Orion protocol core (config, orchestrators, vault factory, vaults, and price-adapter registry) via UUPS and beacon proxies, centralize upgradeable deployment logic in a test helper, tighten accounting around batch limits, and extend tests/CI to validate immutability, adapter correctness, and upgrade flows. Sequence diagram for creating an upgradeable transparent vault via BeaconProxysequenceDiagram
actor VaultOwner
participant TransparentVaultFactory
participant OrionConfig
participant UpgradeableBeacon
participant BeaconProxy
participant OrionTransparentVault_impl as OrionTransparentVaultImplementation
participant OrionVault
VaultOwner->>TransparentVaultFactory: createOrionTransparentVault(vaultOwner,curator,name,symbol,feeType,performanceFee,managementFee,depositAccessControl)
TransparentVaultFactory->>OrionConfig: isWhitelistedVaultOwner(vaultOwner)
OrionConfig-->>TransparentVaultFactory: bool
TransparentVaultFactory->>OrionConfig: isSystemIdle()
OrionConfig-->>TransparentVaultFactory: bool
TransparentVaultFactory->>TransparentVaultFactory: encode initialize(vaultOwner,curator,config,name,symbol,feeType,performanceFee,managementFee,depositAccessControl)
TransparentVaultFactory->>UpgradeableBeacon: read implementation()
UpgradeableBeacon-->>TransparentVaultFactory: OrionTransparentVault_impl
TransparentVaultFactory->>BeaconProxy: new BeaconProxy(vaultBeacon, initData)
BeaconProxy->>OrionTransparentVault_impl: delegatecall initialize(...)
OrionTransparentVault_impl->>OrionVault: __OrionVault_init(...)
OrionVault-->>OrionTransparentVault_impl: initialized
OrionTransparentVault_impl-->>BeaconProxy: initialized
TransparentVaultFactory-->>VaultOwner: vault address = BeaconProxy
TransparentVaultFactory->>OrionConfig: addOrionVault(vaultAddress,VaultType.Transparent)
OrionConfig-->>TransparentVaultFactory: success
Class diagram for upgraded vault and factory contractsclassDiagram
class Initializable
class UUPSUpgradeable
class Ownable2StepUpgradeable
class ReentrancyGuardUpgradeable
class PausableUpgradeable
class ERC20Upgradeable
class ERC4626Upgradeable
class UpgradeableBeacon
class BeaconProxy
class IOrionConfig
class IOrionVault
class IOrionTransparentVault
class OrionVault {
<<abstract>>
+address vaultOwner
+address curator
+IOrionConfig config
+uint256 totalUserShares
+uint256 totalAssets()
+uint8 decimals()
+redeem(uint256 assets,address receiver,address owner) uint256
+deposit(uint256 assets,address receiver) uint256
+mint(uint256 shares,address receiver) uint256
+withdraw(uint256 assets,address receiver,address owner) uint256
+__OrionVault_init(address vaultOwner,address curator,IOrionConfig config,string name,string symbol,uint8 feeType,uint16 performanceFee,uint16 managementFee,address depositAccessControl)
-uint256[50] __gap
}
class OrionTransparentVault {
+EnumerableMap.AddressToUintMap _portfolioIntent
+constructor()
+initialize(address vaultOwner,address curator,IOrionConfig config,string name,string symbol,uint8 feeType,uint16 performanceFee,uint16 managementFee,address depositAccessControl)
-uint256[50] __gap
}
class TransparentVaultFactory {
+IOrionConfig config
+UpgradeableBeacon vaultBeacon
+constructor()
+initialize(address initialOwner,address configAddress,address vaultBeaconAddress)
+createOrionTransparentVault(address vaultOwner,address curator,string name,string symbol,uint8 feeType,uint16 performanceFee,uint16 managementFee,address depositAccessControl) address
+setVaultBeacon(address newVaultBeacon)
+_authorizeUpgrade(address newImplementation)
-uint256[50] __gap
}
OrionVault --|> Initializable
OrionVault --|> ERC4626Upgradeable
OrionVault --|> ReentrancyGuardUpgradeable
OrionVault ..|> IOrionVault
OrionTransparentVault --|> OrionVault
OrionTransparentVault ..|> IOrionTransparentVault
TransparentVaultFactory --|> Initializable
TransparentVaultFactory --|> Ownable2StepUpgradeable
TransparentVaultFactory --|> UUPSUpgradeable
TransparentVaultFactory --> IOrionConfig : uses
TransparentVaultFactory --> UpgradeableBeacon : manages
TransparentVaultFactory --> BeaconProxy : deploys
Class diagram for upgraded orchestrators, config, and price registryclassDiagram
class Initializable
class UUPSUpgradeable
class Ownable2StepUpgradeable
class ReentrancyGuardUpgradeable
class PausableUpgradeable
class IOrionConfig
class ILiquidityOrchestrator
class IInternalStateOrchestrator
class IPriceAdapterRegistry
class OrionConfig {
+address ADMIN
+address guardian
+IERC20 underlyingAsset
+constructor()
+initialize(address initialOwner,address admin,address underlyingAsset)
+admin() address
+_authorizeUpgrade(address newImplementation)
-uint256[50] __gap
}
class LiquidityOrchestrator {
+IOrionConfig config
+address underlyingAsset
+address admin
+int256 deltaBufferAmount
+constructor()
+initialize(address initialOwner,address config,address automationRegistry)
+executeSellOrder(...)
+executeBuyOrder(...)
+pause()
+unpause()
+_authorizeUpgrade(address newImplementation)
-uint256[50] __gap
}
class InternalStatesOrchestrator {
+IOrionConfig config
+IPriceAdapterRegistry registry
+uint256 intentFactor
+constructor()
+initialize(address initialOwner,address config,address automationRegistry)
+updateVaultStates(...)
+triggerLiquidityOrchestrator(...)
+pause()
+unpause()
+_authorizeUpgrade(address newImplementation)
-uint256[50] __gap
}
class PriceAdapterRegistry {
+address configAddress
+uint8 priceAdapterDecimals
+constructor()
+initialize(address initialOwner,address configAddress)
+setPriceAdapter(address asset,address adapter)
+getPrice(address asset) uint256
+_authorizeUpgrade(address newImplementation)
-uint256[50] __gap
}
OrionConfig --|> Initializable
OrionConfig --|> Ownable2StepUpgradeable
OrionConfig --|> UUPSUpgradeable
OrionConfig ..|> IOrionConfig
LiquidityOrchestrator --|> Initializable
LiquidityOrchestrator --|> Ownable2StepUpgradeable
LiquidityOrchestrator --|> ReentrancyGuardUpgradeable
LiquidityOrchestrator --|> PausableUpgradeable
LiquidityOrchestrator --|> UUPSUpgradeable
LiquidityOrchestrator ..|> ILiquidityOrchestrator
InternalStatesOrchestrator --|> Initializable
InternalStatesOrchestrator --|> Ownable2StepUpgradeable
InternalStatesOrchestrator --|> ReentrancyGuardUpgradeable
InternalStatesOrchestrator --|> PausableUpgradeable
InternalStatesOrchestrator --|> UUPSUpgradeable
InternalStatesOrchestrator ..|> IInternalStateOrchestrator
PriceAdapterRegistry --|> Initializable
PriceAdapterRegistry --|> Ownable2StepUpgradeable
PriceAdapterRegistry --|> UUPSUpgradeable
PriceAdapterRegistry ..|> IPriceAdapterRegistry
LiquidityOrchestrator --> IOrionConfig : uses
InternalStatesOrchestrator --> IOrionConfig : uses
InternalStatesOrchestrator --> IPriceAdapterRegistry : uses
PriceAdapterRegistry --> IOrionConfig : reads settings
Flow diagram for UUPS upgrade process of a core contractflowchart TD
A["Owner calls upgradeTo on UUPS contract"] --> B["UUPS proxy receives call"]
B --> C["Proxy delegates call to current implementation"]
C --> D["Implementation _authorizeUpgrade(newImplementation) with onlyOwner"]
D -->|reverts| E["Upgrade reverted"]
D -->|success| F["Implementation performs upgrade to newImplementation"]
F --> G["Proxy now delegates to newImplementation"]
G --> H["State preserved via storage layout and __gap"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 migrates the Orion Finance protocol from non-upgradeable contracts to UUPS and Beacon proxy patterns. Core contracts (OrionConfig, orchestrators, vaults, factories) are converted to use initializers instead of constructors, new upgradeability hooks are added, and test infrastructure is consolidated via a new deployment helper. Dependencies include OpenZeppelin upgradeable contracts, CI audit steps are added, and the package version is bumped to 1.0.0. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 - I've found 4 issues, and left some high level feedback:
- In
OrionVault.__OrionVault_initthe previous constructor checks for non-zerocurator_andconfig_were removed; consider reintroducing these validations to avoid accidentally creating unusable vaults via a misconfigured factory or upgrade. - The new
deployUpgradeableProtocolhelper silently defaultsautomationRegistrytoadminwhen not provided; if this is only meant for tests, consider making that explicit or enforcing a non-zero, distinct registry to avoid masking misconfigurations when the helper is reused elsewhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OrionVault.__OrionVault_init` the previous constructor checks for non-zero `curator_` and `config_` were removed; consider reintroducing these validations to avoid accidentally creating unusable vaults via a misconfigured factory or upgrade.
- The new `deployUpgradeableProtocol` helper silently defaults `automationRegistry` to `admin` when not provided; if this is only meant for tests, consider making that explicit or enforcing a non-zero, distinct registry to avoid masking misconfigurations when the helper is reused elsewhere.
## Individual Comments
### Comment 1
<location> `contracts/vaults/OrionVault.sol:175-178` </location>
<code_context>
/// @param depositAccessControl_ The address of the deposit access control contract (address(0) = permissionless)
- constructor(
+ // solhint-disable-next-line func-name-mixedcase, use-natspec
+ function __OrionVault_init(
address vaultOwner_,
address curator_,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The initializer no longer validates `curator_` and `config_` addresses, weakening constructor-time safety guarantees.
The original constructor reverted when `curator_ == address(0)` or `address(config_) == address(0)`, ensuring vaults were always deployed with valid dependencies. The new `__OrionVault_init` omits these checks, so a misconfigured proxy init could leave the vault unusable. Please add equivalent validations (and any other constructor invariants you rely on) to the initializer to maintain the same safety guarantees.
Suggested implementation:
```
// solhint-disable-next-line func-name-mixedcase, use-natspec
function __OrionVault_init(
address vaultOwner_,
address curator_,
IOrionConfig config_,
uint16 performanceFee_,
uint16 managementFee_,
address depositAccessControl_
) internal onlyInitializing {
// Maintain constructor-time safety guarantees
require(curator_ != address(0), "OrionVault: curator is zero address");
require(address(config_) != address(0), "OrionVault: config is zero address");
// Initialize parent contracts
__ERC20_init(name_, symbol_);
```
1. If the original constructor used custom errors or different revert messages (e.g. `error InvalidCurator()` / `InvalidConfig()`), replace the two `require(...)` statements with the corresponding error checks to keep revert semantics consistent:
- `if (curator_ == address(0)) revert InvalidCurator();`
- `if (address(config_) == address(0)) revert InvalidConfig();`
2. If the original constructor enforced additional invariants (e.g. bounds on `performanceFee_` / `managementFee_`, non-zero `vaultOwner_`, or `depositAccessControl_` constraints), replicate those checks at the top of `__OrionVault_init` as well, mirroring the constructor logic exactly.
</issue_to_address>
### Comment 2
<location> `contracts/vaults/OrionVault.sol:214-223` </location>
<code_context>
function _initializeVaultWhitelist() internal {
address[] memory protocolAssets = config.getAllWhitelistedAssets();
for (uint256 i = 0; i < protocolAssets.length; ++i) {
- bool inserted = _vaultWhitelistedAssets.add(protocolAssets[i]);
- if (!inserted) revert ErrorsLib.AlreadyRegistered();
+ // slither-disable-next-line unused-return
+ _vaultWhitelistedAssets.add(protocolAssets[i]);
}
}
</code_context>
<issue_to_address>
**question (bug_risk):** Behavior change: duplicate assets in `config.getAllWhitelistedAssets()` no longer cause a revert when initializing the vault whitelist.
This change removes the defensive `AlreadyRegistered` invariant and now silently ignores duplicate assets. If `protocolAssets` is always unique this is fine, but if that assumption is ever violated, the vault will still initialize and the inconsistency will be harder to detect. To preserve the invariant while handling the Slither warning, consider keeping the revert and suppressing the warning locally, or enforce uniqueness directly in `OrionConfig` instead.
</issue_to_address>
### Comment 3
<location> `test/OrionConfigVault.test.ts:387-396` </location>
<code_context>
+ describe("Redeem Request Cancellation", function () {
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative tests for cancelling non-existent or other users' redeem requests
To make the suite more robust, please also cover:
1) Calling `cancelRedeemRequest` when the caller has no pending redeem, asserting the expected revert.
2) A different account attempting to cancel someone else’s pending redeem and verifying it reverts.
This will confirm the cancel logic is strictly limited to the original requester.
Suggested implementation:
```typescript
describe("Redeem Request Cancellation", function () {
beforeEach(async function () {
// Setup: Give user shares by depositing and fulfilling
const depositAmount = ethers.parseUnits("1000", 6);
// Mint and approve underlying asset for user
await underlyingAsset.mint(user.address, depositAmount);
await underlyingAsset.connect(user).approve(await vault.getAddress(), depositAmount);
// Request deposit
await vault.connect(user).requestDeposit(depositAmount);
});
it("reverts when caller has no pending redeem", async function () {
// user has gone through the standard deposit flow in beforeEach
// but has NOT opened a redeem request yet
await expect(
vault.connect(user).cancelRedeemRequest(),
).to.be.reverted;
});
it("reverts when a different account attempts to cancel someone else's redeem", async function () {
// Arrange: set up a pending redeem request for `user`
const redeemAmount = ethers.parseUnits("100", 6);
// Give user enough shares / liquidity to request a redeem
// (the beforeEach has already provisioned the initial state)
await vault.connect(user).requestRedeem(redeemAmount);
// Sanity check: original requester can cancel successfully
// (optional, but keeps behaviour explicit)
await expect(
vault.connect(user).cancelRedeemRequest(),
).to.not.be.reverted;
// Re-open a redeem request for the same user to test the negative path
await vault.connect(user).requestRedeem(redeemAmount);
// Act & Assert: another account (e.g., `otherUser`) cannot cancel `user`'s redeem
await expect(
vault.connect(otherUser).cancelRedeemRequest(),
).to.be.reverted;
});
```
The above edits assume:
1. `requestRedeem(uint256)` and `cancelRedeemRequest()` exist on `vault`, and `requestRedeem` takes the same decimal precision as `underlyingAsset`.
2. `otherUser` is already defined in the test fixture (e.g., from `const [deployer, user, otherUser, ...] = await ethers.getSigners();`).
3. A generic `.to.be.reverted` matcher is acceptable. If your contract uses specific custom errors or revert reasons (e.g. `NoPendingRedeem` or `UnauthorizedRedeemCancel`), you should tighten the expectations, for example:
- `await expect(...).to.be.revertedWithCustomError(vault, "NoPendingRedeem");`
- `await expect(...).to.be.revertedWithCustomError(vault, "UnauthorizedRedeemCancel");`
If the actual method names or arguments differ (e.g. `requestRedeemShares`, `cancelPendingRedeem`, or a struct-based API), update the two new tests to match the real interface and any existing helper functions used elsewhere in the suite to open redeem requests.
</issue_to_address>
### Comment 4
<location> `test/OrionConfigVault.test.ts:495-497` </location>
<code_context>
+ const sharesAfterPartialCancel = await vault.balanceOf(user.address);
+ expect(sharesAfterPartialCancel).to.equal(userSharesBefore - remainingRedeem);
+
+ // Verify pending redeems reflects the remaining amount
+ const pendingRedeems = await vault.pendingRedeem(await orionConfig.maxFulfillBatchSize());
+ expect(pendingRedeems).to.be.gte(remainingRedeem);
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Tighten assertion on pendingRedeem after partial cancellation
Given this test only creates a single redeem request for this user, `pendingRedeems` should match `remainingRedeem` exactly. Using `expect(pendingRedeems).to.equal(remainingRedeem);` would make the test stricter and better at catching cases where extra shares are incorrectly kept pending. If `pendingRedeems` is expected to aggregate other users’ requests, it’d be clearer to set up that scenario explicitly and assert on the exact expected sum rather than using `>=`.
```suggestion
// Verify pending redeems matches the remaining amount for this single request
const pendingRedeems = await vault.pendingRedeem(await orionConfig.maxFulfillBatchSize());
expect(pendingRedeems).to.equal(remainingRedeem);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/mainnet-fork/multiAssetRobustness.test.ts (1)
323-397: Critical: Function name doesn't match implementation.The function is named
deployUpgradeableProtocolbut deploys non-upgradeable contracts using regulardeploy()instead ofupgrades.deployProxy(). This contradicts the PR objective of migrating to upgradeable architecture.Compare with the proper upgradeable deployment in
test/helpers/deployUpgradeable.ts(lines 44-140), which uses:
upgrades.deployProxy()withkind: "uups"for UUPS proxiesUpgradeableBeaconfor vault beacon patternEither:
- Import and use the actual
deployUpgradeableProtocolfromtest/helpers/deployUpgradeable.ts, or- Revert the function name to
deployProtocolif this test intentionally uses non-upgradeable contractstest/ExecutionAdapterValidation.test.ts (1)
31-50: Potential decimal mismatch in initial deposit.The
deployUpgradeableProtocolhelper creates aMockUnderlyingAssetwith 6 decimals by default (USDC-like), but line 47 usesethers.parseUnits("10000", 12)with 12 decimals. This mismatch means the deposit amount is 10^6 times larger than intended.🔎 Proposed fix
- const initialDeposit = ethers.parseUnits("10000", 12); + const decimals = await underlyingAsset.decimals(); + const initialDeposit = ethers.parseUnits("10000", decimals);Alternatively, query the decimals from the deployed asset to ensure consistency.
package.json (1)
1-111: Fix Prettier formatting to resolve pipeline failure.The CI pipeline reports a Prettier formatting check failure for this file. Run
pnpm prettier:writeto fix the formatting.#!/bin/bash # Check the actual prettier differences npx prettier --check "package.json" 2>&1 || true
🧹 Nitpick comments (10)
test/OrionVaultExchangeRate.test.ts (1)
41-58: Event parsing logic is robust; minor redundancy noted.The try/catch pattern for parsing logs is defensive. However, line 55 uses non-null assertion (
event!) after the null check on line 51, which is safe but redundant since the code would have thrown before reaching line 55 ifeventwere null.Minor cleanup suggestion
- const parsedEvent = factory.interface.parseLog(event!); + const parsedEvent = factory.interface.parseLog(event);Since the error is thrown on line 52 if
eventis falsy, the non-null assertion is unnecessary.test/Upgrade.test.ts (2)
152-166: Event parsing pattern differs from other test files.This file uses
log.fragment?.name === "OrionVaultCreated"while other test files (e.g.,OrionVaultExchangeRate.test.ts) usefactory.interface.parseLog(log)?.name. Thefragmentproperty may not be present on all log types, making this approach less robust.Consider using consistent event parsing pattern
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const vault1Address = receipt1?.logs.find((log: any) => log.fragment?.name === "OrionVaultCreated")?.args?.[0]; + const vault1Address = receipt1?.logs.find((log) => { + try { + return vaultFactory.interface.parseLog(log)?.name === "OrionVaultCreated"; + } catch { + return false; + } + }); + const parsedLog1 = vault1Address ? vaultFactory.interface.parseLog(vault1Address) : null; + const vault1Addr = parsedLog1?.args[0];This pattern with try/catch is more robust and consistent with other test files in this PR. Consider extracting a helper function to avoid repetition.
95-101: Access control test should verify the specific error.The test verifies that non-owner upgrade attempts are reverted, but the assertion uses
.to.be.revertedwithout checking the specific error. For better test precision, consider matching the expected error.More precise error assertion
- await expect(upgrades.upgradeProxy(proxyAddress, OrionConfigV2Factory.connect(user))).to.be.reverted; + await expect(upgrades.upgradeProxy(proxyAddress, OrionConfigV2Factory.connect(user))) + .to.be.revertedWithCustomError(orionConfig, "OwnableUnauthorizedAccount");test/VaultOwnerRemoval.test.ts (1)
98-116: Test assertions usevoid expect(...)pattern.The
void expect(...)pattern is used throughout the test assertions. While this works, it's unconventional. Thevoidoperator discards the return value but doesn't affect the assertion behavior. This appears to be a style choice, possibly to satisfy a linter rule about floating promises, though Chai assertions are synchronous.test/RedeemBeforeDepositOrder.test.ts (1)
237-237: Console.log statements should be removed for production tests.Multiple
console.logstatements are present in the test file (lines 237, 245, 272, 273, 284, 300, 301, 306, 325, 361, 390-392). While useful for debugging, these should typically be removed or converted to debug-level output for cleaner test runs.Also applies to: 245-245, 272-273
test/mainnet-fork/erc4626VaultCompatibility.test.ts (1)
607-631: Whitelist cleanup should handle test failures gracefully.The whitelist addition and removal are done inline within the try block. If
validateExecutionAdapterfails, the vault won't be removed from the whitelist, potentially affecting subsequent tests.Consider moving the cleanup to a
finallyblock:🔎 Suggested improvement
try { // CRITICAL: Whitelist the vault first so OrionConfig has its decimals await orionConfig .connect(owner) .addWhitelistedAsset( vaultInfo.address, await priceAdapter.getAddress(), await executionAdapter.getAddress(), ); console.log(` ✓ Vault whitelisted in OrionConfig`); // Validate that execution adapter can validate the vault await executionAdapter.validateExecutionAdapter(vaultInfo.address); console.log(` ✓ Execution adapter validation: PASS`); - - // Clean up: Remove from whitelist for next test - await orionConfig.connect(admin).removeWhitelistedAsset(vaultInfo.address); - console.log(` ✓ Vault removed from whitelist`); } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); throw new Error(`${vaultInfo.name}: execution adapter validation failed - ${errorMessage}`); + } finally { + // Clean up: Remove from whitelist for next test (even on failure) + try { + await orionConfig.connect(admin).removeWhitelistedAsset(vaultInfo.address); + console.log(` ✓ Vault removed from whitelist`); + } catch { + // Ignore cleanup errors + } }test/MinimumAmountDOS.test.ts (1)
174-210: Comprehensive spam prevention test, but may be slow.This test creates 160 attacker accounts to validate DOS prevention at scale. While thorough, this may significantly slow down test execution. Consider:
- Adding a
this.timeout()if running in Mocha to prevent timeout failures- Or adding a
.skipcondition for CI with a smaller subset testThe security validation is valuable, so keeping it as-is for comprehensive coverage is acceptable if test duration is not a concern.
test/BatchLimitConsistency.test.ts (1)
152-178: Consider adding a skip condition when effectiveBatchSize cannot meaningfully test the batch limit.When
numUsers <= 2, theeffectiveBatchSizebecomesmax(1, numUsers - 2)which is 1 or less, making the test less meaningful. Consider adding a skip condition or minimum user check.🔎 Optional: Add skip condition for insufficient signers
it("Should return ONLY first maxFulfillBatchSize requests when requests exceed limit", async function () { // NOTE: Hardhat provides limited signers (~18-20), so we can't truly test 150+ requests // This test verifies the batch limiting logic works with available signers const excessUsers = 5; const numUsers = Math.min(maxFulfillBatchSize + excessUsers, users.length); // For this test to be meaningful, we need more requests than batch size // If we don't have enough signers, we use a smaller batch size for testing const effectiveBatchSize = Math.min(maxFulfillBatchSize, Math.max(1, numUsers - 2)); + // Skip if we can't create a meaningful excess scenario + if (effectiveBatchSize >= numUsers) { + this.skip(); + } + // Create deposit requests exceeding effective batch limitcontracts/orchestrators/InternalStatesOrchestrator.sol (1)
33-40: Consider contract decomposition in future iterations.Static analysis flags 23 state declarations (vs 15 recommended limit). Given the orchestrator's complex responsibilities, this is acceptable for now, but consider splitting epoch management or fee processing into separate contracts in future refactors.
contracts/factories/TransparentVaultFactory.sol (1)
107-113: Consider adding system idle check for beacon updates.Updating the vault beacon changes the implementation for all existing vaults on their next call. While this is standard beacon behavior, consider adding
if (!config.isSystemIdle()) revert ErrorsLib.SystemNotIdle();to prevent mid-epoch upgrades that could cause unexpected behavior.🔎 Proposed fix
function setVaultBeacon(address newVaultBeacon) external onlyOwner { if (newVaultBeacon == address(0)) revert ErrorsLib.ZeroAddress(); + if (!config.isSystemIdle()) revert ErrorsLib.SystemNotIdle(); vaultBeacon = UpgradeableBeacon(newVaultBeacon); emit EventsLib.VaultBeaconUpdated(newVaultBeacon); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
.github/workflows/ci.yml(1 hunks).gitignore(2 hunks)Makefile(1 hunks)contracts/OrionConfig.sol(4 hunks)contracts/access_controllers/WhitelistAccessControl.sol(1 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(1 hunks)contracts/factories/TransparentVaultFactory.sol(3 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/interfaces/IOrionAccessControl.sol(1 hunks)contracts/interfaces/IOrionConfig.sol(1 hunks)contracts/interfaces/IOrionStrategy.sol(1 hunks)contracts/interfaces/IOrionTransparentVault.sol(1 hunks)contracts/interfaces/IOrionVault.sol(1 hunks)contracts/interfaces/IPriceAdapter.sol(1 hunks)contracts/interfaces/IPriceAdapterRegistry.sol(1 hunks)contracts/libraries/ErrorsLib.sol(1 hunks)contracts/libraries/EventsLib.sol(2 hunks)contracts/libraries/UtilitiesLib.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(4 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(7 hunks)contracts/price/OrionAssetERC4626PriceAdapter.sol(1 hunks)contracts/price/PriceAdapterRegistry.sol(3 hunks)contracts/strategies/KBestTvlWeightedAverage.sol(1 hunks)contracts/test/OrionConfigV2.sol(1 hunks)contracts/test/OrionTransparentVaultV2.sol(1 hunks)contracts/vaults/OrionTransparentVault.sol(3 hunks)contracts/vaults/OrionVault.sol(9 hunks)package.json(3 hunks)test/AccessControl.test.ts(2 hunks)test/Adapters.test.ts(2 hunks)test/BatchLimitAccounting.test.ts(2 hunks)test/BatchLimitConsistency.test.ts(1 hunks)test/ExecutionAdapterValidation.test.ts(1 hunks)test/FeeCooldown.test.ts(2 hunks)test/MinimumAmountDOS.test.ts(3 hunks)test/OrionConfigVault.test.ts(5 hunks)test/OrionVaultExchangeRate.test.ts(2 hunks)test/PassiveCuratorStrategy.test.ts(4 hunks)test/ProtocolPause.test.ts(3 hunks)test/RedeemBeforeDepositOrder.test.ts(2 hunks)test/Removal.test.ts(4 hunks)test/TransparentVault.test.ts(2 hunks)test/Upgrade.test.ts(1 hunks)test/VaultOwnerRemoval.test.ts(2 hunks)test/helpers/deployUpgradeable.ts(1 hunks)test/mainnet-fork/erc4626VaultCompatibility.test.ts(8 hunks)test/mainnet-fork/multiAssetRobustness.test.ts(2 hunks)test/mainnet-fork/removeWhitelistedAsset.test.ts(1 hunks)test/orchestrator/OrchestratorConfiguration.test.ts(4 hunks)test/orchestrator/OrchestratorSecurity.test.ts(4 hunks)test/orchestrator/Orchestrators.test.ts(3 hunks)test/orchestrator/OrchestratorsZeroState.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
test/mainnet-fork/multiAssetRobustness.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/RedeemBeforeDepositOrder.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/TransparentVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/FeeCooldown.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/BatchLimitAccounting.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/orchestrator/OrchestratorConfiguration.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/orchestrator/OrchestratorsZeroState.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/BatchLimitConsistency.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/ExecutionAdapterValidation.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/Adapters.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/AccessControl.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/MinimumAmountDOS.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/Removal.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/OrionVaultExchangeRate.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/ProtocolPause.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/OrionConfigVault.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
test/VaultOwnerRemoval.test.ts (1)
test/helpers/deployUpgradeable.ts (1)
deployUpgradeableProtocol(45-141)
🪛 GitHub Actions: CI
contracts/factories/TransparentVaultFactory.sol
[warning] 1-1: SOLHINT warnings detected across Solidity sources (e.g., string length, max-states-count). Review and fix to reduce warnings.
package.json
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🪛 GitHub Check: Build, Lint and Test
contracts/factories/TransparentVaultFactory.sol
[warning] 75-75:
GC: String exceeds 32 bytes
contracts/orchestrators/InternalStatesOrchestrator.sol
[warning] 33-33:
Contract has 23 states declarations but allowed no more than 15
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (99)
contracts/interfaces/IOrionAccessControl.sol (1)
9-9: Documentation addition approved.Adding the security contact email to the interface NatSpec is a good practice that provides a clear channel for vulnerability reporting. The tag format and email address are correctly formed and align with the documentation pattern across the PR.
contracts/libraries/UtilitiesLib.sol (1)
9-9: Security contact metadata properly added.The NatSpec
@custom:security-contactannotation is a good addition that follows industry best practices and aligns with metadata augmentations across the PR..gitignore (1)
26-28: LGTM!The new ignore entries align well with the upgradeable contract migration. Ignoring
artifacts/,gasReporterOutput.json, and project-specific directories likeres/andrunning_node/is standard practice for blockchain projects and prevents generated build/deployment artifacts from polluting version control.Also applies to: 39-39, 41-41
contracts/interfaces/IOrionStrategy.sol (1)
11-11: LGTM! Security contact metadata added.Adding the security contact tag is a best practice for production smart contracts, especially for upgradeable protocols. This metadata-only change has no functional impact on the interface.
contracts/interfaces/ILiquidityOrchestrator.sol (1)
10-10: Excellent addition of security contact information.Adding a security contact to the interface documentation is a best practice for DeFi protocols, providing a clear channel for security researchers to report vulnerabilities responsibly.
contracts/strategies/KBestTvlWeightedAverage.sol (2)
16-16: LGTM! Good security practice.Adding the security contact annotation is a best practice that helps security researchers and auditors report vulnerabilities responsibly.
18-34: No changes needed—strategy contracts are correctly designed as non-upgradeable.Strategies like
KBestTvlWeightedAverageare utility contracts called by upgradeable vaults to compute intents; they don't require upgradeable patterns themselves. The statelessIOrionStrategyinterface keeps strategies lightweight and replaceable, making the non-upgradeable design the correct architectural choice. The upgradeable vault (OrionTransparentVault) and non-upgradeable strategy have no incompatibility.contracts/price/OrionAssetERC4626PriceAdapter.sol (1)
1-59: No changes required. The adapter's constructor pattern is correct for its architectural role.The PriceAdapterRegistry uses an upgradeable UUPS pattern and manages which price adapters are registered. Individual adapters like OrionAssetERC4626PriceAdapter are stateless utility contracts deployed directly (not via proxy) and only initialized once through their constructors. The state variables (config address, underlying asset, decimals) are read-only and never modified after deployment. This is the correct architecture: the upgradeable registry can point to different adapter implementations when needed, eliminating the need for individual adapters to be proxy-deployed or use initializer functions.
Likely an incorrect or invalid review comment.
contracts/libraries/ErrorsLib.sol (1)
7-7: LGTM!Adding the
@custom:security-contactNatSpec tag is a good practice for security disclosures and aligns with the broader effort across the codebase.contracts/interfaces/IPriceAdapterRegistry.sol (1)
9-9: LGTM!Security contact metadata addition is consistent with the protocol-wide documentation update.
contracts/interfaces/IPriceAdapter.sol (1)
7-7: LGTM!Security contact metadata addition is consistent with the protocol-wide documentation update.
contracts/access_controllers/WhitelistAccessControl.sol (1)
11-11: LGTM!Security contact metadata addition is consistent with the protocol-wide documentation update.
test/Removal.test.ts (2)
3-6: LGTM!Clean migration to use the centralized
deployUpgradeableProtocolhelper, reducing boilerplate and ensuring consistent upgradeable deployment across tests.
75-80: Verifyuseris the intended admin for this test.The
deployUpgradeableProtocolcall passesuseras the second parameter (admin role). This appears intentional sinceremoveWhitelistedAssetis later called byuser(lines 207, 335), but confirm this aligns with the expected access control for asset removal.contracts/orchestrators/LiquidityOrchestrator.sol (4)
4-8: LGTM!Correct upgradeable imports from OpenZeppelin contracts-upgradeable package, along with SafeCast for safe integer conversions.
Also applies to: 21-22
130-160: LGTM!The upgradeable initialization pattern is correctly implemented:
- Constructor disables initializers on the implementation contract
initializefunction has theinitializermodifier- All parent initializers are called in the correct order
- Zero-address validation is present for all parameters
400-401: LGTM!Good use of
SafeCast.toInt256()for safe conversion. This prevents silent overflow (though practically impossible for token amounts) and makes the conversion intent explicit.Also applies to: 424-425
504-512: _authorizeUpgrade implementation is correct.The empty function with
onlyOwnermodifier properly restricts upgrades to the owner per UUPS pattern. The 50-slot storage gap follows OpenZeppelin conventions and supports future extensibility.test/mainnet-fork/removeWhitelistedAsset.test.ts (1)
275-283: LGTM!The addition of
ethers.ZeroAddressas thedepositAccessControlparameter correctly aligns with the updatedTransparentVaultFactory.createVaultsignature, which usesaddress(0)to indicate permissionless mode.contracts/interfaces/IOrionConfig.sol (1)
10-10: LGTM! Security contact metadata added.The addition of the security contact NatSpec tag is a good practice for protocol security documentation.
contracts/interfaces/IInternalStateOrchestrator.sol (1)
10-10: LGTM! Security contact metadata added.contracts/interfaces/IExecutionAdapter.sol (1)
12-12: LGTM! Security contact metadata added.contracts/interfaces/IOrionVault.sol (1)
11-11: LGTM! Security contact metadata added.contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (1)
20-20: LGTM! Security contact metadata added..github/workflows/ci.yml (1)
34-36: LGTM! Dependency audit added to CI pipeline.Adding
pnpm audit --prod --audit-level highenhances security by catching high-severity vulnerabilities in production dependencies before they reach deployment.test/mainnet-fork/multiAssetRobustness.test.ts (1)
236-236: Verify: Test may not be using upgradeable contracts.The call to
deployUpgradeableProtocol()references a local function (line 323) that doesn't actually deploy upgradeable contracts, despite its name. This test may not be validating the upgradeable architecture as intended.contracts/libraries/EventsLib.sol (2)
7-7: LGTM! Security contact metadata added.
141-143: LGTM! New event supports upgradeable vault architecture.The
VaultBeaconUpdatedevent properly supports the Beacon Proxy pattern introduced in this PR, allowing observers to track when the vault implementation beacon is updated.contracts/interfaces/IOrionTransparentVault.sol (1)
9-9: LGTM! Security contact metadata is a good addition.Adding
@custom:security-contactaligns with security best practices for upgradeable contracts, enabling responsible disclosure. This matches similar additions across other interfaces in this PR.Makefile (1)
8-13: LGTM! The CI command sequence is well-structured.The reordering is logical:
auditfirst provides fail-fast on dependency vulnerabilitiestypechainbeforelintensures TypeScript type definitions existslitherafterlintensures code is well-formed before static analysistestlast as the most time-consuming steptest/FeeCooldown.test.ts (1)
30-51: LGTM! Clean migration to upgradeable protocol helper.The fixture correctly uses
deployUpgradeableProtocol(owner, owner)with owner as both owner and admin. The extracted components are properly typed and the fixture structure is clean.Note:
automationRegistrysigner is obtained at line 31 but not passed to the helper. The helper defaults to usingadmin(i.e.,owner) as automation registry, which is acceptable for these fee cooldown tests since they don't exercise automation-specific access control.test/orchestrator/Orchestrators.test.ts (4)
118-142: Manual UUPS deployment is appropriate for this comprehensive test.Unlike simpler test files that use
deployUpgradeableProtocolhelper, this test maintains manual deployment for granular control over:
- Custom mock assets with specific decimals
- Complex deposit/gain/loss simulation scenarios
- Fine-grained phase transition testing
The deployment pattern correctly follows UUPS initialization with
kind: "uups"and proper initializer functions.
150-167: LGTM! Beacon and factory deployment follows correct upgrade pattern.The deployment correctly:
- Deploys vault implementation contract
- Creates UpgradeableBeacon pointing to implementation with owner as upgrade authority
- Deploys TransparentVaultFactory as UUPS proxy with beacon address
Using the full contract path
@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol:UpgradeableBeaconavoids potential contract name ambiguity.
682-698: Comprehensive upkeep cycle testing with proper phase transitions.The test correctly validates the full upkeep lifecycle:
- Phase transition verification (Idle → PreprocessingTransparentVaults → Buffering → etc.)
- Fee cooldown duration consideration before triggering upkeep
- Proper use of automation registry for performUpkeep calls
The pattern of processing phases in while loops until transition ensures complete phase processing regardless of minibatch sizes.
2702-2730: LGTM! DOS attack protection tests are well-designed.The test suite correctly validates:
- Owner-only access control for configuring minimum amounts
- Rejection of deposits/redemptions below minimum thresholds
- Event emission on configuration changes
The economic infeasibility calculation at lines 2802-2819 provides good documentation of the attack cost with minimum deposit requirements.
contracts/test/OrionConfigV2.sol (1)
12-31: LGTM! Well-structured mock V2 for upgrade testing.The mock V2 implementation correctly:
- Extends
OrionConfigto inherit UUPS upgrade functionality- Adds new state variable and event for verifying upgrade behavior
- Applies
onlyOwneraccess control on the setter- Provides a
version()function for upgrade verification in testsOrionConfig has adequate storage gap (50 uint256 slots reserved), so
newV2Variablewill not cause storage collisions during upgrade.contracts/test/OrionTransparentVaultV2.sol (1)
12-31: Storage gap verification confirmed—safe for upgrade.
OrionTransparentVaulthas auint256[50] __gapstorage slot reservation, providing adequate space for the newvaultDescriptionstring variable in V2. The implementation correctly follows the beacon proxy upgrade pattern, mirroring theOrionConfigV2approach withonlyVaultOwneraccess control and version detection for upgrade verification.test/orchestrator/OrchestratorsZeroState.test.ts (1)
33-42: No changes needed—admin parameter usage is intentional.The test deliberately passes
useras the admin to OrionConfig initialization. This is a valid test scenario where the admin differs from the owner; other tests in the suite similarly use different admin values (owner,other,attacker, etc.) to test various authorization scenarios.test/OrionVaultExchangeRate.test.ts (2)
1-5: Clean migration to upgradeable protocol helper.The imports and helper integration are correctly set up. The
@openzeppelin/hardhat-upgradesimport enables the upgrades plugin, and the centralized deployment helper simplifies the test setup.
19-25: Usingattackeras admin parameter is intentional but worth documenting.The
deployUpgradeableProtocol(owner, attacker)call passesattackeras theadminparameter. Per the helper signature, admin is used as the default automation registry if not provided. For these tests focused on inflation attacks, this appears intentional sinceattackerdoesn't need admin privileges for the test scenarios.test/Upgrade.test.ts (3)
1-23: Comprehensive upgrade test suite with good structure.The test file properly covers all three upgrade patterns (UUPS, Beacon Proxy, Factory) with appropriate test cases for state preservation, access control, storage gaps, and event emissions.
335-394: Good test for dynamic beacon replacement scenario.This test correctly verifies that vaults created before and after
setVaultBeaconuse different implementations. The assertion that vault1 (V1) doesn't haveversion()is validated indirectly by checkingvaultOwner()instead, which is a reasonable approach.
443-504: Integration test covering factory UUPS upgrade with beacon changes.This test validates the complex scenario of upgrading the factory itself via UUPS while also swapping the vault beacon. Good coverage of the combined upgrade path.
test/Adapters.test.ts (2)
28-50: Intentional type cast for testing adapter validation.The cast of
MockUnderlyingAssettoMockERC4626Asset(line 42) is documented and intentional. This allows testing that adapters correctly reject plain ERC20 tokens that don't implement the ERC4626 interface. The comment at line 41 makes this clear.
31-35: Correct usage of deployUpgradeableProtocol with automation registry.The helper is correctly invoked with
automationRegistryas the fourth parameter, ensuring the orchestrators are properly configured for the adapter tests.test/VaultOwnerRemoval.test.ts (2)
31-37: Usingowneras both owner and admin is appropriate for these tests.The
deployUpgradeableProtocol(owner, owner)call uses the same signer for both owner and admin roles. This is acceptable for vault owner removal tests where the admin role distinction isn't being tested.
57-87: Well-structured vault creation helper with proper event parsing.The
createVaulthelper function encapsulates the factory call and event parsing cleanly. The try/catch pattern for log parsing is consistent with best practices seen in other test files.test/RedeemBeforeDepositOrder.test.ts (2)
95-127: Correct integration with custom underlying asset.The test correctly deploys its own
underlyingAsset(line 100) and passes it todeployUpgradeableProtocol(line 116) to maintain precise control over decimal precision for share/asset calculations. TheautomationRegistryis also correctly passed to enable orchestrator interactions.
145-156: Event parsing consistent with other test files.The vault creation and event parsing pattern matches the robust try/catch approach used in other test files. The non-null assertion on
event!at line 154 is safe given the parsing context.test/AccessControl.test.ts (1)
26-37: LGTM - Clean refactoring to use centralized deployment helper.The test setup is correctly simplified by using
deployUpgradeableProtocol. Passingownerfor both owner and admin parameters is appropriate for this access control test context.test/mainnet-fork/erc4626VaultCompatibility.test.ts (2)
188-225: Mainnet fork test uses constructor-based deployment instead of upgradeable pattern.This test deploys contracts directly with constructors (e.g.,
OrionConfigFactory.deploy(...)) rather than using the upgradeable proxy pattern (upgrades.deployProxy) that the rest of the codebase has migrated to.This inconsistency may be intentional for mainnet fork testing scenarios, but it means this test won't validate the upgradeable deployment path. Consider whether this test should also use
deployUpgradeableProtocolor a similar upgradeable deployment approach for consistency.
317-377: Well-designed immutability verification tests.The enhanced immutability checks are thorough:
- Bytecode consistency across blocks
- EIP-1967 implementation slot detection for proxies
- Cross-block consistency for
asset()anddecimals()- Determinism validation within the same block
This is a solid approach for validating that ERC4626 vaults maintain immutable properties critical for Orion protocol integration.
test/orchestrator/OrchestratorConfiguration.test.ts (1)
119-188: LGTM - Proper customization of upgradeable deployment for orchestrator tests.The test correctly:
- Deploys a custom
MockUnderlyingAssetwith 12 decimals before calling the helper- Passes the custom underlying asset and
automationRegistrytodeployUpgradeableProtocol- Extracts needed components from the deployed bundle
This maintains test-specific requirements while leveraging the centralized deployment helper.
contracts/price/PriceAdapterRegistry.sol (3)
36-55: LGTM - Correct UUPS initialization pattern.The implementation correctly:
- Disables initializers in the constructor to prevent implementation contract takeover
- Uses
initializermodifier on theinitializefunction- Validates zero addresses before setting state
- Calls all required parent initializers (
__Ownable_init,__Ownable2Step_init,__UUPSUpgradeable_init)
76-83: LGTM - Proper UUPS upgrade authorization and storage gap.The
_authorizeUpgradefunction correctly restricts upgrades to the owner only. The 50-slot storage gap follows OpenZeppelin conventions and provides adequate space for future state additions.Consider adding a
newImplementationparameter validation if you want to enforce specific upgrade constraints in the future, though the current empty implementation is acceptable for standard UUPS usage.
21-30: Storage layout correctly implements UUPS upgrade safety.The contract properly declares state variables (
configAddress,priceAdapterDecimals,adapterOf) before the 50-slot storage gap at the end. The constructor correctly disables initializers, the initialize function calls all parent contract initializers, and_authorizeUpgradeis properly protected withonlyOwner. No changes needed.test/MinimumAmountDOS.test.ts (1)
20-72: LGTM - Clean refactoring of fixture to use upgradeable deployment.The fixture properly:
- Uses
deployUpgradeableProtocolfor centralized deployment- Extracts necessary components from the deployed bundle
- Uses
VaultFactory.attach(vaultAddress)which works correctly with beacon proxies since the ABI is the sametest/BatchLimitConsistency.test.ts (4)
1-23: LGTM: Well-structured test helper and imports.The impersonation helper is correctly implemented using
@nomicfoundation/hardhat-network-helpers. The imports are appropriate for the test requirements.
88-133: LGTM: Clean test setup using the centralized deployment helper.The
beforeEachcorrectly utilizesdeployUpgradeableProtocoland properly extracts the vault address from theOrionVaultCreatedevent. The vault creation parameters are appropriate for testing.
196-217: LGTM: Proper setup for redeem tests.The nested
beforeEachcorrectly establishes the precondition of users having shares before testing redeem functionality. The flow of deposit → fulfill → shares is properly implemented.
283-356: LGTM: Comprehensive tests for the critical accounting fix.These tests effectively validate that
pendingDepositandpendingRedeemreturn only the processable amount (up tomaxFulfillBatchSize), preventing the double-counting bug described in the file header.test/ProtocolPause.test.ts (3)
48-48: LGTM: Import updated for upgradeable deployment helper.The import correctly references the centralized deployment helper.
89-95: LGTM: Clean migration to upgradeable protocol deployment.The test correctly uses
deployUpgradeableProtocolwith theautomationRegistryparameter and properly extracts the required components (underlyingAsset,orionConfig,internalStatesOrchestrator,liquidityOrchestrator).
135-136: LGTM: Vault factory sourced from deployed protocol.Correctly obtains
transparentVaultFactoryfrom the deployed protocol helper instead of deploying separately.test/BatchLimitAccounting.test.ts (2)
3-6: LGTM: Updated imports for upgradeable protocol testing.The imports correctly include the OpenZeppelin upgrades plugin and the centralized deployment helper.
23-76: LGTM: Fixture properly migrated to use deployUpgradeableProtocol.The fixture correctly:
- Uses
deployUpgradeableProtocolfor protocol deployment- Extracts all required components from the deployed object
- Maintains the vault creation and user funding logic
- Returns a comprehensive object for test consumption
contracts/OrionConfig.sol (4)
4-6: LGTM: Correct imports for UUPS upgradeable pattern.The imports include all necessary OpenZeppelin upgradeable contracts:
Initializable,Ownable2StepUpgradeable, andUUPSUpgradeable.
100-104: LGTM: Correct UUPS constructor pattern.The constructor properly disables initializers for the implementation contract, preventing direct initialization of the implementation. The
@custom:oz-upgrades-unsafe-allow constructorannotation is correctly placed.
113-137: LGTM: Well-structured initializer with proper validation and setup.The
initializefunction:
- Validates all critical addresses are non-zero
- Correctly initializes
__Ownable_init,__Ownable2Step_init, and__UUPSUpgradeable_init- Sets reasonable defaults for protocol parameters
- Properly whitelists the underlying asset and initial owner
453-460: LGTM: Proper upgrade authorization and storage gap.The
_authorizeUpgradefunction correctly restricts upgrades to the owner, and the 50-slot storage gap (__gap) follows OpenZeppelin's recommended practice for upgradeable contracts, allowing future state variables to be added without storage collisions.test/OrionConfigVault.test.ts (4)
3-17: LGTM: Updated imports for upgradeable protocol testing.The imports correctly include OpenZeppelin upgrades plugin and the necessary helpers (
deployUpgradeableProtocol,impersonateAccount,setBalance).
36-41: LGTM: Correctly uses deployUpgradeableProtocol with owner and admin signers.The deployment uses
owneras the protocol owner andotheras the admin, which aligns with the test's access control scenarios (e.g.,othercalling admin-only functions likeremoveWhitelistedAsset).
387-414: LGTM: Proper setup for redeem cancellation tests using impersonation.The
beforeEachcorrectly:
- Deposits and approves underlying assets
- Funds the LiquidityOrchestrator
- Impersonates the LO to call
fulfillDeposit- Properly stops impersonation after use
This gives the user shares needed for testing redeem functionality.
444-498: LGTM: Comprehensive redeem cancellation tests.The tests cover the critical scenarios:
- Successful full cancellation with proper share restoration
- Partial cancellation with correct remaining balance verification
- Assertions verify both user share balances and pending redeem amounts
The math operations use BigInt correctly (e.g.,
userShares / 2n,(redeemAmount * 3n) / 10n).test/ExecutionAdapterValidation.test.ts (1)
1-67: LGTM - Clean refactor to upgradeable deployment helper.The migration to
deployUpgradeableProtocolsimplifies the test setup significantly. The test correctly extractsunderlyingAsset,orionConfig, andliquidityOrchestratorfrom the deployed bundle and proceeds with ERC4626 vault setup.package.json (1)
82-110: Good security practice with pnpm overrides.The pnpm overrides correctly patch vulnerable OpenZeppelin versions (4.3.0-4.8.3) in transitive dependencies to ^4.9.6. This addresses known security vulnerabilities in older OZ versions without affecting your direct v5.4.0 dependencies.
test/orchestrator/OrchestratorSecurity.test.ts (2)
142-212: Clean migration to upgradeable deployment pattern.The beforeEach setup correctly:
- Deploys the underlying asset with 12 decimals first
- Sets up mock ERC4626 assets with initial deposits
- Passes the pre-deployed underlyingAsset and automationRegistry to the helper
- Extracts the required contracts from the deployed bundle
The security test logic for malicious payload protection remains intact.
206-212: No action needed. Theuserparameter passed as the admin role is an intentional pattern used consistently across multiple test files (Removal.test.ts, PassiveCuratorStrategy.test.ts, OrchestratorConfiguration.test.ts). This design is appropriate for security tests that validate orchestrator resilience against malicious payloads, rather than testing admin-specific permissions.contracts/vaults/OrionTransparentVault.sol (2)
34-77: Correct UUPS upgradeable pattern implementation.The implementation follows OpenZeppelin's recommended upgrade pattern:
- Constructor with
_disableInitializers()prevents implementation contract initializationinitialize()function withinitializermodifier ensures one-time setup- Delegates to
__OrionVault_initfor parent initialization before setting contract-specific state
223-226: Storage gap correctly implemented for upgrade safety.The
uint256[50] private __gapprovides 50 storage slots for future state variables, following OpenZeppelin's upgrade-safe storage pattern. This allows adding new state variables in future versions without storage collision.test/PassiveCuratorStrategy.test.ts (2)
42-113: Clean refactor to upgradeable deployment pattern.The test setup correctly:
- Deploys underlying asset with 12 decimals before calling the helper
- Sets up 4 mock ERC4626 assets with different TVLs for strategy testing
- Passes pre-deployed underlyingAsset and automationRegistry to the helper
- Extracts required contracts from the deployment bundle
Note: Same observation as
OrchestratorSecurity.test.ts-useris passed as the admin argument todeployUpgradeableProtocol. If this is intentional, consider adding a clarifying comment.
1-211: LGTM - Well-structured test migration.The refactoring successfully consolidates protocol deployment while maintaining comprehensive test coverage for:
- Strategy configuration (k parameter)
- Intent computation and weight distribution
- Vault integration with strategy
- Parameter updates and whitelist validation
- Error handling edge cases
test/TransparentVault.test.ts (2)
55-58: LGTM! Clean migration to centralized deployment helper.The test correctly uses
deployUpgradeableProtocoland extracts the required contracts. TheunderlyingAssetis properly passed to ensure consistent asset usage across the test.
1-5: Imports correctly set up for upgradeable testing.The side-effect import of
@openzeppelin/hardhat-upgradesregisters the Hardhat upgrades plugin, anddeployUpgradeableProtocolis properly imported from the helper module.contracts/orchestrators/InternalStatesOrchestrator.sol (3)
168-173: Correct implementation of constructor for upgradeable contract.The constructor properly calls
_disableInitializers()to prevent initialization of the implementation contract, following OpenZeppelin's UUPS pattern.
179-205: Well-structured initialization function.The initializer correctly:
- Validates all input addresses against zero address
- Calls all parent contract initializers in proper order
- Sets up contract state from the config contract
731-739: LGTM! Standard UUPS upgrade authorization and storage gap.The
_authorizeUpgradefunction correctly restricts upgrades to the owner. The 50-slot storage gap follows OpenZeppelin's recommendation for upgradeable contracts.test/helpers/deployUpgradeable.ts (4)
45-61: Well-designed helper with sensible defaults.The function properly handles optional parameters with defaults (automationRegistry defaults to admin, underlying asset created if not provided). The 6-decimal mock asset correctly simulates USDC-like tokens.
80-100: Deployment sequence correctly handles cross-contract dependencies.The comment on line 80 correctly documents the critical ordering: LiquidityOrchestrator must be deployed and registered in config before InternalStatesOrchestrator, since the latter reads
liquidityOrchestratorfrom config during initialization.
102-114: Correct Beacon Proxy pattern implementation.The vault implementation is deployed directly (not as a proxy), then the UpgradeableBeacon is created pointing to this implementation. This allows future vault upgrades by updating the beacon.
149-152: Useful helper for vault interaction in tests.The
attachToVaultfunction provides a clean way to get a typed contract instance for vaults created via the factory's BeaconProxy pattern.contracts/factories/TransparentVaultFactory.sol (3)
28-49: Correct upgradeable initialization pattern.The constructor properly disables initializers, and
initializevalidates all inputs and calls parent initializers in the correct order.
74-90: Correct BeaconProxy deployment pattern.The vault creation correctly encodes the initialization call and deploys a BeaconProxy pointing to the shared vault beacon. The static analysis warning about string length (function signature) is a false positive — function signatures commonly exceed 32 bytes and this is expected.
115-122: LGTM! Standard UUPS authorization and storage gap.The upgrade authorization and storage gap follow OpenZeppelin's recommended patterns.
contracts/vaults/OrionVault.sol (5)
158-162: Correct pattern for abstract upgradeable contract.The constructor properly disables initializers on the implementation, preventing direct initialization of the base contract.
174-216: Well-implemented internal initializer for abstract base.The
__OrionVault_initfunction correctly usesinternal onlyInitializingmodifier pattern for abstract upgradeable contracts. All parent initializers (__ERC20_init, __ERC4626_init, __ReentrancyGuard_init) are called, and comprehensive input validation is performed.
229-286: ERC4626 overrides correctly reference upgradeable base.The function overrides properly specify both
ERC4626Upgradeableand the interface (IERC4626orIERC20Metadata) to satisfy Solidity's override requirements for the upgradeable inheritance chain.
221-227: Correct whitelist initialization pattern.The slither-disable comment for
unused-returnon line 224 is appropriate since the return value ofadd()is safely ignorable during initial population of an empty set.
738-740: Storage gap correctly placed.The 50-slot storage gap at the end of the abstract contract reserves space for future state variables. Storage gaps in storage layout are recommended in order to safely add storage variables when using inheritance. The derived contract
OrionTransparentVaultalso includes its own storage gap, following best practices for upgradeable contract design.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
102-109: Consider addressing deprecated package dependencies.The
allowedDeprecatedVersionssection permits several deprecated packages (glob,inflight,rimraf,lodash.isequal,zksync-web3). While this suppresses warnings, deprecated packages may have:
- Unpatched security vulnerabilities
- Compatibility issues with future Node.js versions
- Missing features or bug fixes
Consider investigating which dependencies introduce these deprecated packages and updating them if newer versions are available.
#!/bin/bash # Description: Identify which dependencies introduce deprecated packages echo "=== Checking dependency tree for deprecated packages ===" for pkg in "glob" "inflight" "rimraf" "lodash.isequal" "zksync-web3"; do echo -e "\n--- Checking: $pkg ---" pnpm why "$pkg" 2>/dev/null || echo "Not found in current dependencies" done
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(3 hunks)
⏰ 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 (3)
package.json (3)
4-4: LGTM: Version bump aligns with major architectural changes.The major version bump to 1.0.0 appropriately reflects the introduction of upgradeability patterns (UUPS, Beacon) and significant protocol architecture changes described in the PR objectives.
40-40: Verify package version specification in package.json (line 40).The version ^4.0.0 referenced does not exist for @nomicfoundation/hardhat-toolbox. The current stable version is 6.1.0. Verify whether this entry targets the correct package or requires updating to an actual released version.
82-86: The pnpm overrides won't result in multiple OZ versions; overrides globally enforce a single version across the dependency graph.The override pattern
@openzeppelin/contracts@>=4.3.0 <4.8.3targets transitive dependencies in that version range and replaces them all with ^4.9.6. If you have a direct dependency on a different major version (e.g., ^5.4.0), both versions may exist—but this is not caused by the override mechanism; it's the natural result of having direct dependencies requiring different versions.Verify whether dependencies actually require v4.x OpenZeppelin contracts and consider upgrading them if possible. If mixing v4.x and v5.x is intentional, ensure storage layouts are compatible for your use case.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/OrionConfigVault.test.ts (2)
388-414: Approve setup with optional helper extraction suggestion.The
beforeEachsetup correctly uses impersonation to enable deposit fulfillment and give the user shares for testing redeem cancellation. The pattern (impersonate → fund gas → call → stop impersonation) is sound.💡 Optional: Extract impersonation pattern to helper function
Consider extracting the impersonation pattern into a reusable helper function to improve maintainability and reduce duplication if this pattern is used elsewhere:
async function fulfillDepositAsOrchestrator( vault: OrionTransparentVault, liquidityOrchestrator: LiquidityOrchestrator, amount: bigint ) { const loAddress = await liquidityOrchestrator.getAddress(); await impersonateAccount(loAddress); await setBalance(loAddress, ethers.parseEther("1")); const loSigner = await ethers.getSigner(loAddress); await vault.connect(loSigner).fulfillDeposit(amount); await ethers.provider.send("hardhat_stopImpersonatingAccount", [loAddress]); }Then use it as:
await fulfillDepositAsOrchestrator(vault, liquidityOrchestrator, depositAmount);
444-471: Consider tightening assertion at line 459 for consistency.The test correctly verifies full redeem request cancellation. However, line 459 uses
gte(greater than or equal) when checkingpendingRedeems, while the partial cancellation test at line 497 uses strictequalafter being corrected per past review feedback.Since this
beforeEachcreates a clean state with only one user making a single redeem request,pendingRedeemsshould matchredeemAmountexactly.🔎 Suggested consistency fix
// Verify redeem request was created const pendingRedeems = await vault.pendingRedeem(await orionConfig.maxFulfillBatchSize()); - expect(pendingRedeems).to.be.gte(redeemAmount); + expect(pendingRedeems).to.equal(redeemAmount);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/OrionConfigVault.test.ts(5 hunks)test/OrionVaultExchangeRate.test.ts(2 hunks)test/Upgrade.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/OrionVaultExchangeRate.test.ts
- test/Upgrade.test.ts
⏰ 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 (3)
test/OrionConfigVault.test.ts (3)
3-3: LGTM! Appropriate imports for upgradeable protocol testing.The addition of
@openzeppelin/hardhat-upgrades, the centralizeddeployUpgradeableProtocolhelper, and Hardhat network helpers for impersonation correctly support the new upgradeable architecture and test flow.Also applies to: 16-17
36-41: LGTM! Centralized deployment improves test maintainability.The adoption of
deployUpgradeableProtocolconsolidates deployment logic and eliminates manual contract wiring, making tests more maintainable and consistent across the suite.
473-534: LGTM! Past review feedback properly addressed.The partial cancellation test now uses strict equality at line 497 (addressing previous review feedback), and both negative test cases from past reviews are properly implemented:
- Lines 500-510: Verifies that attempting to cancel when no pending redeem exists reverts with
InsufficientAmount.- Lines 512-534: Verifies that a different account cannot cancel someone else's pending redeem request.
All tests correctly validate the expected behavior and error conditions for redeem request cancellation.
2 participants
Summary by Sourcery
Introduce a fully upgradeable Orion protocol architecture (config, orchestrators, vault factory and vaults) and centralize deployment logic, while tightening accounting, adapter validation and mainnet-fork safety checks.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.