Fix bug allowing anyone to cancel an admin renounce (#4238)

Co-authored-by: Francisco Giordano <fg@frang.io>
OpenZeppelin · May 11, 2023 · 3ec4307 · 3ec4307
1 parent f355bd3
commit 3ec4307
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 12 deletions.
diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol
@@ -106,7 +106,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
      * non-administrated role.
      */
     function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
-        if (role == DEFAULT_ADMIN_ROLE) {
+        if (role == DEFAULT_ADMIN_ROLE && account == defaultAdmin()) {
             (address newDefaultAdmin, uint48 schedule) = pendingDefaultAdmin();
             require(
                 newDefaultAdmin == address(0) && _isScheduleSet(schedule) && _hasSchedulePassed(schedule),
@@ -138,7 +138,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
      * @dev See {AccessControl-_revokeRole}.
      */
     function _revokeRole(bytes32 role, address account) internal virtual override {
-        if (role == DEFAULT_ADMIN_ROLE && account == _currentDefaultAdmin) {
+        if (role == DEFAULT_ADMIN_ROLE && account == defaultAdmin()) {
             delete _currentDefaultAdmin;
         }
         super._revokeRole(role, account);

diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js
@@ -621,14 +621,15 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
   });
 
   describe('renounces admin', function () {
+    let expectedSchedule;
     let delayPassed;
+    let delayNotPassed;
 
     beforeEach(async function () {
       await this.accessControl.beginDefaultAdminTransfer(constants.ZERO_ADDRESS, { from: defaultAdmin });
-      delayPassed = web3.utils
-        .toBN(await time.latest())
-        .add(delay)
-        .addn(1);
+      expectedSchedule = web3.utils.toBN(await time.latest()).add(delay);
+      delayNotPassed = expectedSchedule;
+      delayPassed = expectedSchedule.addn(1);
     });
 
     it('reverts if caller is not default admin', async function () {
@@ -639,6 +640,15 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
       );
     });
 
+    it("renouncing the admin role when not an admin doesn't affect the schedule", async function () {
+      await time.setNextBlockTimestamp(delayPassed);
+      await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from: other });
+
+      const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin();
+      expect(newAdmin).to.equal(constants.ZERO_ADDRESS);
+      expect(schedule).to.be.bignumber.equal(expectedSchedule);
+    });
+
     it('keeps defaultAdmin consistent with hasRole if another non-defaultAdmin user renounces the DEFAULT_ADMIN_ROLE', async function () {
       await time.setNextBlockTimestamp(delayPassed);
 
@@ -677,12 +687,6 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
     });
 
     describe('schedule not passed', function () {
-      let delayNotPassed;
-
-      beforeEach(function () {
-        delayNotPassed = delayPassed.subn(1);
-      });
-
       for (const [fromSchedule, tag] of [
         [-1, 'less'],
         [0, 'equal'],