Dev - #70
Conversation
Reviewer's GuideThis PR implements the full execution pipeline by replacing stub logic and TODOs with concrete minibatch-based sell/buy flows in LiquidityOrchestrator, enhances InternalStatesOrchestrator to emit order arrays with estimated underlying amounts and process vault fees and requests, rewrites execution adapters and vault contracts to support slippage-aware buy/sell signatures, shifts state update responsibilities to the internal orchestrator, and updates all affected interfaces and tests to match the new APIs. Sequence diagram for the new minibatch-based sell/buy execution in LiquidityOrchestratorsequenceDiagram
participant LO as LiquidityOrchestrator
participant ISO as InternalStatesOrchestrator
participant EA as ExecutionAdapter
participant Vault as Vault
LO->>ISO: getOrders()
ISO-->>LO: sellingTokens, sellingAmounts, buyingTokens, buyingAmounts, sellingEstimatedUnderlyingAmounts, buyingEstimatedUnderlyingAmounts
loop For each minibatch in sellingTokens
LO->>EA: sell(token, sharesAmount, estimatedUnderlyingAmount)
EA-->>LO: executionUnderlyingAmount
LO->>LO: update deltaBufferAmount
end
loop For each minibatch in buyingTokens
LO->>EA: buy(token, sharesAmount, estimatedUnderlyingAmount)
EA-->>LO: executionUnderlyingAmount
LO->>LO: update deltaBufferAmount
end
LO->>ISO: updateBufferAmount(deltaBufferAmount)
Class diagram for updated IExecutionAdapter and OrionAssetERC4626ExecutionAdapterclassDiagram
class IExecutionAdapter {
+buy(asset, sharesAmount, maxUnderlyingAmount) uint256
+sell(asset, sharesAmount, minUnderlyingAmount) uint256
}
class OrionAssetERC4626ExecutionAdapter {
+buy(asset, sharesAmount, maxUnderlyingAmount) uint256
+sell(asset, sharesAmount, minUnderlyingAmount) uint256
-underlyingAsset
-liquidityOrchestrator
-underlyingAssetToken
}
IExecutionAdapter <|.. OrionAssetERC4626ExecutionAdapter
Class diagram for updated InternalStatesOrchestrator and order APIclassDiagram
class InternalStatesOrchestrator {
+getOrders() (sellingTokens, sellingAmounts, buyingTokens, buyingAmounts, sellingEstimatedUnderlyingAmounts, buyingEstimatedUnderlyingAmounts)
+getPriceOf(token) uint256
+updateBufferAmount(deltaAmount)
-_countOrders(allTokens)
-_populateOrders(...)
-_currentEpoch
}
Class diagram for updated OrionVault and vault state update logicclassDiagram
class OrionVault {
+convertToSharesWithPITTotalAssets(assets, pointInTimeTotalAssets, rounding) uint256
+accrueCuratorFees(epoch, feeAmount)
+fulfillDeposit(depositTotalAssets)
+fulfillRedeem(redeemTotalAssets)
-pendingCuratorFees
-_depositRequests
-_redeemRequests
}
class OrionTransparentVault {
+updateVaultState(portfolio, newTotalAssets)
}
class OrionEncryptedVault {
+updateVaultState(portfolio, newTotalAssets)
}
OrionVault <|-- OrionTransparentVault
OrionVault <|-- OrionEncryptedVault
Class diagram for updated IOrionVault interfaceclassDiagram
class IOrionVault {
+convertToSharesWithPITTotalAssets(assets, pointInTimeTotalAssets, rounding) uint256
+fulfillDeposit(depositTotalAssets)
+fulfillRedeem(redeemTotalAssets)
+accrueCuratorFees(epoch, feeAmount)
}
Class diagram for updated IInternalStateOrchestrator interfaceclassDiagram
class IInternalStateOrchestrator {
+getOrders() (sellingTokens, sellingAmounts, buyingTokens, buyingAmounts, sellingEstimatedUnderlyingAmounts, buyingEstimatedUnderlyingAmounts)
+getPriceOf(token) uint256
+updateBufferAmount(deltaAmount)
}
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. WalkthroughInterfaces and implementations change buy/sell to share-based APIs with slippage and return values; orchestrators unify order retrieval, track estimated underlying and buffer deltas, and add a FulfillRedeem phase; vaults move control to InternalStatesOrchestrator, add PIT-based conversions and high-water mark updates; mocks, tests, and artifacts updated; package bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ExecAdapter as OrionAssetERC4626ExecutionAdapter
participant Vault as ERC4626 Vault
participant Underlying as Underlying ERC20
rect rgba(230,240,255,0.5)
note right of ExecAdapter: Buy (shares-based, slippage guard)
User->>ExecAdapter: buy(vault, sharesAmount, maxUnderlying)
ExecAdapter->>Vault: previewMint(sharesAmount)
Vault-->>ExecAdapter: estUnderlying
ExecAdapter->>ExecAdapter: require(estUnderlying <= maxUnderlying) else SlippageExceeded
ExecAdapter->>Underlying: transferFrom(User, ExecAdapter, estUnderlying)
ExecAdapter->>Vault: deposit(estUnderlying, ExecAdapter)
Vault-->>ExecAdapter: minted shares
ExecAdapter->>User: transfer shares
ExecAdapter-->>User: return spentUnderlying
end
rect rgba(255,240,230,0.5)
note right of ExecAdapter: Sell (shares-based, slippage guard)
User->>ExecAdapter: sell(vault, sharesAmount, minUnderlying)
ExecAdapter->>Vault: previewMint(sharesAmount)
Vault-->>ExecAdapter: estUnderlying
ExecAdapter->>ExecAdapter: require(estUnderlying >= minUnderlying) else SlippageExceeded
ExecAdapter->>Vault: redeem(sharesAmount, User, ExecAdapter)
Vault-->>User: underlying
ExecAdapter-->>User: return receivedUnderlying
end
sequenceDiagram
autonumber
participant Keeper as Keeper
participant LO as LiquidityOrchestrator
participant ISO as InternalStatesOrchestrator
participant Adapter as ExecutionAdapter
participant Vault as OrionVault
Keeper->>LO: performUpkeep()
LO->>ISO: getOrders()
ISO-->>LO: sellingTokens, sellingAmounts, buyingTokens, buyingAmounts, sellingEst, buyingEst
alt SellingLeg
loop minibatches
LO->>Adapter: sell(token, sharesAmt, minUnderlying)
Adapter-->>LO: executedUnderlying
LO->>LO: deltaBuffer += (executed - est)
end
LO->>LO: switch to BuyingLeg
else BuyingLeg
loop minibatches
LO->>Adapter: buy(token, sharesAmt, maxUnderlying)
Adapter-->>LO: executedUnderlying
LO->>LO: deltaBuffer += (est - executed)
end
LO->>ISO: updateBufferAmount(deltaBuffer)
end
alt FulfillRedeem
LO->>ISO: getVaultTotalAssetsForFulfillRedeem(vault)
LO->>Vault: fulfillRedeem(totalAssets)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![sourcery-ai[bot]](https://avatars.github.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There’s a potential underflow in updateBufferAmount when deltaAmount is more negative than bufferAmount—consider bounding or safe‐checking before subtraction.
- In OrionAssetERC4626ExecutionAdapter, you use IERC4626.previewMint for the sell operation but likely need previewRedeem to estimate underlying out of shares—please verify.
- The loops for estimating underlying amounts and handling minibatches repeat a lot of logic; consider extracting common computations into private helpers for readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a potential underflow in updateBufferAmount when deltaAmount is more negative than bufferAmount—consider bounding or safe‐checking before subtraction.
- In OrionAssetERC4626ExecutionAdapter, you use IERC4626.previewMint for the sell operation but likely need previewRedeem to estimate underlying out of shares—please verify.
- The loops for estimating underlying amounts and handling minibatches repeat a lot of logic; consider extracting common computations into private helpers for readability.
## Individual Comments
### Comment 1
<location> `contracts/orchestrators/LiquidityOrchestrator.sol:371-380` </location>
<code_context>
+ uint256 minUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 - slippageBound, 10000);
</code_context>
<issue_to_address>
**issue (bug_risk):** Slippage calculation uses fixed-point math; consider overflow/underflow risks.
Validation for slippageBound and estimatedUnderlyingAmount is recommended to prevent unexpected results from overflow or underflow.
</issue_to_address>
### Comment 2
<location> `contracts/orchestrators/LiquidityOrchestrator.sol:393-402` </location>
<code_context>
+ uint256 maxUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 + slippageBound, 10000);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Max underlying amount calculation could allow excessive slippage if slippageBound is high.
Enforcing an upper limit on slippageBound would help prevent unintended purchases at unfavorable prices.
Suggested implementation:
```
function _executeBuy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal {
IExecutionAdapter adapter = executionAdapterOf[asset];
if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet();
// Enforce upper limit on slippageBound (e.g., max 10% = 1000)
uint256 MAX_SLIPPAGE_BOUND = 1000;
if (slippageBound > MAX_SLIPPAGE_BOUND) revert ErrorsLib.ExcessiveSlippageBound();
uint256 maxUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 + slippageBound, 10000);
// Approve adapter to spend underlying assets with slippage tolerance
// slither-disable-next-line unused-return
IERC20(underlyingAsset).approve(address(adapter), 0);
// slither-disable-next-line unused-return
IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount);
// Execute buy through adapter, pull underlying assets from this contract and push shares to it.
```
1. You will need to define the `ErrorsLib.ExcessiveSlippageBound()` error in your `ErrorsLib` contract if it does not already exist.
2. If `slippageBound` is not a local or state variable, ensure it is passed as a parameter or available in scope.
3. Adjust `MAX_SLIPPAGE_BOUND` as appropriate for your protocol's risk tolerance.
</issue_to_address>
### Comment 3
<location> `contracts/vaults/OrionVault.sol:557-560` </location>
<code_context>
/// @inheritdoc IOrionVault
- function fulfillDeposit() external onlyLiquidityOrchestrator nonReentrant {
+ function fulfillDeposit(uint256 depositTotalAssets) external onlyInternalStatesOrchestrator nonReentrant {
uint32 length = uint32(_depositRequests.length());
// Collect all requests first to avoid index shifting issues when removing during iteration
</code_context>
<issue_to_address>
**issue (bug_risk):** Switching to convertToSharesWithPITTotalAssets may affect share calculation accuracy.
Verify that depositTotalAssets consistently represents the correct point-in-time value to prevent share misallocation.
</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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
contracts/vaults/OrionVault.sol (2)
258-265: Fix ERC4626 conversion: asymmetric virtual offset breaks pricingUsing +1 in numerator and +10**_decimalsOffset() only in denominator distorts rates and can severely misprice shares. Use symmetric “virtual assets/shares” like OZ’s pattern.
- return shares.mulDiv(pointInTimeTotalAssets + 1, totalSupply() + 10 ** _decimalsOffset(), rounding); + return shares.mulDiv( + pointInTimeTotalAssets + 10 ** _decimalsOffset(), + totalSupply() + 10 ** _decimalsOffset(), + rounding + );
279-296: Update tests & call sites: fulfillDeposit/fulfillRedeem signature/access control changedfulfillDeposit/fulfillRedeem are now onlyInternalStatesOrchestrator and require a new parameter — update tests and any LiquidityOrchestrator call sites that call vault.fulfillDeposit() with no args.
Example failing call sites (tests): test/OrionVaultExchangeRate.test.ts:138, 162, 169, 193, 215, 248, 298, 340, 360, 395, 405, 415, 436, 461
contracts/orchestrators/InternalStatesOrchestrator.sol (3)
328-341: Critical: wrong keys cleared in epoch reset (stale totals leak across epochs).Inside _handleStart you delete vaultsTotalAssets and encryptedVaultsTotalAssets using token keys, but those mappings are keyed by vault addresses. This leaves previous-epoch totals intact for vaults that aren’t re-populated (e.g., encrypted vaults with invalid intents), corrupting protocolTotalAssets, buffer math, and order deltas.
Apply this fix to clear by vault address before you overwrite the epoch vault arrays:
for (uint16 i = 0; i < _currentEpoch.tokens.length; ++i) { address token = _currentEpoch.tokens[i]; delete _currentEpoch.priceArray[token]; delete _currentEpoch.initialBatchPortfolio[token]; - delete _currentEpoch.vaultsTotalAssets[token]; delete _currentEpoch.finalBatchPortfolio[token]; delete _currentEpoch.sellingOrders[token]; delete _currentEpoch.buyingOrders[token]; delete _currentEpoch.tokenExists[token]; - _currentEpoch.encryptedInitialBatchPortfolio[token] = _ezero; - _currentEpoch.encryptedVaultsTotalAssets[token] = _ezero; - _currentEpoch.encryptedFinalBatchPortfolio[token] = _ezero; + _currentEpoch.encryptedInitialBatchPortfolio[token] = _ezero; + _currentEpoch.encryptedFinalBatchPortfolio[token] = _ezero; } + // Clear last epoch totals by vault address (before reloading epoch vault arrays below). + for (uint16 i = 0; i < transparentVaultsEpoch.length; ++i) { + delete _currentEpoch.vaultsTotalAssets[transparentVaultsEpoch[i]]; + } + for (uint16 i = 0; i < encryptedVaultsEpoch.length; ++i) { + delete _currentEpoch.vaultsTotalAssets[encryptedVaultsEpoch[i]]; + _currentEpoch.encryptedVaultsTotalAssets[encryptedVaultsEpoch[i]] = _ezero; + }
702-735: Potential overflow/truncation: Position.value cast to uint32.value can easily exceed 2^32−1 with ERC20 decimals and realistic TVLs. Truncation will corrupt portfolio and downstream order building.
Apply this change (and update the struct in IOrionTransparentVault accordingly):
- IOrionTransparentVault.Position[] memory portfolio = new IOrionTransparentVault.Position[](intentTokens.length); + IOrionTransparentVault.Position[] memory portfolio = new IOrionTransparentVault.Position[](intentTokens.length); ... - portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) }); + portfolio[j] = IOrionTransparentVault.Position({ token: token, value: value });If the interface enforces uint32, switch to weights here and keep shares in-contract; otherwise upgrade Position.value to uint128/uint256.
401-429: Do not invert the math — registry.getPrice returns assets-per-share; update NatSpec (no code change).Verified: PriceAdapterRegistry normalizes adapter.getPriceData and OrionAssetERC4626PriceAdapter.getPriceData returns underlyingAssetAmount for ONE share, so registry.getPrice(token) yields assets-per-share (scaled by priceAdapterPrecision); the existing arithmetic is correct.
Action: Change EpochState.priceArray NatSpec from "[shares/assets]" to "[assets/share]" in contracts/orchestrators/InternalStatesOrchestrator.sol and update any other misleading comments.
🧹 Nitpick comments (29)
contracts/libraries/ErrorsLib.sol (1)
65-66: New slippage error: good addition; consider richer context next.
SlippageExceeded()is fine now. For better UX/debuggability later, consider a variant that includes actual vs. bound amounts or the asset address.artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json (1)
558-586: Document rounding/units for convertToSharesWithPITTotalAssets.Clarify:
- What scale the price ratio uses (e.g., 1e18).
- How Math.Rounding is applied vs convertToAssetsWithPITTotalAssets to guarantee inverse consistency.
Add regression tests for edge cases: 0 assets, 1 wei, and near-overflow totals.
contracts/interfaces/IExecutionAdapter.sol (2)
14-24: Good: buy now slippage-bounded and returns actual underlying spent.Add explicit units in NatSpec (sharesAmount in asset share decimals; maxUnderlyingAmount in underlying decimals) and state MUST revert if execution > max.
interface IExecutionAdapter { + /// @dev Revert convention: implementers SHOULD use ErrorsLib.SlippageExceeded() /// @notice Executes a buy operation by converting underlying assets to asset shares - /// @param sharesAmount The amount of shares to buy - /// @param maxUnderlyingAmount The maximum amount of underlying assets to spend + /// @param sharesAmount Amount of asset shares to buy (asset share decimals) + /// @param maxUnderlyingAmount Max underlying to spend (underlying decimals). MUST revert if exceeded.
25-35: Good: sell now slippage-bounded and returns actual underlying received.Mirror docs as above; MUST revert if execution < minUnderlyingAmount.
- /// @param minUnderlyingAmount The minimum amount of underlying assets to receive + /// @param minUnderlyingAmount Min underlying to receive (underlying decimals). MUST revert if not met.contracts/interfaces/IInternalStateOrchestrator.sol (3)
71-88: getOrders returns six dynamic arrays — specify invariants and consider pagination.
- Document that all arrays have equal length and aligned indices.
- For large minibatches, consider chunked getters to avoid gas-heavy mem copies in on-chain calls.
/// @notice Get selling and buying orders + /// @dev All returned arrays MUST have equal length; i-th entries across arrays refer to the same order. + /// Consider paginated alternatives if minibatch size can grow.
90-94: Define price scale for getPriceOf.Specify decimals/scale (e.g., 1e18) and quote convention (shares per asset or underlying per share) to prevent mismatched math.
95-99: updateBufferAmount access control is only documented — standardize revert.Surface a standard error in the interface (e.g., UnauthorizedAccess) so implementers converge on a single revert reason; document units (underlying decimals).
interface IInternalStateOrchestrator is AutomationCompatibleInterface { + /// @dev Standardized revert for unauthorized callers. + error UnauthorizedAccess(); ... - /// @dev Can only be called by the Liquidity Orchestrator + /// @dev Can only be called by the Liquidity Orchestrator. Reverts UnauthorizedAccess() otherwise. function updateBufferAmount(int256 deltaAmount) external;contracts/vaults/OrionTransparentVault.sol (1)
120-125: Guard high‑water‑mark update when totalSupply is zero.Avoid setting an arbitrary HWM on empty supply; skip until shares exist.
- // Update high watermark if current price is higher - uint256 currentSharePrice = convertToAssets(10 ** decimals()); - - if (currentSharePrice > feeModel.highWaterMark) { - feeModel.highWaterMark = currentSharePrice; - } + // Update high watermark only if shares exist, and if current price is higher + if (totalSupply() > 0) { + uint256 currentSharePrice = convertToAssets(10 ** decimals()); + if (currentSharePrice > feeModel.highWaterMark) { + feeModel.highWaterMark = currentSharePrice; + } + }artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
519-530: Emit event on slippage bound changes.Operationally useful and aids off‑chain monitoring/auditing.
- function setSlippageBound(uint256 _slippageBound) + event SlippageBoundUpdated(uint256 oldValue, uint256 newValue); + function setSlippageBound(uint256 _slippageBound) external { + emit SlippageBoundUpdated(slippageBound, _slippageBound); slippageBound = _slippageBound; }contracts/vaults/OrionEncryptedVault.sol (1)
186-192: Mirror HWM zero‑supply guard here as well.Same rationale as TransparentVault.
- // Update high watermark if current price is higher - uint256 currentSharePrice = convertToAssets(10 ** decimals()); - - if (currentSharePrice > feeModel.highWaterMark) { - feeModel.highWaterMark = currentSharePrice; - } + // Update high watermark only if shares exist, and if current price is higher + if (totalSupply() > 0) { + uint256 currentSharePrice = convertToAssets(10 ** decimals()); + if (currentSharePrice > feeModel.highWaterMark) { + feeModel.highWaterMark = currentSharePrice; + } + }artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json (1)
73-89: Output name divergence vs interface — align for clarity.Interface uses executionUnderlyingAmount; adapter uses spentUnderlyingAmount/receivedUnderlyingAmount. Types match so ABI is fine; consider harmonizing names to reduce confusion in typed clients.
Also applies to: 128-144
artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json (1)
535-563: New convertToSharesWithPITTotalAssets — document rounding semantics for integrators.Make sure docs specify expected rounding behaviors to avoid off‑by‑one issues in clients.
contracts/mocks/MockExecutionAdapter.sol (1)
13-19: LGTM; minor hygiene nitsMock behavior is fine. To silence unused-param warnings in some toolchains, prefix params with underscore or use them in a no-op. Optional only.
- function buy( - address asset, - uint256 sharesAmount, - uint256 maxUnderlyingAmount - ) external returns (uint256 executionUnderlyingAmount) { + function buy( + address /*asset*/, + uint256 /*sharesAmount*/, + uint256 /*maxUnderlyingAmount*/ + ) external returns (uint256 executionUnderlyingAmount) { executionUnderlyingAmount = 1e12; } ... - function sell( - address asset, - uint256 sharesAmount, - uint256 minUnderlyingAmount - ) external returns (uint256 executionUnderlyingAmount) { + function sell( + address /*asset*/, + uint256 /*sharesAmount*/, + uint256 /*minUnderlyingAmount*/ + ) external returns (uint256 executionUnderlyingAmount) { executionUnderlyingAmount = 1e12; }Also applies to: 22-28
contracts/orchestrators/LiquidityOrchestrator.sol (7)
83-94: Public arrays expose mutable epoch state; consider events or gettersPublishing epoch arrays is convenient but increases surface area. Consider private storage + view getters or emitting events at start to reduce accidental external coupling.
256-259: performData length check is misleadingABI-encoded (bytes4,uint8) is 64 bytes. The “< 5” guard is ineffective. Decode and rely on abi.decode revert, or check >= 64.
- if (performData.length < 5) revert ErrorsLib.InvalidArguments(); + // abi.decode will revert on malformed input; explicit length check not needed
287-299: Reset minibatch index at epoch startcurrentMinibatchIndex should reset at _handleStart() to avoid stale indices from prior epochs.
// Clear previous epoch data delete sellingTokens; delete sellingAmounts; delete buyingTokens; delete buyingAmounts; delete sellingEstimatedUnderlyingAmounts; delete buyingEstimatedUnderlyingAmounts; deltaBufferAmount = 0; + currentMinibatchIndex = 0;Also applies to: 301-306
312-332: Minibatch range checks and index bump ordering
- Use >= instead of (i1 > len || i1 == len).
- Increment currentMinibatchIndex after successful processing to better reflect progress.
- ++currentMinibatchIndex; - uint16 i0 = minibatchIndex * executionMinibatchSize; + uint16 i0 = minibatchIndex * executionMinibatchSize; uint16 i1 = i0 + executionMinibatchSize; - if (i1 > sellingTokens.length || i1 == sellingTokens.length) { + if (i1 >= sellingTokens.length) { i1 = uint16(sellingTokens.length); currentPhase = LiquidityUpkeepPhase.BuyingLeg; currentMinibatchIndex = 0; - } + } else { + ++currentMinibatchIndex; + }
337-361: Mirror the sell-path fixes for buyApply the same >= check and post-processing index bump, and keep updateBufferAmount call gated to final minibatch (already correct).
- ++currentMinibatchIndex; uint16 i0 = minibatchIndex * executionMinibatchSize; uint16 i1 = i0 + executionMinibatchSize; - if (i1 > buyingTokens.length || i1 == buyingTokens.length) { + if (i1 >= buyingTokens.length) { i1 = uint16(buyingTokens.length); currentPhase = LiquidityUpkeepPhase.Idle; currentMinibatchIndex = 0; - } + } else { + ++currentMinibatchIndex; + }
371-383: Use SafeERC20 for non‑standard tokensApprove/transfer patterns on some tokens (e.g., missing return booleans) can misbehave. Recommend SafeERC20 for approves/transfers here.
- IERC20(asset).approve(address(adapter), 0); - IERC20(asset).approve(address(adapter), sharesAmount); + SafeERC20.safeApprove(IERC20(asset), address(adapter), 0); + SafeERC20.safeApprove(IERC20(asset), address(adapter), sharesAmount); ... - IERC20(underlyingAsset).approve(address(adapter), 0); - IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount); + SafeERC20.safeApprove(IERC20(underlyingAsset), address(adapter), 0); + SafeERC20.safeApprove(IERC20(underlyingAsset), address(adapter), maxUnderlyingAmount);Remember to import SafeERC20 and use SafeERC20 for transfers too.
Also applies to: 393-405
67-69: Dead/unused field: targetBufferRatiotargetBufferRatio is set but never used. Remove or wire into buffer logic.
- uint256 public targetBufferRatio; + // uint256 public targetBufferRatio; // unusedcontracts/vaults/OrionVault.sol (2)
457-461: Performance fee share price uses asymmetric +1Use symmetric virtual offset to avoid under/overcharging near zero TVL or supply.
- uint256 activeSharePrice = (10 ** decimals()).mulDiv( - feeTotalAssets + 1, - totalSupply() + 10 ** _decimalsOffset(), - Math.Rounding.Floor - ); + uint256 activeSharePrice = (10 ** decimals()).mulDiv( + feeTotalAssets + 10 ** _decimalsOffset(), + totalSupply() + 10 ** _decimalsOffset(), + Math.Rounding.Floor + );
286-288: Consider SafeERC20 for transfersFor robustness with non‑standard ERC‑20s, prefer SafeERC20 for transfer/transferFrom.
Also applies to: 364-367
test/Orchestrators.test.ts (2)
476-482: getOrders: consider asserting new return tails too.Contract now returns 6 arrays; you destructure 4. Optionally also assert on sellingEstimatedUnderlyingAmounts/buyingEstimatedUnderlyingAmounts lengths to guard the new API.
-const [sellingTokens, _sellingAmounts, buyingTokens, _buyingAmounts] = await internalStatesOrchestrator.getOrders(); +const [ + sellingTokens, + _sellingAmounts, + buyingTokens, + _buyingAmounts, + _sellingUnderlyingEst, + _buyingUnderlyingEst, +] = await internalStatesOrchestrator.getOrders(); +expect(_sellingUnderlyingEst.length).to.equal(sellingTokens.length); +expect(_buyingUnderlyingEst.length).to.equal(buyingTokens.length);
537-565: Liquidity orchestrator multi-step flow: OK, but consider tightening assertions.You already assert phase transitions; optionally add an epoch/targetBufferRatio validation after the last step for extra coverage.
contracts/orchestrators/InternalStatesOrchestrator.sol (4)
489-499: Encrypted math TODO: track upstream and guard for precision.The OZ confidential contracts issue is still open; consider bounding intermediate values to avoid overflow when priceAdapterPrecision and decimals differ widely.
570-606: Encrypted vaults: same PIT + price semantics concerns as transparent path.Mirror the verification and any fix applied to the transparent path to keep both branches consistent.
903-907: getPriceOf: consider fallback behavior.Returning zero for unseen tokens may confuse callers; optionally revert on unset price or compute on-demand.
914-921: updateBufferAmount: guard negative deltas to avoid underflow panic.Current subtraction relies on checked arithmetic and will revert with a generic panic if |delta| > bufferAmount. Emit a domain-specific error instead.
function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { if (deltaAmount > 0) { bufferAmount += uint256(deltaAmount); } else if (deltaAmount < 0) { - bufferAmount -= uint256(-deltaAmount); + uint256 abs = uint256(-deltaAmount); + if (abs > bufferAmount) revert ErrorsLib.InsufficientAmount(); + bufferAmount -= abs; } }contracts/execution/OrionAssetERC4626ExecutionAdapter.sol (1)
49-55: Failing TXs TODO: capture per-asset failures and propagate deltas.Agree with the TODO. Consider returning a bitmap and per-asset deltas so the LO can reconcile partial executions against ISO totals without blocking epochs.
I can sketch an error-reporting interface and the LO reconciliation flow if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(1 hunks)artifacts/contracts/execution/OrionAssetERC4626ExecutionAdapter.sol/OrionAssetERC4626ExecutionAdapter.json(4 hunks)artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json(2 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(2 hunks)artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json(2 hunks)artifacts/contracts/interfaces/IOrionTransparentVault.sol/IOrionTransparentVault.json(2 hunks)artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json(2 hunks)artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json(2 hunks)artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json(1 hunks)artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json(2 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(6 hunks)artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json(1 hunks)artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json(1 hunks)artifacts/contracts/vaults/OrionVault.sol/OrionVault.json(2 hunks)contracts/execution/OrionAssetERC4626ExecutionAdapter.sol(1 hunks)contracts/interfaces/IExecutionAdapter.sol(1 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/interfaces/IOrionVault.sol(2 hunks)contracts/libraries/ErrorsLib.sol(1 hunks)contracts/mocks/MockERC4626Asset.sol(1 hunks)contracts/mocks/MockExecutionAdapter.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(8 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(5 hunks)contracts/price/OrionAssetERC4626PriceAdapter.sol(0 hunks)contracts/vaults/OrionEncryptedVault.sol(2 hunks)contracts/vaults/OrionTransparentVault.sol(2 hunks)contracts/vaults/OrionVault.sol(5 hunks)package.json(1 hunks)test/EncryptedVault.test.ts(0 hunks)test/Orchestrators.test.ts(7 hunks)test/OrionConfigVault.test.ts(0 hunks)test/TransparentVault.test.ts(0 hunks)
💤 Files with no reviewable changes (4)
- contracts/price/OrionAssetERC4626PriceAdapter.sol
- test/OrionConfigVault.test.ts
- test/TransparentVault.test.ts
- test/EncryptedVault.test.ts
🧰 Additional context used
🧬 Code graph analysis (8)
contracts/interfaces/IOrionVault.sol (1)
test/OrionVaultExchangeRate.test.ts (5)
it(428-474)impersonateLiquidityOrchestrator(5-475)it(124-181)it(381-426)it(183-235)
contracts/vaults/OrionTransparentVault.sol (2)
test/OrionVaultExchangeRate.test.ts (4)
impersonateLiquidityOrchestrator(5-475)it(428-474)it(381-426)it(124-181)test/EncryptedVault.test.ts (1)
newTotalAssets(434-447)
artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json (2)
test/EncryptedVault.test.ts (1)
MockERC4626AssetFactory(315-342)test/TransparentVault.test.ts (1)
MockERC4626AssetFactory(261-283)
artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (1)
test/OrionVaultExchangeRate.test.ts (9)
it(428-474)it(124-181)it(183-235)it(331-379)impersonateLiquidityOrchestrator(5-475)loadFixture(125-150)it(381-426)it(237-329)loadFixture(332-349)
artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json (3)
test/OrionVaultExchangeRate.test.ts (7)
it(428-474)it(124-181)it(183-235)impersonateLiquidityOrchestrator(5-475)it(331-379)it(237-329)it(381-426)test/EncryptedVault.test.ts (1)
newTotalAssets(434-447)test/OrionConfigVault.test.ts (1)
it(594-615)
contracts/mocks/MockERC4626Asset.sol (2)
test/EncryptedVault.test.ts (1)
MockERC4626AssetFactory(315-342)test/TransparentVault.test.ts (1)
MockERC4626AssetFactory(261-283)
contracts/orchestrators/InternalStatesOrchestrator.sol (2)
test/OrionVaultExchangeRate.test.ts (6)
impersonateLiquidityOrchestrator(5-475)it(428-474)it(381-426)deployVaultFixture(14-95)it(124-181)loadFixture(125-150)test/OrionConfigVault.test.ts (1)
describe(429-679)
contracts/vaults/OrionEncryptedVault.sol (2)
test/EncryptedVault.test.ts (1)
newTotalAssets(434-447)test/OrionVaultExchangeRate.test.ts (2)
impersonateLiquidityOrchestrator(5-475)it(428-474)
⏰ 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 (35)
package.json (1)
4-4: Version bump looks good; remember to tag and publish.Ensure a 0.4.4 release tag and changelog entry accompany this, so downstreams can pin artifacts reliably.
artifacts/contracts/price/OrionAssetERC4626PriceAdapter.sol/OrionAssetERC4626PriceAdapter.json (1)
91-92: Sanity‑check creation vs runtime bytecode.The artifact flips changed both fields. Just double‑check they are not swapped to avoid deployment/runtime mismatches.
artifacts/contracts/interfaces/IOrionVault.sol/IOrionVault.json (2)
535-563: Add boundary/rounding tests for PIT conversion API
- Test convertToSharesWithPITTotalAssets(assets, pointInTimeTotalAssets, rounding) and convertToAssetsWithPITTotalAssets(...): rounding edges (0, 1 wei, max values), discontinuities, and monotonicity between the two conversions (both Floor and Ceil).
- Call-sites/interfaces: contracts/interfaces/IOrionVault.sol; contracts/vaults/OrionVault.sol — convertToShares at line ~560, convertToAssets at line ~590; contracts/orchestrators/InternalStatesOrchestrator.sol — calls at lines ~416 and ~592.
- Repo search shows no tests referencing these functions — add unit tests (test/**/*.ts) covering rounding modes, boundary values, and conversion consistency.
647-658: fulfillRedeem signature changed — callers migrated; validate accounting
- Definition/interface: contracts/vaults/OrionVault.sol:568, contracts/interfaces/IOrionVault.sol:189.
- Call sites updated: contracts/orchestrators/InternalStatesOrchestrator.sol:421 and 597.
- No zero‑arg invocations found.
- Action: Manually verify accounting under slippage/fee paths and that deposit/redeem symmetry (fee paths) is correct.
contracts/mocks/MockERC4626Asset.sol (1)
8-13: Mock constructor simplification: OK — tests expect 18 share decimals.
Tests deploy MockERC4626Asset without a decimals arg and the vault decimals test asserts 18 (test/OrionConfigVault.test.ts:473–477); no tests assume shares mirror the underlying token's decimals.artifacts/contracts/OrionConfig.sol/OrionConfig.json (1)
586-588: ABI added/changed in artifacts — confirm no ABI/interface drift.
artifacts/contracts/OrionConfig.sol/OrionConfig.json: origin/main had no ABI at .abi; this PR file contains a full ABI (new ABI added). Confirm this is intentional and that the contract interface hasn't changed; if not intended, revert or regenerate artifacts.artifacts/contracts/mocks/MockERC4626Asset.sol/MockERC4626Asset.json (1)
821-823: Constructor change — update deployments to 3 argsMockERC4626Asset dropped the decimals constructor parameter; update all tests/scripts/factories that deploy it to pass 3 constructor args (not 4). Automated searches in this environment failed to locate usages — run a repo-wide search and update any remaining deploy calls.
artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json (3)
20-36: ABI aligned with shares-based buy; ensure return value is used.buy(...) now returns executionUnderlyingAmount. Verify orchestrators/tests read and propagate this return for buffer/slippage accounting.
49-65: ABI aligned with shares-based sell; ensure return value is used.sell(...) now returns executionUnderlyingAmount. Verify all call sites.
70-71: Bytecode changed without ABI change — confirm expected rebuild.
ABI unchanged: buy(address,uint256,uint256), sell(address,uint256,uint256).
Artifact (artifacts/contracts/mocks/MockExecutionAdapter.sol/MockExecutionAdapter.json) has no compiler/metadata; bytecode len=468 (sha256=6d3fce9f48c02eebbb7ed0faf1f36061a2063435d30cc26ea6906c9262d4d70d), deployedBytecode len=416 (sha256=51d97ad033d1ab79685b9c81c691f176acc5980a76bec6c87cb008ef90d9a737).
Confirm this recompile is intentional and limited to build settings (compiler/version/optimizer); if yes, include compiler+metadata or document the build config; if not, revert or rebuild with the original settings.artifacts/contracts/price/PriceAdapterRegistry.sol/PriceAdapterRegistry.json (1)
226-227: Bytecode-only change — ABI unchanged; confirm compiler/optimizer/metadataABI from the artifact matches the existing external surface (adapterOf, configAddress, getPrice, owner, priceAdapterDecimals, renounceOwnership, setPriceAdapter, transferOwnership, unsetPriceAdapter).
- Verify compiler version, optimizer settings and build metadata (hardhat/foundry/solc config and CI) to confirm the bytecode diff is non-functional and to restore reproducible builds if needed.
- If logic did change, add/extend tests for owner/set/unset flows.
artifacts/contracts/interfaces/IOrionEncryptedVault.sol/IOrionEncryptedVault.json (1)
670-681: No action required — fulfillRedeem callers already updated.
contracts/orchestrators/InternalStatesOrchestrator.sol:421,597 call vault.fulfillRedeem(totalAssets); contracts/vaults/OrionVault.sol:568 and contracts/interfaces/IOrionVault.sol:189 declare fulfillRedeem(uint256 redeemTotalAssets).artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (2)
837-865: convertToSharesWithPITTotalAssets added — ensure symmetry with convertToAssetsWithPITTotalAssetsConfirm both functions use identical rounding policies (same Math.Rounding parameter handling and rounding direction) to preserve invertibility; inspect contracts/vaults/OrionVault.sol (functions near lines ~259 and ~268) and align if they differ.
977-988: fulfillDeposit/fulfillRedeem — confirm orchestrator wiringInternalStatesOrchestrator forwards the local variable totalAssets to the vault calls (vault.fulfillRedeem(totalAssets) at contracts/orchestrators/InternalStatesOrchestrator.sol:421 & 597; vault.fulfillDeposit(totalAssets) at …:426 & 602). Order: fulfillRedeem is invoked before totalAssets -= pendingRedeem; fulfillDeposit is invoked after that subtraction but before totalAssets += pendingDeposit; the epoch value _currentEpoch.vaultsTotalAssets[address(vault)] is set after adding pendingDeposit. Confirm OrionVault.fulfillRedeem(uint256) and fulfillDeposit(uint256 depositTotalAssets) expect the exact pre/post-adjusted totals forwarded here — if their semantics differ, adjust the orchestrator call-site or vault logic to avoid epoch-state drift. See artifacts/contracts/vaults/OrionVault.sol/OrionVault.json (fulfillDeposit ABI around lines 977–988 and 990–1001).
artifacts/contracts/libraries/ErrorsLib.sol/ErrorsLib.json (1)
67-71: SlippageExceeded added — ABI updated; confirm uniform usage across adapters/orchestrators.ErrorsLib defines SlippageExceeded (contracts/libraries/ErrorsLib.sol:66). Revert occurrences found at contracts/execution/OrionAssetERC4626ExecutionAdapter.sol:74 and :100; artifacts include the ABI entry. Confirm other adapters/orchestrators that revert on slippage reference ErrorsLib.SlippageExceeded.
contracts/vaults/OrionEncryptedVault.sol (1)
169-169: Confirmed — access restricted to InternalStatesOrchestrator and tests/orchestrator call sites present.InternalStatesOrchestrator invokes updateVaultState (contracts/orchestrators/InternalStatesOrchestrator.sol:734). Tests call updateVaultState via impersonated liquidity orchestrator (test/OrionVaultExchangeRate.test.ts) and EncryptedVault.test asserts non-orchestrator calls revert (test/EncryptedVault.test.ts:442). Transparent/Encrypted vaults both use onlyInternalStatesOrchestrator (contracts/vaults/OrionTransparentVault.sol:109; contracts/vaults/OrionEncryptedVault.sol:169).
artifacts/contracts/interfaces/IExecutionAdapter.sol/IExecutionAdapter.json (1)
15-31: Breaking signature changes for buy/sell — callers updated and return value consumed.
Interface and adapters use the new 3-arg signature and LiquidityOrchestrator assigns the returned executionUnderlyingAmount.
Locations: contracts/interfaces/IExecutionAdapter.sol; contracts/orchestrators/LiquidityOrchestrator.sol:380,402; contracts/execution/OrionAssetERC4626ExecutionAdapter.sol:57,83; contracts/mocks/MockExecutionAdapter.sol:13,22artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
194-212: Index-aligned arrays — verified; no additional runtime checks required.
InternalStatesOrchestrator.getOrders() allocates selling/buying/estimated arrays with matching sizes and _populateOrders fills the estimated arrays using the same indices; LiquidityOrchestrator assigns those arrays atomically and iterates using token array lengths, so index reads are safe.
Locations: contracts/orchestrators/InternalStatesOrchestrator.sol (~819–824, 881–896); contracts/orchestrators/LiquidityOrchestrator.sol (~283–297, 318–355).contracts/orchestrators/LiquidityOrchestrator.sol (1)
237-247: checkUpkeep action selection: OKBranching covers start/sell/buy phases cleanly.
contracts/vaults/OrionVault.sol (2)
133-136: Access control shift: goodonlyInternalStatesOrchestrator modifier is correct for the new flow.
560-564: Incorrect — fulfillDeposit uses the vault's point-in-time totalAssets (PIT) as the denominatorInternalStatesOrchestrator computes the vault's point-in-time totalAssets and calls vault.fulfillDeposit(totalAssets); fulfillDeposit forwards that value into convertToSharesWithPITTotalAssets(..., depositTotalAssets, ...), so shares are minted against the PIT totalAssets, not the sum of deposits. See contracts/orchestrators/InternalStatesOrchestrator.sol (STEP 6 calls at ~lines 423–427 and 599–603) and contracts/vaults/OrionVault.sol (convertToSharesWithPITTotalAssets ~268–271; fulfillDeposit ~538–563).
Likely an incorrect or invalid review comment.
contracts/interfaces/IOrionVault.sol (2)
96-106: Rounding enum mapping verified — no action requiredInterfaces declare Math.Rounding and implementations/call sites pass Math.Rounding.* (e.g., contracts/interfaces/IOrionVault.sol; contracts/vaults/OrionVault.sol; contracts/orchestrators/InternalStatesOrchestrator.sol); the compiled ABI encodes the enum as uint8 (expected). No additional mapping or change needed.
183-189: ```shell
#!/bin/bash
set -euo pipefailecho "Listing tracked .sol files (git ls-files):"
git ls-files '*.sol' || true
echoecho "Searching for fulfillDeposit occurrences:"
rg -nP "\bfulfillDeposit\s*(" -C3 --hidden --no-ignore-vcs || true
echoecho "Searching for fulfillRedeem occurrences:"
rg -nP "\bfulfillRedeem\s*(" -C3 --hidden --no-ignore-vcs || true
echoecho "Searching for PIT / totalAssets / InternalStatesOrchestrator / param names:"
rg -nP "InternalStatesOrchestrator|pointInTimeTotalAssets|depositTotalAssets|redeemTotalAssets|pointInTime|PIT|totalAssets" -C3 --hidden --no-ignore-vcs || true
echoecho "Locate IOrionVault interface file(s) and print lines ~160-200:"
FILES=$(rg --files-with-matches -n "interface\s+IOrionVault" || true)
if [ -n "$FILES" ]; then
for f in $FILES; do
echo "---- $f ----"
sed -n '1,260p' "$f" | sed -n '160,200p' || true
done
else
echo "No IOrionVault interface file found by rg."
fi</blockquote></details> <details> <summary>artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (2)</summary><blockquote> `94-125`: **Incorrect — getOrders already enforces matching array lengths.** getOrders computes selling/buying counts with _countOrders, allocates arrays of those exact sizes, and populates them with _populateOrders using the same criteria; mismatched lengths cannot occur. See contracts/orchestrators/InternalStatesOrchestrator.sol (getOrders/_countOrders/_populateOrders). > Likely an incorrect or invalid review comment. --- `201-213`: **Signed delta OK — checked arithmetic and access control verified** updateBufferAmount(int256) (contracts/orchestrators/InternalStatesOrchestrator.sol:915–920) converts the signed delta to uint and applies +=/− (Solidity checked arithmetic will revert on overflow/underflow); onlyLiquidityOrchestrator (contracts/orchestrators/InternalStatesOrchestrator.sol:158–161) restricts callers to the LiquidityOrchestrator. No changes required. </blockquote></details> <details> <summary>test/Orchestrators.test.ts (6)</summary><blockquote> `31-34`: **Adapter vars wiring looks correct.** Declaring distinct Orion price/execution adapters per asset aligns with the new API surface. --- `108-111`: **Third price adapter deployment LGTM.** Consistent with prior two instances and the config address wiring. --- `162-179`: **Execution adapters deployment LGTM.** Factory + config injection are correct and consistent across 3 instances. --- `185-199`: **Whitelist entries wired to Orion adapters — good.** Each asset whitelisted with its corresponding Orion price and execution adapter. --- `345-347`: **Idle-guard coverage for addWhitelistedAsset is good.** Revert expectation matches SystemNotIdle gating. --- `525-535`: **Amount sanity checks are fine.** Loops correctly no-op when arrays are empty; > 0 guards are appropriate. </blockquote></details> <details> <summary>contracts/orchestrators/InternalStatesOrchestrator.sol (4)</summary><blockquote> `413-428`: **Order of deposit/redeem accounting: double-check PIT usage.** You pass totalAssets (post-fee, pre-withdraw/deposit) into fulfillRedeem/fulfillDeposit and then adjust totalAssets locally. If vault math assumes PIT is pre-withdraw/pre-deposit, this is OK; otherwise, exchange-rate drift can occur. If vault expects different PIT points, swap the arithmetic: - subtract pendingRedeem before calling fulfillRedeem if PIT should reflect post-withdrawals. - add pendingDeposit before calling fulfillDeposit if PIT should reflect post-deposits. --- `803-836`: **getOrders: good zero-elision and estimated-underlying outputs.** API and helpers look clean and efficient. --- `839-853`: **_countOrders: OK.** Linear scan bounded by small token set; fine. --- `856-901`: **_populateOrders: price usage consistent with getOrders semantics.** Estimates derived via mulDiv(price, precision) match “assets per share” interpretation; see earlier note on price units. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 (4)
contracts/orchestrators/InternalStatesOrchestrator.sol (4)
330-336: Bug: clearing vaultsTotalAssetsForFulfillRedeem by token key leaves stale per‑vault stateYou delete vaultsTotalAssetsForFulfillRedeem using token addresses, but the mapping is keyed by vault addresses. This leaves previous-epoch values uncleared and can leak stale exchange-rate inputs across epochs.
Apply this diff to delete by prior epoch vault addresses and remove the wrong delete:
for (uint16 i = 0; i < _currentEpoch.tokens.length; ++i) { address token = _currentEpoch.tokens[i]; delete _currentEpoch.priceArray[token]; delete _currentEpoch.initialBatchPortfolio[token]; delete _currentEpoch.vaultsTotalAssets[token]; - delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[token]; delete _currentEpoch.finalBatchPortfolio[token]; delete _currentEpoch.sellingOrders[token]; delete _currentEpoch.buyingOrders[token]; delete _currentEpoch.tokenExists[token]; _currentEpoch.encryptedInitialBatchPortfolio[token] = _ezero; _currentEpoch.encryptedVaultsTotalAssets[token] = _ezero; _currentEpoch.encryptedFinalBatchPortfolio[token] = _ezero; } +// Clear fulfillRedeem totals for previous epoch vaults +for (uint16 i = 0; i < transparentVaultsEpoch.length; ++i) { + delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[transparentVaultsEpoch[i]]; +} +for (uint16 i = 0; i < encryptedVaultsEpoch.length; ++i) { + delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[encryptedVaultsEpoch[i]]; +}
571-605: Encrypted path never fulfills redeem — withdrawals not appliedYou compute pendingRedeem and set PIT totals but never call fulfillRedeem. This leaves redemption shares unburned and exchange-rate state inconsistent with the transparent path.
uint256 pendingRedeem = vault.convertToAssetsWithPITTotalAssets( vault.pendingRedeem(), totalAssets, Math.Rounding.Floor ); _currentEpoch.vaultsTotalAssetsForFulfillRedeem[address(vault)] = totalAssets; +vault.fulfillRedeem(totalAssets); // STEP 6: DEPOSIT PROCESSING (add deposits, subtract withdrawals) totalAssets -= pendingRedeem;
653-675: Division-by-zero risk in _buffer when TVL=0 and delta=0When protocolTotalAssets==0 and deltaBufferAmount==0, mulDiv(0, x, 0) will revert. Short‑circuit before proportional allocation.
uint256 targetBufferAmount = protocolTotalAssets.mulDiv( liquidityOrchestrator.targetBufferRatio(), BASIS_POINTS_FACTOR ); // Only increase buffer if current buffer is below target (conservative approach) if (bufferAmount > targetBufferAmount) return; uint256 deltaBufferAmount = targetBufferAmount - bufferAmount; +if (protocolTotalAssets == 0 || deltaBufferAmount == 0) { + return; +}
701-729: uint32 truncation in Position.valueCasting value to uint32 can silently truncate for larger vaults/tokens. Add a bound check (or widen the Position type if possible).
-portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) }); +if (value > type(uint32).max) revert ErrorsLib.InvalidArguments(); +portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });
♻️ Duplicate comments (1)
contracts/orchestrators/LiquidityOrchestrator.sol (1)
156-163: Slippage cap added — confirm 20% is acceptable.Cap of 2000bps is implemented; prior feedback suggested enforcing an upper limit. If your risk policy prefers 10% max, lower this to 1000bps.
🧹 Nitpick comments (22)
contracts/interfaces/IInternalStateOrchestrator.sol (4)
71-88: Fix units in docs for buyingAmounts (shares vs underlying).Code elsewhere treats
buyingAmountsas shares (and returns separate estimated-underlying arrays). Update the NatSpec to “amounts to buy in shares” to avoid confusion.Apply this doc-only diff:
- /// @return buyingAmounts The amounts to buy in underlying assets + /// @return buyingAmounts The amounts to buy in shares
90-94: Clarify price units/scale returned by getPriceOf.Specify whether this is assets-per-share or shares-per-asset and the fixed‑point scale (e.g., 1e18). Ambiguity here propagates to adapters/orchestrators.
Proposed doc tweak:
- /// @return price The corresponding price [shares/assets] + /// @return price Assets per 1 share, scaled by 1e18 (assets/share, 18 decimals)
95-99: Enforce caller restriction for updateBufferAmount in implementation.NatSpec says “only Liquidity Orchestrator” but the interface can’t enforce it. Ensure the implementation has an explicit access control check (e.g.,
onlyLiquidityOrchestratorusing the address from config).
100-103: Name/semantics of getVaultTotalAssetsForFulfillRedeem.Make clear in docs this returns underlying-asset units (and scale) captured for the current epoch’s fulfillRedeem, not live totals. Helps avoid misuse.
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (1)
654-656: Note on artifacts in VCS.If build artifacts are not required for consumption, consider excluding them to reduce churn. If they are required, no action needed.
test/Orchestrators.test.ts (5)
177-191: Whitelist wiring LGTM.Single price/execution adapters for all assets is fine for tests; consider per‑asset adapters in future integration tests.
468-474: Use all getOrders() returns in assertions.You ignore the two estimated-underlying arrays. Consider asserting length consistency to catch mismatches.
Example:
const [sellingTokens, sellingAmts, buyingTokens, buyingAmts, sEst, bEst] = await internalStatesOrchestrator.getOrders(); expect(sellingAmts.length).to.eq(sellingTokens.length); expect(buyingAmts.length).to.eq(buyingTokens.length); expect(sEst.length).to.eq(sellingTokens.length); expect(bEst.length).to.eq(buyingTokens.length);
472-473: Assumptions on tokens count.Asserting
sellingTokens.length === 0may become brittle with future flows (e.g., redemptions). Consider asserting non-negativity or relative expectations instead.Also applies to: 529-531
533-564: Phase progression assertions could be stronger.Also assert minibatch index changes and that
PortfolioRebalancedis emitted on each step; optionally checkupdateBufferAmountis invoked (via spy/mock or event).
622-634: Edge slippage cases LGTM; add targetBufferRatio check.Given it’s derived from slippageBound, add
expect(await liquidityOrchestrator.targetBufferRatio()).to.equal(110);when bound is 100.contracts/orchestrators/LiquidityOrchestrator.sol (4)
261-275: Harden performUpkeep action dispatch.Add an
else revert InvalidArguments()for unknown actions; consider emittingPortfolioRebalancedonly on epoch completion to reduce noise.Apply:
} else if (action == ACTION_PROCESS_FULFILL_REDEEM) { _processMinibatchFulfillRedeem(); - } + } else { + revert ErrorsLib.InvalidArguments(); + } - emit EventsLib.PortfolioRebalanced(); + emit EventsLib.PortfolioRebalanced();
319-339: Indexing and bounds nits in _processMinibatchSell.
- Use
>=instead of redundant> || ==.- Prefer
uint256for indices to avoid 16‑bit overflow on larger lists.- Increment index deterministically:
currentMinibatchIndex = minibatchIndex + 1;.Apply:
- ++currentMinibatchIndex; - uint16 i0 = minibatchIndex * executionMinibatchSize; - uint16 i1 = i0 + executionMinibatchSize; - if (i1 > sellingTokens.length || i1 == sellingTokens.length) { - i1 = uint16(sellingTokens.length); + currentMinibatchIndex = minibatchIndex + 1; + uint256 i0 = uint256(minibatchIndex) * executionMinibatchSize; + uint256 i1 = i0 + executionMinibatchSize; + if (i1 >= sellingTokens.length) { + i1 = sellingTokens.length; currentPhase = LiquidityUpkeepPhase.BuyingLeg; currentMinibatchIndex = 0; } - for (uint16 i = i0; i < i1; ++i) { + for (uint256 i = i0; i < i1; ++i) { address token = sellingTokens[i]; if (token == address(underlyingAsset)) continue; uint256 amount = sellingAmounts[i]; _executeSell(token, amount, sellingEstimatedUnderlyingAmounts[i]); }
344-368: Same indexing fixes for _processMinibatchBuy; update buffer only once.Mirror the sell fixes and keep
updateBufferAmountwheni1 >= buyingTokens.length.Apply:
- ++currentMinibatchIndex; - uint16 i0 = minibatchIndex * executionMinibatchSize; - uint16 i1 = i0 + executionMinibatchSize; - if (i1 > buyingTokens.length || i1 == buyingTokens.length) { - i1 = uint16(buyingTokens.length); + currentMinibatchIndex = minibatchIndex + 1; + uint256 i0 = uint256(minibatchIndex) * executionMinibatchSize; + uint256 i1 = i0 + executionMinibatchSize; + if (i1 >= buyingTokens.length) { + i1 = buyingTokens.length; currentPhase = LiquidityUpkeepPhase.FulfillRedeem; currentMinibatchIndex = 0; } - for (uint16 i = i0; i < i1; ++i) { + for (uint256 i = i0; i < i1; ++i) { address token = buyingTokens[i]; if (token == address(underlyingAsset)) continue; uint256 amount = buyingAmounts[i]; _executeBuy(token, amount, buyingEstimatedUnderlyingAmounts[i]); } - if (i1 == buyingTokens.length) { + if (i1 >= buyingTokens.length) { internalStatesOrchestrator.updateBufferAmount(deltaBufferAmount); }
378-381: Defensive note: signed deltas from uints.Casting
uint256toint256will revert on very large inputs. If upstream estimates could ever exceedint256.max, clamp or pre‑validate earlier.Also applies to: 389-391, 411-412
contracts/orchestrators/InternalStatesOrchestrator.sol (2)
418-433: Transparent vaults: persist fulfillRedeem PIT totalAssetsYou compute pendingRedeem against totalAssets but don’t store the per‑vault PIT totals for the transparent path. For feature parity with encrypted and to support external consumers via getVaultTotalAssetsForFulfillRedeem, persist it.
uint256 pendingRedeem = vault.convertToAssetsWithPITTotalAssets( vault.pendingRedeem(), totalAssets, Math.Rounding.Floor ); +_currentEpoch.vaultsTotalAssetsForFulfillRedeem[address(vault)] = totalAssets; vault.fulfillRedeem(totalAssets);
899-903: getPriceOf returns zero for unseen tokensIf a token wasn’t touched this epoch, price is zero. Consider computing/caching on-demand or reverting for unset tokens to avoid misleading zeros.
test/OrionVaultExchangeRate.test.ts (6)
34-41: Constructor param naming: automation registry vs signerYou pass internalStatesOrchestratorSigner.address as automationRegistry_. That’s fine for tests, but rename the local variable to automationRegistry to avoid confusion when reading.
80-97: Fixture return key misleads: liquidityOrchestrator is a Signer, not the contractRename liquidityOrchestrator → liquidityOrchestratorSigner to avoid accidental misuse.
- liquidityOrchestrator: liquidityOrchestratorSigner, + liquidityOrchestratorSigner,
128-152: Fragile assertion: shares > depositVirtual-offset implementations vary; asserting shares > deposit can be flaky. Prefer a symmetric closeness check to 1:1 and drop the strict inequality.
- expect(shares).to.be.gt(depositAmount); + expect(shares).to.be.closeTo(depositAmount, ethers.parseUnits("1", 6));
289-333: Ratio check uses Number() on bigints — precision riskCast to JS Number can lose precision. Use bigint cross‑multiplication for proportionality.
- const expectedRatio = Number(shares1Before) / Number(shares2Before); - const actualRatio = Number(increase1) / Number(increase2); - expect(actualRatio).to.be.closeTo(expectedRatio, 0.01); + // Cross-multiply to avoid floating precision: increase1/shares1 ≈ increase2/shares2 + expect(increase1 * shares2Before).to.be.closeTo(increase2 * shares1Before, (increase2 * shares1Before) / 100n);
155-174: Reduce duplication with a small helper for “seed deposit then update”These blocks repeat the same 3 calls. Extract a helper to make tests shorter and less error‑prone.
Example helper:
async function seed(vault, orchestratorAddr, totalBefore, totalAfter) { const imp = await impersonateOrchestrator(orchestratorAddr); await vault.connect(imp).fulfillDeposit(totalBefore); await vault.connect(imp).updateVaultState([], totalAfter); }Also applies to: 188-198, 210-220, 338-347, 357-367, 388-402, 407-412, 419-422, 435-443, 466-468
242-266: Derive virtualOffset from decimals — don’t hard‑code itReplace the hard-coded line in test/OrionVaultExchangeRate.test.ts (around line 276) with a runtime computation, e.g.:
const virtualOffset = 10n ** BigInt((await vault.decimals()) - (await underlyingAsset.decimals()));
Keeps the test resilient to changes in share/underlying decimals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
artifacts/contracts/OrionConfig.sol/OrionConfig.json(1 hunks)artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json(2 hunks)artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json(6 hunks)contracts/interfaces/IInternalStateOrchestrator.sol(1 hunks)contracts/interfaces/ILiquidityOrchestrator.sol(1 hunks)contracts/orchestrators/InternalStatesOrchestrator.sol(12 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(7 hunks)test/Orchestrators.test.ts(9 hunks)test/OrionVaultExchangeRate.test.ts(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/interfaces/ILiquidityOrchestrator.sol
- artifacts/contracts/OrionConfig.sol/OrionConfig.json
⏰ 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 (15)
artifacts/contracts/orchestrators/LiquidityOrchestrator.sol/LiquidityOrchestrator.json (3)
27-31: ABI addition LGTM (AdapterNotSet).
53-57: ABI addition LGTM (InvalidState).
194-212: Public getters for estimation arrays and delta buffer are aligned with on-chain inspection needs.Good visibility for ops/monitoring.
Also applies to: 309-320, 449-467
artifacts/contracts/interfaces/IInternalStateOrchestrator.sol/IInternalStateOrchestrator.json (4)
94-129: ABI change to getOrders(...) matches interface updates.
131-148: ABI add getPriceOf(address) LGTM.
150-167: ABI add getVaultTotalAssetsForFulfillRedeem(address) LGTM.
220-232: ABI add updateBufferAmount(int256) LGTM — verify access control in implementation.test/Orchestrators.test.ts (4)
9-10: Switch to Orion adapters looks correct.Deployments and typed bindings align with the new interfaces.
Also applies to: 29-31, 94-98, 165-172
148-163: Slippage bound validation coverage LGTM.Good negative and positive cases.
337-339: Idle-only guard test LGTM.Asserts revert paths when system not idle.
517-526: Good sanity on positive amounts.Covers minimal order integrity.
contracts/orchestrators/LiquidityOrchestrator.sol (2)
84-95: State additions for estimations + delta buffer LGTM.Public arrays and
deltaBufferAmountare appropriate for introspection.
246-252: FulfillRedeem action wiring in checkUpkeep LGTM.Adds a clean terminal leg.
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
354-356: Reentrancy suppression comment — ensure guard coverageThese blocks disable slither’s reentrancy warning. Confirm all external calls from performUpkeep paths are covered by nonReentrant and cannot reenter via callbacks (e.g., vault hooks). If any path can be called externally outside performUpkeep, add function-level nonReentrant or restructure to CEI.
Also applies to: 756-757
test/OrionVaultExchangeRate.test.ts (1)
6-10: Helper LGTMImpersonation + funding pattern is clean and reusable.
| for (uint16 i = 0; i < allTokensLength; ++i) { | ||
| address token = allTokens[i]; | ||
| tokens[i] = token; | ||
| amounts[i] = _currentEpoch.buyingOrders[token]; | ||
| uint256 sellingAmount = _currentEpoch.sellingOrders[token]; | ||
| uint256 buyingAmount = _currentEpoch.buyingOrders[token]; | ||
|
|
||
| if (sellingAmount > 0) { | ||
| sellingTokens[sellingIndex] = token; | ||
| sellingAmounts[sellingIndex] = sellingAmount; | ||
| sellingEstimatedUnderlyingAmounts[sellingIndex] = sellingAmount.mulDiv( | ||
| _currentEpoch.priceArray[token], | ||
| priceAdapterPrecision | ||
| ); | ||
| ++sellingIndex; | ||
| } | ||
| if (buyingAmount > 0) { | ||
| buyingTokens[buyingIndex] = token; | ||
| buyingAmounts[buyingIndex] = buyingAmount; | ||
| buyingEstimatedUnderlyingAmounts[buyingIndex] = buyingAmount.mulDiv( | ||
| _currentEpoch.priceArray[token], | ||
| priceAdapterPrecision | ||
| ); | ||
| ++buyingIndex; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// @inheritdoc IInternalStateOrchestrator | ||
| function getPriceOf(address token) external view returns (uint256 price) { | ||
| return _currentEpoch.priceArray[token]; | ||
| } | ||
|
|
||
| /// @inheritdoc IInternalStateOrchestrator | ||
| function subtractPendingProtocolFees(uint256 amount) external onlyLiquidityOrchestrator { | ||
| if (amount > pendingProtocolFees) revert ErrorsLib.InsufficientAmount(); | ||
| pendingProtocolFees -= amount; | ||
| } | ||
|
|
||
| /// @inheritdoc IInternalStateOrchestrator | ||
| function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { | ||
| if (deltaAmount > 0) { | ||
| bufferAmount += uint256(deltaAmount); | ||
| } else if (deltaAmount < 0) { | ||
| bufferAmount -= uint256(-deltaAmount); | ||
| } | ||
| } | ||
|
|
||
| /// @notice Get total assets for fulfill redeem for a specific vault | ||
| /// @param vault The vault address | ||
| /// @return totalAssets The total assets for fulfill redeem | ||
| function getVaultTotalAssetsForFulfillRedeem(address vault) external view returns (uint256 totalAssets) { |
There was a problem hiding this comment.
Estimated underlying amounts ignore token vs underlying decimals
sellingEstimatedUnderlyingAmounts/buyingEstimatedUnderlyingAmounts multiply shares by price but don’t convert token decimals → underlying decimals, producing mis‑scaled estimates for non‑underlying tokens.
- sellingEstimatedUnderlyingAmounts[sellingIndex] = sellingAmount.mulDiv(
- _currentEpoch.priceArray[token],
- priceAdapterPrecision
- );
+ {
+ uint256 est = sellingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision);
+ sellingEstimatedUnderlyingAmounts[sellingIndex] = UtilitiesLib.convertDecimals(
+ est,
+ config.getTokenDecimals(token),
+ underlyingDecimals
+ );
+ }
...
- buyingEstimatedUnderlyingAmounts[buyingIndex] = buyingAmount.mulDiv(
- _currentEpoch.priceArray[token],
- priceAdapterPrecision
- );
+ {
+ uint256 est = buyingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision);
+ buyingEstimatedUnderlyingAmounts[buyingIndex] = UtilitiesLib.convertDecimals(
+ est,
+ config.getTokenDecimals(token),
+ underlyingDecimals
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @notice Populates the order arrays with non-zero values | |
| /// @param allTokens Array of all tokens | |
| /// @param sellingTokens Array to populate with selling tokens | |
| /// @param sellingAmounts Array to populate with selling amounts | |
| /// @param sellingEstimatedUnderlyingAmounts Array to populate with selling estimated amounts | |
| /// @param buyingTokens Array to populate with buying tokens | |
| /// @param buyingAmounts Array to populate with buying amounts | |
| /// @param buyingEstimatedUnderlyingAmounts Array to populate with buying estimated amounts | |
| function _populateOrders( | |
| address[] memory allTokens, | |
| address[] memory sellingTokens, | |
| uint256[] memory sellingAmounts, | |
| uint256[] memory sellingEstimatedUnderlyingAmounts, | |
| address[] memory buyingTokens, | |
| uint256[] memory buyingAmounts, | |
| uint256[] memory buyingEstimatedUnderlyingAmounts | |
| ) private view { | |
| uint16 allTokensLength = uint16(allTokens.length); | |
| tokens = new address[](allTokensLength); | |
| amounts = new uint256[](allTokensLength); | |
| uint16 sellingIndex = 0; | |
| uint16 buyingIndex = 0; | |
| for (uint16 i = 0; i < allTokensLength; ++i) { | |
| address token = allTokens[i]; | |
| tokens[i] = token; | |
| amounts[i] = _currentEpoch.buyingOrders[token]; | |
| uint256 sellingAmount = _currentEpoch.sellingOrders[token]; | |
| uint256 buyingAmount = _currentEpoch.buyingOrders[token]; | |
| if (sellingAmount > 0) { | |
| sellingTokens[sellingIndex] = token; | |
| sellingAmounts[sellingIndex] = sellingAmount; | |
| sellingEstimatedUnderlyingAmounts[sellingIndex] = sellingAmount.mulDiv( | |
| _currentEpoch.priceArray[token], | |
| priceAdapterPrecision | |
| ); | |
| ++sellingIndex; | |
| } | |
| if (buyingAmount > 0) { | |
| buyingTokens[buyingIndex] = token; | |
| buyingAmounts[buyingIndex] = buyingAmount; | |
| buyingEstimatedUnderlyingAmounts[buyingIndex] = buyingAmount.mulDiv( | |
| _currentEpoch.priceArray[token], | |
| priceAdapterPrecision | |
| ); | |
| ++buyingIndex; | |
| } | |
| } | |
| } | |
| /// @notice Populates the order arrays with non-zero values | |
| /// @param allTokens Array of all tokens | |
| /// @param sellingTokens Array to populate with selling tokens | |
| /// @param sellingAmounts Array to populate with selling amounts | |
| /// @param sellingEstimatedUnderlyingAmounts Array to populate with selling estimated amounts | |
| /// @param buyingTokens Array to populate with buying tokens | |
| /// @param buyingAmounts Array to populate with buying amounts | |
| /// @param buyingEstimatedUnderlyingAmounts Array to populate with buying estimated amounts | |
| function _populateOrders( | |
| address[] memory allTokens, | |
| address[] memory sellingTokens, | |
| uint256[] memory sellingAmounts, | |
| uint256[] memory sellingEstimatedUnderlyingAmounts, | |
| address[] memory buyingTokens, | |
| uint256[] memory buyingAmounts, | |
| uint256[] memory buyingEstimatedUnderlyingAmounts | |
| ) private view { | |
| uint16 allTokensLength = uint16(allTokens.length); | |
| uint16 sellingIndex = 0; | |
| uint16 buyingIndex = 0; | |
| for (uint16 i = 0; i < allTokensLength; ++i) { | |
| address token = allTokens[i]; | |
| uint256 sellingAmount = _currentEpoch.sellingOrders[token]; | |
| uint256 buyingAmount = _currentEpoch.buyingOrders[token]; | |
| if (sellingAmount > 0) { | |
| sellingTokens[sellingIndex] = token; | |
| sellingAmounts[sellingIndex] = sellingAmount; | |
| { | |
| uint256 est = sellingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision); | |
| sellingEstimatedUnderlyingAmounts[sellingIndex] = UtilitiesLib.convertDecimals( | |
| est, | |
| config.getTokenDecimals(token), | |
| underlyingDecimals | |
| ); | |
| } | |
| +sellingIndex; | |
| } | |
| if (buyingAmount > 0) { | |
| buyingTokens[buyingIndex] = token; | |
| buyingAmounts[buyingIndex] = buyingAmount; | |
| { | |
| uint256 est = buyingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision); | |
| buyingEstimatedUnderlyingAmounts[buyingIndex] = UtilitiesLib.convertDecimals( | |
| est, | |
| config.getTokenDecimals(token), | |
| underlyingDecimals | |
| ); | |
| } | |
| +buyingIndex; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
updateBufferAmount can underflow on negative delta
A negative delta larger than bufferAmount reverts via underflow. Make it explicit and emit a clear error.
function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator {
if (deltaAmount > 0) {
bufferAmount += uint256(deltaAmount);
} else if (deltaAmount < 0) {
- bufferAmount -= uint256(-deltaAmount);
+ uint256 dec = uint256(-deltaAmount);
+ if (dec > bufferAmount) revert ErrorsLib.InsufficientAmount();
+ bufferAmount -= dec;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { | |
| if (deltaAmount > 0) { | |
| bufferAmount += uint256(deltaAmount); | |
| } else if (deltaAmount < 0) { | |
| bufferAmount -= uint256(-deltaAmount); | |
| } | |
| } | |
| function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { | |
| if (deltaAmount > 0) { | |
| bufferAmount += uint256(deltaAmount); | |
| } else if (deltaAmount < 0) { | |
| uint256 dec = uint256(-deltaAmount); | |
| if (dec > bufferAmount) revert ErrorsLib.InsufficientAmount(); | |
| bufferAmount -= dec; | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/orchestrators/InternalStatesOrchestrator.sol around lines 911 to
917, the subtraction when deltaAmount < 0 can underflow if uint256(-deltaAmount)
> bufferAmount; add an explicit guard that checks that the absolute negative
delta is <= bufferAmount and revert with a clear error message (or a custom
error) when it is not, then perform the subtraction with the safe cast; ensure
positive branch still casts and updates bufferAmount as before.
| // slither-disable-next-line unused-return | ||
| IERC20(asset).approve(address(adapter), amount); | ||
|
|
||
| // TODO: pass slippageBound, oracle price and number of shares to adapters. | ||
|
|
||
| // TODO: not performing trade if slippage is too high, record we still have | ||
| // the open position. | ||
| IERC20(asset).approve(address(adapter), sharesAmount); | ||
|
|
||
| // Execute sell through adapter, pull shares from this contract and push underlying assets to it. | ||
| adapter.sell(asset, amount); | ||
| uint256 executionUnderlyingAmount = adapter.sell(asset, sharesAmount, minUnderlyingAmount); | ||
|
|
||
| deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount); | ||
| } | ||
|
|
||
| /// @notice Executes a buy order | ||
| /// @param asset The asset to buy | ||
| /// @param amount The amount of shares to buy | ||
| function _executeBuy(address asset, uint256 amount) internal { | ||
| /// @param sharesAmount The amount of shares to buy | ||
| /// @param estimatedUnderlyingAmount The estimated underlying amount to spend | ||
| function _executeBuy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal { | ||
| IExecutionAdapter adapter = executionAdapterOf[asset]; | ||
| if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet(); | ||
|
|
There was a problem hiding this comment.
Revoke allowances after adapter.sell to avoid leftover spend.
Reset share allowance back to 0 post‑call to minimize exposure window.
Apply:
IERC20(asset).approve(address(adapter), sharesAmount);
// Execute sell through adapter, pull shares from this contract and push underlying assets to it.
uint256 executionUnderlyingAmount = adapter.sell(asset, sharesAmount, minUnderlyingAmount);
+ // Revoke leftover allowance
+ IERC20(asset).approve(address(adapter), 0);
deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @param sharesAmount The amount of shares to sell | |
| /// @param estimatedUnderlyingAmount The estimated underlying amount to receive | |
| function _executeSell(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal { | |
| IExecutionAdapter adapter = executionAdapterOf[asset]; | |
| if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet(); | |
| uint256 minUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 - slippageBound, 10000); | |
| // Approve adapter to spend shares | |
| // slither-disable-next-line unused-return | |
| IERC20(asset).approve(address(adapter), 0); | |
| // slither-disable-next-line unused-return | |
| IERC20(asset).approve(address(adapter), amount); | |
| // TODO: pass slippageBound, oracle price and number of shares to adapters. | |
| // TODO: not performing trade if slippage is too high, record we still have | |
| // the open position. | |
| IERC20(asset).approve(address(adapter), sharesAmount); | |
| // Execute sell through adapter, pull shares from this contract and push underlying assets to it. | |
| adapter.sell(asset, amount); | |
| uint256 executionUnderlyingAmount = adapter.sell(asset, sharesAmount, minUnderlyingAmount); | |
| deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount); | |
| } | |
| /// @param sharesAmount The amount of shares to sell | |
| /// @param estimatedUnderlyingAmount The estimated underlying amount to receive | |
| function _executeSell(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal { | |
| IExecutionAdapter adapter = executionAdapterOf[asset]; | |
| if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet(); | |
| uint256 minUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 - slippageBound, 10000); | |
| // Approve adapter to spend shares | |
| // slither-disable-next-line unused-return | |
| IERC20(asset).approve(address(adapter), 0); | |
| // slither-disable-next-line unused-return | |
| IERC20(asset).approve(address(adapter), sharesAmount); | |
| // Execute sell through adapter, pull shares from this contract and push underlying assets to it. | |
| uint256 executionUnderlyingAmount = adapter.sell(asset, sharesAmount, minUnderlyingAmount); | |
| // Revoke leftover allowance | |
| IERC20(asset).approve(address(adapter), 0); | |
| deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount); | |
| } |
🤖 Prompt for AI Agents
In contracts/orchestrators/LiquidityOrchestrator.sol around lines 372 to 390,
the function _executeSell leaves the adapter allowance set to sharesAmount after
calling adapter.sell; after the sell completes, reset the ERC20 allowance for
the adapter back to 0 to minimize the exposure window. Modify the function to
call IERC20(asset).approve(address(adapter), 0) immediately after adapter.sell
(and before updating deltaBufferAmount) ensuring the approval reset executes
regardless of sell outcome (e.g., keep it in the same flow since adapter.sell is
external) so leftover spend is removed.
| // Approve adapter to spend underlying assets with slippage tolerance | ||
| // slither-disable-next-line unused-return | ||
| IERC20(underlyingAsset).approve(address(adapter), 0); | ||
| // slither-disable-next-line unused-return | ||
| IERC20(underlyingAsset).approve(address(adapter), amount); | ||
| IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount); | ||
|
|
||
| // Execute buy through adapter, pull underlying assets from this contract and push shares to it. | ||
| adapter.buy(asset, amount); | ||
| uint256 executionUnderlyingAmount = adapter.buy(asset, sharesAmount, maxUnderlyingAmount); | ||
|
|
||
| deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount); | ||
| } | ||
|
|
||
| /// @notice Handles the fulfill redeem action | ||
| function _processMinibatchFulfillRedeem() internal { | ||
| if (currentPhase != LiquidityUpkeepPhase.FulfillRedeem) { | ||
| revert ErrorsLib.InvalidState(); | ||
| } | ||
|
|
||
| currentPhase = LiquidityUpkeepPhase.Idle; | ||
|
|
||
| // Process transparent vaults | ||
| address[] memory transparentVaults = config.getAllOrionVaults(EventsLib.VaultType.Transparent); | ||
| for (uint16 i = 0; i < transparentVaults.length; ++i) { |
There was a problem hiding this comment.
Revoke allowances after adapter.buy; guard int256 casts.
- Reset underlying allowance to 0 post‑call.
- Add a safety check before casting large uints to int256.
Apply:
IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount);
// Execute buy through adapter, pull underlying assets from this contract and push shares to it.
uint256 executionUnderlyingAmount = adapter.buy(asset, sharesAmount, maxUnderlyingAmount);
- deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount);
+ // Revoke leftover allowance
+ IERC20(underlyingAsset).approve(address(adapter), 0);
+ // Safe cast guards (defensive; estimated/executed should be well below int256 max)
+ if (estimatedUnderlyingAmount > uint256(type(int256).max) || executionUnderlyingAmount > uint256(type(int256).max)) {
+ revert ErrorsLib.InvalidArguments();
+ }
+ deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _executeBuy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal { | |
| IExecutionAdapter adapter = executionAdapterOf[asset]; | |
| if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet(); | |
| // TODO: analogous API updates as sell. | |
| // TODO: underlying asset/numeraire needs to be part of the whitelisted investment universe, | |
| // as if an order does not pass the underlying equivalent | |
| // is set into the portfolio state for all vaults. | |
| uint256 maxUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 + slippageBound, 10000); | |
| // Approve adapter to spend underlying assets | |
| // Approve adapter to spend underlying assets with slippage tolerance | |
| // slither-disable-next-line unused-return | |
| IERC20(underlyingAsset).approve(address(adapter), 0); | |
| // slither-disable-next-line unused-return | |
| IERC20(underlyingAsset).approve(address(adapter), amount); | |
| IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount); | |
| // Execute buy through adapter, pull underlying assets from this contract and push shares to it. | |
| adapter.buy(asset, amount); | |
| uint256 executionUnderlyingAmount = adapter.buy(asset, sharesAmount, maxUnderlyingAmount); | |
| deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount); | |
| } | |
| function _executeBuy(address asset, uint256 sharesAmount, uint256 estimatedUnderlyingAmount) internal { | |
| IExecutionAdapter adapter = executionAdapterOf[asset]; | |
| if (address(adapter) == address(0)) revert ErrorsLib.AdapterNotSet(); | |
| uint256 maxUnderlyingAmount = estimatedUnderlyingAmount.mulDiv(10000 + slippageBound, 10000); | |
| // Approve adapter to spend underlying assets with slippage tolerance | |
| // slither-disable-next-line unused-return | |
| IERC20(underlyingAsset).approve(address(adapter), 0); | |
| // slither-disable-next-line unused-return | |
| IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount); | |
| // Execute buy through adapter, pull underlying assets from this contract and push shares to it. | |
| uint256 executionUnderlyingAmount = adapter.buy(asset, sharesAmount, maxUnderlyingAmount); | |
| // Revoke leftover allowance | |
| IERC20(underlyingAsset).approve(address(adapter), 0); | |
| // Safe cast guards (defensive; estimated/executed should be well below int256 max) | |
| if (estimatedUnderlyingAmount > uint256(type(int256).max) || executionUnderlyingAmount > uint256(type(int256).max)) { | |
| revert ErrorsLib.InvalidArguments(); | |
| } | |
| deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount); | |
| } |
🤖 Prompt for AI Agents
In contracts/orchestrators/LiquidityOrchestrator.sol around lines 396-413, the
function _executeBuy needs two fixes: after calling adapter.buy, reset the
underlyingAsset allowance back to 0 to avoid leaving approvals open; and before
casting estimatedUnderlyingAmount and executionUnderlyingAmount to int256,
validate they fit within int256 range (i.e. <= uint256(type(int256).max) ) and
revert or handle the overflow case, then perform the safe casts and adjust
deltaBufferAmount. Ensure the post-call approve(address(adapter), 0) uses
IERC20(underlyingAsset).approve and that any failure paths also reset approvals
as necessary.
There was a problem hiding this comment.
FulfillRedeem: set Idle after processing; consider minibatching vaults.
- Don’t expose Idle mid‑processing; move
currentPhase = Idleto the end. - Looping over all vaults can exceed gas; add minibatch support similar to buy/sell.
Apply minimal safety tweak:
- currentPhase = LiquidityUpkeepPhase.Idle;
+ // Move to Idle after fulfill loops completeAnd set at end:
for (uint16 i = 0; i < encryptedVaults.length; ++i) {
...
}
+ currentPhase = LiquidityUpkeepPhase.Idle;Follow‑up (non‑blocking): add minibatch indices for vault fulfillments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @notice Handles the fulfill redeem action | |
| function _processMinibatchFulfillRedeem() internal { | |
| if (currentPhase != LiquidityUpkeepPhase.FulfillRedeem) { | |
| revert ErrorsLib.InvalidState(); | |
| } | |
| currentPhase = LiquidityUpkeepPhase.Idle; | |
| // Process transparent vaults | |
| address[] memory transparentVaults = config.getAllOrionVaults(EventsLib.VaultType.Transparent); | |
| for (uint16 i = 0; i < transparentVaults.length; ++i) { | |
| address vault = transparentVaults[i]; | |
| uint256 totalAssetsForRedeem = internalStatesOrchestrator.getVaultTotalAssetsForFulfillRedeem(vault); | |
| if (totalAssetsForRedeem > 0) { | |
| // Call fulfillRedeem on the vault with the stored totalAssets | |
| IOrionVault(vault).fulfillRedeem(totalAssetsForRedeem); | |
| } | |
| } | |
| // Process encrypted vaults | |
| address[] memory encryptedVaults = config.getAllOrionVaults(EventsLib.VaultType.Encrypted); | |
| for (uint16 i = 0; i < encryptedVaults.length; ++i) { | |
| address vault = encryptedVaults[i]; | |
| uint256 totalAssetsForRedeem = internalStatesOrchestrator.getVaultTotalAssetsForFulfillRedeem(vault); | |
| if (totalAssetsForRedeem > 0) { | |
| // Call fulfillRedeem on the vault with the stored totalAssets | |
| IOrionVault(vault).fulfillRedeem(totalAssetsForRedeem); | |
| } | |
| } | |
| } | |
| /// @notice Handles the fulfill redeem action | |
| function _processMinibatchFulfillRedeem() internal { | |
| if (currentPhase != LiquidityUpkeepPhase.FulfillRedeem) { | |
| revert ErrorsLib.InvalidState(); | |
| } | |
| // Move to Idle after fulfill loops complete | |
| // Process transparent vaults | |
| address[] memory transparentVaults = config.getAllOrionVaults(EventsLib.VaultType.Transparent); | |
| for (uint16 i = 0; i < transparentVaults.length; ++i) { | |
| address vault = transparentVaults[i]; | |
| uint256 totalAssetsForRedeem = internalStatesOrchestrator.getVaultTotalAssetsForFulfillRedeem(vault); | |
| if (totalAssetsForRedeem > 0) { | |
| // Call fulfillRedeem on the vault with the stored totalAssets | |
| IOrionVault(vault).fulfillRedeem(totalAssetsForRedeem); | |
| } | |
| } | |
| // Process encrypted vaults | |
| address[] memory encryptedVaults = config.getAllOrionVaults(EventsLib.VaultType.Encrypted); | |
| for (uint16 i = 0; i < encryptedVaults.length; ++i) { | |
| address vault = encryptedVaults[i]; | |
| uint256 totalAssetsForRedeem = internalStatesOrchestrator.getVaultTotalAssetsForFulfillRedeem(vault); | |
| if (totalAssetsForRedeem > 0) { | |
| // Call fulfillRedeem on the vault with the stored totalAssets | |
| IOrionVault(vault).fulfillRedeem(totalAssetsForRedeem); | |
| } | |
| } | |
| currentPhase = LiquidityUpkeepPhase.Idle; | |
| } |
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 (2)
contracts/orchestrators/InternalStatesOrchestrator.sol (2)
330-343: Fix incorrect clearing of vault mappings keyed by token addresses.You’re deleting
vaultsTotalAssets,encryptedVaultsTotalAssets, andvaultsTotalAssetsForFulfillRedeemusing token keys. These mappings are keyed by vault addresses, so entries from the previous epoch can persist and corrupt fulfillment. Clear by iterating prior epoch vault arrays before reassigning them.for (uint16 i = 0; i < _currentEpoch.tokens.length; ++i) { address token = _currentEpoch.tokens[i]; delete _currentEpoch.priceArray[token]; delete _currentEpoch.initialBatchPortfolio[token]; - delete _currentEpoch.vaultsTotalAssets[token]; - delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[token]; delete _currentEpoch.finalBatchPortfolio[token]; delete _currentEpoch.sellingOrders[token]; delete _currentEpoch.buyingOrders[token]; delete _currentEpoch.tokenExists[token]; _currentEpoch.encryptedInitialBatchPortfolio[token] = _ezero; - _currentEpoch.encryptedVaultsTotalAssets[token] = _ezero; _currentEpoch.encryptedFinalBatchPortfolio[token] = _ezero; } delete _currentEpoch.tokens; delete _decryptedValues; - transparentVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Transparent); - encryptedVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Encrypted); + // Clear vault-keyed mappings using previous-epoch vault lists + for (uint16 i = 0; i < transparentVaultsEpoch.length; ++i) { + address v = transparentVaultsEpoch[i]; + delete _currentEpoch.vaultsTotalAssets[v]; + delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[v]; + _currentEpoch.encryptedVaultsTotalAssets[v] = _ezero; + } + for (uint16 i = 0; i < encryptedVaultsEpoch.length; ++i) { + address v = encryptedVaultsEpoch[i]; + delete _currentEpoch.vaultsTotalAssets[v]; + delete _currentEpoch.vaultsTotalAssetsForFulfillRedeem[v]; + _currentEpoch.encryptedVaultsTotalAssets[v] = _ezero; + } + + transparentVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Transparent); + encryptedVaultsEpoch = config.getAllOrionVaults(EventsLib.VaultType.Encrypted);
364-366: Fix uint8 multiplication overflow in all minibatch windows.Same overflow pattern as in LiquidityOrchestrator: multiply two uint8s after widening.
- uint16 i0 = minibatchIndex * transparentMinibatchSize; - uint16 i1 = i0 + transparentMinibatchSize; + uint16 i0 = uint16(minibatchIndex) * uint16(transparentMinibatchSize); + uint16 i1 = i0 + uint16(transparentMinibatchSize);- uint16 i0 = minibatchIndex * encryptedMinibatchSize; - uint16 i1 = i0 + encryptedMinibatchSize; + uint16 i0 = uint16(minibatchIndex) * uint16(encryptedMinibatchSize); + uint16 i1 = i0 + uint16(encryptedMinibatchSize);Also applies to: 448-450, 685-687, 742-744
♻️ Duplicate comments (6)
contracts/orchestrators/LiquidityOrchestrator.sol (4)
290-315: Reset minibatch index at epoch start.
currentMinibatchIndexisn’t reset in_handleStart(), risking stale indices and skipped work on new epochs. Reset to 0 when initializing the epoch. This mirrors prior feedback.// Clear previous epoch data delete sellingTokens; delete sellingAmounts; delete buyingTokens; delete buyingAmounts; delete sellingEstimatedUnderlyingAmounts; delete buyingEstimatedUnderlyingAmounts; deltaBufferAmount = 0; + currentMinibatchIndex = 0;
158-164: Acknowledgement: upper slippage bound added.Max 20% slippage bound enforces sane limits and prevents underflow in
(10000 - slippageBound). Good safeguard; aligns with earlier suggestion.
381-391: Revoke allowances after adapter calls; add safe int256 cast guards.
- Leave-no-allowance principle: reset approvals to 0 after
sell/buy.- Guard
int256casts of largeuint256amounts to avoid UB on extreme values. This mirrors prior feedback on the buy path; apply to both.IERC20(asset).approve(address(adapter), sharesAmount); // Execute sell through adapter, pull shares from this contract and push underlying assets to it. uint256 executionUnderlyingAmount = adapter.sell(asset, sharesAmount, minUnderlyingAmount); - - deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount); + // Revoke leftover allowance + IERC20(asset).approve(address(adapter), 0); + // Safe cast guards + if ( + estimatedUnderlyingAmount > uint256(type(int256).max) || + executionUnderlyingAmount > uint256(type(int256).max) + ) revert ErrorsLib.InvalidArguments(); + deltaBufferAmount += int256(executionUnderlyingAmount) - int256(estimatedUnderlyingAmount);IERC20(underlyingAsset).approve(address(adapter), maxUnderlyingAmount); // Execute buy through adapter, pull underlying assets from this contract and push shares to it. uint256 executionUnderlyingAmount = adapter.buy(asset, sharesAmount, maxUnderlyingAmount); - - deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount); + // Revoke leftover allowance + IERC20(underlyingAsset).approve(address(adapter), 0); + // Safe cast guards + if ( + estimatedUnderlyingAmount > uint256(type(int256).max) || + executionUnderlyingAmount > uint256(type(int256).max) + ) revert ErrorsLib.InvalidArguments(); + deltaBufferAmount += int256(estimatedUnderlyingAmount) - int256(executionUnderlyingAmount);Also applies to: 403-413
421-446: Move Idle transition to the end of FulfillRedeem.Setting
currentPhase = Idlebefore the loops can expose Idle mid‑processing and race the next upkeep. Move it after both loops finish. Mirrors prior feedback.- currentPhase = LiquidityUpkeepPhase.Idle; + // set to Idle after fulfill loops complete ... - } - } + } + currentPhase = LiquidityUpkeepPhase.Idle; + }contracts/orchestrators/InternalStatesOrchestrator.sol (2)
878-896: Estimated underlying amounts ignore token↔underlying decimals.
price * shares / precisionneeds decimal conversion into underlying units. This was previously flagged.- sellingEstimatedUnderlyingAmounts[sellingIndex] = sellingAmount.mulDiv( - _currentEpoch.priceArray[token], - priceAdapterPrecision - ); + { + uint256 unscaled = sellingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision); + sellingEstimatedUnderlyingAmounts[sellingIndex] = UtilitiesLib.convertDecimals( + unscaled, + config.getTokenDecimals(token), + underlyingDecimals + ); + } ... - buyingEstimatedUnderlyingAmounts[buyingIndex] = buyingAmount.mulDiv( - _currentEpoch.priceArray[token], - priceAdapterPrecision - ); + { + uint256 unscaled = buyingAmount.mulDiv(_currentEpoch.priceArray[token], priceAdapterPrecision); + buyingEstimatedUnderlyingAmounts[buyingIndex] = UtilitiesLib.convertDecimals( + unscaled, + config.getTokenDecimals(token), + underlyingDecimals + ); + }
911-917: Guard buffer subtraction on negative deltas.
bufferAmount -= uint256(-deltaAmount)underflows when the negative delta exceeds the buffer. Make it explicit, as previously suggested.function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { if (deltaAmount > 0) { bufferAmount += uint256(deltaAmount); } else if (deltaAmount < 0) { - bufferAmount -= uint256(-deltaAmount); + uint256 dec = uint256(-deltaAmount); + if (dec > bufferAmount) revert ErrorsLib.InsufficientAmount(); + bufferAmount -= dec; } }
🧹 Nitpick comments (6)
contracts/orchestrators/LiquidityOrchestrator.sol (2)
328-333: Simplify boundary check to use >=.Use
>= lengthinstead ofi1 > length || i1 == length. Clearer and avoids redundant branch checks.- if (i1 > sellingTokens.length || i1 == sellingTokens.length) { + if (i1 >= sellingTokens.length) { ... - if (i1 > buyingTokens.length || i1 == buyingTokens.length) { + if (i1 >= buyingTokens.length) {Also applies to: 353-357
172-174: Prefer SafeERC20 for transfers/approvals.Many ERC20s are non‑standard on return values. Use
SafeERC20’ssafeTransfer/forceApproveto avoid silent failures and weird tokens.Would you like a follow-up PR converting these call sites to SafeERC20?
Also applies to: 205-207, 219-220, 229-231, 381-386, 403-408
contracts/orchestrators/InternalStatesOrchestrator.sol (4)
354-356: Scope of slither reentrancy suppression.Reentrancy is disabled for a broad region. Consider narrowing the suppression to only the exact external-call sites to keep static analysis useful.
Also applies to: 756-757
364-371: Also simplify boundary check to use >=.Minor readability tweak mirroring LO.
- if (i1 > transparentVaultsEpoch.length || i1 == transparentVaultsEpoch.length) { + if (i1 >= transparentVaultsEpoch.length) {
818-821: Nit: allocate arrays once; early return if both counts are zero.Skip
_populateOrderswhen both counts are zero.
879-896: Add decimal conversion tests.Once patched, please add tests covering tokens with non‑18 decimals to avoid regressions.
I can draft unit tests hitting 6/8/18‑dec tokens for both buy/sell paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/orchestrators/InternalStatesOrchestrator.sol(12 hunks)contracts/orchestrators/LiquidityOrchestrator.sol(7 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 (6)
contracts/orchestrators/InternalStatesOrchestrator.sol (6)
798-833: getOrders: good API shape; verify zero-order elision.The split arrays and non-zero filtering look right. Please verify callers assume dense arrays without gaps.
653-675: Buffering math LGTM; dependency on targetBufferRatio basis-points.
targetBufferAmount = protocolTotalAssets * targetBufferRatio / 1e4assumes bps. Matches LiquidityOrchestrator’s computation; keep that invariant in docs/tests.
450-453: Consistent phase transitions; ensurecurrentMinibatchIndexresets.You reset
currentMinibatchIndex = 0on phase changes—good. Keep this consistent if further phases are added.Also applies to: 744-753
571-605: Encrypted path: good separation of fulfillRedeem values vs deposits.Setting
vaultsTotalAssetsForFulfillRedeemfor encrypted vaults before deposits aligns with deferred fulfillment by LO.Also applies to: 591-605
819-833: Counting and populating orders separated: good cohesion.Private helpers are clean and testable. Keep it.
364-366: Repo‑wide verification: search forminibatchIndex * <X>MinibatchSize(uint multiply pattern)rg search earlier failed due to a file‑type filter; cannot confirm absence of other matches. Run these from the repo root and paste results:
rg (preferred):
rg -nP -C2 '\bminibatchIndex\s**\s*(?:[A-Za-z_]\wMinibatchSize|\d+)\b' -g '.sol' --hidden -S || truefallback (find + grep):
find . -type f -name '.sol' -print0 | xargs -0 -r grep -n -E -C2 "minibatchIndex[[:space:]]*[[:space:]]*([A-Za-z_][A-Za-z0-9_]*MinibatchSize|[0-9]+)" || truequick scan for size symbols:
rg -n '\bMinibatchSize\b' -g '*.sol' --hidden -S || trueReference: contracts/orchestrators/InternalStatesOrchestrator.sol:364-366
| @@ -742,6 +753,8 @@ contract InternalStatesOrchestrator is SepoliaConfig, Ownable, ReentrancyGuard, | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🧩 Analysis chain
Potential truncation: casting portfolio value to uint32.
value can exceed 32 bits post conversions; truncation corrupts state. Either bound-check before cast or widen Position.value if possible.
- IOrionTransparentVault.Position[] memory portfolio = new IOrionTransparentVault.Position[](
+ IOrionTransparentVault.Position[] memory portfolio = new IOrionTransparentVault.Position[](
intentTokens.length
);
...
- portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });
+ if (value > type(uint32).max) revert ErrorsLib.InvalidArguments();
+ portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });If the interface allows, prefer changing value to uint256 in Position.
Also applies to: 724-729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "pwd: $(pwd)"
git rev-parse --abbrev-ref HEAD 2>/dev/null || true
echo
echo "Searching for InternalStatesOrchestrator.sol..."
rg -n --hidden "InternalStatesOrchestrator.sol" || true
FILE="contracts/orchestrators/InternalStatesOrchestrator.sol"
if [ -f "$FILE" ]; then
echo
echo "== Showing $FILE (lines 1-240) =="
sed -n '1,240p' "$FILE"
echo
echo "== Showing $FILE (lines 660-760) =="
sed -n '660,760p' "$FILE"
else
echo
echo "$FILE not found at that path; searching for symbol 'InternalStatesOrchestrator'..."
rg -n --hidden "InternalStatesOrchestrator" || true
fi
echo
echo "== Searching for IOrionTransparentVault occurrences =="
rg -n --hidden "IOrionTransparentVault" || true
echo
echo "== Searching for 'interface IOrionTransparentVault' files =="
rg -n --hidden "interface IOrionTransparentVault" || true
if files=$(rg -n --hidden "interface IOrionTransparentVault" -l || true); then
for f in $files; do
echo
echo "== Found interface file: $f (showing up to 300 lines) =="
sed -n '1,300p' "$f" || true
done
fi
echo
echo "== Searching for 'struct Position' definitions =="
rg -n --hidden "struct Position" || true
echo
echo "== Searching for Position[] usages, assignments and uint32 casts =="
rg -n --hidden "IOrionTransparentVault.Position\[" -S || true
rg -n --hidden "Position\[\]" -S || true
rg -n --hidden "Position\s*\{" -S || true
rg -n --hidden "uint32(value)" -S || true
rg -n --hidden "value\s*:\s*uint32" -S || true
if [ -f "$FILE" ]; then
echo
echo "== Showing $FILE (lines 712-736) =="
sed -n '712,736p' "$FILE" || true
fi
echo
echo "Done."Length of output: 24459
Fix uint32 cast that can truncate portfolio value (contracts/orchestrators/InternalStatesOrchestrator.sol ≈701–736)
IOrionTransparentVault.Position.value is uint32 (interface) but value is computed as a uint256 and cast with no bounds check — risk of truncation/corrupted state.
- Remediation: either bounds-check before casting or change Position.value to uint256 in the interface and update implementations (breaking change).
- Suggested quick patch:
- portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });
+ if (value > type(uint32).max) revert ErrorsLib.InvalidArguments();
+ portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });Also applies to the portfolio assignment in the same function (around line ~724).
Committable suggestion skipped: line range outside the PR's diff.
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 (1)
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
652-674: Buffer rounding creates assets from dust.You add
deltaBufferAmounttobufferAmount, but per‑vault proportional deductions floor, so the sum of deductions ≤deltaBufferAmount. The difference mints buffer out of thin air.Minimal fix: add what you actually deducted.
- uint256 deltaBufferAmount = targetBufferAmount - bufferAmount; + uint256 deltaBufferAmount = targetBufferAmount - bufferAmount; + uint256 totalDeducted = 0; for (uint16 i = 0; i < nTransparentVaults; ++i) { address vault = transparentVaultsEpoch[i]; uint256 vaultAssets = _currentEpoch.vaultsTotalAssets[address(vault)]; uint256 vaultBufferCost = deltaBufferAmount.mulDiv(vaultAssets, protocolTotalAssets); _currentEpoch.vaultsTotalAssets[address(vault)] -= vaultBufferCost; + totalDeducted += vaultBufferCost; } for (uint16 i = 0; i < nEncryptedVaults; ++i) { address vault = encryptedVaultsEpoch[i]; uint256 vaultAssets = _currentEpoch.vaultsTotalAssets[address(vault)]; uint256 vaultBufferCost = deltaBufferAmount.mulDiv(vaultAssets, protocolTotalAssets); _currentEpoch.vaultsTotalAssets[address(vault)] -= vaultBufferCost; + totalDeducted += vaultBufferCost; } - bufferAmount += deltaBufferAmount; + bufferAmount += totalDeducted;Optional refinement: push the remainder to the last vault to hit the exact target.
♻️ Duplicate comments (3)
contracts/orchestrators/InternalStatesOrchestrator.sol (3)
717-724: Potential truncation: casting portfolio value to uint32 (re‑raise).
valueisuint256. Silent down‑cast touint32can corrupt state.- portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) }); + if (value > type(uint32).max) revert ErrorsLib.InvalidArguments(); + portfolio[j] = IOrionTransparentVault.Position({ token: token, value: uint32(value) });If the interface allows, prefer widening
Position.valuetouint256across the stack in a follow‑up PR.
877-885: Estimated underlying amounts ignore token vs underlying decimals (re‑raise).Selling/Buying estimated underlying amounts are computed without converting token decimals → underlying decimals, leading to mis‑scaled values for non‑underlying tokens. This can break slippage bounds and buffer accounting.
Apply this diff inside
_populateOrders:- sellingEstimatedUnderlyingAmounts[sellingIndex] = sellingAmount.mulDiv( - _currentEpoch.priceArray[token], - priceAdapterPrecision - ); + { + uint256 est = sellingAmount.mulDiv( + _currentEpoch.priceArray[token], + priceAdapterPrecision + ); + sellingEstimatedUnderlyingAmounts[sellingIndex] = UtilitiesLib.convertDecimals( + est, + config.getTokenDecimals(token), + underlyingDecimals + ); + } ... - buyingEstimatedUnderlyingAmounts[buyingIndex] = buyingAmount.mulDiv( - _currentEpoch.priceArray[token], - priceAdapterPrecision - ); + { + uint256 est = buyingAmount.mulDiv( + _currentEpoch.priceArray[token], + priceAdapterPrecision + ); + buyingEstimatedUnderlyingAmounts[buyingIndex] = UtilitiesLib.convertDecimals( + est, + config.getTokenDecimals(token), + underlyingDecimals + ); + }Also applies to: 886-894
910-916: updateBufferAmount can underflow on negative delta (re‑raise).Subtracting
uint256(-deltaAmount)without a bound check can revert. Guard explicitly.function updateBufferAmount(int256 deltaAmount) external onlyLiquidityOrchestrator { if (deltaAmount > 0) { bufferAmount += uint256(deltaAmount); } else if (deltaAmount < 0) { - bufferAmount -= uint256(-deltaAmount); + uint256 dec = uint256(-deltaAmount); + if (dec > bufferAmount) revert ErrorsLib.InsufficientAmount(); + bufferAmount -= dec; } }
🧹 Nitpick comments (6)
contracts/orchestrators/InternalStatesOrchestrator.sol (6)
393-401: Add zero‑price guard before usingprice.Division by
priceelsewhere and economic correctness both requireprice > 0.uint256 price = _currentEpoch.priceArray[token]; + if (price == 0) revert ErrorsLib.InvalidArguments();
715-721: Add zero‑price guard in post‑processing too.Protects allocation math and avoids division by zero.
uint256 price = _currentEpoch.priceArray[token]; + if (price == 0) revert ErrorsLib.InvalidArguments();
872-895: Conservative rounding for min/max constraints.For estimated underlying amounts: sells should bias down (floor), buys bias up (ceil) to be protective under slippage.
UtilitiesLib.convertDecimalslikely floors; consider ceiling for buys or adding +1 when decimals shrink.Would you like me to propose a small helper to apply ceiling on decimal conversion for buy estimates?
366-371: Minor: simplify boundary checks.
if (i1 > len || i1 == len)can beif (i1 >= len)in four places.Also applies to: 449-451, 687-692, 743-752
119-125: Counters and loop index widths may be too tight.
epochCounterisuint16; long‑running deployments can overflow.- Several loops use
uint16. If tokens/vaults ever exceed 65,535, indices break.Consider
uint32for counters/indices.Also applies to: 788-791
898-902: Optional: price fallback.
getPriceOfreturns 0 for unseen tokens in the current epoch. Consider falling back to the registry for convenience, or document the 0 behavior explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/orchestrators/InternalStatesOrchestrator.sol(12 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: Build, Lint and Test
- GitHub Check: Sourcery review
🔇 Additional comments (1)
contracts/orchestrators/InternalStatesOrchestrator.sol (1)
507-534: Confirm FHE handling for invalid intents.You request decryption for all
nVaults, but earlier you skip invalid intents. For those entries,encryptedVaultsTotalAssets[vault]may be default‑zero; ensureFHE.allowThis/FHE.toBytes32on such values is supported by the FHE lib and won’t revert or taint proofs.If unsupported, build
cipherTextsonly for valid intents and track indices, or initialize mapping entries to_ezerowhen skipping.
1 participant
Summary by Sourcery
Implement full liquidity execution flow with slippage handling, buffer management, and enriched order estimation; refactor orchestrators, adapters, and vaults to support iterative minibatch processing and accurate state updates; update interfaces and tests accordingly.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Breaking Changes
Tests
Chores