test: signed context length is bound by the signature; ABI decoder rejects overstated context lengths - #132
Conversation
…jects overstated context lengths Adds fuzz tests that a signature over `[x]` does not authenticate `[x, y]`, `[x, 0]` or `[]`, and that a signature over `[x, y]` does not authenticate `[x]`, each reverting `InvalidSignature(0)`. The signing digest is computed in the test without `LibHashNoAlloc`. Adds calldata tests for `buildExternal` that patch the `context.length` word by an offset derived from the ABI head/tail layout: a length past the end of the calldata fails in the decoder with empty return data; a length of 2^59 or more fails with the allocation panic; a length that overstates the context but stays inside the calldata decodes (reading the signature tail as context words) and is then rejected by the signature; a length of zero decodes with the context word as trailing calldata and presents `[]`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvfmeRQKubdbFW1GL3kmib
`slither .` builds with `--skip ./test/**`, so the annotations on the raw calls in the test contract have nothing to act on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvfmeRQKubdbFW1GL3kmib
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test suite adds signature helpers and fuzz tests for signed-context mutations. It also builds calldata, patches encoded context lengths, and verifies decoder errors, memory overflow, signature failures, and successful presentation of an empty context. ChangesSigned Context Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This adds coverage for signed-context validation and ABI length decoding without changing production behavior. The current test changes are ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ 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 |
|
@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:
|
Refs rainlanguage/rain.lib.hash#22
LibContext.buildverifies each signed context againsttoEthSignedMessageHash(LibHashNoAlloc.hashWords(context)).hashWordshashes32 * lengthbytes with no length prefix, so the context length is bound to the signature only because a differentlengthhashes a different number of bytes. Nothing in the suite pinned that, and nothing pinned what the ABI decoder does when thecontext.lengthword in the calldata disagrees with the words actually present. This PR adds both, intest/src/lib/caller/LibContext.t.sol.Length is part of what is signed (fuzzed
x,y; each revertsInvalidSignature(0)):testBuildSignedContextAppendedWordReverts— signed[x], presented[x, y]testBuildSignedContextAppendedZeroReverts— signed[x], presented[x, 0]testBuildSignedContextEmptiedReverts— signed[x], presented[]testBuildSignedContextTruncatedReverts— signed[x, y], presented[x]testBuildStructureReferenceImplementationand signed context 0 oftestBuildInvalidSignatureSecondIndex; cited from the tests, not duplicated.The ABI decoder and an overstated / understated
context.length— calldata forbuildExternal(bytes32[][], SignedContextV1[])is built withabi.encodeCalland the singlecontext.lengthword is patched at an offset derived by following the head/tail offset words (contextLengthOffset, layout documented inline), then sent withaddress(this).call:testBuildCalldataUnpatchedSucceeds— positive control: the unpatched payload builds[base, signers, [x]].testBuildCalldataContextLengthBeyondCalldataReverts— length in[wordsToEnd + 1, 2^58](more words than remain in the calldata): the decoder reverts with empty return data.testBuildCalldataContextLengthUnallocatableReverts— length in[2^59, 2^256 - 1]: the decoder reverts withPanic(0x41)(allocation check runs before the calldata bound).testBuildCalldataContextLengthOverstatedWithinCalldataReverts— length in[2, wordsToEnd](overstates the context but not the calldata): the decoder accepts it, reading the signature length/bytes behind the context word as context words;buildthen revertsInvalidSignature(0).testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts— length 0 withxstill encoded: decodes, presented context is[], signature over[x]rejected withInvalidSignature(0).testBuildCalldataContextLengthUnderstatedPresentsShorterContext— same calldata with a signature over[]: builds, and the signed column is[].Decoder behaviour worth knowing (pinned by the tests above): the decoder bounds an array by the end of the whole calldata, not by the end of the value it belongs to. For this payload the 65-byte signature tail (length word + 3 words) sits behind the single context word, so
context.lengthup to 5 decodes and only 6+ is rejected; "length 4 with one word present" is therefore caught by the signature, not the decoder. And a huge length (2^200) is not an empty revert butPanic(0x41).QA
testBuildSignedContextAppendedWordReverts,testBuildSignedContextAppendedZeroReverts,testBuildSignedContextEmptiedReverts,testBuildSignedContextTruncatedReverts,testBuildCalldataUnpatchedSucceeds,testBuildCalldataContextLengthBeyondCalldataReverts,testBuildCalldataContextLengthUnallocatableReverts,testBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts,testBuildCalldataContextLengthUnderstatedPresentsShorterContext— the code under test is unchanged and correct, so these pass on base (forge teston main 20b2343: 118 passed; on this branch: 128 passed); discrimination is shown by the mutation table below (verified withmutation-probe, baseline green, every restore byte-verified).src/lib/caller/LibContext.solline 207 isLibHashNoAlloc.hashWords(signedContexts[i].context); line 212revert InvalidSignature(i);; line 204!SignatureChecker.isValidSignatureNow(;test/src/lib/caller/LibContext.t.solcontextLengthOffsetreturn line andcontextWordsToEndreturn line):keccak256(abi.encodePacked(signedContexts[i].context[0]))(length ignored) → KILLED bytestBuildSignedContextAppendedWordReverts,testBuildSignedContextAppendedZeroReverts,testBuildSignedContextEmptiedReverts(panic on[]),testBuildStructureReferenceImplementation(existing), and the calldata teststestBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts,testBuildCalldataContextLengthUnderstatedPresentsShorterContext.keccak256(abi.encodePacked(context.length > 0 ? context[0] : bytes32(0)))(length ignored, no panic) → KILLED bytestBuildSignedContextAppendedWordReverts,testBuildSignedContextAppendedZeroReverts,testBuildSignedContextEmptiedReverts,testBuildStructureReferenceImplementation(existing),testBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnderstatedPresentsShorterContext.[x]and[x, 0]hash alike) → KILLED bytestBuildSignedContextTruncatedReverts(the fuzzer'sy = 0case),testBuildInvalidSignatureSecondIndexandtestBuildStructureReferenceImplementation(existing),testBuildCalldataUnpatchedSucceeds,testBuildCalldataContextLengthUnderstatedPresentsShorterContext. Not killed bytestBuildSignedContextAppendedZeroRevertson its own: that test signs the real digest of[x], which no verifier mutant reproduces from[x, 0].revert InvalidSignature(i);removed → KILLED bytestBuildSignedContextAppendedWordReverts,testBuildSignedContextAppendedZeroReverts,testBuildSignedContextEmptiedReverts,testBuildSignedContextTruncatedReverts,testBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts, and the existingtestBuildInvalidSignatureEmpty,testBuildInvalidSignatureReverts,testBuildInvalidSignatureSecondIndex,testBuildInvalidSignatureWrongContext.testBuildSignedContextAppendedWordReverts,testBuildSignedContextAppendedZeroReverts,testBuildSignedContextEmptiedReverts,testBuildSignedContextTruncatedReverts,testBuildCalldataUnpatchedSucceeds,testBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts,testBuildCalldataContextLengthUnderstatedPresentsShorterContext, and the existingtestBuildInvalidSignatureEmpty,testBuildInvalidSignatureReverts,testBuildInvalidSignatureSecondIndex,testBuildInvalidSignatureWrongContext.contextLengthOffsetreturn+ 0x20(patch lands oncontext[0]) → KILLED bytestBuildCalldataUnpatchedSucceeds(pre-patch word is not1),testBuildCalldataContextLengthBeyondCalldataReverts(getsInvalidSignature(0), not empty),testBuildCalldataContextLengthUnallocatableReverts(getsInvalidSignature(0), notPanic(0x41)),testBuildCalldataContextLengthUnderstatedPresentsShorterContext(presented[0], not[]).contextLengthOffsetreturn- 0x20(patch lands on thesignatureoffset word) → KILLED bytestBuildCalldataContextLengthBeyondCalldataReverts,testBuildCalldataContextLengthOverstatedWithinCalldataReverts,testBuildCalldataContextLengthUnallocatableReverts,testBuildCalldataContextLengthUnderstatedPresentsShorterContext,testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts.contextWordsToEnd+ 1(decoder boundary moved up one word) → KILLED bytestBuildCalldataContextLengthOverstatedWithinCalldataReverts(lengthwordsToEnd + 1is an empty decoder revert, notInvalidSignature(0)).contextWordsToEnd- 1(boundary moved down one word) → KILLED bytestBuildCalldataContextLengthBeyondCalldataReverts(lengthwordsToEnddecodes and revertsInvalidSignature(0), not empty).forge test, baseline 128 passed). Killer names above are the union of the full-suite run (which reports at most five killers per mutant) and re-runs of M01–M05 with the suite narrowed to the signed-context and invalid-signature tests.MessageHashUtils.toEthSignedMessageHash(keccak256(abi.encodePacked(words))), not viaLibHashNoAlloc; decoder behaviour taken from the raw(success, returnData)ofaddress(this).call(payload)and compared against literal ABI encodings (abi.encodeWithSelector(InvalidSignature.selector, 0),stdError.memOverflowError, empty bytes); the derived patch offset is asserted to hold the pre-patch length1before it is overwritten, and the positive control decodes the returned context.[x, y],[],[x, 0], prefix of[x, y], positive control cited; (2) ABI decoder vs an overstatedcontext.length(beyond the calldata; 2^200) with an unpatched positive control, and an understated length with the actual behaviour pinned. Covered (1), (2); the brief's "length 4 with one word present" lands in the decoder-accepts regime for this layout and is pinned as such.🤖 Generated with Claude Code
https://claude.ai/code/session_01FvfmeRQKubdbFW1GL3kmib
Summary by CodeRabbit