ENS contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

Using Solidity's transfer function has some notable shortcomings when the caller is a smart contract, which can render registers via ETH impossible. Specifically, the transfer will inevitably fail when:

• The caller smart contract does not implement a payable fallback function.
• The caller smart contract implements a payable fallback function which uses more than 2300 gas units.
• The caller smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

The sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

Proof of Concept

ethregistrar/ETHRegistrarController.sol#L183-L185

payable(msg.sender).transfer(
    msg.value - (price.base + price.premium)
);

ethregistrar/ETHRegistrarController.sol#L204

if (msg.value > price.base) {
    payable(msg.sender).transfer(msg.value - price.base);
}

ethregistrar/ETHRegistrarController.sol#L211

function withdraw() public {
    payable(owner()).transfer(address(this).balance);
}

Tools Used

Manual review

Recommended mitigation steps

Use Solidity's low-level call() function or the sendValue function available in OpenZeppelin Contract’s Address library to send Ether.

Lines of code

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

Vulnerability details

Impact

When unwrapping a .eth domain via NameWrapper.unwrapETH2LD, the specified newRegistrant receives an ERC721 token representing the ownership of the domain. However, the newRegistrant receiver receives the ERC721 token via the unsafe transferFrom instead of the safe counterpart safeTransferFrom. If the receiver is a contract and is not aware of incoming ERC721 tokens, the sent ERC721 tokens could be locked.

safeTransferFrom checks that contract recipients are aware of the ERC721 protocol to prevent tokens from being forever locked.

Proof of Concept

wrapper/NameWrapper.sol#L341-L345

function unwrapETH2LD(
    bytes32 labelhash,
    address newRegistrant,
    address newController
) public override onlyTokenOwner(_makeNode(ETH_NODE, labelhash)) {
    _unwrap(_makeNode(ETH_NODE, labelhash), newController);
    registrar.transferFrom(
        address(this),
        newRegistrant,
        uint256(labelhash)
    );
}

Tools Used

Manual review

Recommended mitigation steps

Consider using safeTransferFrom() within the NameWrapper.unwrapETH2LD function.

Lines of code

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

Vulnerability details

Impact

To prevent front running, the ETHRegistrarController contract uses a two-step process to register names. First one has to call ETHRegistrarController.commit with the desired configuration parameters and wait for minCommitmentAge to pass by. Then a call to ETHRegistrarController.register with the same parameters as in the previous step to finally register the name. This ETHRegistrarController.register can be front-run without any consequences by anyone else. At least that's the case for registering the name.

If reverseRecord is set to true and the ETHRegistrarController.register function is called by anyone else than the owner, a reverse record with name is set for the caller address msg.sender instead of the owner.

Proof of Concept

ETHRegistrarController.sol#L170

if (reverseRecord) {
    _setReverseRecord(name, resolver, msg.sender); // @audit-info the caller address `msg.sender` of `ETHRegistrarController.register` will be used as the `owner` for `_setReverseRecord`
}

ETHRegistrarController._setReverseRecord

function _setReverseRecord(
    string memory name,
    address resolver,
    address owner
) internal {
    reverseRegistrar.setNameForAddr(
        msg.sender,
        owner,
        resolver,
        string.concat(name, ".eth")
    );
}

Example

1. Bob wants to register foo.eth and calls ETHRegistrarController.commit with the appropriate parameters and reverseRecord = true
2. After the waiting period of minCommitmentAge, Bob calls ETHRegistrarController.register with the same parameters as in the step before
3. Alice monitors the mempool and spots the transaction from Bob. She decides to front run with more gas and wins the race. Her transaction get's processed first
4. The name foo.eth is successfully registered (with Bob as the owner), however, Alice has now her address associated with a reverse record set to foo.eth and Bob is missing the reverse record for his address.

Copy-paste the following test into the TestEthRegistrarController.js file and run the tests:

it.only("should set the reverse record of the account from someone else", async () => {
  const commitment = await controller.makeCommitment(
    "reverse",
    registrantAccount,
    REGISTRATION_TIME,
    secret,
    resolver.address,
    [],
    true,
    0,
    0
  );
  await controller.commit(commitment);

  await evm.advanceTime((await controller.minCommitmentAge()).toNumber());
  await controller
    .connect(signers[2])
    .register(
      "reverse",
      registrantAccount,
      REGISTRATION_TIME,
      secret,
      resolver.address,
      [],
      true,
      0,
      0,
      { value: BUFFERED_REGISTRATION_COST }
    );

  const signer2Address = await signers[2].getAddress();

  expect(await resolver.name(getReverseNode(signer2Address))).to.equal(
    "reverse.eth"
  ); // @audit-info caller "steals" reverse record
  expect(await resolver.name(getReverseNode(registrantAccount))).to.equal(""); // @audit-info owner of registered name lacks reverse record
});

Tools Used

Manual review

Recommended mitigation steps

Consider using the owner instead of msg.sender:

ETHRegistrarController.sol#L170

if (reverseRecord) {
    _setReverseRecord(name, resolver, owner); // @audit-info use `owner` instead of `msg.sender`
}

ETHRegistrarController._setReverseRecord

function _setReverseRecord(
    string memory name,
    address resolver,
    address owner
) internal {
    reverseRegistrar.setNameForAddr(
        owner, // @audit-info use `owner` instead of `msg.sender`
        owner,
        resolver,
        string.concat(name, ".eth")
    );
}