test: cover every answer length the resolved-address read guard refuses - #122
Conversation
`checkResolvedAddresses` refuses a read whose answer is not exactly one word, and only two of the lengths that can be were exercised: zero, from a target with no code, and exactly one word, from a real address getter. Nothing produced an answer LONGER than a word — what a read pointed at a getter returning `(address, address)`, or any dynamic type, answers with — and nothing produced one shorter but non-empty either. Both gaps were mutation-visible in opposite directions: weakening the guard to `returnData.length < 0x20` accepted the long answer, decoded it down to its first word and compared that as if the read had answered one address, and `> 0x20` accepted the short one into a decoder that reverts with no data of its own — the bare, undiagnosable revert `ResolvedAddressReadFailed` exists to replace. Covered as the rule rather than as two examples: one test fuzzes the whole tail past the first word, so 0x21 bytes is as much an instance as 0x40, and the other fuzzes every length between an empty answer and a whole word. The long answer's leading word is exactly the address the caller expects, so nothing but the length is wrong with it and a guard that accepted any answer of at least one word PASSES against a read that never gave a single address. `MockRawAnswerOwner` is what makes those answers reachable from a contract rather than from a cheatcode. It returns in assembly because the length is the point: every Solidity return type ABI-encodes to a whole number of words, so nothing declared can answer with twenty bytes — which is exactly what the Zoltu factory answers with. Closes #62 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 55 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe PR adds a raw byte-response test contract and fuzz tests for ChangesMalformed owner response coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adds coverage for short and oversized resolved-address responses without changing production behavior. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
…ord-length-guard # Conflicts: # test/src/lib/LibRainDeploy.t.sol
|
Reviewed cabd100: ready — merging. Conflict resolutionThis branch was
Semantic-conflict check on the merged tree, not just on the markers:
What was actually run on the merge commitWhole suite, not just the changed tests:
The one red, and why it is not this PR
Review coverageCodeRabbit's check is green but reports The diff itself, against #62Test-only: one new The issue asked for the
|
2 participants
Closes #62 — audit finding
cov-09, dimension 2, severity LOW.What was uncovered
checkResolvedAddressesrefuses a read whose answer is not exactly one word, atsrc/lib/LibRainDeploy.sol:291:Only two answer lengths were ever produced by the suite —
0, from a target with no code and from a reverting selector, and exactly0x20, from a real address getter. Nothing produced an answer LONGER than a word, which is what a consumer-supplied read of a getter returning(address, address)or any dynamic type answers with, and nothing produced one shorter but non-empty either.What is covered now
Two fuzz tests in
test/src/lib/LibRainDeploy.t.sol, written as the RULE rather than as two point cases:testCheckResolvedAddressesLongAnswerRevertsfuzzes the whole tail past the first word, so an answer of0x21bytes is as much an instance as one of0x40. The answer's leading word is exactly the address the caller expects, so nothing but the length is wrong with it — a guard that accepted any answer of at least one word would decode that word and PASS against a read that never gave a single address.testCheckResolvedAddressesShortAnswerRevertsfuzzes every length between an empty answer and a whole word. Length 20 is exactly the shape the Zoltu factory itself answers with.test/concrete/MockRawAnswerOwner.solis what makes those answers reachable from a contract rather than from a cheatcode, alongside the existingMockDirtyWordOwner. It returns in assembly because the length is the point: every Solidity return type ABI-encodes to a whole number of words, so nothing declared can answer with twenty bytes.No source change. The guard is correct as written; this pins it.
QA
testCheckResolvedAddressesLongAnswerReverts,testCheckResolvedAddressesShortAnswerReverts— neither fails on base, and neither can: the guard is CORRECT onmain, which is why issue 62 is a coverage finding at severity LOW with no live defect. What each discriminates against is a specific weakening of the guard that the pre-existing suite does not catch, verified by running the guard mutated and restored under a script that printed the guard source line andgit statusbefore every run (finalgit statusclean, guard line back to!= 0x20), so no result can come from a stale or unmutated build. For each mutant below I ran the ten pre-existingCheckResolvedAddressestests separately from the two new ones, so "which test kills it" is an observation and not an inference.src/lib/LibRainDeploy.sol:291. (1)returnData.length != 0x20→returnData.length < 0x20, accepting every answer of at least one word: all 10 pre-existing tests PASS (mutant survives them — this is exactly the finding in issue 62), killed bytestCheckResolvedAddressesLongAnswerRevertswithnext call did not revert as expected, because the long answer passed the weakened guard, was decoded down to its first word and compared as if it were the one address. (2)returnData.length != 0x20→returnData.length == 0 || returnData.length > 0x20, accepting exactly the1..0x1fregime and nothing else: all 10 pre-existing tests PASS, killed bytestCheckResolvedAddressesShortAnswerRevertswithcall reverted as expected, but without data— the short answer reachedabi.decode, which reverts with no data of its own, the bare undiagnosable revertResolvedAddressReadFailedexists to replace. (3) Reported because it is the obvious mutant and it is NOT load-bearing:→ returnData.length > 0x20is killed by the new short test but ALSO by the pre-existingtestCheckResolvedAddressesUnreadableTargetReverts, since a zero-length answer is<= 0x20too — so it proves nothing about the short regime, which is why mutant (2) was constructed to isolate it. Baseline!= 0x20: both new tests pass.addressis exactly one 32-byte word, so every other answer length is by construction not an address; the docstring atLibRainDeploy.sol:288-290states the error's subject is a read that "answers with something that is not a single address-sized word". The addresses passed to the check are the fuzzer's own inputs, and the expected revert data is rebuilt from the answer bytes the test itself constructed — nothing is read back out of the code under test.(address,address)return, and the short test fuzzes every length1..0x1frather than only a packed 20 bytes. Lengths0and0x20were already covered (testCheckResolvedAddressesUnreadableTargetReverts/testCheckResolvedAddressesRevertingReadReverts, andtestCheckResolvedAddressesMatch), so all four length regimes are now complete.Other checks
forge fmt— no changes.reuse lint— compliant, 72/72 files, including the new mock.forge test— 209 passed, 8 failed. All 8 failures arevm.rollFork/vm.createSelectForkerrors against the local Base endpoint inisStartBlock*/findDeployBlock*, which need an archive node; the same 8 fail identically on cleanmainat86f8d96in this environment, so they are pre-existing and environmental, not from this change. CI binds reachable endpoints via rainix's rpc-preflight. EverycheckResolvedAddressestest passes, including the two fork-basedOnNetworksones.🤖 Generated with Claude Code
Summary by CodeRabbit