test(reentrancy): ERC1155-callback reentrancy regression for both vault types

[thedavidmeister]

Summary

Adds fuzz-backed reentrancy regression tests covering the ERC1155 acceptance-callback surface documented in issue #316 (rows 1/2/7 of the master reentrancy call-site table).

• OffchainAssetReceiptVault.reentrant.t.sol: testDepositReentrancyGuardFires — a malicious receiver that re-enters vault.deposit from inside onERC1155Received is blocked by the nonReentrant guard; the outer deposit reverts with ReentrancyGuardReentrantCall. testDepositSucceedsWithNonReentrantReceiver — positive control confirming the setup is valid.
• ERC20PriceOracleReceiptVault.reentrant.t.sol: same two tests for the oracle vault variant, exercising the base _beforeDeposit ERC20 transfer path in addition to the receipt callback.

Both tests run 256 fuzz rounds locally (all pass).

No production code changes — test-only addition.

Closes #316

Test plan

• forge test --match-path "test/src/concrete/vault/*reentrant*" — 4/4 pass, 256 fuzz rounds each
• CI rainix-sol/test green
• CI copy-artifacts green (no artifact changes)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Strengthened vault protection against reentrant deposits, withdrawals, receipt transfers, confiscations, and payment-token callbacks.
  • Prevented share overspending during reentrant authorization flows.
  • Verified certification state is updated correctly before authorization checks.
  • Confirmed ETH refund callbacks can safely trigger independent deposits.

• Tests

  • Added comprehensive regression coverage for reentrancy scenarios across asset, oracle, authorization, receipt, and payment flows.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c7b7b975-f1cd-4cb7-95c9-b58e1b2905b7

📥 Commits

Reviewing files that changed from the base of the PR and between 6640bb7 and 6ebc80e.

📒 Files selected for processing (1)

• test/src/concrete/vault/OffchainAssetReceiptVaultPaymentMint.reentrant.t.sol

Walkthrough

Adds Solidity reentrancy test doubles and regression suites for ERC20, oracle, receipt callback, authorizer, certification, share transfer, ETH refund, confiscation, and payment-mint flows.

Changes

Reentrancy regression tests

