Upgraded Q -> 2 from #298 [1683710120837]

[c4-judge]

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

[L-03] Redundant and dangerous len parameter in readKeyValue

Links

https://github.com/code-423n4/2023-04-ens/blob/45ea10bacb2a398e14d711fe28d1738271cd7640/contracts/dnsregistrar/RecordParser.sol#L14-L40

Impact

If the len is not set to input.length minus the offset, there may be unpredictable results due how the algorithm works.

Proof of Concept

Let's consider the following three inputs that will be parsed by the readKeyValue with an offset of zero:

1. bytes("aaa=bbb")
2. bytes("aaa=bbb ")
3. bytes("aaa=bbb ccc=ddd")

The first input will be correctly and integrally parsed for any len that goes past the equal sign (len >= 4) because terminator is first set to type(uint256).max, then to input.length.

The second input will be correctly parsed only if the len is equal to the input.length (8), because otherwise the terminator is not found, and the value is retrieved by substring(input, offset:3 + 1, len:8 - 3 - 1) causing the value to be 0x62626220 instead of 0x626262.

The third input suffers the same problems as the first and second.

Tools Used

n/a

Recommended Mitigation Steps

Either remove the len parameter and calculate it internally as len = input.length - offset, or update the algorithm to be more robust.

[c4-judge]

dmvt marked the issue as duplicate of #246

[c4-judge]

dmvt marked the issue as satisfactory