test reentrancy guard on store + ERC721/ERC1155 recipient paths#471
Conversation
The `Flow.flow` `nonReentrant` guard has four documented entry points (LibFlow.sol:142). PR #446 covers ERC20/ERC777-style reentrancy via a malicious token. This adds tests for the remaining three: 1. Malicious interpreter store: `MaliciousReenteringStore.set` re-enters `flow.flow`. Etched at the framework's STORE address. 2. Malicious ERC721 recipient: `MaliciousReenteringRecipient .onERC721Received` re-enters. The framework's TOKEN_B is etched with `StubERC721WithReceiverHook` so the underlying `safeTransferFrom` call propagates into the recipient hook. 3. Malicious ERC1155 recipient: same shape via `StubERC1155WithReceiverHook` at TOKEN_C and `onERC1155Received`. Mutation verified: dropping `nonReentrant` from `Flow.flow` makes all three new tests fail; reverting passes. Closes #309. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
`generateFlowStack` / `findEvent` / `findEvents` were thin wrappers in `FlowUtilsAbstractTest`. The wrappers exist only because Solidity's `using X for T` clauses don't inherit, so child contracts of `FlowTest` couldn't otherwise call the library forms directly. Replace each callsite with the explicit static form: - `generateFlowStack(t)` -> `LibStackGeneration.generateFlowStack(Sentinel.unwrap(RAIN_FLOW_SENTINEL), t)` - `findEvent(logs, sig)` -> `LibLogHelper.findEvent(logs, sig)` `FlowTest` now inherits `FlowTransferOperation` directly. `FlowUtilsAbstractTest.sol` is removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… callsites" This reverts commit 0754b80.
Resolve the single conflict in test/src/concrete/Flow.transfer.t.sol as the
union of both sides' additions: both branches appended disjoint new test
functions and a new import after testFlowProcessesERC721BeforeERC1155. Keep
this branch's three reentrancy-guard tests
(testFlowReentrancyGuardFiresOn{StoreCallback,ERC721Recipient,ERC1155Recipient})
plus their four stub/recipient imports, and main's six tests
(testFlowERC{20,721,1155}TokenRevertBubblesUp, testFlowAtomicRollbackOnLaterTransferFailure,
testFlowRevertsOn{Empty,OneSentinelOnly}EvaluatedStack) plus its MissingSentinel
import. No money-path or contract logic touched; test fixtures only. All 21
tests in the merged contract pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviewed dc29a94: rebased onto main (Flow.transfer.t.sol union — this PR 3 reentrancy-guard tests coexist with main 6 revert tests) and the forge fmt --check failure in the new tests fixed. 21 tests pass, build + fmt green. Test-only. LGTM. |
1 participant
Summary
Flow.flowcarriesnonReentrantand theLibFlow.flowNatSpec names four reentrancy entry points. PR #446 covers ERC20/ERC777-style reentrancy via a malicious token. This adds tests for the remaining three:MaliciousReenteringStore.setre-entersflow.flow. Etched at the framework'sSTOREaddress. Storage slots from the deployed contract are copied across so the etched code retains its evaluable.MaliciousReenteringRecipient.onERC721Receivedre-enters. The framework'sTOKEN_Bis etched withStubERC721WithReceiverHookso the underlyingsafeTransferFromcall propagates into the recipient hook (the standard mock token does not call the hook).StubERC1155WithReceiverHookatTOKEN_CandonERC1155Received.Mutation verified: dropping
nonReentrantfromFlow.flowmakes all three new tests fail; reverting passes.Replaces the closed PR #440 (NatSpec-only enumeration). Closes #309.
Test plan
nonReentrant→ all 3 fail🤖 Generated with Claude Code