Upgraded Q -> 2 from #298 [1683709930306]

[c4-judge]

Judge has assessed an item in Issue #298 as 2 risk. The relevant finding follows:

[L-01] Valid hex string is not decoded correctly by hexStringToBytes32 and reads memory out-of-boundary Links https://github.com/code-423n4/2023-04-ens/blob/main/contracts/utils/HexUtils.sol#L11-L60

https://github.com/code-423n4/2023-04-ens/blob/45ea10bacb2a398e14d711fe28d1738271cd7640/contracts/utils/HexUtils.sol#L47-L50

Impact Valid hexadecimal strings are not decoded correctly. Decoding reads out-of-bounds memory returning undefined/unpreditcable values.

At this time of writing I am not sure whether the undefined / unpredictable values may be predictably injected by an attacker and therefore and/or constitute a more serious security issue or just wrong / fail decoding.

Proof of Concept The hexStringToBytes32 function reads an input string and converts it to a bytes32 value.

"aaaa", "aaa", "aa", or "a" (or "AAAA", "AAA", "AA", or "A") are all valid hexadecimal values, but due to the way the decoding loop is implemented, when the input string (or the selected part of it) is an odd number of characters, the decoding produces mixed, wrong, and undefined results.

Below is an example of decoding an odd number of characters out of an even number of characters and resulting in incorrect output (note that the idx and lastIdx are respectively the "cursor" start and end, therefore 0, 3 oi A to C, while 1, 4 is B to D).

hexStringToBytes32(bytes("ABCD"), 0, 2); // produces ( 0xab, true) - correct hexStringToBytes32(bytes("ABCD"), 1, 3); // produces ( 0xbc, true) - correct

hexStringToBytes32(bytes("ABCD"), 0, 3); // produces (0xabcd, true) - incorrect hexStringToBytes32(bytes("ABCD"), 1, 4); // produces ( 0xbc, false) - incorrect

The first and second examples works as expected, but the third and fourth does not.

In the third example hexStringToBytes32(bytes("ABCD"), 0, 3) four characters are decoded instead of three, and the output is 0xabcd instead of 0xabc because each loop iteration reads two nibbles in sequence and then increment the index by two, so instead of stopping at "C" it also reads the non-expected "D" character and adds it to the result.

In the fourth example hexStringToBytes32(bytes("ABCD"), 1, 4); two characters are decoded instead of three, and the output is 0xbc instead of 0xbcd because each loop reads two nibbles in sequence per iteration and then increment the index by two, so instead of stopping at "D" it reads some extra value out-of-bounds and the value is undefined. In this case it happens to be some value that the getHex discards as non in the ascii range 0-9, A-F, a-f and therefore terminates the loop, but the result may vary.

Tools Used n/a

Recommended Mitigation Steps Correct the hexStringToBytes32 function by replacing it with the following:

function hexStringToBytes32(bytes memory str, uint256 idx, uint256 lastIdx) internal pure returns (bytes32 r, bool valid) { valid = true; assembly { // check that the index to read to is not past the end of the string if gt(lastIdx, mload(str)) { revert(0, 0) }

    function getHex(c) -> ascii {
        // chars 48-57: 0-9
        if and(gt(c, 47), lt(c, 58)) {
            ascii := sub(c, 48)
            leave
        }
        // chars 65-70: A-F
        if and(gt(c, 64), lt(c, 71)) {
            ascii := add(sub(c, 65), 10)
            leave
        }
        // chars 97-102: a-f
        if and(gt(c, 96), lt(c, 103)) {
            ascii := add(sub(c, 97), 10)
            leave
        }
        // invalid char
        ascii := 0xff
    }      

    for {
        let ptr := add(str, add(32, idx))
        let ptrEnd := add(str, add(32, lastIdx))
    } lt(ptr, ptrEnd) {
        ptr := add(ptr, 1)
    } {
        let nibble := getHex(byte(0, mload(ptr)))                
        if eq(nibble, 0xff) {
            valid := false
            break
        }
        r := or(shl(4, r), nibble)                
    }
}

} I would also add a stricter check to the indexes so that idx cannot be greater than lastIdx and also so that idx cannot be equal to lastIdx if we consider an empty string an invalid string.

if or(gt(lastIdx, mload(str)), or(gt(idx, lastIdx), eq(idx, lastIdx))) { revert(0, 0) }

[c4-judge]

dmvt marked the issue as duplicate of #281

[c4-judge]

dmvt marked the issue as satisfactory