fix(LibConvert): unsafeTo16BitBytes reverts on a length it cannot double

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;
     }
 }

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