AccessControl Admin Rules

[frangio]

The default admin in AccessControl is a very sensitive role and it requires special treatment. In the docs we recommend "extra precautions" but this refers mostly to off-chain operational security:

Is there something we can do at the contract level to encourage better security practices around the default admin role?

I am thinking we can have an extension of AccessControl that adds special rules for this role.

Some ideas I've gathered from @nchamo and @nventuro:

1. Enforce that just one account holds the role.
2. Enforce a 2-step process to transfer the role to another account: grant then claim.
3. Enforce a configurable delay between the two steps, with the ability to cancel in between.

Related to #3593

[hrik2001]

Hello @frangio, you might remember me from #3598.
Interested in implementing this, but I believe that you might have more things in mind.

[frangio]

We're still discussing ideas. If you want to help I think it would be great to put together a proof of concept implementing the 3 points I mentioned. You can share it here in a comment. Please no PRs yet until we pin down the design!

[Amxx]

POC:

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

import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

contract AccessControlWithDelayedOwnership is Ownable, AccessControl {
    // Making this updatable causes issues:
    // - who can update it? Only the owner!
    // - if the owner what to instant transfer, can he just set it to 0 and do an instant transfer?
    // - should we enforce a minimum, non updatable delay?
    // - if so, why do we need an updatable delay and not just stick to the minimum value.
    uint32  public immutable delay;
    
    address public pendingOwner;
    uint32  public pendingOwnerTimer;
    
    constructor(uint32 initialDelay) {
        delay = initialDelay;
    }

    // Admin is owner
    function hasRole(bytes32 role, address account) public view override returns (bool) {
        return role == DEFAULT_ADMIN_ROLE ? account == owner() : super.hasRole(role, account);
    }

    function _grantRole(bytes32 role, address account) internal override {
        require(role != DEFAULT_ADMIN_ROLE, "Admin role overriden with ownership");
        super._grantRole(role, account);
    }

    function _revokeRole(bytes32 role, address account) internal override {
        require(role != DEFAULT_ADMIN_ROLE, "Admin role overriden with ownership");
        super._revokeRole(role, account);
    }

    // Owner can call with address(0) during the delay to cancel
    function transferOwnership(address newOwner) public virtual override onlyOwner {
        pendingOwner = newOwner;
        pendingOwnerTimer = uint32(block.timestamp) + delay;
        // TODO: emit event
    }

    function transferOwnership(address newOwner, uint32 timestamp) public virtual override onlyOwner {
        require(block.timestamp + delay < timestamp);
        pendingOwner = newOwner;
        pendingOwnerTimer = timestamp;
        // TODO: emit event
    }

    // TODO: remove renounceOwnership ?

    function acceptOwnership() public virtual {
        require(pendingOwner == _msgSender(), "Only pending owner can accept ownership");
        require(pendingOwnerTimer < block.timestamp, "Pending ownership delay");
        _transferOwnership(pendingOwner);
    }

    function _transferOwnership(address newOwner) internal virtual override {
        super._transferOwnership(newOwner);
        delete pendingOwner;
        delete pendingOwnerTimer;
    }
}

[frangio]

I would not use Ownable for this. What are the benefits to using it?

[Amxx]

I'd say

