diff --git a/src/LibConvert.sol b/src/LibConvert.sol index 4abc42f..8d60e24 100644 --- a/src/LibConvert.sol +++ b/src/LibConvert.sol @@ -41,6 +41,15 @@ library LibConvert { /// 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. + /// /// The truncation is the only unsafety. This does NOT consume `us`. The /// result is a freshly allocated buffer and `us` is only read, so `us` is /// still a valid `uint256[]` after the call and the caller may keep using @@ -50,25 +59,30 @@ library LibConvert { /// @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 would silently pack a + // different count of elements than `us` claims, which 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 94693ae..54c9ea7 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"; @@ -71,6 +71,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