Skip to content

test: salvage audit triage tests from #2514#2581

Open
thedavidmeister wants to merge 2 commits into
mainfrom
2026-05-20-audit-triage-tests
Open

test: salvage audit triage tests from #2514#2581
thedavidmeister wants to merge 2 commits into
mainfrom
2026-05-20-audit-triage-tests

Conversation

@thedavidmeister
Copy link
Copy Markdown
Contributor

@thedavidmeister thedavidmeister commented May 20, 2026

Salvages commit `91f6e9503` from the now-closed #2514 onto post-rename main, with file paths and identifiers translated for the OrderBook → Raindex rename.

What's in here

New tests covering audit-flagged paths:

test tracked issue
GenericPoolRaindexV6.splitSpender — spender != pool path in _exchange #2533
RouteProcessorRaindexV6ArbOrderTaker.fallback / .receive — payable entry points #2534
RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz (broader RaindexV6 path coverage)
LibRaindexArb.finalizeArbZeroBalance / .finalizeArbFuzz #2537
LibRaindex.doPost edge cases #2536
RaindexV6.takeOrder.minimumIOIsOutput / .zeroAmount #2535
RaindexV6.multicall, .negativePullPush, .negativeVaultBalance(Change) (extra coverage)
LibOrder hash-mutation fuzz

Test infrastructure changes:

  • LibTestArb.setup and LibTestFlashBorrowerArb.setup now take separate spender and pool addresses, with a single-argument convenience overload (exchange == spender == pool) so existing callers keep working. Lets split-spender tests exercise the exchange-data unpacking path.
  • RaindexV6SelfTest deploys the TOFU singleton in its constructor via LibRainDeploy so internal token-decimals lookups work in tests that exercise them.
  • New test/util/concrete/SplitSpenderExchange.sol exposing a SpenderProxy + SplitSpenderPool pair for the split-spender tests.

Provenance

Original commit on the old 2026-03-13-audit branch: 91f6e9503 Add audit triage tests for OB internals, arb paths, and doPost (21 files, +639/-36). #2514 itself is being closed as superseded — the other two commits there were a now-obsolete iOrderbook→constant refactor (rendered moot by the rename's sweeping changes) and a triage-MD update against a file that was deleted in #2526.

Path/identifier translation applied:

  • test/concrete/ob/OrderBookV6.*.t.soltest/concrete/raindex/RaindexV6.*.t.sol
  • test/concrete/arb/GenericPoolOrderBookV6* / RouteProcessorOrderBookV6**RaindexV6*
  • test/lib/LibOrderBook*test/lib/LibRaindex*
  • src/abstract/OrderBookV6FlashBorrower.solsrc/abstract/RaindexV6FlashBorrower.sol
  • OrderBookV6 → RaindexV6, LibOrderBook → LibRaindex, iOrderbook → iRaindex, setup.orderBook → setup.raindex, obPullAmount → raindexPullAmount, obOutputAmount → raindexOutputAmount

forge build passes on the resulting tree.

🤖 Generated with Claude Code

Summary by CodeRabbit

Tests

  • Comprehensive Test Coverage – Added extensive test suites validating arbitrage flows, split-spender approval patterns, multicall operations, negative amount handling, vault balance management, and order evaluation scenarios.
  • Edge Case Validation – New tests ensure proper error handling for zero amounts, negative values, minimum output thresholds, and direct ETH transfers.
  • Setup Utilities Enhanced – Updated test helpers to support flexible spender and pool configuration for improved test scenario coverage.

Review Change Stack

Brings across commit 91f6e95 from the closed PR #2514 onto post-rename
main, with file paths and identifiers updated for the OrderBook ->
Raindex rename:

