fix(LibConvert): unsafeTo16BitBytes reverts on a length it cannot double - #25
thedavidmeister wants to merge 2 commits into
Conversation
The NatSpec gives the caller exactly one obligation, and it is about the values: they must fit in `type(uint16).max` or silent overflow must be safe. It says nothing about `us.length`. The whole body was `unchecked` though, which put the allocation size under that too, so a `us` whose length prefix cannot be doubled without wrapping was packed as the wrapped count instead: a forged prefix claiming `2 ** 255 + 3` elements returned a well formed six byte result and no error. The only arithmetic the `unchecked` block guarded was the allocation size, so the block goes and the multiply becomes checked. A length that cannot be doubled is now Panic(0x11) rather than a shorter array. Nothing Solidity can build is affected: the check first fires at `2 ** 255`, and `new bytes` already panics 0x41 from `2 ** 63` up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Limit details: You’ve used the included review currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (2)
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 |
…length-overflow-reverts # Conflicts: # src/LibConvert.sol
|
Closing unmerged — #21 closed as by-design. The premise this PR rests on does not hold: a length prefix that exceeds its allocation cannot come from The measurement work stands and is worth keeping in the record: the silent window was exactly Branch left in place. |
1 participant
Closes #21.
The NatSpec of
unsafeTo16BitByteshands the caller exactly one obligation, andit is about the values:
Nothing is said about
us.length. The body was wrapped inunchecked, which putthe allocation size under that same silence, so the code was relying on a length
obligation the docs never handed over.
us.length * 2wrapped, and auswhoselength prefix cannot be doubled was packed as the wrapped count: the issue's
repro, three real elements behind a prefix claiming
2 ** 255 + 3, returned0xaaaabbbbcccc— six bytes, no error, a well formed result of the wrong length.This is option (a) from the issue. The
uncheckedblock guarded exactly oneexpression, the allocation size, so the block goes and the multiply is checked. A
length prefix that cannot be doubled is now
Panic(0x11).Chosen over option (b), documenting a length precondition, because the caller
cannot discharge that obligation more cheaply than the library can check it, and
because widening the documented unsafety of a function whose name promises one
specific unsafety trades a silent wrong answer for a silent wrong answer the
caller has been told to expect. Rejecting a length prefix it cannot honour is the
library's job, not the caller's.
Panic(0x11)rather than a named error because a memory array whose lengthprefix does not match its own allocation is an invariant violation, not caller
input to validate, and because it costs no new API surface in a library that
currently declares no errors at all.
Reachability, measured on this tree rather than argued
new bytes(n)already panics0x41forn > type(uint64).max, so onmainevery forged length from
2 ** 63up already reverted, and below that memoryexpansion runs out of gas first. The silent window was exactly
us.length >= 2 ** 255, where the doubling wraps back down into a size thatallocates — which is exactly the window the checked multiply closes. Nothing
Solidity itself can build is affected, and no honest caller can reach the check.
The memory-corruption reading recorded as REFUTED in the issue stays refuted;
this PR does not re-raise it. The loop bound
mul(mload(us), 0x20)still wrapsin principle, at
2 ** 251, and is still unreachable — now for a strongerreason than "it wraps by the same factor as the allocation": the allocation above
it reverts first, from
2 ** 63. That is now a comment on the line rather thanonly a paragraph in a closed issue.
Gas
gasleft()either side of the call, solc 0.8.25, optimizer 100000 runs. "main"here is the pre-fix
unsafeTo16BitBytes; #24 has landed since and changes onlyNatSpec, so the bytecode being compared against is unchanged:
us.length+86 gas, flat in length. The cheaper shape —
if (us.length > type(uint256).max / 2) revert SomeError();with the multiplyleft
unchecked— measured +31 instead. I did not take it: 55 gas is not wortha new error type in this library's surface, and
Panic(0x11)is the right revertfor a corrupt length prefix. Happy to switch if the 55 gas is wanted.
QA
Discriminating tests:
testUnsafeTo16BitBytesForgedLengthOverflowReverts(uint256[],uint256)fuzzesthe forged prefix over the whole silent window,
[2 ** 255, type(uint256).max];testUnsafeTo16BitBytesForgedLengthOverflowRevertsForThreeRealElements()pinsthe issue's exact repro. Both assert
stdError.arithmeticError, i.e.Panic(0x11)specifically, not a bare revert — a mutant that reverts withPanic(0x41)instead still fails them. Both forge the prefix behind anexternal call boundary, because
expectRevertneeds a call to watch and anarray claiming more elements than it has cannot be ABI encoded across one.
Mutations applied: 2 against
src/LibConvert.solvianix run github:rainlanguage/adversarial-mutation-test#mutation-probe -- mutants.toml,baseline green at 17 passed,
2/2 killed; survived: 0; no-run: 0; harness errors: 0. M01 restores the pre-fix code exactly (bytes memory bs;+unchecked { bs = new bytes(us.length * 2); }); M02 reaches the same wrap by aroute that needs no
uncheckedat all (new bytes(us.length << 1)), so a testthat pinned the keyword rather than the behaviour would survive it. Both are
killed by both new tests. Re-run on the merged tree after docs(LibConvert): the toX convention marks cost, not consumption #24 landed, same
result.
Oracle: the function's own NatSpec, which names one unsafety and puts it on the
values. A length obligation that appears in the code and nowhere in the docs is
the defect; the fix is to make the code stop needing it, and the tests assert
the revert that follows from that, not the implementation that produces it.
Category check: arithmetic/off-by-one (the allocation size wrapping) is the
category the issue reports and the one fixed here. Re-checked for the same
shape elsewhere in the repo:
unsafeToBytes'smul(0x20, mload(bs))is Yuland wraps at
2 ** 251, but it rewrites a length prefix in place and allocatesnothing, so a wrapped result is a corrupt
bytesthe caller was already toldis unsafe to use — out of scope for LibConvert.unsafeTo16BitBytes sizes its allocation with an unchecked multiply, so a forged length truncates silently instead of reverting #21 and not touched.
LibCastdoes noarithmetic at all. No other
uncheckedblock exists insrc.Static analysis:
forge fmt --check,slither .(2 contracts, 99 detectors,0 results) and the full suite green locally in the rainix shell, on the merged
tree.
#24 (the fix for #22) landed on
mainwhile this was open, touching the sameNatSpec block and the same test file.
mainis merged in here and both sides arekept: its "the truncation is the only unsafety / does NOT consume
us" paragraphand this PR's length-obligation paragraph both stand, in that order after the
values paragraph, and its reworded comment in
testUnsafeTo16BitBytesReferenceImplementationis untouched. Suite green at 17tests on the merged tree; every number above is measured there.
🤖 Generated with Claude Code