• compatibility with some ownable toolling.
• reusing code means a smaller contract
• easily to track/understand (including by things like OpenSea that looks for owner() to give admin access to a collection's page)

It's technically an ownership pattern we are talking about, so to me using Ownable for that is a no brainer.

[hrik2001]

I also had one question, should delay be immutable? I was thinking of more along the lines of having an overloaded function for _transferOwnership where the second parameter would be delay.

[frangio]

The delay should be enforced. The idea of the delay is to have a time period to detect if an admin transfer was malicious and have the ability to cancel it before it comes into effect.

It should probably not be immutable but changing it should be done by the admin and with a delay.

[Amxx]

Considering all these "value updatable with a delay", I wonder if we should include a library for that. Something like

import "@openzeppelin/contracts/utils/Timers.sol";

library Delay {
    using Timers for Timers.Timestamp;

    struct AddressWithUpdateDelay {
        address value;
        address pendingValue;
        Timers.Timestamp delay;
    }

    function getValue(AddressWithUpdateDelay storage self) internal view returns (address) {
        return self.value;
    }

    function getPendingValue(AddressWithUpdateDelay storage self) internal view returns (address) {
        return self.pendingValue;
    }

    function getDeadline(AddressWithUpdateDelay storage self) internal view returns (uint64) {
        return self.delay.getDeadline();
    }

    function scheduleUpdate(AddressWithUpdateDelay storage self, address newValue, uint32 deadline) internal {
        self.pendingValue = newValue;
        self.delay.setDeadline(deadline);
    }

    function executeUpdate(AddressWithUpdateDelay storage self) internal returns (bool) {
        if (self.delay.isExpired()) {
            self.value = self.pendingValue;
            delete self.pendingValue;
            delete self.delay;
            return true;
        } else {
            return false;
        }
    }
}

(that could be available for different sub-types)

That could be a first good issue that would pave the way for this (and other PR)

[frangio]

@Amxx Hm, I think that abstraction is a bit on the overengineered side.

[hrik2001]

@frangio so are you against the idea of a library for delays? Or the library is over engineered you mean.

[frangio]

Yes I'd say a library for delayed updates is not necessary.

[nventuro]

Changing delays is a bit tricky: if decreasing the delay, you'd then need to wait at least the difference between delays before making the change, and if increasing it you technically don't need to wait at all. But you also don't want for delays to e.g. be immediately increased to 100 years.

We chose to have an absolute maximum delay (2 years), and a minimum delay of 5 days whenever a delay is changed, including when the delay is increased.

Given you don't have any other delay-changing mechanism, the complexity of the above, and that changing delays is a very infrequent action, I'd just make it immutable.

[hrik2001]

@nventuro, very interesting. I really liked the idea of minimum and maximum delay since oopsies can happen there. What I suggest is this. We can do an immutable min and max delay and maybe should remove the delay variable altogether and except go with providing timestamp (just how @Amxx had implemented it). We can check if timestamp lies in the bound or not.

The thing is this, no one is going to manually put timestamp in real life use case, it will be programmatically called, hence the frontend can plugin the desired timestamp accordingly. Having a default delay sounds good on paper, but I believe providing custom timestamp while checking if it lies in the bound would be best. This will sort out a lot of complexity.

But I could be wrong, there could be a case where providing timestamp won't be feasible at all.

[frangio]

The goal of the delay is to ensure there is time to cancel a malicious transfer. A malicious transfer would always use the minimum delay possible, and the option to transfer with a longer delay wouldn't make a difference there, so what would be the purpose?

I'd say let's just do an immutable delay set in the constructor.

[hrik2001]

Fair enough, btw should ownable be used or do you have from scratch implementation in mind?

…

On Wed, 24 Aug, 2022, 2:48 am Francisco, ***@***.***> wrote: The goal of the delay is to ensure there is time to cancel a malicious transfer. A malicious transfer would always use the minimum delay possible, and the option to transfer with a longer delay wouldn't make a difference there, so what would be the purpose? I'd say let's just do an immutable delay set in the constructor. — Reply to this email directly, view it on GitHub <#3623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZQUYB6ZNBQ5F3BSX6CHPTV2U5ZZANCNFSM56XGEOCQ> . You are receiving this because you commented.Message ID: ***@***.***>

[frangio]

IMO it should not be Ownable.

[ernestognw]

I made a prototype without using the Ownable contract and this is the result:

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.5.0) (access/AccessControlEnumerable.sol)

pragma solidity ^0.8.0;

import "./AccessControl.sol";

/**
 * @dev Extension of {AccessControl} that allows to specify special rules to manage
 * the `DEFAULT_ADMIN_ROLE` owner, which is a sensitive role.
 *
 * If a specific role doesn't have an `adminRole` assigned, the holder of the
 * `DEFAULT_ADMIN_ROLE` will have the ability to manage it, as determined by the
 * function {getRoleAdmin}.
 *
 * This contract implements the following risk mitigations:
 *
 * - Only one account holds the `DEFAULT_ADMIN_ROLE`.
 * - Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account.
 * - Enforce a configurable delay between the two steps, with the ability to cancel in between.
 *
 * NOTE: `delay` is only configurable in the constructor
 */