New tests covering audit-flagged paths:
- spender != pool path in GenericPool _exchange (#2533)
- receive() and fallback() payable in RouteProcessor arb (#2534)
- onTakeOrders2 fuzz on RouteProcessor arb
- LibRaindexArb finalizeArb zero-balance and fuzz (#2537)
- LibRaindex doPost edge cases (#2536)
- RaindexV6 takeOrder min IO is output, zero amount (#2535)
- RaindexV6 multicall, negativePullPush, negativeVaultBalance(Change)

Test infrastructure changes:
- LibTestArb.setup and LibTestFlashBorrowerArb.setup now take separate
  spender and pool addresses (with a single-argument convenience
  overload) so split-spender exchange data can be exercised.
- RaindexV6SelfTest deploys the TOFU singleton in its constructor so
  internal token-decimals lookups work in tests that exercise them.
- New SplitSpenderExchange helper exposing a SpenderProxy and
  SplitSpenderPool pair.

LibOrder.t.sol gets a fuzzed hash-mutation test that proves changing a
single field always changes the hash.

forge build passes on the resulting tree.
@thedavidmeister thedavidmeister self-assigned this May 20, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

Warning

Rate limit exceeded

@thedavidmeister has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 39 minutes and 21 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d439e740-2288-428b-9376-a6063da20466

📥 Commits

Reviewing files that changed from the base of the PR and between 33fb9af and c97b44f.

📒 Files selected for processing (1)
  • test/lib/LibOrder.t.sol
📝 Walkthrough

Walkthrough

This PR adds comprehensive test coverage for Raindex V6 core flows and edge cases. New tests validate split-spender approval patterns, ETH receive/fallback acceptance, negative-amount reverts, zero-amount order handling, Float parameter fuzz testing, and arbitrage finalization invariants. Test infrastructure is updated to support TOFU token-decimals initialization and to distinguish spender and pool roles in exchange operations.

Changes

Test Coverage Expansion for Raindex V6 Core Flows and Edge Cases

Layer / File(s) Summary
Test infrastructure foundation: TOFU decimals setup and utility contracts for split-spender pattern
test/util/abstract/RaindexV6SelfTest.sol, test/util/concrete/SplitSpenderExchange.sol, test/util/lib/LibTestArb.sol, test/util/lib/LibTestFlashBorrowerArb.sol
Base test class constructor deploys TOFU decimals singleton and Zoltu factory. New SpenderProxy and SplitSpenderPool utility contracts enable split-approval flow testing. LibTestArb and LibTestFlashBorrowerArb now provide setup overloads accepting separate spender and pool addresses, with input tokens minted to pool and exchange data encoding updated to distinguish the two roles.
Split-spender approval pattern validation for order-taker and flash-borrower flows
test/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.splitSpender.t.sol, test/concrete/arb/GenericPoolRaindexV6FlashBorrower.splitSpender.t.sol
Tests verify that split-spender architectures successfully revoke approvals for the spender role while pool approvals remain unset, validating the pattern across both arb order-taker (arb5) and flash-borrower (arb4) exchange paths.
RouteProcessor ETH receive and fallback acceptance, plus onTakeOrders2 fuzz testing
test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.receive.t.sol, test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.fallback.t.sol, test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz.t.sol
Tests confirm RouteProcessor ETH acceptance via receive() and fallback() with non-empty calldata. Fuzz test exercises onTakeOrders2 with fuzzed Float parameters and empty routes, asserting no token balance stranding on successful execution via try/catch handling of invalid Float reverts.
Raindex V6 edge case and negative-amount revert path coverage
test/concrete/raindex/RaindexV6.negativePullPush.t.sol, test/concrete/raindex/RaindexV6.negativeVaultBalance.t.sol, test/concrete/raindex/RaindexV6.negativeVaultBalanceChange.t.sol, test/concrete/raindex/RaindexV6.takeOrder.minimumIOIsOutput.t.sol, test/concrete/raindex/RaindexV6.takeOrder.zeroAmount.t.sol, test/concrete/raindex/RaindexV6.multicall.t.sol
Negative-path tests verify pullTokens, pushTokens, and vault balance operations revert with NegativePull, NegativePush, and NegativeVaultBalance[Change] when provided negative or over-decreased amounts. Zero-amount orders emit OrderZeroAmount events and return zero totals. MinimumIO validation reverts when output falls below threshold. Multicall test batches multiple deposit payloads via OpenZeppelin Multicall.
Library-level contract behavior and order hashing tests
test/lib/LibOrder.t.sol, test/lib/LibRaindex.doPost.t.sol
Fuzz test validates LibOrder.hash distinguishes orders with mutated owner fields. LibRaindex.doPost tests cover empty task arrays, empty-bytecode task skipping, and confirm store.set is not invoked when post-evaluation yields no writes, verified via vm.record() and storage access tracking.
Finalization and arbitrage fuzz and invariant testing
test/lib/LibRaindexArb.finalizeArbFuzz.t.sol, test/lib/LibRaindexArb.finalizeArbZeroBalance.t.sol
Fuzz test validates arbitrage finalization with fuzzed amounts, asserting correct token distribution across sender, arb contract, order book, and exchange positions. Zero-balance finalization test confirms sender and arb contract both end with zero balances after equal in/out finalization, validating full token consumption and no leftover holdings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • findolor
  • hardyjosh
  • Siddharth2207

Poem

🐰 Tests bloom in spring like carrots in the ground,
From split-spenders' dances to ETH sounds,
Negative amounts and zeros we've found,
Each edge case caught and tightly bound,
Raindex now tested, sturdy and sound! 🥕✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: salvaging audit triage tests from a previous PR (#2514) onto the current main branch.
Linked Issues check ✅ Passed The PR successfully implements all test coverage objectives from #2514: split-spender patterns, ETH handling, edge cases, fuzz tests, multicall, zero-amount/minimum-IO validation, and test infrastructure updates.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to adding test files and updating test infrastructure; no production code modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-05-20-audit-triage-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz.t.sol (1)

42-49: ⚡ Quick win

Avoid swallowing all revert causes in fuzz path.

The empty catch {} will also hide unexpected regressions (not just invalid float conversions), so this test can pass while behavior is broken. Please assert/whitelist expected revert classes, or add a dedicated non-catching success-case test alongside this fuzz test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz.t.sol`
around lines 42 - 49, The fuzz test currently swallows all reverts with an empty
catch when calling arb.onTakeOrders2(address(tokenA), address(tokenB),
inputAmountSent, totalOutputAmount, route), hiding real regressions; update the
test to either catch and assert only the expected revert types/messages
(whitelist the known float-conversion or invalid-parameter revert reasons) or
remove the blanket catch and add a separate deterministic success-case test that
calls onTakeOrders2 without catching so tokenA.balanceOf(address(arb)) and
tokenB.balanceOf(address(arb)) are asserted on success; ensure you reference the
same call site (arb.onTakeOrders2) and balance assertions when implementing the
whitelist or the dedicated success test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz.t.sol`:
- Around line 42-49: The fuzz test currently swallows all reverts with an empty
catch when calling arb.onTakeOrders2(address(tokenA), address(tokenB),
inputAmountSent, totalOutputAmount, route), hiding real regressions; update the
test to either catch and assert only the expected revert types/messages
(whitelist the known float-conversion or invalid-parameter revert reasons) or
remove the blanket catch and add a separate deterministic success-case test that
calls onTakeOrders2 without catching so tokenA.balanceOf(address(arb)) and
tokenB.balanceOf(address(arb)) are asserted on success; ensure you reference the
same call site (arb.onTakeOrders2) and balance assertions when implementing the
whitelist or the dedicated success test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 17891719-8aa4-4814-8d22-10aafbc640cc

📥 Commits

Reviewing files that changed from the base of the PR and between 40cd096 and 33fb9af.

📒 Files selected for processing (19)
  • test/concrete/arb/GenericPoolRaindexV6ArbOrderTaker.splitSpender.t.sol
  • test/concrete/arb/GenericPoolRaindexV6FlashBorrower.splitSpender.t.sol
  • test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.fallback.t.sol
  • test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.onTakeOrders2Fuzz.t.sol
  • test/concrete/arb/RouteProcessorRaindexV6ArbOrderTaker.receive.t.sol
  • test/concrete/raindex/RaindexV6.multicall.t.sol
  • test/concrete/raindex/RaindexV6.negativePullPush.t.sol
  • test/concrete/raindex/RaindexV6.negativeVaultBalance.t.sol
  • test/concrete/raindex/RaindexV6.negativeVaultBalanceChange.t.sol
  • test/concrete/raindex/RaindexV6.takeOrder.minimumIOIsOutput.t.sol
  • test/concrete/raindex/RaindexV6.takeOrder.zeroAmount.t.sol
  • test/lib/LibOrder.t.sol
  • test/lib/LibRaindex.doPost.t.sol
  • test/lib/LibRaindexArb.finalizeArbFuzz.t.sol
  • test/lib/LibRaindexArb.finalizeArbZeroBalance.t.sol
  • test/util/abstract/RaindexV6SelfTest.sol
  • test/util/concrete/SplitSpenderExchange.sol
  • test/util/lib/LibTestArb.sol
  • test/util/lib/LibTestFlashBorrowerArb.sol

Salvaged tests almost-perfectly but dropped a 1-line comment from the
upstream audit commit. Restoring it so the new PR matches the original
intent verbatim.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant