Skip to content

test: signed context length is bound by the signature; ABI decoder rejects overstated context lengths - #132

Merged
thedavidmeister merged 2 commits into
mainfrom
2026-09-03-signed-context-length-and-decoder-tests
Sep 3, 2026
Merged

test: signed context length is bound by the signature; ABI decoder rejects overstated context lengths#132
thedavidmeister merged 2 commits into
mainfrom
2026-09-03-signed-context-length-and-decoder-tests

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Refs rainlanguage/rain.lib.hash#22

LibContext.build verifies each signed context against toEthSignedMessageHash(LibHashNoAlloc.hashWords(context)). hashWords hashes 32 * length bytes with no length prefix, so the context length is bound to the signature only because a different length hashes a different number of bytes. Nothing in the suite pinned that, and nothing pinned what the ABI decoder does when the context.length word in the calldata disagrees with the words actually present. This PR adds both, in test/src/lib/caller/LibContext.t.sol.

Length is part of what is signed (fuzzed x, y; each reverts InvalidSignature(0)):

  • testBuildSignedContextAppendedWordReverts — signed [x], presented [x, y]
  • testBuildSignedContextAppendedZeroReverts — signed [x], presented [x, 0]
  • testBuildSignedContextEmptiedReverts — signed [x], presented []
  • testBuildSignedContextTruncatedReverts — signed [x, y], presented [x]
  • Positive control (a signature does authenticate exactly what it signed) is the existing testBuildStructureReferenceImplementation and signed context 0 of testBuildInvalidSignatureSecondIndex; cited from the tests, not duplicated.

