Anyone can call `ETHRegistrarController.register` for already existing commitments and set a reverse record to the caller instead of the owner of a record

[code423n4]

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")
    );
}

[jefflau]

The reverse for a name can be set to any name a user wants to, so despite this being a bug that it is possible, the name is still registered to the correct account and alice has just registered the name for bob and paid the registration fee, and has set her reverse to bob's name (but she could do that already, because setName can be set to anything that you would like).

[dmvt]

I'm downgrading this to QA. There is a bug, but no negative impact on Bob.