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

[L-04] dnsEncodeName can have node hash collisions

Links

https://github.com/code-423n4/2023-04-ens/blob/45ea10bacb2a398e14d711fe28d1738271cd7640/contracts/utils/NameEncoder.sol#L9-L51

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

Impact

The dnsEncodeName may encode multiple different names that have the same/colliding node hash. Moreover the label length of a non-zero length label may be set to zero.

Proof of Concept

The dnsEncodeName function has an encoding loop that runs inside an unchecked block. The labelLength is incremented at each step, and reset when a "." is encountered. Since there is no label length check and the labelLength increment is in an unchecked block, passing a 256 long label will set the labelLength to zero.

Let's take for example two names where the label lengts are 256.3 like the following two.

abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijxxxxxx.eth

abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijyyyyyy.eth

Only the first part is different because the first one ends with x, the second one ends with y.

When looping backwards, first "eth" will be parsed and we'll get

➜ bytes32 node ➜ node = keccak256(abi.encodePacked(bytes32(0), keccak256(bytes("eth")))) Type: bytes32 └ Data: 0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae

then the 256 bytes long label will be parsed with labelLength starting at 0, and overflowing back to 0 at the 256th increment, since labelLength is uint8. We then have:

➜ node = keccak256(abi.encodePacked(node, keccak256(bytes("")))) Type: bytes32 └ Data: 0x8cc9f31a5e7af6381efc751d98d289e3f3589f1b6f19b9b989ace1788b939cf7

Therefore the dnsEncodeName function will return a node hash of 0x8cc9f31a5e7af6381efc751d98d289e3f3589f1b6f19b9b989ace1788b939cf7 for both the two names in the example.

Result { 'dnsName': '0x006162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a7878787878780365746800', 'node': '0x8cc9f31a5e7af6381efc751d98d289e3f3589f1b6f19b9b989ace1788b939cf7' }

Result { 'dnsName': '0x006162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a6162636465666768696a7979797979790365746800', 'node': '0x8cc9f31a5e7af6381efc751d98d289e3f3589f1b6f19b9b989ace1788b939cf7' }

In the two examples above the 'node' has is the same: 0x8cc9f31a5e7af6381efc751d98d289e3f3589f1b6f19b9b989ace1788b939cf7

The label length of the two 256 characters long labels in dnsName(s) is set to 0.

Still progressing through contracts in scope, therefore I cannot tell yet if there are any other related or major security concerns.

Tools Used

n/a

Recommended Mitigation Steps

Add a length limit to each label and/or update the labelLength type if necessary.

code-423n4 / 2023-04-ens-findings