abstract contract AccessControlAdminRules is AccessControl {
    uint32 private immutable _delay;

    address public pendingAdminOwner;
    uint64 public pendingAdminOwnerTimer;

    address private _previousAdminOwner;

    event AdminRoleChangeStarted(address indexed previousOwner, address indexed newOwner, uint64 timer);

    /**
     * @dev Initializes the contract setting a delay.
     *
     * There should be always an initial admin, since {_transferAdmin} revokes the role
     * and the zero address never emitted an {AccessControl-GrantRole} event.
     */
    constructor(uint32 initialDelay, address initialAdminOwner) {
        super.grantRole(DEFAULT_ADMIN_ROLE, initialAdminOwner);
        _previousAdminOwner = initialAdminOwner;

        _delay = initialDelay;
    }

    /**
     * @dev `DEFAULT_ADMIN_ROLE` can't be granted to ensure there's always 1 owner of it.
     * Ownership updates of `DEFAULT_ADMIN_ROLE` should be done using {_transferAdmin} instead.
     */
    function _grantRole(bytes32 role, address account) internal override {
        require(role != DEFAULT_ADMIN_ROLE, "AccessControlAdminRules: Only one DEFAULT_ADMIN_ROLE is allowed");
        super._grantRole(role, account);
    }

    /**
     * @dev `DEFAULT_ADMIN_ROLE` can't be revoked to ensure there's always 1 owner of it.
     * Ownership updates of `DEFAULT_ADMIN_ROLE` should be done using {_transferAdmin} instead.
     */
    function _revokeRole(bytes32 role, address account) internal override {
        require(role != DEFAULT_ADMIN_ROLE, "AccessControlAdminRules: Only one DEFAULT_ADMIN_ROLE is allowed");
        super._revokeRole(role, account);
    }

    /**
     * @dev Start `DEFAULT_ADMIN_ROLE` transfership by setting new pending owner and a timer.
     * Updates `_previousAdminOwner` specifically after `AdminRoleChangeStarted` is emitted.
     *
     * NOTE: Owner can call with address(0) during the delay to cancel
     */
    function transferAdmin(address newOwner) public virtual onlyRole(DEFAULT_ADMIN_ROLE) {
        pendingAdminOwnerTimer = uint64(block.timestamp) + _delay;
        pendingAdminOwner = newOwner;

        emit AdminRoleChangeStarted(_previousAdminOwner, pendingAdminOwner, pendingAdminOwnerTimer);
        _previousAdminOwner = newOwner;
    }

    /**
     * @dev Completes the admin transfership after delay has passed
     */
    function acceptAdmin() public virtual {
        require(pendingAdminOwner == _msgSender(), "AccessControlAdminRules: Only pending owner can accept ownership");
        require(pendingAdminOwnerTimer < block.timestamp, "AccessControlAdminRules: Pending ownership delay");

        _transferAdmin(_previousAdminOwner, pendingAdminOwner);
    }

    /**
     * @dev Transfers the `DEFAULT_ADMIN_ROLE`, effectively allowing only 1 owner of the role.
     */
    function _transferAdmin(address previousOwner, address newOwner) internal virtual {
        super.revokeRole(DEFAULT_ADMIN_ROLE, previousOwner);
        super.grantRole(DEFAULT_ADMIN_ROLE, newOwner);

        // Cancels any other pending transfership process
        delete pendingAdminOwnerTimer;
        delete pendingAdminOwner;
    }
}

I'd say it is almost the same as the one @Amxx shared, but I didn't include function transferOwnership(address newOwner, uint32 timestamp) since it seems frontrunable to me, am I missing something?

btw, I'm not sure if we should remove the _setRoleAdmin for the DEFAULT_ADMIN_ROLE, but is useless to be the admin of the DEFAULT_ADMIN_OWNER as far as I can tell

[frangio]