The ABI decoder and an overstated / understated context.length — calldata for buildExternal(bytes32[][], SignedContextV1[]) is built with abi.encodeCall and the single context.length word is patched at an offset derived by following the head/tail offset words (contextLengthOffset, layout documented inline), then sent with address(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 with Panic(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; build then reverts InvalidSignature(0).
  • testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts — length 0 with x still encoded: decodes, presented context is [], signature over [x] rejected with InvalidSignature(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.length up 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 but Panic(0x41).

QA

  • Discriminating tests: testBuildSignedContextAppendedWordReverts, testBuildSignedContextAppendedZeroReverts, testBuildSignedContextEmptiedReverts, testBuildSignedContextTruncatedReverts, testBuildCalldataUnpatchedSucceeds, testBuildCalldataContextLengthBeyondCalldataReverts, testBuildCalldataContextLengthUnallocatableReverts, testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts, testBuildCalldataContextLengthUnderstatedPresentsShorterContext — the code under test is unchanged and correct, so these pass on base (forge test on main 20b2343: 118 passed; on this branch: 128 passed); discrimination is shown by the mutation table below (verified with mutation-probe, baseline green, every restore byte-verified).
  • Mutations applied (src/lib/caller/LibContext.sol line 207 is LibHashNoAlloc.hashWords(signedContexts[i].context); line 212 revert InvalidSignature(i);; line 204 !SignatureChecker.isValidSignatureNow(; test/src/lib/caller/LibContext.t.sol contextLengthOffset return line and contextWordsToEnd return line):
    • M01 line 207 → keccak256(abi.encodePacked(signedContexts[i].context[0])) (length ignored) → KILLED by testBuildSignedContextAppendedWordReverts, testBuildSignedContextAppendedZeroReverts, testBuildSignedContextEmptiedReverts (panic on []), testBuildStructureReferenceImplementation (existing), and the calldata tests testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts, testBuildCalldataContextLengthUnderstatedPresentsShorterContext.
    • M02 line 207 → keccak256(abi.encodePacked(context.length > 0 ? context[0] : bytes32(0))) (length ignored, no panic) → KILLED by testBuildSignedContextAppendedWordReverts, testBuildSignedContextAppendedZeroReverts, testBuildSignedContextEmptiedReverts, testBuildStructureReferenceImplementation (existing), testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnderstatedPresentsShorterContext.
    • M03 line 207 → keccak of a fixed zero-padded 2-word prefix ([x] and [x, 0] hash alike) → KILLED by testBuildSignedContextTruncatedReverts (the fuzzer's y = 0 case), testBuildInvalidSignatureSecondIndex and testBuildStructureReferenceImplementation (existing), testBuildCalldataUnpatchedSucceeds, testBuildCalldataContextLengthUnderstatedPresentsShorterContext. Not killed by testBuildSignedContextAppendedZeroReverts on its own: that test signs the real digest of [x], which no verifier mutant reproduces from [x, 0].
    • M04 line 212 revert InvalidSignature(i); removed → KILLED by testBuildSignedContextAppendedWordReverts, testBuildSignedContextAppendedZeroReverts, testBuildSignedContextEmptiedReverts, testBuildSignedContextTruncatedReverts, testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts, and the existing testBuildInvalidSignatureEmpty, testBuildInvalidSignatureReverts, testBuildInvalidSignatureSecondIndex, testBuildInvalidSignatureWrongContext.
    • M05 line 204 check inverted → KILLED by testBuildSignedContextAppendedWordReverts, testBuildSignedContextAppendedZeroReverts, testBuildSignedContextEmptiedReverts, testBuildSignedContextTruncatedReverts, testBuildCalldataUnpatchedSucceeds, testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts, testBuildCalldataContextLengthUnderstatedPresentsShorterContext, and the existing testBuildInvalidSignatureEmpty, testBuildInvalidSignatureReverts, testBuildInvalidSignatureSecondIndex, testBuildInvalidSignatureWrongContext.
    • M06 test contextLengthOffset return + 0x20 (patch lands on context[0]) → KILLED by testBuildCalldataUnpatchedSucceeds (pre-patch word is not 1), testBuildCalldataContextLengthBeyondCalldataReverts (gets InvalidSignature(0), not empty), testBuildCalldataContextLengthUnallocatableReverts (gets InvalidSignature(0), not Panic(0x41)), testBuildCalldataContextLengthUnderstatedPresentsShorterContext (presented [0], not []).
    • M07 test contextLengthOffset return - 0x20 (patch lands on the signature offset word) → KILLED by testBuildCalldataContextLengthBeyondCalldataReverts, testBuildCalldataContextLengthOverstatedWithinCalldataReverts, testBuildCalldataContextLengthUnallocatableReverts, testBuildCalldataContextLengthUnderstatedPresentsShorterContext, testBuildCalldataContextLengthUnderstatedSignatureOverLongerReverts.
    • M08 test contextWordsToEnd + 1 (decoder boundary moved up one word) → KILLED by testBuildCalldataContextLengthOverstatedWithinCalldataReverts (length wordsToEnd + 1 is an empty decoder revert, not InvalidSignature(0)).
    • M09 test contextWordsToEnd - 1 (boundary moved down one word) → KILLED by testBuildCalldataContextLengthBeyondCalldataReverts (length wordsToEnd decodes and reverts InvalidSignature(0), not empty).
    • Full-suite probe: 9/9 killed, 0 survived, 0 no-run (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.
  • Oracle: signing digest computed in the test as MessageHashUtils.toEthSignedMessageHash(keccak256(abi.encodePacked(words))), not via LibHashNoAlloc; decoder behaviour taken from the raw (success, returnData) of address(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 length 1 before it is overwritten, and the positive control decodes the returned context.
  • Category check: brief asks (1) length is part of what is signed — [x, y], [], [x, 0], prefix of [x, y], positive control cited; (2) ABI decoder vs an overstated context.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

  • Tests
    • Expanded coverage for signed-context length validation and calldata patching.
    • Added checks for appended, zero-extended, emptied, and truncated contexts.
    • Added validation for modified context lengths, including expected decoder and signature errors.
    • Added coverage confirming valid signatures succeed when context lengths are understated consistently.

thedavidmeister and others added 2 commits September 3, 2026 15:35
…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
@thedavidmeister thedavidmeister self-assigned this Sep 3, 2026
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: a9e5ee06-1dce-42d8-9681-d35896ddb1c5

📥 Commits

Reviewing files that changed from the base of the PR and between 20b2343 and 12bec86.

📒 Files selected for processing (1)
  • test/src/lib/caller/LibContext.t.sol

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

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

Changes

Signed Context Validation

Layer / File(s) Summary
Signature matching tests
test/src/lib/caller/LibContext.t.sol
New helpers sign word sequences and construct signed contexts. Fuzz tests reject appended, emptied, and truncated contexts.
Calldata construction and baseline
test/src/lib/caller/LibContext.t.sol
Assembly helpers build and inspect calldata offsets. A baseline test validates the unpatched calldata result.
Context length validation
test/src/lib/caller/LibContext.t.sol
Tests patch encoded context lengths and verify decoder rejection, memory overflow, signature failure, and successful presentation of an empty context.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 12bec

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)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: signed context length validation and ABI decoder rejection of overstated context lengths.
Docstring Coverage ✅ Passed 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…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

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)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-09-03-signed-context-length-and-decoder-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.

@thedavidmeister
thedavidmeister merged commit cf122c6 into main Sep 3, 2026
4 checks passed
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

@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:

  • Simple bug fixes, typos, or minor refactoring
  • Single-purpose changes affecting 1-2 files
  • Documentation updates
  • Configuration tweaks
  • Changes that require minimal context to review

Review Effort: Would have taken 5-10 minutes

Examples:

  • Fix typo in variable name
  • Update README with new instructions
  • Adjust configuration values
  • Simple one-line bug fixes
  • Import statement cleanup

Medium (M)

Characteristics:

  • Feature additions or enhancements
  • Refactoring that touches multiple files but maintains existing behavior
  • Breaking changes with backward compatibility
  • Changes requiring some domain knowledge to review

Review Effort: Would have taken 15-30 minutes

Examples:

  • Add new feature or component
  • Refactor common utility functions
  • Update dependencies with minor breaking changes
  • Add new component with tests
  • Performance optimizations
  • More complex bug fixes

Large (L)

Characteristics:

  • Major feature implementations
  • Breaking changes or API redesigns
  • Complex refactoring across multiple modules
  • New architectural patterns or significant design changes
  • Changes requiring deep context and multiple review rounds

Review Effort: Would have taken 45+ minutes

Examples:

  • Complete new feature with frontend/backend changes
  • Protocol upgrades or breaking changes
  • Major architectural refactoring
  • Framework or technology upgrades

Additional Factors to Consider

When deciding between sizes, also consider:

  • Test coverage impact: More comprehensive test changes lean toward larger classification
  • Risk level: Changes to critical systems bump up a size category
  • Team familiarity: Novel patterns or technologies increase complexity

Notes:

  • the assessment must be for the totality of the PR, that means comparing the base branch to the last commit of the PR
  • the assessment output must be exactly one of: S, M or L (single-line comment) in format of: SIZE={S/M/L}
  • do not include any additional text, only the size classification
  • your assessment comment must not include tips or additional sections
  • do NOT tag me or anyone else on your comment

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