From d4fe3f24d25fc9ded66dc4b055c66a87bdf45302 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Fri, 21 Aug 2026 14:37:15 +0000 Subject: [PATCH] fix(convert): unsafeTo16BitBytes reverts on a length it cannot double 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) --- src/LibConvert.sol | 50 +++++++++++++++++++++++++++---------------- test/LibConvert.t.sol | 42 +++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 19 deletions(-) diff --git a/src/LibConvert.sol b/src/LibConvert.sol index e24d514..4e48d5b 100644 --- a/src/LibConvert.sol +++ b/src/LibConvert.sol @@ -32,28 +32,42 @@ library LibConvert { /// values are not checked for overflow due to the truncation. The caller /// MUST ensure that all values fit in `type(uint16).max` or that silent /// overflow is safe. + /// + /// The values are the caller's only obligation; the length is this + /// library's. `us.length` is doubled with a checked multiply, so a length + /// prefix that cannot be doubled without wrapping reverts, rather than + /// packing the wrapped count of elements and handing back a well formed + /// result of the wrong length. Solidity cannot build an array long enough + /// to reach that, but this library is used from assembly that builds memory + /// arrays by hand, and a forged or corrupt length prefix is an error rather + /// than a shorter array. + /// /// @param us The `uint256[]` to truncate and concatenate to 16 bit `bytes`. /// @return The concatenated 2-byte chunks. function unsafeTo16BitBytes(uint256[] memory us) internal pure returns (bytes memory) { - unchecked { - // We will keep 2 bytes (16 bits) from each integer. - bytes memory bs = new bytes(us.length * 2); - assembly ("memory-safe") { - let replaceMask := 0xFFFF - let preserveMask := not(replaceMask) - for { - let cursor := add(us, 0x20) - let end := add(cursor, mul(mload(us), 0x20)) - let bytesCursor := add(bs, 0x02) - } lt(cursor, end) { - cursor := add(cursor, 0x20) - bytesCursor := add(bytesCursor, 0x02) - } { - let data := mload(bytesCursor) - mstore(bytesCursor, or(and(preserveMask, data), and(replaceMask, mload(cursor)))) - } + // Deliberately NOT `unchecked`. The allocation size is the only + // arithmetic in this function, and wrapping it is exactly the silent + // truncation the docs above rule out. + // We will keep 2 bytes (16 bits) from each integer. + bytes memory bs = new bytes(us.length * 2); + assembly ("memory-safe") { + let replaceMask := 0xFFFF + let preserveMask := not(replaceMask) + for { + let cursor := add(us, 0x20) + // Yul `mul` wraps, but `mload(us)` cannot reach a length that + // makes it wrap. The allocation above already reverted for any + // length at or above 2**63, and this wraps only at 2**251. + let end := add(cursor, mul(mload(us), 0x20)) + let bytesCursor := add(bs, 0x02) + } lt(cursor, end) { + cursor := add(cursor, 0x20) + bytesCursor := add(bytesCursor, 0x02) + } { + let data := mload(bytesCursor) + mstore(bytesCursor, or(and(preserveMask, data), and(replaceMask, mload(cursor)))) } - return bs; } + return bs; } } diff --git a/test/LibConvert.t.sol b/test/LibConvert.t.sol index fc86f30..8ec8c69 100644 --- a/test/LibConvert.t.sol +++ b/test/LibConvert.t.sol @@ -2,7 +2,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity =0.8.25; -import {Test} from "forge-std-1.16.1/src/Test.sol"; +import {Test, stdError} from "forge-std-1.16.1/src/Test.sol"; import {LibConvert} from "../src/LibConvert.sol"; import {LibConvertSlow} from "./LibConvertSlow.sol"; @@ -70,6 +70,46 @@ contract LibConvertTest is Test { } } + /// The NatSpec puts an obligation on the caller about the values and none + /// about the length, so a length prefix that cannot be doubled without + /// wrapping has to revert rather than pack the wrapped count of elements and + /// hand back a well formed result of the wrong length. + function testUnsafeTo16BitBytesForgedLengthOverflowReverts(uint256[] memory us, uint256 forgedLength) external { + forgedLength = bound(forgedLength, 2 ** 255, type(uint256).max); + vm.expectRevert(stdError.arithmeticError); + this.unsafeTo16BitBytesWithForgedLength(us, forgedLength); + } + + /// Three real elements behind a length prefix claiming `2 ** 255 + 3`, which + /// used to return the six bytes of the three real elements as though the + /// claim had been honoured. + function testUnsafeTo16BitBytesForgedLengthOverflowRevertsForThreeRealElements() external { + uint256[] memory us = new uint256[](3); + us[0] = 0xAAAA; + us[1] = 0xBBBB; + us[2] = 0xCCCC; + + vm.expectRevert(stdError.arithmeticError); + this.unsafeTo16BitBytesWithForgedLength(us, (2 ** 255) + 3); + } + + /// Forging the length prefix has to happen behind an external call boundary. + /// `expectRevert` needs a call to watch, and an array claiming more elements + /// than it has cannot be ABI encoded to get across one, so the forgery is + /// done on this side of it. The assembly is deliberately not annotated + /// `memory-safe`: it leaves `us` inconsistent with its own allocation, which + /// is the whole point of the test. + function unsafeTo16BitBytesWithForgedLength(uint256[] memory us, uint256 forgedLength) + external + pure + returns (bytes memory) + { + assembly { + mstore(us, forgedLength) + } + return LibConvert.unsafeTo16BitBytes(us); + } + /// The packing loop writes whole words at two byte offsets, so it has to /// stop before it runs off the end of what it allocated. Comparing the /// result value alone cannot see a write past that end, because such a write