B34-36-40-42-59 - #199
Merged
Merged
Conversation
…s, and the price registry from CONFIG once per call, then delegates each asset to the new _normalizedTvl helper
…ategist and KBestTvlWeightedAverage contracts
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRedemption transfers now catch failures and record per-user pending claims; users can claim held underlying later. Orchestrator authorizes decommissioned vaults and forwards failed tokens to config. Config preserves failed decommissioning assets for retry. Strategist submitIntent made owner-only; TVL normalization updated; interfaces and tests adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OrionVault
participant LiquidityOrchestrator
participant OrionConfig
User->>OrionVault: fulfillRedeem(batch)
loop per redeem
OrionVault->>LiquidityOrchestrator: transferRedemptionFunds(user, amount)
alt success
LiquidityOrchestrator-->>OrionVault: success
OrionVault->>OrionVault: emit Redeem
else failure
LiquidityOrchestrator--x OrionVault: revert/failed
OrionVault->>OrionVault: pendingUnderlyingClaims[user] += amount
OrionVault->>OrionVault: emit RedemptionFailed
end
end
Note over User,OrionVault: later
User->>OrionVault: claimUnderlying()
OrionVault->>LiquidityOrchestrator: transferRedemptionFunds(user, amount)
LiquidityOrchestrator-->>OrionVault: success
OrionVault->>OrionVault: emit RedemptionClaimed
Note over LiquidityOrchestrator,OrionConfig: epoch final minibatch
LiquidityOrchestrator->>OrionConfig: completeAssetsRemoval(failedTokens)
loop per decommissioning asset
alt asset in failedTokens
OrionConfig->>OrionConfig: skip removal (retain)
else
OrionConfig->>OrionConfig: remove whitelist entry & emit WhitelistedAssetRemoved
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/price/ChainlinkPriceAdapter.sol (1)
172-202:⚠️ Potential issue | 🟡 MinorAdd NatSpec documentation to
getPriceDataexplaining staleness validation strategy.The removal of
answeredInRound < roundIdchecks is aligned with Chainlink's current best practices (as of 2025–2026,answeredInRoundis deprecated in favor ofupdatedAt-based staleness). Modern Chainlink feeds guaranteeansweredInRound == roundId, making the check redundant. Your implementation correctly relies onupdatedAtstaleness validation via the per-feed configurablemaxStalenessthreshold.However,
getPriceDatacurrently lacks NatSpec documentation explaining this staleness strategy. Add a brief note clarifying that freshness relies solely onupdatedAtcomparison, so that operators understand the requirement to tunemaxStalenessconservatively (e.g., heartbeat + safety margin per feed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/price/ChainlinkPriceAdapter.sol` around lines 172 - 202, Add NatSpec to getPriceData documenting the staleness strategy: explain that freshness is determined solely by comparing updatedAt against block.timestamp using the per-feed maxStaleness threshold (no answeredInRound checks are used), note operators must tune maxStaleness conservatively (heartbeat + safety margin), and mention that this applies to both primary feed and optional quoteFeed checks (see variables updatedAt, qUpdatedAt, maxStaleness, and the function getPriceData which returns (price, PRICE_DECIMALS)).test/StrategistLinking.test.ts (1)
171-189:⚠️ Potential issue | 🟠 MajorUpdate the strategist submitIntent tests for owner-only access.
This suite expects non-owner callers to submit intents successfully, but the PR changes strategist
submitIntent()to owner-gated behavior. BothKBestTvlWeightedAverageandKBestApyStrategistnow enforceonlyOwneronsubmitIntent(). Update tests to assert owner success and non-ownerOwnableUnauthorizedAccountreverts:
- Line 189:
strategy.connect(user).submitIntent()should revert- Line 215:
strategy.connect(user).submitIntent()should revert- Line 313:
strategyB.connect(user).submitIntent()should revert- Line 361: Already correctly checks for
ZeroAddress- Lines 369–401: The "submitIntent permissionlessness" test suite is now stale and should be renamed/restructured to test owner-only behavior instead
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/StrategistLinking.test.ts` around lines 171 - 189, Update the tests to reflect that submitIntent() is now owner-only: for KBestTvlWeightedAverage and KBestApyStrategist replace assertions that non-owner calls succeed (e.g., strategy.connect(user).submitIntent(), strategyB.connect(user).submitIntent()) with assertions that the call reverts with the OwnableUnauthorizedAccount/owner-unauthorized error, and change the corresponding positive assertions to use the owner signer (owner.connect(...).submitIntent()) to assert success; also rename/restructure the stale "submitIntent permissionlessness" test suite to indicate and test owner-only behavior instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/interfaces/IOrionConfig.sol`:
- Around line 116-120: The NatSpec for function completeAssetsRemoval is too
narrow: update its `@param` description to state that failedTokens represents
tokens that failed to be sold or purchased (i.e., failures from both SellingLeg
and BuyingLeg) during the epoch so it matches how _failedEpochTokens is
populated; reference the completeAssetsRemoval function and the
_failedEpochTokens variable when making the wording change.
In `@contracts/LiquidityOrchestrator.sol`:
- Line 510: The call to config.completeAssetsRemoval(_failedEpochTokens) is
being invoked every minibatch during ProcessVaultOperations even though
_decommissioningAssets and _failedEpochTokens are stable, wasting gas; change
the flow so completeAssetsRemoval is only called once when the phase transitions
to Idle (i.e., after ProcessVaultOperations completes). Concretely, move or gate
the completeAssetsRemoval invocation so it runs on the phase transition to Idle
(check the phase change from ProcessVaultOperations to Idle) rather than inside
the per-minibatch loop, and rely on existing invariants around
removeWhitelistedAsset and isSystemIdle to ensure the asset lists are stable.
- Around line 509-510: The bug is that delete _failedEpochTokens is executed
inside _processMinibatchVaultsOperations (when i1 == vaultsEpoch.length), so
control returns to config.completeAssetsRemoval(_failedEpochTokens) with an
empty array; fix by preserving the failed tokens before the delete or by
reordering so completeAssetsRemoval is called while _failedEpochTokens is still
populated: e.g., inside the caller that invokes
_processMinibatchVaultsOperations(states.vaults) and then
config.completeAssetsRemoval(_failedEpochTokens), either take a local snapshot
(uint[] memory failedSnapshot = _failedEpochTokens) and pass that to
completeAssetsRemoval, or move the delete _failedEpochTokens out of
_processMinibatchVaultsOperations and only delete it after
config.completeAssetsRemoval(...) completes; reference symbols:
_processMinibatchVaultsOperations, _failedEpochTokens,
config.completeAssetsRemoval, and the delete _failedEpochTokens call.
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Line 50: The submitIntent() function in KBestTvlWeightedAverage should be
hardened with the nonReentrant modifier to match KBestApyStrategist and prevent
future reentrancy risks; update the contract declaration to inherit
OpenZeppelin's ReentrancyGuard (or import it if missing) and add nonReentrant to
the submitIntent() signature, ensuring any required constructor or import
adjustments for ReentrancyGuard are applied so the modifier compiles and behaves
as expected.
- Around line 95-131: The call to IERC20Metadata(vaultUnderlying).decimals()
inside _normalizedTvl is not protected and can revert (or vaultUnderlying may be
address(0)), breaking the intended "fallback to 1 on any external failure";
update _normalizedTvl to wrap the decimals lookup in a try/catch (similar to the
existing totalAssets/asset/getPrice calls), and if the call reverts or
vaultUnderlying == address(0) return 1; ensure you reference the same symbols
(function _normalizedTvl, IERC4626.asset, IERC4626.totalAssets,
priceRegistry.getPrice, IERC20Metadata.decimals) so every external call that can
fail returns the fallback value 1 instead of bubbling a revert.
In `@contracts/vaults/OrionVault.sol`:
- Around line 746-753: claimUnderlying zeroes
pendingUnderlyingClaims[msg.sender] then calls
liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount) and is
protected by nonReentrant, but the function is callable in any system phase
which can cause confusing transient reverts if the LO lacks buffer; either add a
phase check or document the retry behavior. Fix options: 1) add a gate at the
top of claimUnderlying using the existing config.isSystemIdle() (or similar
phase predicate) to require the system be idle before proceeding, referencing
claimUnderlying, pendingUnderlyingClaims, and
liquidityOrchestrator.transferRedemptionFunds; or 2) update the IOrionVault
NatSpec for claimUnderlying to explicitly state callers must expect and retry on
transient transfer reverts (mention pendingUnderlyingClaims and
transferRedemptionFunds) so user expectations are clear. Make only one change
(gate OR NatSpec) and keep nonReentrant and CEI ordering as-is.
---
Outside diff comments:
In `@contracts/price/ChainlinkPriceAdapter.sol`:
- Around line 172-202: Add NatSpec to getPriceData documenting the staleness
strategy: explain that freshness is determined solely by comparing updatedAt
against block.timestamp using the per-feed maxStaleness threshold (no
answeredInRound checks are used), note operators must tune maxStaleness
conservatively (heartbeat + safety margin), and mention that this applies to
both primary feed and optional quoteFeed checks (see variables updatedAt,
qUpdatedAt, maxStaleness, and the function getPriceData which returns (price,
PRICE_DECIMALS)).
In `@test/StrategistLinking.test.ts`:
- Around line 171-189: Update the tests to reflect that submitIntent() is now
owner-only: for KBestTvlWeightedAverage and KBestApyStrategist replace
assertions that non-owner calls succeed (e.g.,
strategy.connect(user).submitIntent(), strategyB.connect(user).submitIntent())
with assertions that the call reverts with the
OwnableUnauthorizedAccount/owner-unauthorized error, and change the
corresponding positive assertions to use the owner signer
(owner.connect(...).submitIntent()) to assert success; also rename/restructure
the stale "submitIntent permissionlessness" test suite to indicate and test
owner-only behavior instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7fa6fb5-6d7c-45a2-9c31-30e1cfe5f83b
📒 Files selected for processing (14)
contracts/LiquidityOrchestrator.solcontracts/OrionConfig.solcontracts/execution/UniswapV3ExecutionAdapter.solcontracts/interfaces/IOrionConfig.solcontracts/interfaces/IOrionStrategist.solcontracts/interfaces/IOrionVault.solcontracts/price/ChainlinkPriceAdapter.solcontracts/strategies/KBestApyStrategist.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/test/MockOrionConfig.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.soltest/LiquidityOrchestratorSlippage.test.tstest/StrategistLinking.test.ts
…ight after the call.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/StrategistLinking.test.ts (1)
400-409: 🧹 Nitpick | 🔵 TrivialDeterminism test is now tautological under owner-only access control.
With
submitIntentrestricted to the owner, comparing two consecutive owner calls against unchanged state is equivalent to asserting the function is idempotent. The previous "output depends on state not caller" semantics can no longer be exercised. Consider strengthening the test by mutating intermediate state (e.g., depositing more TVL between calls) and validating the relationship between the two intents, or rename it to clarify it now asserts idempotency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/StrategistLinking.test.ts` around lines 400 - 409, The test "Owner produces deterministic intent — output depends on state not caller" is now tautological because submitIntent is owner-only; either rename the test to assert idempotency or change it to mutate intermediate state between calls so you exercise state-dependence: keep the first submitIntent()/vault.getIntent() call, then change TVL (e.g., call a deposit function on the vault/strategy or directly modify totalValueLocked via an available helper) and call submitIntent()/vault.getIntent() again, then assert the relationship expected given the TVL change (or if you prefer the simpler approach, rename the test to something like "Owner submitIntent is idempotent" and keep the existing assertions). Ensure you reference submitIntent, vault.getIntent, and whichever deposit helper (e.g., deposit/transferToVault) you use when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/strategies/KBestTvlWeightedAverage.sol`:
- Around line 118-138: The code uses 10 ** underlyingDecimals (where
underlyingDecimals comes from IERC20Metadata.decimals()) which can overflow for
attacker-controlled large decimals; guard this by validating/clamping
underlyingDecimals before exponentiation: after the try that sets
underlyingDecimals, if underlyingDecimals > 77 then return 1 (or set
underlyingDecimals = 77) so that uint256 denominator = 10 **
uint256(underlyingDecimals) cannot overflow, then use that denominator in
Math.mulDiv(rawTvl, underlyingPrice, denominator); reference
IERC20Metadata.decimals, underlyingDecimals, and Math.mulDiv to locate and apply
the check.
In `@contracts/vaults/OrionVault.sol`:
- Around line 769-779: The vault records failed-redemption amounts in
pendingUnderlyingClaims when liquidityOrchestrator.transferRedemptionFunds
reverts, but the LiquidityOrchestrator's bufferAmount (and
pendingProtocolFees/pendingVaultFees) aren’t updated, so update the LO or vault
to maintain the invariant: either (A) increment a tracked counter in
LiquidityOrchestrator (e.g., pendingClaimsTotal) whenever
transferRedemptionFunds fails and ensure all LO methods that spend from balance
(withdrawLiquidity, _executeBuy/_updateBufferAmount, any rescue/rebalance)
factor in pendingClaimsTotal, or (B) explicitly document the invariant and add
defensive checks in LiquidityOrchestrator methods to prevent using balance
beyond bufferAmount - pendingClaimsTotal; locate uses around
transferRedemptionFunds, pendingUnderlyingClaims, bufferAmount,
withdrawLiquidity, _executeBuy/_updateBufferAmount and claimUnderlying to
implement the chosen fix.
---
Outside diff comments:
In `@test/StrategistLinking.test.ts`:
- Around line 400-409: The test "Owner produces deterministic intent — output
depends on state not caller" is now tautological because submitIntent is
owner-only; either rename the test to assert idempotency or change it to mutate
intermediate state between calls so you exercise state-dependence: keep the
first submitIntent()/vault.getIntent() call, then change TVL (e.g., call a
deposit function on the vault/strategy or directly modify totalValueLocked via
an available helper) and call submitIntent()/vault.getIntent() again, then
assert the relationship expected given the TVL change (or if you prefer the
simpler approach, rename the test to something like "Owner submitIntent is
idempotent" and keep the existing assertions). Ensure you reference
submitIntent, vault.getIntent, and whichever deposit helper (e.g.,
deposit/transferToVault) you use when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e124243b-1d5f-43c9-9fd4-2a4ed1801415
📒 Files selected for processing (8)
contracts/LiquidityOrchestrator.solcontracts/OrionConfig.solcontracts/interfaces/IOrionVault.solcontracts/strategies/KBestTvlWeightedAverage.solcontracts/vaults/OrionVault.solpackage.jsontest/NewStrategies.test.tstest/StrategistLinking.test.ts
…Withdraw functions in OrionVault for enhanced deposit and withdrawal controls
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/vaults/OrionVault.sol`:
- Around line 250-268: The decommissioned check in maxRedeem is inverted:
currently it returns 0 when config.isDecommissionedVault(address(this)) is true,
but redeem() is only allowed when the vault IS decommissioned; change the branch
to return 0 when the vault is NOT decommissioned (i.e., use the negation of
config.isDecommissionedVault(address(this))). Update maxRedeem(address owner) so
it still checks system idle and minRedeemAmount as before, and ensure
maxWithdraw() behavior (which calls maxRedeem and convertToAssets) will then
correctly reflect redeem availability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb8ca3ba-3a6b-4052-b2a5-e3744f8efffe
📒 Files selected for processing (1)
contracts/vaults/OrionVault.sol
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Security
Tests
Chores