B36 - #206
Closed
matteoettam09 wants to merge 9 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR introduces system idle state gating for critical operations, implements conditional asset decommissioning with failed token retry logic, adds deferred redemption claiming for transfer-blocked redeemers, and removes stale round validation from price feed checks. Changes
Sequence DiagramssequenceDiagram
participant User
participant Vault as OrionVault
participant LO as LiquidityOrchestrator
User->>Vault: fulfillRedeem (batch)
Vault->>LO: transferRedemptionFunds (per user)
alt Transfer Succeeds
LO-->>Vault: Success
Vault->>Vault: Emit Redeem
else Transfer Fails (denylist, etc.)
LO-->>Vault: Revert
Vault->>Vault: pendingUnderlyingClaims[user] += amount
Vault->>Vault: Emit RedemptionTransferFailed
end
Note over Vault: Later: transfer blocker resolved
User->>Vault: claimUnderlying()
Vault->>Vault: amount = pendingUnderlyingClaims[caller]
Vault->>Vault: pendingUnderlyingClaims[caller] = 0
Vault->>LO: transferRedemptionFunds (retry)
LO-->>Vault: Success
Vault->>Vault: Emit RedemptionClaimed
sequenceDiagram
participant Upkeep
participant LO as LiquidityOrchestrator
participant Config as OrionConfig
Upkeep->>LO: Epoch finalization
LO->>Config: completeAssetsRemoval(failedTokens)
Note over Config: For each decommissioning asset:
loop Asset Processing
alt Asset in failedTokens
Config->>Config: Retain in _decommissioningAssets
else Asset sold successfully
Config->>Config: Remove from whitelistedAssets
Config->>Config: Emit AssetRemoved
Config->>Config: Swap-and-pop from _decommissioningAssets
end
end
Config-->>LO: Return
Note over LO: Later: Decommission finalization
LO->>Config: Check queue drained & portfolio liquidated
alt Conditions met
LO->>Config: completeVaultDecommissioning
else Blocked
LO->>LO: Revert (retry next epoch)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/vaults/OrionVault.sol`:
- Around line 746-753: The current claimUnderlying function zeroes
pendingUnderlyingClaims[msg.sender] before calling
liquidityOrchestrator.transferRedemptionFunds, which loses the user's claim if
the external call reverts; to fix, either (A) move the line that sets
pendingUnderlyingClaims[msg.sender] = 0 to after the successful transfer so the
mapping is only cleared on success, or (B) keep the CEI order but wrap
liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount) in a try/catch
and in the catch restore pendingUnderlyingClaims[msg.sender] = amount and
re-revert or emit an error; update emit RedemptionClaimed only after successful
transfer and keep nonReentrant modifier unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b89b63f7-1bda-4d1a-831b-08949b518218
📒 Files selected for processing (9)
contracts/LiquidityOrchestrator.solcontracts/OrionConfig.solcontracts/execution/UniswapV3ExecutionAdapter.solcontracts/interfaces/IOrionConfig.solcontracts/interfaces/IOrionVault.solcontracts/price/ChainlinkPriceAdapter.solcontracts/test/MockOrionConfig.solcontracts/vaults/OrionTransparentVault.solcontracts/vaults/OrionVault.sol
Comment on lines
+746
to
+753
| /// @inheritdoc IOrionVault | ||
| function claimUnderlying() external nonReentrant { | ||
| uint256 amount = pendingUnderlyingClaims[msg.sender]; | ||
| if (amount == 0) revert ErrorsLib.InsufficientAmount(); | ||
| pendingUnderlyingClaims[msg.sender] = 0; | ||
| liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount); | ||
| emit RedemptionClaimed(msg.sender, amount); | ||
| } |
There was a problem hiding this comment.
User loses funds permanently if claimUnderlying fails again.
The claim amount is zeroed at line 750 before the external call at line 751. If transferRedemptionFunds reverts again (e.g., user is still on the denylist), the user permanently loses their underlying tokens since the mapping was already cleared.
Consider reverting on transfer failure to preserve the user's claim:
🐛 Proposed fix to preserve claim on failure
function claimUnderlying() external nonReentrant {
uint256 amount = pendingUnderlyingClaims[msg.sender];
if (amount == 0) revert ErrorsLib.InsufficientAmount();
- pendingUnderlyingClaims[msg.sender] = 0;
- liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount);
- emit RedemptionClaimed(msg.sender, amount);
+ // Call first, only clear on success to preserve claim if transfer fails again
+ liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount);
+ pendingUnderlyingClaims[msg.sender] = 0;
+ emit RedemptionClaimed(msg.sender, amount);
}Note: This changes from CEI pattern but is safe here because nonReentrant prevents reentrancy, and the external call is to a trusted contract (liquidityOrchestrator). If you prefer maintaining CEI, wrap the external call in a try/catch that re-stores the amount on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/vaults/OrionVault.sol` around lines 746 - 753, The current
claimUnderlying function zeroes pendingUnderlyingClaims[msg.sender] before
calling liquidityOrchestrator.transferRedemptionFunds, which loses the user's
claim if the external call reverts; to fix, either (A) move the line that sets
pendingUnderlyingClaims[msg.sender] = 0 to after the successful transfer so the
mapping is only cleared on success, or (B) keep the CEI order but wrap
liquidityOrchestrator.transferRedemptionFunds(msg.sender, amount) in a try/catch
and in the catch restore pendingUnderlyingClaims[msg.sender] = amount and
re-revert or emit an error; update emit RedemptionClaimed only after successful
transfer and keep nonReentrant modifier unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Security