Description

The NameToken contract overrides the Ownable contract's internal ownership check (_requireCallerIsContractOwner()) to use the AccessControl contract's DEFAULT_ADMIN_ROLE check instead.

However, when ownership is transferred via Ownable::transferOwnership, the new owner does not automatically receive the DEFAULT_ADMIN_ROLE.

Impact

This will result in the new owner not being able to satisfy the role check, making owner-only functions inaccessible.

In the worst-case scenario, if the original admin renounces the DEFAULT_ADMIN_ROLE immediately on transferring ownership, no account will hold the DEFAULT_ADMIN_ROLE, and thus all protected functionality will be permanently locked forever.

We think the likelihood of this issue occurring is high, and thus the impact is Critical.

Proof of Concept

Add the following testcase to test/OwnershipToken.test.ts and run it using npx hardhat test --grep audit:

describe('audit test', () => {
  it('audit - transfer ownership lockout', async () => {
    const newOwner = user;
    const newProxyDomaRecord = "0x00000000000000000000000000000000000000AA";
    
    // ownership is transfered to newOwner
    await ownershipTokenContract.connect(owner).transferOwnership(newOwner);

    // newOwner should be able to call onlyOwner functions
    const tx = ownershipTokenContract.connect(newOwner).setProxyDomaRecord(newProxyDomaRecord);
    await expect(tx).to.not.be.reverted;

    // previous owner should not be able to call onlyOwner functions
    await ownershipTokenContract.connect(owner).setProxyDomaRecord(newProxyDomaRecord);
    const tx2 = ownershipTokenContract.connect(owner).setProxyDomaRecord(newProxyDomaRecord);
    await expect(tx2).to.be.reverted;
  });
});

The new owner's transaction to the ownership token contract will revert, even though the ownership has already been transferred.

The test won't get past that transaction, but we've added in another transaction to ensure that after the issue is fixed, the previous owner is not still able to call a protected function on the ownership contract.

Recommendations

Override the Ownable::transferOwnership() function, and ensure that the DEFAULT_ADMIN_ROLE is passed onto the new owner prior to the old owner renouncing their ownership. Also ensure that the DEFAULT_ADMIN_ROLE is removed from the old owner.

Remediation

This issue has been acknowledged by D3, and a fix was implemented in commit 341c38ae↗.