Merged
Conversation
…rderBook from config - Add BadLender check in onFlashLoan against deterministic orderbook address - Fix flash loan amount using outputDecimals instead of inputDecimals - Remove orderBook field from OrderBookV6ArbConfig (now compile-time constant) - Add approve-call-revoke safety comments for A05-1/A06-1 - New tests: lender validation, mixed decimals, onTakeOrders2 end-to-end - Refactor mocks to use MockOrderBookBase, etch at deterministic address - Update triage.md with fix statuses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…NDED OrderBookV6 was 74 bytes over the EIP-170 24KB contract size limit at optimizer_runs=1000, causing deploy failure on Arbitrum. Lowering to 800 brings it to 23,437 bytes (1,139 under limit). Pointers regenerated. Also adds FlashLoanFailed and RouteProcessor onTakeOrders2 tests, extracts CONTEXT_COLUMNS_EXTENDED constant, and updates triage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR refactors OrderBookV6 arb contracts to use a deterministic deployed orderbook address instead of passing it as a constructor parameter, adds lender validation via BadLender error in flash borrowing, optimizes compiler settings to avoid contract size limits, extends context column definitions, and significantly expands test infrastructure with new mock base classes and comprehensive test scenarios. Changes
Sequence DiagramsequenceDiagram
actor Caller
participant Lender as Malicious Lender
participant FlashBorrower as OrderBookV6<br/>FlashBorrower
participant OrderBook as Deterministic<br/>OrderBook
participant Token as Token
Caller->>Lender: attack(arb, token, amount)
Lender->>Token: transfer(amount) to FlashBorrower
Lender->>FlashBorrower: onFlashLoan(lender_addr, token, amount, data)
Note over FlashBorrower: sender check:<br/>LibOrderBookDeploy.<br/>ORDERBOOK_DEPLOYED_ADDRESS
alt sender == ORDERBOOK_DEPLOYED_ADDRESS
FlashBorrower->>OrderBook: proceed with loan
else sender != ORDERBOOK_DEPLOYED_ADDRESS
FlashBorrower->>Caller: revert BadLender(sender)
end
rect rgba(200, 150, 100, 0.5)
Note over Lender,OrderBook: Expected flow with<br/>legitimate orderbook
Caller->>OrderBook: initiates flash loan
OrderBook->>FlashBorrower: onFlashLoan(orderbook_addr, ...)
FlashBorrower->>OrderBook: callback validated
OrderBook->>Token: receive flashLoan repayment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/abstract/OrderBookV6FlashBorrower.missingApproval.t.sol (1)
33-36: File name doesn't match test purpose.The file is named
OrderBookV6FlashBorrower.missingApproval.t.solbut the test function istestArb4RealTokenTransfers(). Consider renaming for clarity if "missing approval" is no longer the test's focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/abstract/OrderBookV6FlashBorrower.missingApproval.t.sol` around lines 33 - 36, The file name OrderBookV6FlashBorrower.missingApproval.t.sol no longer matches the test's intent; rename the file to reflect the actual test (e.g., OrderBookV6FlashBorrower.realTokenTransfers.t.sol) or, if the file should still test missing approvals, revert the test function name to reflect that purpose; locate the test function testArb4RealTokenTransfers() in the OrderBookV6FlashBorrower* contract and either change the filename to match this function's real-token-transfer scenario or rename the function back to a missing-approval-focused name so file and test purpose stay consistent.test/abstract/OrderBookV6FlashBorrower.lenderValidation.t.sol (1)
23-23: UnusedorderBookvariable.The
orderBookvariable is created but never used in the test. The test validates thatMaliciousLender(asmsg.sender) is rejected, which doesn't require the mock orderbook.🧹 Remove unused variable
function testMaliciousLenderCannotExploitOnFlashLoan() external { - RealisticFlashLendingMockOrderBook orderBook = new RealisticFlashLendingMockOrderBook(); MockToken token = new MockToken("Token", "TKN", 18);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/abstract/OrderBookV6FlashBorrower.lenderValidation.t.sol` at line 23, Remove the unused RealisticFlashLendingMockOrderBook instance: the test creates orderBook via RealisticFlashLendingMockOrderBook orderBook = new RealisticFlashLendingMockOrderBook(); but never uses it; simply delete that line (or alternatively use the instance in the test assertion if you actually intended to test interactions with RealisticFlashLendingMockOrderBook). Ensure the test continues to assert that MaliciousLender (msg.sender) is rejected without referencing orderBook.test/concrete/arb/RouteProcessorOrderBookV6ArbOrderTaker.onTakeOrders2.t.sol (1)
79-93: Consider adding post-execution assertions.The test currently only verifies that
arb5doesn't revert. Adding balance checks or state verification would strengthen the test coverage.💡 Example assertions to add
arb.arb5( IRaindexV6(address(orderBook)), TakeOrdersConfigV5({ minimumIO: LibDecimalFloat.packLossless(100, 0), maximumIO: LibDecimalFloat.packLossless(type(int224).max, 0), maximumIORatio: LibDecimalFloat.packLossless(type(int224).max, 0), IOIsInput: true, orders: orders, data: takeOrdersData }), TaskV2({ evaluable: EvaluableV4(IInterpreterV4(address(0)), IInterpreterStoreV3(address(0)), hex""), signedContext: new SignedContextV1[](0) }) ); + + // Verify expected token movements occurred + // assertGt(inputToken.balanceOf(address(arb)), 0, "arb should have received input tokens"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/concrete/arb/RouteProcessorOrderBookV6ArbOrderTaker.onTakeOrders2.t.sol` around lines 79 - 93, Add concrete post-execution assertions after the arb.arb5(...) call to verify expected state changes instead of only checking for no revert: inspect balances and state on orderBook and involved accounts (e.g., check token or ETH balances of the taker and maker, verify filled amounts in orderBook, or validate changes to orders/positions using the same IRaindexV6/orderBook, orders, and takeOrdersData inputs). Use helpers or existing getters on orderBook/IRaindexV6 and assert against expected values derived from TakeOrdersConfigV5 (minimumIO/maximumIO/IOIsInput) and the orders array to ensure the operation had the intended effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/abstract/OrderBookV6FlashBorrower.sol`:
- Around line 82-86: The code grants max allowance to a user-supplied orderBook
before validating it, allowing a malicious flashLoan to steal funds; update the
approval-call-revoke sequences in onFlashLoan and the similar block (around the
approval/flashLoan path using forceApprove/forceRevoke) to validate the
orderBook address before performing any approve/transferFrom/revoke actions by
checking that the provided orderBook equals
LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS and revert (BadLender) immediately
if not, or alternatively use LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS
directly for the approve/flashLoan/revoke flow to ensure approvals are only
given to the canonical orderBook.
In `@test/abstract/OrderBookV6ArbOrderTaker.onTakeOrders2.t.sol`:
- Around line 33-35: The test testArb5RealTokenTransfers currently only ensures
arb5 doesn't revert but doesn't assert token balances after
takeOrders/onTakeOrders2/finalize; add explicit balance assertions for
orderBook, exchange, and arb (arb5) after the flow to verify ERC20 transfers
occurred to the expected recipients. Locate the testArb5RealTokenTransfers
function and, after the onTakeOrders2 callback and finalize steps, fetch token
balances for the ERC20 used and assert that orderBook/exchange/arb balances
match the expected deltas (e.g., orderBook received seller funds, exchange
received fees/remaining, arb received profit), mirroring the same pattern for
the other affected test block around lines 80-94 (the other test that currently
only checks non-revert). Ensure you reference and use the same token contract
instance and transfer amounts used earlier in the test to compute expected
balances.
---
Nitpick comments:
In `@test/abstract/OrderBookV6FlashBorrower.lenderValidation.t.sol`:
- Line 23: Remove the unused RealisticFlashLendingMockOrderBook instance: the
test creates orderBook via RealisticFlashLendingMockOrderBook orderBook = new
RealisticFlashLendingMockOrderBook(); but never uses it; simply delete that line
(or alternatively use the instance in the test assertion if you actually
intended to test interactions with RealisticFlashLendingMockOrderBook). Ensure
the test continues to assert that MaliciousLender (msg.sender) is rejected
without referencing orderBook.
In `@test/abstract/OrderBookV6FlashBorrower.missingApproval.t.sol`:
- Around line 33-36: The file name
OrderBookV6FlashBorrower.missingApproval.t.sol no longer matches the test's
intent; rename the file to reflect the actual test (e.g.,
OrderBookV6FlashBorrower.realTokenTransfers.t.sol) or, if the file should still
test missing approvals, revert the test function name to reflect that purpose;
locate the test function testArb4RealTokenTransfers() in the
OrderBookV6FlashBorrower* contract and either change the filename to match this
function's real-token-transfer scenario or rename the function back to a
missing-approval-focused name so file and test purpose stay consistent.
In
`@test/concrete/arb/RouteProcessorOrderBookV6ArbOrderTaker.onTakeOrders2.t.sol`:
- Around line 79-93: Add concrete post-execution assertions after the
arb.arb5(...) call to verify expected state changes instead of only checking for
no revert: inspect balances and state on orderBook and involved accounts (e.g.,
check token or ETH balances of the taker and maker, verify filled amounts in
orderBook, or validate changes to orders/positions using the same
IRaindexV6/orderBook, orders, and takeOrdersData inputs). Use helpers or
existing getters on orderBook/IRaindexV6 and assert against expected values
derived from TakeOrdersConfigV5 (minimumIO/maximumIO/IOIsInput) and the orders
array to ensure the operation had the intended effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f5def4c-2b88-4762-904a-265c3621e802
⛔ Files ignored due to path filters (3)
audit/2026-03-13-01/triage.mdis excluded by!audit/**src/generated/OrderBookV6.pointers.solis excluded by!**/generated/**src/generated/OrderBookV6SubParser.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (25)
foundry.tomlscript/Deploy.solsrc/abstract/OrderBookV6ArbCommon.solsrc/abstract/OrderBookV6FlashBorrower.solsrc/concrete/arb/GenericPoolOrderBookV6ArbOrderTaker.solsrc/concrete/arb/GenericPoolOrderBookV6FlashBorrower.solsrc/concrete/parser/OrderBookV6SubParser.solsrc/lib/LibOrderBook.solsrc/lib/LibOrderBookSubParser.soltest/abstract/OrderBookV6ArbOrderTaker.onTakeOrders2.t.soltest/abstract/OrderBookV6FlashBorrower.badInitiator.t.soltest/abstract/OrderBookV6FlashBorrower.flashLoanFailed.t.soltest/abstract/OrderBookV6FlashBorrower.ierc165.t.soltest/abstract/OrderBookV6FlashBorrower.lenderValidation.t.soltest/abstract/OrderBookV6FlashBorrower.missingApproval.t.soltest/abstract/OrderBookV6FlashBorrower.mixedDecimals.t.soltest/concrete/arb/RouteProcessorOrderBookV6ArbOrderTaker.onTakeOrders2.t.soltest/util/abstract/ArbTest.soltest/util/abstract/MockOrderBookBase.soltest/util/concrete/ChildOrderBookV6ArbOrderTaker.soltest/util/concrete/MaliciousLender.soltest/util/concrete/MockRouteProcessor.soltest/util/concrete/MockToken.soltest/util/concrete/RealisticFlashLendingMockOrderBook.soltest/util/concrete/RealisticOrderTakerMockOrderBook.sol
💤 Files with no reviewable changes (4)
- test/abstract/OrderBookV6FlashBorrower.ierc165.t.sol
- script/Deploy.sol
- test/util/concrete/ChildOrderBookV6ArbOrderTaker.sol
- src/abstract/OrderBookV6ArbCommon.sol
Comment on lines
82
to
+86
| function onFlashLoan(address initiator, address, uint256, uint256, bytes calldata data) external returns (bytes32) { | ||
| // Only the deterministic orderbook deployment may call this callback. | ||
| if (msg.sender != LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS) { | ||
| revert BadLender(msg.sender); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate orderBook before the approval-call-revoke sequence.
The BadLender check on Lines 83-86 only executes if the user-supplied orderBook actually calls back into onFlashLoan. But Lines 157-159 give that address max allowances first, so a malicious flashLoan implementation can transferFrom any balances held by the borrower and return true without ever hitting the callback. Since this PR makes the lender deterministic, reject non-canonical orderBook values before the first forceApprove, or use LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS directly for the approval/flash-loan path.
Also applies to: 129-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/abstract/OrderBookV6FlashBorrower.sol` around lines 82 - 86, The code
grants max allowance to a user-supplied orderBook before validating it, allowing
a malicious flashLoan to steal funds; update the approval-call-revoke sequences
in onFlashLoan and the similar block (around the approval/flashLoan path using
forceApprove/forceRevoke) to validate the orderBook address before performing
any approve/transferFrom/revoke actions by checking that the provided orderBook
equals LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS and revert (BadLender)
immediately if not, or alternatively use
LibOrderBookDeploy.ORDERBOOK_DEPLOYED_ADDRESS directly for the
approve/flashLoan/revoke flow to ensure approvals are only given to the
canonical orderBook.
Comment on lines
+33
to
+35
| /// arb5 completes a full order-taker cycle with real ERC20 transfers: | ||
| /// takeOrders, onTakeOrders2 callback with exchange, and finalize. | ||
| function testArb5RealTokenTransfers() external { |
Contributor
There was a problem hiding this comment.
Assert the transfer results, not just the happy path.
The docstring on Lines 33-35 says this covers the full transfer cycle, but the body only proves arb5 doesn't revert. A regression where the callback succeeds but tokens land on the wrong contract would still pass unless you assert the post-call balances for orderBook, exchange, and arb.
💡 Example assertions
arb.arb5(
IRaindexV6(address(orderBook)),
TakeOrdersConfigV5({
minimumIO: LibDecimalFloat.packLossless(100, 0),
maximumIO: LibDecimalFloat.packLossless(type(int224).max, 0),
maximumIORatio: LibDecimalFloat.packLossless(type(int224).max, 0),
IOIsInput: true,
orders: orders,
data: takeOrdersData
}),
TaskV2({
evaluable: EvaluableV4(IInterpreterV4(address(0)), IInterpreterStoreV3(address(0)), hex""),
signedContext: new SignedContextV1[](0)
})
);
+
+ assertEq(outputToken.balanceOf(address(orderBook)), 0);
+ assertEq(inputToken.balanceOf(address(orderBook)), 100e18);
+ assertEq(outputToken.balanceOf(address(exchange)), 100e18);
+ assertEq(inputToken.balanceOf(address(exchange)), 0);
+ assertEq(outputToken.balanceOf(address(arb)), 0);
+ assertEq(inputToken.balanceOf(address(arb)), 0);Also applies to: 80-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/abstract/OrderBookV6ArbOrderTaker.onTakeOrders2.t.sol` around lines 33 -
35, The test testArb5RealTokenTransfers currently only ensures arb5 doesn't
revert but doesn't assert token balances after
takeOrders/onTakeOrders2/finalize; add explicit balance assertions for
orderBook, exchange, and arb (arb5) after the flow to verify ERC20 transfers
occurred to the expected recipients. Locate the testArb5RealTokenTransfers
function and, after the onTakeOrders2 callback and finalize steps, fetch token
balances for the ERC20 used and assert that orderBook/exchange/arb balances
match the expected deltas (e.g., orderBook received seller funds, exchange
received fees/remaining, arb received profit), mirroring the same pattern for
the other affected test block around lines 80-94 (the other test that currently
only checks non-revert). Ensure you reference and use the same token contract
instance and transfer amounts used earlier in the test to compute expected
balances.
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1380 SIZE=L 🧠 Learnings used |
This was referenced Mar 14, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
New Features
Chores