Clean up pending admin schedule on renounce in DefaultAdminRules

[frangio]

In AccessControlDefaultAdminRules we realized that we are not cleaning up the schedule value in storage after renouncing. This doesn't seem to have any consequences other than misrepresenting the state of the contract through one of its getters.

I'm also including an unrelated change in this PR but I noticed two of the descriptions in the test suite were not using the right wording (delay vs schedule).

Fixes LIB-862

[changeset-bot]

🦋 Changeset detected

Latest commit: 7ff0d45

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[frangio]

How should we handle this in terms of changeset? 🤔 It sounds like we need a changeset for the release automation to create a new release candidate for us, but this would be a part of the existing changeset for AccessControlDefaultAdminRules.

[Amxx]

Small comment:

If a dev calls _revokeRole internally, the schedule is not reset. This means that if someone does a beginTransferAdmin and then a _revokeRole, the contract appears to be administered by address(0), but the pending owner can still claim the ownership.

IMO this delete should go (in addition to delete _pendingDefaultAdmin) in _revokeRole.

    function _revokeRole(bytes32 role, address account) internal virtual override {
        if (role == DEFAULT_ADMIN_ROLE && account == _currentDefaultAdmin) {
            delete _currentDefaultAdmin;
        }
        super._revokeRole(role, account);
    }

[Amxx]

I tested with

diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol
index 6cdda81a..08173dd2 100644
--- a/contracts/access/AccessControlDefaultAdminRules.sol
+++ b/contracts/access/AccessControlDefaultAdminRules.sol
@@ -139,6 +139,8 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu 
     function _revokeRole(bytes32 role, address account) internal virtual override {
         if (role == DEFAULT_ADMIN_ROLE && account == _currentDefaultAdmin) {
             delete _currentDefaultAdmin;
+            delete _pendingDefaultAdmin;
+            delete _pendingDefaultAdminSchedule;
         }
         super._revokeRole(role, account);
     }
@@ -249,8 +251,6 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu 
         require(_isScheduleSet(schedule) && _hasSchedulePassed(schedule), "AccessControl: transfer delay not passed");
         _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin());
         _grantRole(DEFAULT_ADMIN_ROLE, newAdmin);
-        delete _pendingDefaultAdmin;
-        delete _pendingDefaultAdminSchedule;
     }

     ///

and the tests pass. I believe it is an overall better fix

[Amxx]

(I'm increasingly worried by the number of corner cases that this contract has, and that we are discovering quite late IMO)

[ernestognw]

The problem I see with your approach is that we're now tying the _revokeRole with the scheduling mechanism for DEFAULT_ADMIN_TRANSFER, when I consider that shouldn't be the goal since it's more restrictive.

For example, a developer might want to extend the contract in a way in which a multisig has the ability to disable a DEFAULT_ADMIN before a new one enters into effect. That's a valid use case for using _revokeRole while keeping the already scheduled one.

To be clear, I agree with the corner cases that we're "discovering" and the concerns you have, but I think we also followed a very well thought process of defining responsibilities between external/internal functions. That's part of the reason why we ended up leaving the _set* functions private.

[Amxx]

For example, a developer might want to extend the contract in a way in which a multisig has the ability to disable a DEFAULT_ADMIN before a new one enters into effect. That's a valid use case for using _revokeRole while keeping the already scheduled one.

That is something I raised on slack as being a possible attack:

• you initiate a default admin transfer
• you use the internal _revoke. This emits the revoke event, and set the current admin to 0
  • at the point users will think the contract has no admin, and will not get a new one
• the pending admin accepts, and "resurrect" the admin rights
  • this goes against the expectation (by the user) that the admin was removed from the system.

If someone reported that on Immunefi, would we say its the expected behavior? As a user wouldn't expect this behavior.

---

I personally like the fact that my proposal makes _acceptDefaultAdminTransfer "cleaner"

• we cache the pending admin
• we check the schedule
• we use revoke to clear old state
• we use grant to set the new one

Reseting the state in revoke is clearly opinionated. I like the idea of revoke really "cleaning" the state, by remove the admin and cleaning up the schedule. You may argue that a DefaultAdminTransferCanceled would be needed :/ (sidenote: never been a fan of that event)

---

I think what it comes down to is whether we consider the

beginDefaultAdminTransfer → _revoke → acceptDefaultAdminTransfer

workflow being dangerous or not.

[frangio]

Yeah I also don't see a reason why _revokeRole should reset the schedule.

if someone does a beginTransferAdmin and then a _revokeRole, the contract appears to be administered by address(0), but the pending owner can still claim the ownership.

This is an interesting observation but I don't think it's specific to this situation with revoke. In general, anyone that wants to check the permissions should always also check if there's a pending/scheduled admin. For example, the same strategy could be used to mislead someone into thinking that account A is the admin when in fact account B is a pending admin that can claim at any moment.

[ernestognw]

@Amxx: If someone reported that on Immunefi, would we say its the expected behavior?

We'd say the same as in the audit. Although we recognize this is a scenario that can be used by a malicious developer, it requires overriding the current contract behavior, which is stated in the docs as developer's responsibility.

In any case, I don't want to dismiss the risks of this, is just that we're again in a discussion between "better safe than sorry" and "we're focused on extensibility".

@Amxx: I think what it comes down to is whether we consider the beginDefaultAdminTransfer → _revoke → acceptDefaultAdminTransfer workflow being dangerous or not.

I agree it can be dangerous, but I disagree it's inherently dangerous. I just think it's also dangerous if we reset the schedule and someone builds a mechanism on top _revoke assuming the schedule is not cleared.

@frangio: I also don't see a reason why _revokeRole should reset the schedule.

I agree with this statement. I'd be more in favor of an internal _renounceAdmin that does both the _revoke and the schedule reset, but _revoke resetting the schedule on its own sounds to me like a misleading assumption

[Amxx]

I agree with this statement. I'd be more in favor of an internal _renounceAdmin that does both the _revoke and the schedule reset, but _revoke resetting the schedule on its own sounds to me like a misleading assumption

Fair!

[ernestognw]

How should we handle this in terms of changeset? 🤔 It sounds like we need a changeset for the release automation to create a new release candidate for us, but this would be a part of the existing changeset for AccessControlDefaultAdminRules.

Easiest is to add a changeset as a placeholder. We'll need to review and edit each changesets PR opened when cherry-picking this.

A more complex solution could be configuring the release cycle to ignore certain files when converting to the Changelog, so we can add some keyword like @changesets:ignore

In any of both cases, the Changelog processing will add an empty entry for 4.9.0-rc.1, which will also be extracted into the GitHub Release page.

[ernestognw]

LGTM assuming no changes with the _revokeRole. I think that requires more discussion so far.

[Amxx]

Lets merge that, and open an issue to continue the discussion

[frangio]

Had to add a changeset first. Please re-approve.