Convertible Deposits: Limit Orders (Fork) - #188
Conversation
Release: Convertible Deposits
WalkthroughAdds a new CDAuctioneerLimitOrders contract implementing per-order limit orders, incentive budgets, and sUSDS-based yield accrual/sweep, plus a comprehensive test suite with mocks exercising creation, fills, cancellations, yield, admin flows, and view utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Filler
participant LimitOrders as CDAuctioneerLimitOrders
participant USDS
participant sUSDS
participant Auctioneer as CD_AUCTIONEER
participant ReceiptToken
participant PositionNFT
User->>LimitOrders: createOrder(period, depositBudget, incentiveBudget, maxPrice, minFill)
LimitOrders->>USDS: transferFrom(user, deposit+incentive)
USDS-->>LimitOrders: USDS received
LimitOrders->>sUSDS: deposit USDS (mint sUSDS)
sUSDS-->>LimitOrders: sUSDS balance updated
LimitOrders->>LimitOrders: store order, update totalUsdsOwed
Filler->>LimitOrders: fillOrder(orderId, fillAmount)
LimitOrders->>Auctioneer: previewBid(period, fillAmount)
Auctioneer-->>LimitOrders: price, ohmOut
LimitOrders->>LimitOrders: validate price, minFill, budgets
LimitOrders->>Auctioneer: bid(period, fillAmount, incentive, ...)
Auctioneer-->>LimitOrders: positionId, receiptToken, ohmOut
LimitOrders->>ReceiptToken: transfer receipt token to order owner
LimitOrders->>PositionNFT: transfer NFT to filler (or owner)
LimitOrders->>LimitOrders: update depositSpent, incentiveSpent, totalUsdsOwed
Admin->>LimitOrders: sweepYield()
LimitOrders->>sUSDS: calculate accrued shares & transfer to yieldRecipient
sUSDS-->>LimitOrders: shares transferred (yield swept)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-08-08T07:33:13.575ZApplied to files:
📚 Learning: 2025-08-08T11:14:54.317ZApplied to files:
📚 Learning: 2025-08-04T11:03:24.007ZApplied to files:
📚 Learning: 2025-11-07T11:09:53.808ZApplied to files:
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol (2)
557-571: Consider adding a more precise yield assertion.The test correctly validates that yield is greater than zero after fills, but a more precise assertion (e.g., approximate expected yield based on remaining balance and exchange rate) would strengthen confidence in the yield calculation logic.
689-701: Test coverage gap: consider testing PERIOD_6 fillability.
getFillableOrderstest creates a PERIOD_6 order but only queries PERIOD_3. Consider adding a test that verifies PERIOD_6 orders are correctly returned when querying that period.+ function test_getFillableOrders_differentPeriod() public { + vm.prank(alice); + limitOrders.createOrder(PERIOD_6, 10_000e18, 50e18, 35e18, 1_000e18); + + uint256[] memory fillable = limitOrders.getFillableOrders(PERIOD_6); + + assertEq(fillable.length, 1); + assertEq(fillable[0], 0); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/policies/deposits/LimitOrders.sol(1 hunks)src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/policies/deposits/LimitOrders.sol
🔇 Additional comments (19)
src/policies/deposits/LimitOrders.sol (11)
1-57: LGTM!The imports, interface definition, and error declarations are well-structured. Custom errors provide gas-efficient and descriptive revert reasons.
59-88: LGTM!Events are appropriately indexed for efficient filtering, and the
OHM_SCALEconstant correctly reflects OHM's 9-decimal precision.
90-127: LGTM!State variables and the
LimitOrderstruct are well-organized. ThetotalUsdsOwedtracking is a sound approach for isolating principal from yield.
130-159: LGTM!Constructor validation is thorough with zero-address checks for all critical addresses and array length validation. The max approval for SUSDS deposits is a standard pattern.
180-232: LGTM!Comprehensive input validation including deposit period enablement check, minimum bid validation against auctioneer, and proper use of
safeTransferFrom. The order creation flow is secure with reentrancy protection.
296-300: Receipt token transfer relies on full balance assumption.Transferring
balanceOf(address(this))works under the assumption that the contract never holds residual receipt tokens. This is acceptable givennonReentrant, but consider tracking the expected receipt amount from thebid()call for more robust accounting if the auctioneer's behavior changes.
319-337: LGTM!Cancel order logic is correct with proper ownership validation and direct withdrawal to the order owner.
362-373:sweepYieldis callable by anyone.This is likely intentional to allow permissionless yield distribution to
yieldRecipient. Just confirming this is the desired behavior and not an access control oversight.
450-469: LGTM with appropriate warning.The gas-intensive warning for
getFillableOrdersis appropriate. The O(2n) iteration pattern is acceptable for off-chain queries.
508-515: LGTM!Standard
onERC721Receivedimplementation required for the contract to receive position NFTs from the auctioneer.
282-290: Consider usingsafeIncreaseAllowanceand validatingactualAmount.
- For consistency with SafeERC20 usage elsewhere, consider
USDS.safeIncreaseAllowance()instead of directapprove().- The
actualAmountreturn value frombid()is ignored. Verify that the auctioneer always returns exactlyfillAmount_asactualAmount; if it can differ, thedepositSpentaccounting would be incorrect.src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol (8)
15-25: LGTM!Simple and correct ERC20 mock for USDS.
27-47: LGTM!Well-designed ERC4626 mock with configurable exchange rate for yield simulation testing.
49-67: LGTM!Simple and correct mocks for receipt tokens and position NFTs.
69-144: LGTM!MockCDAuctioneer correctly implements the interface with configurable parameters for various test scenarios. The previewBid returning 0 below minimum bid correctly exercises the
ZeroOhmOuterror path.
148-210: LGTM!Test setup is comprehensive with all necessary mock deployments, user funding, and approvals.
212-297: LGTM!Comprehensive test coverage for order creation including validation of all order fields and proper error revert cases.
299-452: LGTM!Excellent test coverage for fill order functionality including partial fills, final fill incentive handling, cap logic, and all error conditions. The test for
test_fillOrder_zeroOhmOutcorrectly validates the scenario where minimum bid is raised after order creation.
1-779: Comprehensive test suite.The test file provides excellent coverage of the CDAuctioneerLimitOrders contract functionality including order lifecycle, yield management, admin functions, and edge cases. The mock implementations are well-designed for testing various scenarios.
Saves a bit of gas
Saves a bit of gas
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/policies/deposits/LimitOrders.sol (2)
292-292: Major: Use safeTransferFrom for ERC721 position NFT.Line 292 uses
transferFrom()which doesn't verify thatorder.ownercan receive ERC721 tokens. If the owner is a contract withoutonERC721Received(), the position NFT becomes permanently stuck in that contract.Apply this diff:
- POSITION_NFT.transferFrom(address(this), order.owner, positionId); + POSITION_NFT.safeTransferFrom(address(this), order.owner, positionId);Additionally, consider validating at order creation (line 185) that
msg.sendercan receive ERC721 tokens to fail fast, though this requires reproducing ERC721's_checkOnERC721Received()logic.
201-204: Critical: ERC4626 rounding mismatch in accounting.Lines 203-204 create an accounting discrepancy:
deposit()returns shares minted, but line 204 adds the inputtotalDeposit(asset amount) tototalUsdsOwed. Due to ERC4626 rounding, the shares may not redeem to exactlytotalDeposit. This mismatch compounds across orders and can causetotalUsdsOwedto drift from actual redeemable assets, breaking yield calculations ingetAccruedYieldShares()(line 356) and potentially preventing withdrawals.Apply this diff to fix the accounting:
- USDS.safeTransferFrom(msg.sender, address(this), totalDeposit); - SUSDS.deposit(totalDeposit, address(this)); - totalUsdsOwed += totalDeposit; + USDS.safeTransferFrom(msg.sender, address(this), totalDeposit); + uint256 sharesMinted = SUSDS.deposit(totalDeposit, address(this)); + uint256 redeemableAmount = SUSDS.previewRedeem(sharesMinted); + totalUsdsOwed += redeemableAmount;Note: You'll also need to handle the small difference (
totalDeposit - redeemableAmount) when splitting betweendepositBudgetandincentiveBudgetin the order struct, or document that users effectively deposit slightly less due to rounding.
🧹 Nitpick comments (3)
src/policies/deposits/LimitOrders.sol (3)
329-330: Consider replacing ternaries with checked subtraction.Lines 329-330 use ternaries to prevent underflow, but if
depositSpent > depositBudgetorincentiveSpent > incentiveBudget(which violates contract invariants), this silently returns 0 instead of reverting. This masks potential accounting bugs infillOrder().Apply this diff to enforce invariants:
- uint256 remainingDeposit = order.depositBudget > order.depositSpent ? order.depositBudget - order.depositSpent : 0; - uint256 remainingIncentive = order.incentiveBudget > order.incentiveSpent ? order.incentiveBudget - order.incentiveSpent : 0; + uint256 remainingDeposit = order.depositBudget - order.depositSpent; + uint256 remainingIncentive = order.incentiveBudget - order.incentiveSpent;Alternatively, add explicit checks:
+ if (order.depositSpent > order.depositBudget || order.incentiveSpent > order.incentiveBudget) { + revert InvalidParam("accounting invariant violated"); + } uint256 remainingDeposit = order.depositBudget - order.depositSpent; uint256 remainingIncentive = order.incentiveBudget - order.incentiveSpent;
502-509: Consider restricting ERC721 acceptance to CD_AUCTIONEER.The current implementation accepts ERC721 tokens from any source. While not a security risk (the contract immediately forwards position NFTs to order owners in
fillOrder()), restricting acceptance toCD_AUCTIONEERprovides defense in depth against unexpected NFT transfers.Optional enhancement:
function onERC721Received( - address, + address operator, address, uint256, bytes calldata ) external pure returns (bytes4) { + if (operator != address(CD_AUCTIONEER)) revert InvalidParam("operator"); return this.onERC721Received.selector; }
179-185: Add emergency pause mechanism for order creation.The
createOrder()function lacks an emergency stop capability. Consider adding a pausable pattern (similar to OpenZeppelin's Pausable) with anonlyOwnermodifier to allow the contract owner to disable new order creation during security incidents or maintenance, without affecting existing active orders.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/policies/deposits/LimitOrders.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-04T11:03:24.007Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 84
File: src/test/policies/YieldDepositFacility/execute.t.sol:41-43
Timestamp: 2025-08-04T11:03:24.007Z
Learning: In YieldDepositFacility, 0xJem intentionally allows snapshots to be overwritten/updated within the same block to ensure they reflect the latest vault conversion rates. This is not a gaming vulnerability but desired behavior - the gaming fix was removing timestamp hints, not preventing snapshot updates within blocks.
Applied to files:
src/policies/deposits/LimitOrders.sol
🔇 Additional comments (4)
src/policies/deposits/LimitOrders.sol (4)
129-158: LGTM: Constructor validation is comprehensive.The constructor properly validates all address parameters, ensures array length consistency, and initializes the USDS→sUSDS approval. The max approval on line 157 is appropriate for ERC4626 vault deposits.
343-374: LGTM: Yield accounting is sound.The yield calculation (lines 352-361) correctly uses
previewWithdraw()with upward rounding (line 356) to ensure principal protection. This guaranteestotalUsdsOwedcan always be withdrawn even if yield is swept. The early return on line 367 for zero yield is efficient.
376-498: LGTM: View functions are comprehensive.The view functions provide thorough order inspection capabilities for MEV bots and users:
previewFillOrder(),canFillOrder(), and_isOrderFillable()validate fill conditions accuratelygetFillableOrders()with both overloads allows chunked scanning to avoid gas limits (lines 443-454)- The gas warning comments (lines 444, 450) appropriately flag off-chain-only usage
4-9: No action needed. The OpenZeppelin version usage is intentional and correct:ReentrancyGuardTransientonly exists in v5.3.0 (explicitly imported via@openzeppelin-5.3.0/), while the other imports from lines 5-9 use the default mapping to v4.8.0 (both versions are available through remappings.txt). This aligns with the project's dual-version strategy where v4.8.0 is standard and v5.3.0 is available through explicit opt-in paths.Likely an incorrect or invalid review comment.
| uint256 usdsNeeded = fillAmount_ + incentive; | ||
| SUSDS.withdraw(usdsNeeded, address(this), address(this)); | ||
|
|
||
| // Update accounting | ||
| order.depositSpent += fillAmount_; | ||
| order.incentiveSpent += incentive; | ||
| totalUsdsOwed -= usdsNeeded; |
There was a problem hiding this comment.
Capture withdraw() return value for precise accounting.
Line 273 withdraws usdsNeeded but doesn't capture the actual assets returned. ERC4626 withdraw() can return slightly different amounts due to rounding. While the re-wrap logic (lines 305-308) mitigates excess USDS, totalUsdsOwed (line 278) is decremented by the requested amount, not the actual, which could cause minor accounting drift over many fills.
Apply this diff for precision:
- SUSDS.withdraw(usdsNeeded, address(this), address(this));
+ uint256 actualWithdrawn = SUSDS.withdraw(usdsNeeded, address(this), address(this));
// Update accounting
order.depositSpent += fillAmount_;
order.incentiveSpent += incentive;
- totalUsdsOwed -= usdsNeeded;
+ totalUsdsOwed -= actualWithdrawn;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/policies/deposits/LimitOrders.sol around lines 272-278, capture the
uint256 returned from SUSDS.withdraw(...) instead of assuming the requested
usdsNeeded was actually returned; use that actualWithdrawn value when updating
accounting: compute usedForFill = min(fillAmount_, actualWithdrawn) and
usedIncentive = actualWithdrawn - usedForFill, increment order.depositSpent by
usedForFill and order.incentiveSpent by usedIncentive, and decrement
totalUsdsOwed by actualWithdrawn so the ledger reflects the real amount
withdrawn (the existing re-wrap logic for any excess remains unchanged).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/policies/deposits/LimitOrders.sol (2)
265-266: UsesafeTransferFromfor ERC721 transfer.
transferFromdoesn't verify the recipient can receive ERC721 tokens. Iforder.owneris a contract withoutonERC721Received, the NFT will be transferred but potentially locked.This was flagged in previous review and marked as a remaining to-do.
- POSITION_NFT.transferFrom(address(this), order.owner, positionId); + POSITION_NFT.safeTransferFrom(address(this), order.owner, positionId);
245-252: Consider capturing withdraw return value for precise accounting.ERC4626
withdraw()returns the actual shares burned, which may differ slightly from the requested assets due to rounding. While the re-wrap logic at lines 279-282 handles excess USDS, using the actual withdrawn amount fortotalUsdsOwedwould be more precise.This was flagged in previous review. The current implementation works correctly due to the re-wrap safety net, but capturing the return value would be more precise.
🧹 Nitpick comments (3)
src/policies/deposits/LimitOrders.sol (3)
2-9: Pragma version may be incompatible with transient storage.
ReentrancyGuardTransientuses EIP-1153 transient storage which requires Solidity ≥0.8.24 and the Cancun hardfork. The pragma>=0.8.20allows compilation with versions that don't support transient opcodes, which would fail at deployment.Additionally, the imports mix versioned (
@openzeppelin-5.3.0) and unversioned (@openzeppelin) paths. Consider standardizing on one approach for consistency.-pragma solidity >=0.8.20; +pragma solidity >=0.8.24;
103-132: Consider validating sUSDS underlying asset matches USDS.The constructor doesn't verify that
sUsds_is actually a vault forusds_. A misconfiguration would cause silent failures when depositing.if (sUsds_ == address(0)) revert InvalidParam("sUsds"); +if (ERC4626(sUsds_).asset() != usds_) revert InvalidParam("sUsds asset mismatch");
400-405:getRemaininglacks underflow protection unlikecancelOrder.
cancelOrderuses defensive ternaries (lines 303-304) to prevent underflow, butgetRemainingdirectly subtracts without guards. While this should never occur in normal operation, adding consistency would make the code more defensive.function getRemaining(uint256 orderId_) external view returns (uint256 deposit, uint256 incentive) { LimitOrder memory order = orders[orderId_]; - deposit = order.depositBudget - order.depositSpent; - incentive = order.incentiveBudget - order.incentiveSpent; + deposit = order.depositBudget > order.depositSpent ? order.depositBudget - order.depositSpent : 0; + incentive = order.incentiveBudget > order.incentiveSpent ? order.incentiveBudget - order.incentiveSpent : 0; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/policies/deposits/LimitOrders.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-04T11:03:24.007Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 84
File: src/test/policies/YieldDepositFacility/execute.t.sol:41-43
Timestamp: 2025-08-04T11:03:24.007Z
Learning: In YieldDepositFacility, 0xJem intentionally allows snapshots to be overwritten/updated within the same block to ensure they reflect the latest vault conversion rates. This is not a gaming vulnerability but desired behavior - the gaming fix was removing timestamp hints, not preventing snapshot updates within blocks.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-11-07T11:09:53.808Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Applied to files:
src/policies/deposits/LimitOrders.sol
🔇 Additional comments (9)
src/policies/deposits/LimitOrders.sol (9)
19-69: LGTM!Error definitions, events, and constants are well-defined.
OHM_SCALE = 1e9correctly matches OHM's 9 decimal places.
70-100: LGTM!State variables and struct design are appropriate.
totalUsdsOwedcorrectly tracks principal for yield calculation.
134-143: LGTM!Admin function properly validates input and emits event. Access control via
onlyOwneris appropriate.
153-205: LGTM!Order creation validates all critical parameters including the auctioneer's minimum bid requirement. The
totalUsdsOwedtracking correctly records the nominal USDS deposited.Note: Orders with
incentiveBudget_ = 0are allowed, meaning fillers receive no incentive. This is valid behavior if the owner wants fills executed at favorable prices without paying for them, but may result in orders never being filled.
207-292: LGTM with noted duplicates above.The fill logic is sound: proper validation of fill constraints, proportional incentive calculation with final-fill handling for dust, price protection via
previewBid, and leftover USDS re-wrapping. The slippage protection by passingexpectedOhmOutasminOhmOuttobid()is a good safeguard.
294-315: LGTM!Cancel logic correctly follows CEI pattern. The defensive ternaries at lines 303-304 properly guard against underflow, and state is updated before the external
withdrawcall.
317-348: LGTM!Yield calculation correctly uses
previewWithdrawwhich rounds up, ensuring sufficient shares are retained to cover all obligations. Sweeping sUSDS shares directly (vs. redeeming to USDS) is more gas efficient and avoids rounding losses.
417-447: LGTM!
getFillableOrdersappropriately warns about gas intensity and provides an indexed variant for pagination. The dual-loop pattern (count then populate) is a common Solidity pattern for dynamic array building.
476-483: LGTM!Correct implementation of
IERC721Receiverto accept position NFTs viasafeTransferFrom.
| /// @notice Calculate incentive and incentive rate for a given fill amount | ||
| function calculateIncentive( | ||
| uint256 orderId_, | ||
| uint256 fillAmount_ | ||
| ) public view returns (uint256 incentive, uint256 incentiveRate) { | ||
| LimitOrder memory order = orders[orderId_]; | ||
| if (order.depositBudget == 0) return (0, 0); | ||
| incentive = (fillAmount_ * order.incentiveBudget) / order.depositBudget; | ||
| incentiveRate = (order.incentiveBudget * 10_000) / order.depositBudget; | ||
| } |
There was a problem hiding this comment.
calculateIncentive doesn't match final-fill logic in fillOrder.
In fillOrder (lines 230-236), the final fill receives all remaining incentive to avoid dust. However, calculateIncentive always uses proportional calculation. This causes previewFillOrder to return incorrect incentive amounts for final fills.
function calculateIncentive(
uint256 orderId_,
uint256 fillAmount_
) public view returns (uint256 incentive, uint256 incentiveRate) {
LimitOrder memory order = orders[orderId_];
if (order.depositBudget == 0) return (0, 0);
- incentive = (fillAmount_ * order.incentiveBudget) / order.depositBudget;
+
+ uint256 remainingDeposit = order.depositBudget - order.depositSpent;
+ uint256 remainingIncentive = order.incentiveBudget - order.incentiveSpent;
+
+ // Cap fillAmount to remaining deposit
+ uint256 actualFill = fillAmount_ > remainingDeposit ? remainingDeposit : fillAmount_;
+
+ // Final fill gets all remaining incentive (matches fillOrder logic)
+ if (actualFill == remainingDeposit) {
+ incentive = remainingIncentive;
+ } else {
+ incentive = (actualFill * order.incentiveBudget) / order.depositBudget;
+ }
+
incentiveRate = (order.incentiveBudget * 10_000) / order.depositBudget;
}📝 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 Calculate incentive and incentive rate for a given fill amount | |
| function calculateIncentive( | |
| uint256 orderId_, | |
| uint256 fillAmount_ | |
| ) public view returns (uint256 incentive, uint256 incentiveRate) { | |
| LimitOrder memory order = orders[orderId_]; | |
| if (order.depositBudget == 0) return (0, 0); | |
| incentive = (fillAmount_ * order.incentiveBudget) / order.depositBudget; | |
| incentiveRate = (order.incentiveBudget * 10_000) / order.depositBudget; | |
| } | |
| /// @notice Calculate incentive and incentive rate for a given fill amount | |
| function calculateIncentive( | |
| uint256 orderId_, | |
| uint256 fillAmount_ | |
| ) public view returns (uint256 incentive, uint256 incentiveRate) { | |
| LimitOrder memory order = orders[orderId_]; | |
| if (order.depositBudget == 0) return (0, 0); | |
| uint256 remainingDeposit = order.depositBudget - order.depositSpent; | |
| uint256 remainingIncentive = order.incentiveBudget - order.incentiveSpent; | |
| // Cap fillAmount to remaining deposit | |
| uint256 actualFill = fillAmount_ > remainingDeposit ? remainingDeposit : fillAmount_; | |
| // Final fill gets all remaining incentive (matches fillOrder logic) | |
| if (actualFill == remainingDeposit) { | |
| incentive = remainingIncentive; | |
| } else { | |
| incentive = (actualFill * order.incentiveBudget) / order.depositBudget; | |
| } | |
| incentiveRate = (order.incentiveBudget * 10_000) / order.depositBudget; | |
| } |
🤖 Prompt for AI Agents
In src/policies/deposits/LimitOrders.sol around lines 360 to 369,
calculateIncentive currently always pro-rates incentive by fillAmount but does
not mirror fillOrder's final-fill behavior where the last fill receives the
remaining incentive; update the function so that if order.depositBudget is zero
return (0,0) as before, and if fillAmount_ equals the order.depositBudget (i.e.,
this is the final/remaining fill) return the entire order.incentiveBudget as
incentive and compute incentiveRate appropriately, otherwise keep the existing
proportional calculation.
For front end use
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/policies/deposits/LimitOrders.sol (4)
252-257: Capturewithdraw()return value for precise accounting.Line 252 withdraws
usdsNeededbut doesn't capture the actual assets returned. ERC4626'swithdraw()can return slightly different amounts due to rounding. Line 257 decrementstotalUsdsOwedbyusdsNeeded(the requested amount) rather than the actual withdrawn amount, which could cause accounting drift over many fills.This is a duplicate concern from the previous bot review.
Apply this diff for precision:
- SUSDS.withdraw(usdsNeeded, address(this), address(this)); + uint256 actualWithdrawn = SUSDS.withdraw(usdsNeeded, address(this), address(this)); // Update accounting order.depositSpent += fillAmount_; order.incentiveSpent += incentive; - totalUsdsOwed -= usdsNeeded; + totalUsdsOwed -= actualWithdrawn;
366-374:calculateIncentivedoesn't match final-fill logic infillOrder.In
fillOrder(lines 237-241), the final fill receives all remaining incentive to avoid dust. However,calculateIncentivealways uses proportional calculation(fillAmount_ * order.incentiveBudget) / order.depositBudget. This causespreviewFillOrderto return incorrect incentive amounts for final fills.This is a duplicate of the previous bot review comment.
function calculateIncentive( uint256 orderId_, uint256 fillAmount_ ) public view returns (uint256 incentive, uint256 incentiveRate) { LimitOrder memory order = orders[orderId_]; if (order.depositBudget == 0) return (0, 0); - incentive = (fillAmount_ * order.incentiveBudget) / order.depositBudget; + + uint256 remainingDeposit = order.depositBudget - order.depositSpent; + uint256 remainingIncentive = order.incentiveBudget - order.incentiveSpent; + + // Cap fillAmount to remaining deposit + uint256 actualFill = fillAmount_ > remainingDeposit ? remainingDeposit : fillAmount_; + + // Final fill gets all remaining incentive (matches fillOrder logic) + if (actualFill == remainingDeposit) { + incentive = remainingIncentive; + } else { + incentive = (actualFill * order.incentiveBudget) / order.depositBudget; + } + incentiveRate = (order.incentiveBudget * 10_000) / order.depositBudget; }
271-271: UsesafeTransferFromfor ERC721 transfers.Replace unsafe
transferFromwith OpenZeppelin'ssafeTransferFromto ensure the receiver can handle ERC721 tokens. Without this check, NFTs may become permanently stuck if sent to contracts that don't implementIERC721Receiver.- POSITION_NFT.transferFrom(address(this), order.owner, positionId); + POSITION_NFT.safeTransferFrom(address(this), order.owner, positionId);
178-181: UsepreviewWithdraw()or capture actual shares to ensure accounting precision with ERC4626 vault.Line 180 deposits
totalDepositUSDS to sUSDS but ignores the returned share amount, while line 181 tracks only the asset amount intotalUsdsOwed. When depositing tokens, ERC4626 rounds down the shares issued, which can create accounting drift between tracked assets and redeemable shares. sUSDS has no fees, and share to asset conversions are real-time, which mitigates but doesn't eliminate rounding loss with virtual shares in OpenZeppelin's ERC4626 implementation.To ensure precise accounting across multiple deposits and partial withdrawals, either capture and track actual shares received from deposit, or consistently use
previewWithdraw(totalUsdsOwed)before each withdrawal to verify sufficient shares exist (as done at line 335 in the cancel function).
🧹 Nitpick comments (2)
src/policies/deposits/LimitOrders.sol (2)
16-16: Consider inheriting fromPeripheryEnablerfor emergency stop capability.Past review comment suggested inheriting from
PeripheryEnablerand adding anonlyEnabledmodifier tocreateOrder. This would allow the owner to disable the contract and prevent any further orders in case of emergency.Based on learnings, this pattern is used elsewhere in the OlympusDAO codebase for administrative control.
-contract CDAuctioneerLimitOrders is ReentrancyGuardTransient, Ownable { +contract CDAuctioneerLimitOrders is ReentrancyGuardTransient, Ownable, PeripheryEnabler {Then add
onlyEnabledtocreateOrder:- ) external nonReentrant returns (uint256 orderId) { + ) external nonReentrant onlyEnabled returns (uint256 orderId) {
129-132: Consider automating receipt token configuration.Past review comment suggested calling
ReceiptTokenManager.getWrappableTokens()to automatically retrieve the list of token IDs and wrapped tokens, rather than requiring manual configuration via constructor parameters.This would reduce deployment complexity and ensure consistency with the ReceiptTokenManager's configuration. However, the current approach provides explicit control over which tokens are supported.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/policies/deposits/LimitOrders.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-04T11:03:24.007Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 84
File: src/test/policies/YieldDepositFacility/execute.t.sol:41-43
Timestamp: 2025-08-04T11:03:24.007Z
Learning: In YieldDepositFacility, 0xJem intentionally allows snapshots to be overwritten/updated within the same block to ensure they reflect the latest vault conversion rates. This is not a gaming vulnerability but desired behavior - the gaming fix was removing timestamp hints, not preventing snapshot updates within blocks.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-11-07T11:09:53.808Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Applied to files:
src/policies/deposits/LimitOrders.sol
🔇 Additional comments (9)
src/policies/deposits/LimitOrders.sol (9)
1-11: LGTM: Imports are appropriate.The imports include necessary OpenZeppelin contracts and the CDAuctioneer interface. The use of
ReentrancyGuardTransientis correct for gas-efficient reentrancy protection.
19-58: LGTM: Errors and events are well-defined.Comprehensive error coverage for validation, order state, and execution conditions. Events are appropriately indexed for efficient filtering.
59-102: LGTM: State variables and struct are well-organized.The
totalUsdsOwedvariable provides critical accounting for yield separation. TheLimitOrderstruct comprehensively tracks order state.
106-135: LGTM: Constructor validation is thorough.All address parameters are validated, array lengths are checked, and infinite USDS approval for SUSDS is appropriately set for gas efficiency.
137-145: LGTM: Admin function is properly secured.The
setYieldRecipientfunction includes appropriate validation and access control.
284-287: Re-wrap logic doesn't updatetotalUsdsOwedaccounting.Lines 284-287 re-wrap any remaining USDS balance back into sUSDS. However, this doesn't update
totalUsdsOwed. If there's excess USDS (e.g., from rounding in the bid), wrapping it increases the sUSDS balance buttotalUsdsOwedremains unchanged, which means the excess would be counted as yield.While this might be intentional (excess becomes yield), it's worth clarifying the design intent. If the bid returns less than
fillAmount_inactualAmount, the difference becomes untracked.Consider whether this behavior is intentional or if accounting should be adjusted:
uint256 remainingBalance = USDS.balanceOf(address(this)); if (remainingBalance > 0) { - SUSDS.deposit(remainingBalance, address(this)); + uint256 shares = SUSDS.deposit(remainingBalance, address(this)); + // Note: remainingBalance is now treated as yield (not added to totalUsdsOwed) }
322-353: LGTM: Yield accounting is sound.The use of
previewWithdraw(which rounds up) ensures sufficient shares are reserved for user obligations. The logic correctly identifies excess as yield and transfers it to the configured recipient.
479-488: LGTM: ERC721 receiver is correctly implemented.The
onERC721Receivedfunction properly implements the IERC721Receiver interface, allowing the contract to receive position NFTs from the auctioneer.
262-268: Verify bid parameters handle zero OHM output correctly.Lines 244-245 check that
expectedOhmOutis non-zero and revert if it is. Line 265 then passesexpectedOhmOutasminOhmOutto the bid function. However, based on learnings,ConvertibleDepositAuctioneer.bid()reverts withConvertibleDepositAuctioneer_ConvertedAmountZerowhenohmOut == 0, before evaluating theminOhmOutslippage check.The current logic should work correctly since the zero check at line 245 prevents calling bid with problematic parameters.
Based on learnings, the zero-output check is correctly placed before the bid call.
| if (totalRemaining > 0) { | ||
| SUSDS.withdraw(totalRemaining, order.owner, address(this)); | ||
| } |
There was a problem hiding this comment.
Capture withdraw() return value in cancellation.
Similar to fillOrder, line 316 withdraws totalRemaining but doesn't capture the actual assets returned. If the withdrawal returns less due to ERC4626 rounding, the user receives less than expected while totalUsdsOwed is decremented by the full totalRemaining.
Apply this diff:
if (totalRemaining > 0) {
- SUSDS.withdraw(totalRemaining, order.owner, address(this));
+ uint256 actualWithdrawn = SUSDS.withdraw(totalRemaining, order.owner, address(this));
+ // Note: If actualWithdrawn < totalRemaining, the difference remains as yield
}🤖 Prompt for AI Agents
In src/policies/deposits/LimitOrders.sol around lines 315-317, the cancellation
calls SUSDS.withdraw(totalRemaining, order.owner, address(this)) but does not
capture the actual amount returned; change the call to capture the returned
amount (e.g., uint256 withdrawn = SUSDS.withdraw(...)), use that withdrawn value
when updating totalUsdsOwed (decrement by withdrawn instead of totalRemaining),
and propagate the withdrawn amount into any subsequent state updates/emit events
or checks so ERC4626 rounding shortfalls are handled correctly and accounting
remains consistent.
Allows user to change details of an open order. Functionally equivalent to canceling an existing order and opening a new one, with orderID retained and only one transaction required. Difference between existing and new order details is transferred from or to user.
|
Merging into branch in main repo to allow for changes |
0xJem
changed the title
3 participants
Summary
Introduces a limit order system for the Convertible Deposit Auctioneer, enabling users to place persistent buy orders that execute when the auction price reaches their specified maximum. MEV bots are incentivized to fill orders, creating a permissionless execution layer.
Motivation
Currently, users must actively monitor CDAuctioneer prices and manually execute bids. This creates friction and may result in missed opportunities when prices briefly dip to favorable levels. Limit orders allow users to set-and-forget, capturing favorable prices without active management.
Features
Limit Orders
MEV Incentives
Yield Generation
sweepYield()transfers accrued yield as sUSDS sharesPrice Validation
previewBid()to check actual execution price including slippage across ticksUser Flow
createOrder()with:depositPeriod: CD term lengthdepositBudget: USDS to spend on bidsincentiveBudget: USDS to pay fillersmaxPrice: Maximum USDS per OHMminFillSize: Minimum fill per transactionfillOrder()when profitablecancelOrder()anytime to reclaim remaining fundsBot Flow
getFillableOrders()or indexOrderCreatedeventscanFillOrder()for fillability and effective pricefillOrder()with order ID and fill amountContract Architecture
Security Considerations
nonReentrantOwnablefor yield recipient management; order cancellation restricted to ownerpreviewBid()for actual execution price, not tick pricetotalUsdsOwedto isolate user principal from yieldTesting
Comprehensive test suite covering:
Files
src/CDAuctioneerLimitOrders.sol— Main contracttest/CDAuctioneerLimitOrders.t.sol— Test suiteDeployment Parameters
owner_cdAuctioneer_usds_sUsds_positionNft_yieldRecipient_depositPeriods_receiptTokens_Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.