NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix AccessControlDefaultAdminRules admin consistency (#4177)

Browse Source 
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
This commit is contained in:
Ernesto García
2023-04-28 15:09:58 +02:00
committed by GitHub
parent 44d6053b43
commit d23f818a59
2 changed files with 19 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
contracts/access/AccessControlDefaultAdminRules.sol
View File

@ -136,7 +136,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
* @dev See {AccessControl-_revokeRole}. * @dev See {AccessControl-_revokeRole}.
*/ */
function _revokeRole(bytes32 role, address account) internal virtual override { function _revokeRole(bytes32 role, address account) internal virtual override {
if (role == DEFAULT_ADMIN_ROLE) { if (role == DEFAULT_ADMIN_ROLE && account == _currentDefaultAdmin) {
delete _currentDefaultAdmin; delete _currentDefaultAdmin;
} }
super._revokeRole(role, account); super._revokeRole(role, account);

30
test/access/AccessControl.behavior.js
View File

@ -513,15 +513,12 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
}); });
describe('when caller is pending default admin and delay has passed', function () { describe('when caller is pending default admin and delay has passed', function () {
let from;
beforeEach(async function () { beforeEach(async function () {
await time.setNextBlockTimestamp(acceptSchedule.addn(1)); await time.setNextBlockTimestamp(acceptSchedule.addn(1));
from = newDefaultAdmin;
}); });
it('accepts a transfer and changes default admin', async function () { it('accepts a transfer and changes default admin', async function () {
const receipt = await this.accessControl.acceptDefaultAdminTransfer({ from }); const receipt = await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin });
// Storage changes // Storage changes
expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false;
@ -625,10 +622,9 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
describe('renounces admin', function () { describe('renounces admin', function () {
let delayPassed; let delayPassed;
let from = defaultAdmin;
beforeEach(async function () { beforeEach(async function () {
await this.accessControl.beginDefaultAdminTransfer(constants.ZERO_ADDRESS, { from }); await this.accessControl.beginDefaultAdminTransfer(constants.ZERO_ADDRESS, { from: defaultAdmin });
delayPassed = web3.utils delayPassed = web3.utils
.toBN(await time.latest()) .toBN(await time.latest())
.add(delay) .add(delay)
@ -638,27 +634,37 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
it('reverts if caller is not default admin', async function () { it('reverts if caller is not default admin', async function () {
await time.setNextBlockTimestamp(delayPassed); await time.setNextBlockTimestamp(delayPassed);
await expectRevert( await expectRevert(
this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from: defaultAdmin }),
`${errorPrefix}: can only renounce roles for self`, `${errorPrefix}: can only renounce roles for self`,
); );
}); });
it('keeps defaultAdmin consistent with hasRole if another non-defaultAdmin user renounces the DEFAULT_ADMIN_ROLE', async function () {
await time.setNextBlockTimestamp(delayPassed);
// This passes because it's a noop
await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from: other });
expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.true;
expect(await this.accessControl.defaultAdmin()).to.be.equal(defaultAdmin);
});
it('renounces role', async function () { it('renounces role', async function () {
await time.setNextBlockTimestamp(delayPassed); await time.setNextBlockTimestamp(delayPassed);
const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin });
expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false;
expect(await this.accessControl.hasRole(constants.ZERO_ADDRESS, defaultAdmin)).to.be.false; expect(await this.accessControl.defaultAdmin()).to.be.equal(constants.ZERO_ADDRESS);
expectEvent(receipt, 'RoleRevoked', { expectEvent(receipt, 'RoleRevoked', {
role: DEFAULT_ADMIN_ROLE, role: DEFAULT_ADMIN_ROLE,
account: from, account: defaultAdmin,
}); });
expect(await this.accessControl.owner()).to.equal(constants.ZERO_ADDRESS); expect(await this.accessControl.owner()).to.equal(constants.ZERO_ADDRESS);
}); });
it('allows to recover access using the internal _grantRole', async function () { it('allows to recover access using the internal _grantRole', async function () {
await time.setNextBlockTimestamp(delayPassed); await time.setNextBlockTimestamp(delayPassed);
await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin });
const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other);
expectEvent(grantRoleReceipt, 'RoleGranted', { expectEvent(grantRoleReceipt, 'RoleGranted', {
@ -681,7 +687,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
it(`reverts if block.timestamp is ${tag} to schedule`, async function () { it(`reverts if block.timestamp is ${tag} to schedule`, async function () {
await time.setNextBlockTimestamp(delayNotPassed.toNumber() + fromSchedule); await time.setNextBlockTimestamp(delayNotPassed.toNumber() + fromSchedule);
await expectRevert( await expectRevert(
this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }),
`${errorPrefix}: only can renounce in two delayed steps`, `${errorPrefix}: only can renounce in two delayed steps`,
); );
}); });