Check the hex charset in LibHexString.bytesToHex

src/lib/LibHexString.sol

@@ -4,9 +4,12 @@ pragma solidity ^0.8.25;
 
 import {Vm} from "forge-std-1.16.2/src/Vm.sol";
 
-/// Thrown when the `Vm` handed to `bytesToHex` does not return the string that
-/// `toString(bytes)` is defined to return, which is "0x" followed by exactly two
-/// hexadecimal characters per input byte.
+/// Thrown when the `Vm` handed to `bytesToHex` does not return a string shaped
+/// the way `toString(bytes)` is defined to shape its return, which is "0x"
+/// followed by exactly two lower case hexadecimal characters per input byte.
+/// Only the shape is checked. A return of the right shape that encodes bytes
+/// other than the input cannot be told from a correct one without redoing the
+/// conversion, so it is accepted.
 /// @param hexString The string the `Vm` returned.
 /// @param expectedLength The length the returned string was required to have.
 error UnexpectedHexString(string hexString, uint256 expectedLength);
@@ -21,19 +24,39 @@ library LibHexString {
     /// accept the prefix, such as in `hex"..."` literals.
     ///
     /// Reverts with `UnexpectedHexString` if the `Vm` does not return "0x"
-    /// followed by two hexadecimal characters per input byte. That is what
-    /// foundry's own `Vm` always returns, but `vm` is a parameter, so the string
-    /// the prefix is stripped from is whatever the caller's `Vm` hands back.
-    /// Stripping two characters from a shorter string underflows the length
-    /// word, and stripping them from an unprefixed one silently discards two
-    /// characters of real data into generated source that still compiles.
+    /// followed by two lower case hexadecimal characters per input byte. That is
+    /// what foundry's own `Vm` always returns, but `vm` is a parameter, so the
+    /// string the prefix is stripped from is whatever the caller's `Vm` hands
+    /// back. Stripping two characters from a shorter string underflows the
+    /// length word, stripping them from an unprefixed one silently discards two
+    /// characters of real data into generated source that still compiles, and a
+    /// character outside the hexadecimal charset reaches generated source inside
+    /// a `hex"..."` literal that does not compile, naming the generated file
+    /// rather than the `Vm` that produced it.
     /// @param vm The Vm instance used for conversion.
     /// @param data The bytes array to convert.
     /// @return The hexadecimal string representation of the bytes array.
     function bytesToHex(Vm vm, bytes memory data) internal pure returns (string memory) {
         string memory hexString = vm.toString(data);
         uint256 expectedLength = data.length * 2 + 2;
 
+        // Every character behind the prefix must be a lower case hexadecimal
+        // nibble, which is what `toString(bytes)` is defined to emit. The scan
+        // starts past the prefix, so it does nothing at all on a string too
+        // short to hold one, which the length check below rejects. It reads
+        // through solidity's bounds checked indexing rather than in the
+        // assembly block, because this is a build time function where the gas a
+        // hand rolled scan would save is not a consideration.
+        bytes memory returned = bytes(hexString);
+        bool hexCharset = true;
+        for (uint256 i = 2; i < returned.length; i++) {
+            uint8 c = uint8(returned[i]);
+            if (!((c >= 0x30 && c <= 0x39) || (c >= 0x61 && c <= 0x66))) {
+                hexCharset = false;
+                break;
+            }
+        }
+
         bool stripped;
         assembly ("memory-safe") {
             let len := mload(hexString)
@@ -42,10 +65,11 @@ library LibHexString {
             // at least 2, so a length that matches it guarantees the word read
             // below is within the string's own allocation.
             if eq(len, expectedLength) {
-                // The first two bytes of the data word against "0x". One word
-                // load rather than two indexed byte reads, and it reuses the
-                // pointer the strip already needs.
-                if eq(shr(240, mload(add(hexString, 0x20))), 0x3078) {
+                // The first two bytes of the data word against "0x", alongside
+                // the charset of everything behind them. One word load rather
+                // than two indexed byte reads, and it reuses the pointer the
+                // strip already needs.
+                if and(hexCharset, eq(shr(240, mload(add(hexString, 0x20))), 0x3078)) {
                     // Remove the leading 0x, which solidity does not always
                     // accept — such as in `hex"..."` literals.
                     let newHexString := add(hexString, 2)

test/src/lib/LibHexString.bytesToHex.t.sol

@@ -349,6 +349,91 @@ contract LibHexStringBytesToHexTest is Test {
         iExternal.bytesToHex(badVm, "");
     }
 
+    /// The payload is checked against the hexadecimal charset, not merely
+    /// counted. A return of the right length behind the right prefix whose
+    /// payload is not hexadecimal reaches generated source inside a `hex"..."`
+    /// literal that does not compile, and the compiler names the generated file
+    /// rather than the `Vm` that produced the characters.
+    function testBytesToHexRevertsOnNonHexVmOutput() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0xZZZZ")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0xZZZZ", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// `toString(bytes)` is defined to return lower case, so upper case nibbles
+    /// are a non conforming return even though they name the same bytes.
+    function testBytesToHexRevertsOnUpperCaseVmOutput() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0xAABB")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0xAABB", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// The character immediately after the prefix is checked, so a scan that
+    /// begins one character late does not accept this.
+    function testBytesToHexRevertsOnNonHexFirstPayloadCharacter() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0xZabb")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0xZabb", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// The final character of the payload is checked, so a scan that stops one
+    /// character early does not accept this.
+    function testBytesToHexRevertsOnNonHexLastPayloadCharacter() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0xaabZ")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0xaabZ", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// `/` is the character immediately below `0`, the bottom of the digit
+    /// range, so only the lower bound of that range rejects it.
+    function testBytesToHexRevertsOnCharacterBelowDigitRange() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0x/abb")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0x/abb", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// `:` is the character immediately above `9`, the top of the digit range,
+    /// so only the upper bound of that range rejects it.
+    function testBytesToHexRevertsOnCharacterAboveDigitRange() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0x:abb")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0x:abb", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// A backtick is the character immediately below `a`, the bottom of the
+    /// letter range, so only the lower bound of that range rejects it.
+    function testBytesToHexRevertsOnCharacterBelowLetterRange() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0x`abb")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0x`abb", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// `g` is the character immediately above `f`, the top of the letter range,
+    /// so only the upper bound of that range rejects it.
+    function testBytesToHexRevertsOnCharacterAboveLetterRange() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm badVm = Vm(address(new NonConformingVm("0xgabb")));
+        vm.expectRevert(abi.encodeWithSelector(UnexpectedHexString.selector, "0xgabb", uint256(6)));
+        external_.bytesToHex(badVm, hex"aabb");
+    }
+
+    /// Every character of the hexadecimal charset is accepted, which includes
+    /// the four that sit on the boundaries of the two accepted ranges: `0`, `9`,
+    /// `a` and `f`. The charset check narrows what is accepted, so what it must
+    /// not narrow is pinned alongside it.
+    function testBytesToHexAcceptsEveryHexNibble() external {
+        LibHexStringExternal external_ = new LibHexStringExternal();
+        Vm conformingVm = Vm(address(new NonConformingVm("0x0123456789abcdef")));
+        assertEq(external_.bytesToHex(conformingVm, hex"0011223344556677"), "0123456789abcdef");
+    }
+
     /// The check gates on the shape of the returned string, not on the identity
     /// of the `Vm`. A conforming `Vm` that is not foundry's is still stripped
     /// and returned, so the guard does not quietly narrow the parameter to the
@@ -383,7 +468,11 @@ contract LibHexStringBytesToHexTest is Test {
     /// an unconstructed return conforms 0 times in 2048 runs. `conforming`
     /// picks which half of the property a run aims at, and the payload is
     /// filled from `filler` so that an accepted string is arbitrary in
-    /// everything except the length and prefix the library actually checks.
+    /// everything except the shape the library actually checks. That shape
+    /// includes the hexadecimal charset, which an arbitrary `filler` almost
+    /// never satisfies, so a constructed payload still lands in the rejected
+    /// half most of the time: the accepted half is reached on the order of 55
+    /// runs in 2048 rather than on the order of 1024.
     /// Whether a string conforms is still read off its own characters below, so
     /// a `filler` that happens to conform is checked against the accepted half
     /// rather than expected to revert.
@@ -407,6 +496,13 @@ contract LibHexStringBytesToHexTest is Test {
         // The length equality implies at least 2 characters, so the prefix
         // reads only happen once indexing them is in bounds.
         bool conforms = returned.length == expectedLength && returned[0] == bytes1("0") && returned[1] == bytes1("x");
+        // `toString(bytes)` is defined to emit two lower case hexadecimal
+        // nibbles per input byte, so a payload character outside that charset is
+        // as non conforming as a wrong length or a missing prefix.
+        for (uint256 i = 2; conforms && i < returned.length; i++) {
+            uint8 c = uint8(returned[i]);
+            conforms = (c >= 0x30 && c <= 0x39) || (c >= 0x61 && c <= 0x66);
+        }
 
         if (conforms) {
             bytes memory expected = new bytes(returned.length - 2);