The use of super.revokeRole and super.grantRole in _transferAdmin is a use of inheritance that makes me uneasy because it's potentially skipping overrides "downstream". Imagine someone overrides revokeRole and grantRole so that they can't be used on the weekends, now depending on the order in which you inherit things, _transferAdmin may or may not be usable on the weekends. (I should write this down as potential guidelines we might want to use regarding inheritance, I'm not sure we will all agree on them.)

An alternative proposal would be to keep the count of members of the role, and add require(members <= 1) in grantRole (only for the default admin role). This same approach doesn't work with revoke though, because revokeRole needs to follow the "rules" as well (delay and 2-step), so the alternative could be:

function _revokeRole(bytes32 role, address account) internal override {
    if (role == DEFAULT_ADMIN_ROLE) {
        require(_pendingAdminTimer > 0 && _pendingAdminTimer < block.timestamp);
        _pendingAdminTimer = 0;
        super._revokeRole(role, account);
        if (_pendingAdmin != address(0)) {
            _grantRole(role, _pendingAdmin);
        }
    } else {
        super._revokeRole(role, account);
    }
}

What do people think of this?

---

transferOwnership(address newOwner, uint32 timestamp) isn't frontrunnable as far as I can tell but see #3623 (comment).

---

Note that the prototypes above are also missing a way to cancel a pending admin transfer, which I think had been suggested needs to be a part of this.

[ernestognw]

I like the idea of removing _transferAdmin and keeping the super. in _revoke, however, members is not available in this context unless we save a boolean to represent that there's an owner.

transferOwnership(address newOwner, uint32 timestamp) isn't frontrunnable as far as I can tell but see #3623 (comment).

We can also avoid to start a transfer admin if a timer has been set.

It can be something like these:

pragma solidity ^0.8.0;

import "./AccessControl.sol";

/**
 * @dev Extension of {AccessControl} that allows to specify special rules to manage
 * the `DEFAULT_ADMIN_ROLE` owner, which is a sensitive role.
 *
 * If a specific role doesn't have an `adminRole` assigned, the holder of the
 * `DEFAULT_ADMIN_ROLE` will have the ability to manage it, as determined by the
 * function {getRoleAdmin}.
 *
 * This contract implements the following risk mitigations:
 *
 * - Only one account holds the `DEFAULT_ADMIN_ROLE`.
 * - Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account.
 * - Enforce a configurable delay between the two steps, with the ability to cancel in between.
 *
 * NOTE: `delay` is only configurable in the constructor
 */
abstract contract AccessControlAdminRules is AccessControl {
    uint32 private immutable _delay;
    bool private _adminOwned;

    address public pendingAdmin;
    uint64 public pendingAdminTimer;

    event AdminRoleChangeStarted(address indexed previousOwner, address indexed newOwner, uint64 timer);

    /**
     * @dev Initializes the contract with an admin transfer delay
     */
    constructor(uint32 initialDelay) {
        _delay = initialDelay;
    }

    /**
     * @dev See {AccessControl-_grantRole}.
     *
     * If the role is `DEFAULT_ADMIN_ROLE`, it can only be granted to a different account
     * if it's not already owned. Calling `grantRole` for an already admin owner is allowed.
     */
    function _grantRole(bytes32 role, address account) internal override {
        if (role == DEFAULT_ADMIN_ROLE) {
            if (_adminOwned) {
                require(
                    hasRole(role, account), // NOOP allowed
                    "AccessControlAdminRoles: Only current owner can be granted the role again."
                );
            } else {
                require(!_adminOwned, "AccessControlAdminRules: Admin role is already owned");
                _adminOwned = true;
            }
        }

        super._grantRole(role, account);
    }

    /**
     * @dev See {AccessControl-_revokeRole}.
     *
     * If the role is `DEFAULT_ADMIN_ROLE`, it can only be revoked after a timer has been set and met, and
     * it also grants the role to a previously set `pendingOwner`.
     *
     * See {transferAdmin}.
     */
    function _revokeRole(bytes32 role, address account) internal override {
        if (role == DEFAULT_ADMIN_ROLE) {
            require(pendingAdminTimer > 0, "AccessControlAdminRules: Timer for admin transfer not set");
            require(pendingAdminTimer < block.timestamp, "AccessControlAdminRules: Timer for admin transfer not met");
            delete pendingAdminTimer;

            if (_adminOwned) _adminOwned = false;
            // If it wasn't owned before, `_revokeRole` doesn't revoke it.
            super._revokeRole(role, account);

            address _newOwner = pendingAdmin;
            delete pendingAdmin;
            _grantRole(role, _newOwner); // Address 0 is not a NOO for `renounceRole`
        } else {
            super._revokeRole(role, account);
        }
    }

    /**
     * @dev Start `DEFAULT_ADMIN_ROLE` transfership by setting new pending owner and a timer.
     */
    function transferAdmin(address previousOwner, address newOwner) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
        require(pendingAdminTimer == 0, "AccessControlAdminRules: Timer for admin transfer already set");
        pendingAdminTimer = uint64(block.timestamp) + _delay;
        pendingAdmin = newOwner;

        if (!_adminOwned) {
            require(
                previousOwner == address(0),
                "AccessControlAdminRules: Previous owner can only be address 0 if owner hasn't been owned"
            );
        } else {
            _checkRole(DEFAULT_ADMIN_ROLE, previousOwner);
        }

        emit AdminRoleChangeStarted(previousOwner, pendingAdmin, pendingAdminTimer);
    }

    /**
     * @dev Cancels {transferAdmin}.
     */
    function cancelTransferAdmin() external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
        require(
            pendingAdminTimer > block.timestamp,
            "AccessControlAdminRules: Can't cancel admin transfer if timer is already met"
        );
        delete pendingAdmin;
        delete pendingAdminTimer;
        // Emit event?
    }
}