Layer / File(s)	Summary
Reentrant test doubles test/concrete/*.sol	Adds configurable contracts that re-enter through ERC20 hooks, authorizer callbacks, oracle pricing, certification, share transfers, ETH refunds, and ERC1155 callbacks.
ERC20 and oracle vault regressions test/src/concrete/vault/ERC20PriceOracleReceiptVault.reentrant.t.sol	Tests guarded receipt callbacks and asset hooks, successful normal deposits, and nested oracle deposits using distinct prices.
Offchain vault regressions test/src/concrete/vault/OffchainAssetReceiptVault.reentrant.t.sol	Tests guarded callbacks, swapped authorizers, share-transfer balance protection, certification state observation, ETH refund reentry, and confiscation callbacks.
Payment-mint reentry regression test/src/concrete/vault/OffchainAssetReceiptVaultPaymentMint.reentrant.t.sol	Tests payment-token reentry during mint authorization and verifies the operation reverts without increasing total supply.

Estimated code review effort: 4 (Complex) | ~45 minutes

🚥 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 is related to the PR, but it only highlights ERC1155 callbacks and omits the broader reentrancy coverage added.
Linked Issues check	✅ Passed	The PR adds the requested regression tests for the #316 reentrancy call sites across receipt, vault, authorizer, oracle, refund, payment, and confiscation paths.
Out of Scope Changes check	✅ Passed	The changes stay within the reentrancy test and helper-contract scope and do not introduce unrelated production-code edits.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 2026-06-21-issue-316-reentrancy-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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thedavidmeister]

🤖 ai:vetter
Reviewed 1d086b8: ready — adds testDepositReentrancyGuardFires + positive control for OffchainAssetReceiptVault and ERC20Price
cost 350 — reentrancy regression tests for both vault types

[thedavidmeister]

Rework note (human): incomplete coverage of #316. The issues test-coverage checklist enumerates ~7 distinct reentry categories; this PR covers only the ERC1155 deposit-callback rows (1/2/7, per the PR body). Still uncovered: asset-ERC20 transfer hooks (rows 8/9 — the asset mock bypasses them), payment-token reentry (row 19), ETH-refund sendValue (row 11), swapped-authorizer (rows 12–15), oracle _nextId (row 20), confiscation (row 16). Exercise the remaining categories against REAL reentrant dependencies (not always-succeed mocks).

[thedavidmeister]

🤖 ai:producer

Producer note: rework executed — rows covered (each against a REAL reentrant dependency, all mutation-validated):

• 8: testDepositAssetTransferFromReentrancyGuardFires (hooked asset ERC20 re-enters from _beforeDeposit pull)
• 9: testWithdrawAssetTransferReentrancyGuardFires (hooked asset ERC20 re-enters from _afterWithdraw push)
• 11: testDepositEthRefundReentryIsBenign (reentry via post-guard sendValue refund → fresh independent deposit, distinct ids)
• 12: testDepositSwappedAuthorizerReceiptTransferGuardFires (swapped authorizer re-enters from authorizeReceiptTransfer3)
• 13: testDepositSwappedAuthorizerAfterDepositGuardFires (swapped authorizer re-enters from _afterDeposit)
• 14: testCertifyReentrantAuthorizerObservesWrittenCertification (certifiedUntil written before authorize)
• 15: testShareTransferSwappedAuthorizerCannotOverspend (unguarded share _update window → ERC20 underflow blocks overspend)
• 16: testConfiscateReceiptReentrantConfiscatorGuardFires (confiscator re-enters from ERC1155 acceptance callback)
• 19: testPaymentMintReentrantPaymentTokenGuardFires (reentrant payment token during payment-mint pull; supply cap intact)
• 20: testDepositOracleReentryIsBenignWithFreshPrice (reentrant oracle in pre-guard _nextId → fresh price per deposit)

Rows found inapplicable: none.

Mutation validation (7 mutants, each killed by exactly the claiming tests, restored after): _deposit guard removed → 7 tests fail; confiscateReceipt guard removed → row 16 fails; certifiedUntil write reordered after authorize → row 14 fails; _update authorize removed → row 15 fails; guard widened to span refund + oracle read → rows 11/20 fail; per-site authorize removal at authorizeReceiptTransfer3 / _afterDeposit → exactly rows 12 / 13(+19) fail.

Local suite: 392 passed, 1 pre-existing env-only failure (PythInterfaceProdTest needs ARBITRUM_RPC_URL, supplied in CI). All 14 reentrancy tests pass at 256 fuzz runs.

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In @.pre-commit-config.yaml:
- Line 1: The pre-commit configuration is an environment-specific symlink into
the Nix store and is not portable. Replace the `.pre-commit-config.yaml` symlink
with the actual repository configuration, or add setup logic that generates the
symlink locally while keeping a portable config tracked in the repository.

🪄 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: 35e762fd-3b6d-4d82-8aaa-7a6ef1e6c38d

📥 Commits

Reviewing files that changed from the base of the PR and between 8f68b12 and b2ccda1.

📒 Files selected for processing (11)

• .pre-commit-config.yaml
• test/concrete/CertifyObserverAuthorizer.sol
• test/concrete/EthRefundReenterDepositor.sol
• test/concrete/ReentrantAsset.sol
• test/concrete/ReentrantAuthorizer.sol
• test/concrete/ReentrantConfiscator.sol
• test/concrete/ReentrantOracle.sol
• test/concrete/ReentrantShareTransferAuthorizer.sol
• test/src/concrete/vault/ERC20PriceOracleReceiptVault.reentrant.t.sol
• test/src/concrete/vault/OffchainAssetReceiptVault.reentrant.t.sol
• test/src/concrete/vault/OffchainAssetReceiptVaultPaymentMint.reentrant.t.sol

[thedavidmeister]

🤖 ai:vetter
Reviewed 6640bb7: ready — closes #316 — every reentry row the human rework note enumerated is covered against real reentrant dependencies (14 tests, 7 malicious helper contracts, both vault types + payment-mint); 7 mutants each killed by exactly the claiming tests; test-only
cost 430 — 14 reentrancy tests, real reentrant deps

[thedavidmeister]

Reviewed 6ebc80e: approve

Merge-updated and fixed two version-drift breaks before merging. Both were introduced by the branch being five weeks stale, not by the tests themselves.

What I fixed, and how I nearly missed it

The first CI run after the merge-update showed one red check, and my initial read — "every [FAIL line is the drpc quota" — was wrong in the dangerous direction. The grep came back empty because there were zero [FAIL lines at all: the suite never compiled. Absence of failing tests read as absence of failures.

The actual error, caused by this branch:

Error (6275): Source "dependencies/rain-factory-0.1.1/src/concrete/CloneFactory.sol" not found

OffchainAssetReceiptVaultPaymentMint.reentrant.t.sol:12 imported through the rain-factory-0.1.1/ prefix. Every other file in the repo uses 0.1.5, foundry.toml declares "rain-factory" = "0.1.5", and soldeer.lock carries only the 0.1.5 archive. Fixed the prefix — which surfaced the second layer:

Error (9582): Member "clone" not found or not visible after argument-dependent lookup in contract CloneFactory.

rain-factory 0.1.5 removed clone() in favour of cloneDeterministic() with an explicit salt. Switched to factory.cloneDeterministic(address(implementation), initData, bytes32(0)), matching the sibling OffchainAssetReceiptVaultPaymentMintAuthorizerV1.construct.t.sol, which does exactly that. Salt 0 is safe here because the helper constructs a fresh CloneFactory per call, so sibling clones cannot collide. The clone is fixture setup, not the behaviour under test.

Now the red really is environmental, demonstrated rather than asserted

On the final run: 0 Compiler run failed lines, 0 non-quota test failures, and the single failing test is testPythGetPriceNoOlderThan — pre-existing, unrelated to this diff, failing on vm.createSelectFork … lb.drpc.live … error code -32001: You've reached the usage limit for your current plan. Main fails the same way.

The tests

Twelve functions across three files, driven by seven purpose-built attacker contracts — and every fixture is referenced, none is scaffolding. The body claims "rows 1/2/7" of #316's table and badly understates itself; the suite reaches rows 8, 9, 11, 12, 14, 15 and 20 as well.

The best property is that the test names pin the audit's reasoning, not merely "does not revert". Row 14's mitigation is "certifiedUntil written before the call" → testCertifyReentrantAuthorizerObservesWrittenCertification. Row 15's is "ERC20 underflow prevents overspend" → testShareTransferSwappedAuthorizerCannotOverspend. If the argument stops holding, the test fails — which is what a regression test for an audit finding should do.

testDepositSucceedsWithNonReentrantReceiver is the right control: without it, a guard test passes identically whether the guard works or the callback is never reached.

Scope of Closes #316

The table is 22 rows: 16 rated none (nothing to regression-test), 4 low, 1 low (benign), 1 minor. Every non-none row has a matching test except row 18 — _setAuthorizer calling supportsInterface on an untrusted address, rated minor and onlyOwner-gated. Filed as a follow-up rather than held against this PR.