ENS Contest - Lilyjjo's results

Lines of code

https://github.com/code-423n4/2023-04-ens/blob/45ea10bacb2a398e14d711fe28d1738271cd7640/contracts/dnsregistrar/DNSClaimChecker.sol#L66-L74

Vulnerability details

Currently users can create malformed DNSSEC _ens records which are erroneously accepted as valid in the ENS system. This is bad because users could experience loss of funds if they fail to notice that their ENS was not set to their intended address.

DNSClaimChecker.sol’s function getOwnerAddress() is used to determine which address a DNSSEC's _ens record points to. Currently this function fails to properly validate that an address is of the correct length and will accept “address” strings which are longer than 20 bytes. Problematically, the function will take the last 20 bytes of the input as the address.

For example, let a user fat-finger their address when creating an _ens record and add two extra characters at the end of the address string:

// note, the hex string is 22 bytes long and isn't a valid address
a=0xf39Fd6e51aad88F6F4ce6aB8827279cffFb9226666

The address parsed during DNSClaimChecker.sol’s function getOwnerAddress() for this record will be 0x9FD6E51aaD88F6F4Ce6aB8827279CffFb9226666, which is the last 20 bytes of the _ens record, which is definitely not what the user intended.

The issue is that DNSClaimChecker.sol’s parseString() function fails to ensure that the string is logically restricted to the proper length (and is linked to in the provided code link).

Recommended mitigation steps:

• Performing proper length validation on the _ens record’s data

Proof of Concept

diff --git a/test/dnsregistrar/TestDNSRegistrar.js b/test/dnsregistrar/TestDNSRegistrar_modified.js
index 365d936..4df433a 100644
--- a/test/dnsregistrar/TestDNSRegistrar.js
+++ b/test/dnsregistrar/TestDNSRegistrar_modified.js
@@ -103,6 +103,56 @@ contract('DNSRegistrar', function (accounts) {
     assert.equal(await ens.owner(namehash.hash('foo.test')), accounts[0])
   })
 
+  it.only('ENS records with too long address accepted as valid', async function () {
+    assert.equal(await registrar.oracle(), dnssec.address)
+    assert.equal(await registrar.ens(), ens.address)
+
+    const testRrsetExtraChars = (name, account) => ({
+      name,
+      sig: {
+        name: 'test',
+        type: 'RRSIG',
+        ttl: 0,
+        class: 'IN',
+        flush: false,
+        data: {
+          typeCovered: 'TXT',
+          algorithm: 253,
+          labels: name.split('.').length + 1,
+          originalTTL: 3600,
+          expiration,
+          inception,
+          keyTag: 1278,
+          signersName: '.',
+          signature: new Buffer([]),
+        },
+      },
+      rrs: [
+        {
+          name: `_ens.${name}`,
+          type: 'TXT',
+          class: 'IN',
+          ttl: 3600,
+          data: Buffer.from(`a=${account}6666`, 'ascii'), // note extra chars were added
+        },
+      ],
+    })
+
+    // construct _ens record with too many chars
+    const proof = [
+      hexEncodeSignedSet(rootKeys(expiration, inception)),
+      hexEncodeSignedSet(testRrsetExtraChars('gooo.test', accounts[0])), 
+    ]
+
+    // This should fail to run!!
+    await registrar.proveAndClaim(utils.hexEncodeName('gooo.test'), proof, {
+      from: accounts[1],
+    })
+
+    // This does fail 
+    assert.equal(await ens.owner(namehash.hash('gooo.test')), accounts[0])
+  })
+
   it('allows claims on names that are not TLDs', async function () {
     const proof = [
       hexEncodeSignedSet(rootKeys(expiration, inception)),