However, I don't like the transferAdmin and cancelTransferAdmin functions.
Maybe those can be also handled in _revokeRole.

1. Use the _revokeRole to start the transfer process.
2. Cancel by calling _revokeRole if the transfer has started but hasn't passed.
3. Use the _grantRole instead to accept the transfer.

Still, I don't like number 2 and the fact that there would be an intermediate time with no DEFAULT_ADMIN_ROLE if we decide to keep the super. calls.

[Amxx]

I'm showked by how complexe _revokeRole and _grantRole are. IMO, these function should remain simple.

Also, why do we introduce a new function transferAdmin ?
Either want the transfer to be based on the existing AccessControl interface (grantRole), or we want a separate interfaces, and then that should be Ownable2Step transferOwnership/acceptOwnership. This will make it easier for everyone to display what is going on (including Defender).

[Amxx]

IMO, the same features can be achieved with a significantly better readability (which I believe is one of our guidelines)

Code example

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/access/Ownable2Steps.sol";

contract AccessControlAdmin is AccessControl, Ownable2Steps
{
    uint256 private immutable _delay;
    uint256 private _deadline;

    constructor(uint256 delay) {
        _delay = delay;
    }

    // enforce a delay on admin transfers
    function transferOwnership(address newOwner) public virtual override {
        super.transferOwnership(newOwner); // this is onlyOwner
        deadline = block.timestamp + delay;
    }

    function acceptOwnership() public virtual override {
        require(deadline <= block.timestamp, "too early to access ownership");
        delete deadline;
        super.acceptOwnership();
    }

    // Owner is the admin
    function hasRole(bytes32 role, address account)
        public
        view
        virtual
        override
        returns (bool)
    {
        return role == DEFAULT_ADMIN_ROLE 
            ? account == owner() 
            : super.hasRole(role, account);
    }

    function _grantRole(bytes32 role, address account) internal virtual override {
        require(role != DEFAULT_ADMIN_ROLE, "Admin role is managed by owner");
        super._grantRole(role, account);
    }

    function _revokeRole(bytes32 role, address account) internal virtual override {
        require(role != DEFAULT_ADMIN_ROLE, "Admin role is managed by owner");
        super._revokeRole(role, account);
    }
}

[Amxx]

    function transferAdmin(address previousOwner, address newOwner) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {

Are there cases where previousOwner is not _msgSender() ?

[Amxx]

                require(!_adminOwned, "AccessControlAdminRules: Admin role is already owned");

if is in the else part of an if (_adminOwned) ... so the check is not necessary, is it ?

AFAIK, this would be sufficient

        if (role == DEFAULT_ADMIN_ROLE) {
            require(
                !_adminOwned || hasRole(role, account), // NOOP allowed
                "AccessControlAdminRoles: Only current owner can be granted the role again."
            );
            _adminOwned = true;
        }

[Amxx]

Also one thing, to finalize the transfer you must call _revokeRole, which is publicly accessible only to accounts that have the (admin) role. This means that the pending owner cannot accept ownership. The current owner needs to "come back" and finish the transfer by revoking its own admin role.

[Amxx]

Also (sorry for the many message), I think we should have internal functions to initiate the transfer ... so that devs can trigger that internally based on custom logic (signature ?)

[Amxx]

Also, not being able to transferAdmin it its already initiated + disable cancel when the timer is done means that if you set transferAdmin to an invalid address, and realize when its to late, you've "bricked" your contract.

IMO we should:

• allow the current owner to reinitialize the transfer at any point (without having to call cancel)
• remove the cancel function. In order to cancel you just restart a transfer by setting address(0) as the receiver

[ernestognw]

I'm showked by how complexe _revokeRole and _grantRole are. IMO, these function should remain simple.

Also, why do we introduce a new function transferAdmin ?
Either want the transfer to be based on the existing AccessControl interface (grantRole), or we want a separate interfaces, and then that should be Ownable2Step transferOwnership/acceptOwnership.

I agree. Actually, that implementation with Ownable2Step you just shared looks nice to me and simple enough.

This will make it easier for everyone to display what is going on (including Defender).

This is not completely true, Defender will know that the address is the owner, but that that it has the corresponding rights.
I think we'd have to add a grant/revoke event to correctly signal to the AccessControl tooling out there.

    function acceptOwnership(address previousOwner) public virtual override {
        require(deadline <= block.timestamp, "too early to access ownership");
        delete deadline;
        super.acceptOwnership(); // `pendingOwner == _msgSender()` checked here
        _checkRole(previousOwner, DEFAULT_ADMIN_ROLE); // To check previousOwner is actually previousOwner
        emit RoleGranted(DEFAULT_ADMIN_ROLE, previousOwner, _msgSender());
        emit RoleGranted(DEFAULT_ADMIN_ROLE, _msgSender(), _msgSender());
    }

The same applies for renounceOwnership. I think it'll have to correctly signal the removal of the admin role.

Still, looks more readable to me but it adds extra logic just to comply with the AccessControl "rules", and feels off to me.

    function transferAdmin(address previousOwner, address newOwner) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {

Are there cases where previousOwner is not _msgSender() ?

Glad you catch this! With the code as it is right now, no, there's no way. This is related to the AdminRoleChangeStarted event I'm proposing so we'd need an onlyRoleOrNotAdminOwned modifier. I don't like that.

if is in the else part of an if (_adminOwned) ... so the check is not necessary, is it ?

That's correct

Also one thing, to finalize the transfer you must call _revokeRole, which is publicly accessible only to accounts that have the (admin) role. This means that the pending owner cannot accept ownership. The current owner needs to "come back" and finish the transfer by revoking its own admin role.

That's also correct

I think we should have internal functions to initiate the transfer ... so that devs can trigger that internally based on custom logic (signature ?)

Make sense to me, but I'd focus on reaching a agreement over the implementation first. I'll consider it for the next iteration.

Also, not being able to transferAdmin it its already initiated + disable cancel when the timer is done means that if you set transferAdmin to an invalid address, and realize when its to late, you've "bricked" your contract.

I also dislike the cancelTransferAdmin, but I'd like to hear thoughts from @frangio because of this

Note that the prototypes above are also missing a way to cancel a pending admin transfer, which I think had been suggested needs to be a part of this.

[ernestognw]

After a few attempts, this is the best I could come up with following overall comments:

Code:

abstract contract AccessControlAdminRules is AccessControl {
    uint32 private immutable _delay;

    address public pendingAdmin;
    uint64 public pendingAdminTimer;

    event AdminRoleChangeStarted(address indexed newOwner, uint64 timer);

    constructor(uint32 initialDelay, address initialAdmin) {
        _delay = initialDelay;
        super.grantRole(DEFAULT_ADMIN_ROLE, initialAdmin);
    }

    // Owner can call with address(0) during the delay to cancel
    function transferAdmin(address newAdmin) public virtual {
        pendingAdminTimer = uint64(block.timestamp) + _delay;
        pendingAdmin = newAdmin;
        emit AdminRoleChangeStarted(pendingAdmin, pendingAdminTimer);
    }

    function acceptAdmin(address previousAdmin) public virtual {
        require(
            pendingAdmin == address(0) || _msgSender() == pendingAdmin,
            "AccessControlAdminRules: caller is not the new owner"
        );
        _transferAdmin(previousAdmin);
    }

    function _grantRole(bytes32 role, address account) internal override {
        if (role != DEFAULT_ADMIN_ROLE) super._grantRole(role, account);
    }

    function _revokeRole(bytes32 role, address account) internal override {
        if (role != DEFAULT_ADMIN_ROLE) super._revokeRole(role, account);
    }

    function _transferAdmin(address previousAdmin) internal virtual {
        require(
            pendingAdminTimer > 0 && pendingAdminTimer < block.timestamp,
            "AccessControlAdminRules: timer for admin transfer not set nor met"
        );
        _checkRole(DEFAULT_ADMIN_ROLE, previousAdmin);
        delete pendingAdminTimer;
        super._revokeRole(DEFAULT_ADMIN_ROLE, previousAdmin);
        super._grantRole(DEFAULT_ADMIN_ROLE, pendingAdmin);
        delete pendingAdmin;
    }
}

The proposed contract acts the same as an Ownable2Step and only adds around 10 LOC (not including the renounceOwnership override required).

Although I like the simplicity of the Ownable2Step, I still think we:

1. Should not mess with emitting events such as OwnershipTransferStarted and RoleGranted. I think they belong to their own category and shouldn't be mixed if the purpose is to make it compatible with current tooling (this will make it confusing for users overriding)
2. Avoid requires in grantRole and revokeRole, I assume there's a reason behind the if (hasRole(..., ...)) in AccessControl, so my feeling is that we have to respect that NOOP
3. Ownable2Step grants the owner role to the sender by default, and we've heard people from the community requesting to explicitly state who holds the admin role (and we have this)

Aside from that opinion, I'd have to research these open questions:

1. Do we want the DEFAULT_ADMIN_ROLE to be renounceable? (I added that functionality in my PoC)
2. Should we add a minimum delay? Currently the pendingAdminTimer < block.timestamp check doesn't allow for the same block, but if same-block role transfers are valuable, we can do a pendingAdminTimer <= block.timestamp with a require(delay != 0) at the end.

[Amxx]

   function transferAdmin(address newAdmin) public virtual {

this needs some protection so not everyone can call it!

---

_transferAdmin being the internal variant of acceptAdmin is confusing. It should probably be named _acceptAdmin

---

        require(
            pendingAdminTimer > 0 && pendingAdminTimer < block.timestamp,
            "AccessControlAdminRules: timer for admin transfer not set nor met"
        );

This is not correct

---

I'm not a big fan of having to provide previousAdmin. We are building an invariant that make sure there is only one address that is admin, but we are expecting an external caller to give us clues about this invariant ? That means the caller must figure that out first. If the caller is an EOA that might be ok, but if the caller is a contract that will make everything difficult. I think we need to have this value stored and easily accessible.

---

    function _grantRole(bytes32 role, address account) internal override {
        if (role != DEFAULT_ADMIN_ROLE) super._grantRole(role, account);
    }

    function _revokeRole(bytes32 role, address account) internal override {
        if (role != DEFAULT_ADMIN_ROLE) super._revokeRole(role, account);
    }

IMO we should revert of the role is DEFAULT_ADMIN_ROLE. Doing nothing and not reverting could cause confusion.

[Amxx]

    // Owner can call with address(0) during the delay to cancel

If you do that and let the delay expire, then anyone can call acceptAdmin to validate you renouncing the admin role.
To cancel you should use something like address(1) or address(0xdead)

[ernestognw]

_transferAdmin being the internal variant of acceptAdmin is confusing. It should probably be named _acceptAdmin

Similar logic to OwnableTwoSteps. Naming can be discussed within a PR.

I'm not a big fan of having to provide previousAdmin. We are building an invariant that make sure there is only one address that is admin, but we are expecting an external caller to give us clues about this invariant ? That means the caller must figure that out first. If the caller is an EOA that might be ok, but if the caller is a contract that will make everything difficult. I think we need to have this value stored and easily accessible.

If you think the storage is worth it, we can go for it. I don't think so.

IMO we should revert of the role is DEFAULT_ADMIN_ROLE. Doing nothing and not reverting could cause confusion.

I'd say is similar confusion about calling a grantRole successfully without state changes. Still not the same but I don't have a strong opinion of reverting or not besides our no-op policy.

• I see more costly to add the revert reasons.
• The case of "keeping tooling working" may break if we revert here.
• I see it fine a long as we don't emit an event (unless indexers consider the call as a successful state change).

I'd put this into the open questions.

If you do that and let the delay expire, then anyone can call acceptAdmin to validate you renouncing the admin role.
To cancel you should use something like address(1) or address(0xdead)

See open questions