diff --git a/.changeset/silent-dancers-type.md b/.changeset/silent-dancers-type.md new file mode 100644 index 00000000000..74ecf500d01 --- /dev/null +++ b/.changeset/silent-dancers-type.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessControlDefaultAdminRules`: Add an extension of `AccessControl` with additional security rules for the `DEFAULT_ADMIN_ROLE`. diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 5f2829e74fb..3a73de78b55 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -44,7 +44,8 @@ import "../utils/introspection/ERC165.sol"; * * WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to * grant and revoke this role. Extra precautions should be taken to secure - * accounts that have been granted it. + * accounts that have been granted it. We recommend using {AccessControlDefaultAdminRules} + * to enforce additional security measures for this role. */ abstract contract AccessControl is Context, IAccessControl, ERC165 { struct RoleData { diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol new file mode 100644 index 00000000000..e3bce6b7505 --- /dev/null +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -0,0 +1,240 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.8.0) (access/AccessControlDefaultAdminRules.sol) + +pragma solidity ^0.8.0; + +import "./AccessControl.sol"; +import "./IAccessControlDefaultAdminRules.sol"; +import "../utils/math/SafeCast.sol"; +import "../interfaces/IERC5313.sol"; + +/** + * @dev Extension of {AccessControl} that allows specifying special rules to manage + * the `DEFAULT_ADMIN_ROLE` holder, which is a sensitive role with special permissions + * over other roles that may potentially have privileged rights in the system. + * + * If a specific role doesn't have an admin role assigned, the holder of the + * `DEFAULT_ADMIN_ROLE` will have the ability to grant it and revoke it. + * + * This contract implements the following risk mitigations on top of {AccessControl}: + * + * * Only one account holds the `DEFAULT_ADMIN_ROLE` since deployment until it's potentially renounced. + * * 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. + * - Even after the timer has passed to avoid locking it forever. + * * It is not possible to use another role to manage the `DEFAULT_ADMIN_ROLE`. + * + * Example usage: + * + * ```solidity + * contract MyToken is AccessControlDefaultAdminRules { + * constructor() AccessControlDefaultAdminRules( + * 3 days, + * msg.sender // Explicit initial `DEFAULT_ADMIN_ROLE` holder + * ) {} + *} + * ``` + * + * NOTE: The `delay` can only be set in the constructor and is fixed thereafter. + * + * _Available since v4.9._ + */ +abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { + uint48 private immutable _defaultAdminDelay; + + address private _currentDefaultAdmin; + address private _pendingDefaultAdmin; + + uint48 private _defaultAdminTransferDelayedUntil; + + /** + * @dev Sets the initial values for {defaultAdminDelay} in seconds and {defaultAdmin}. + * + * The `defaultAdminDelay` value is immutable. It can only be set at the constructor. + */ + constructor(uint48 defaultAdminDelay_, address initialDefaultAdmin) { + _defaultAdminDelay = defaultAdminDelay_; + _grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin); + } + + /** + * @dev See {IERC5313-owner}. + */ + function owner() public view virtual returns (address) { + return defaultAdmin(); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function defaultAdminDelay() public view virtual returns (uint48) { + return _defaultAdminDelay; + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function defaultAdmin() public view virtual returns (address) { + return _currentDefaultAdmin; + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function pendingDefaultAdmin() public view virtual returns (address) { + return _pendingDefaultAdmin; + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { + return _defaultAdminTransferDelayedUntil; + } + + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _beginDefaultAdminTransfer(newAdmin); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function acceptDefaultAdminTransfer() public virtual { + require(_msgSender() == pendingDefaultAdmin(), "AccessControl: pending admin must accept"); + _acceptDefaultAdminTransfer(); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _resetDefaultAdminTransfer(); + } + + /** + * @dev Revokes `role` from the calling account. + * + * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required + * that the {defaultAdminTransferDelayedUntil} has passed and the pending default admin is the zero address. + * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` + * functions. + * + * For other roles, see {AccessControl-renounceRole}. + * + * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without a defaultAdmin, + * thereby disabling any functionality that is only available to the default admin, and the + * possibility of reassigning a non-administrated role. + */ + function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + if (role == DEFAULT_ADMIN_ROLE) { + require( + pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), + "AccessControl: only can renounce in two delayed steps" + ); + } + super.renounceRole(role, account); + } + + /** + * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role"); + super.grantRole(role, account); + } + + /** + * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role"); + super.revokeRole(role, account); + } + + /** + * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules"); + super._setRoleAdmin(role, adminRole); + } + + /** + * @dev Grants `role` to `account`. + * + * For `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a role's holder + * or if the role has been previously renounced. + * + * For other roles, see {AccessControl-renounceRole}. + * + * NOTE: Exposing this function through another mechanism may make the + * `DEFAULT_ADMIN_ROLE` assignable again. Make sure to guarantee this is + * the expected behavior in your implementation. + */ + function _grantRole(bytes32 role, address account) internal virtual override { + if (role == DEFAULT_ADMIN_ROLE) { + require(defaultAdmin() == address(0), "AccessControl: default admin already granted"); + _currentDefaultAdmin = account; + } + super._grantRole(role, account); + } + + /** + * @dev See {acceptDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + function _acceptDefaultAdminTransfer() internal virtual { + require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not passed"); + _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); + _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin()); + _resetDefaultAdminTransfer(); + } + + /** + * @dev See {beginDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + function _beginDefaultAdminTransfer(address newAdmin) internal virtual { + _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + defaultAdminDelay(); + _pendingDefaultAdmin = newAdmin; + emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); + } + + /** + * @dev See {AccessControl-_revokeRole}. + */ + function _revokeRole(bytes32 role, address account) internal virtual override { + if (role == DEFAULT_ADMIN_ROLE) { + delete _currentDefaultAdmin; + } + super._revokeRole(role, account); + } + + /** + * @dev Resets the pending default admin and delayed until. + */ + function _resetDefaultAdminTransfer() private { + delete _pendingDefaultAdmin; + delete _defaultAdminTransferDelayedUntil; + } + + /** + * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and passed. + */ + function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { + uint48 delayedUntil = defaultAdminTransferDelayedUntil(); + return delayedUntil > 0 && delayedUntil < block.timestamp; + } +} diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol new file mode 100644 index 00000000000..753c7c80274 --- /dev/null +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts v4.9.0 (access/IAccessControlDefaultAdminRules.sol) + +pragma solidity ^0.8.0; + +import "./IAccessControl.sol"; + +/** + * @dev External interface of AccessControlDefaultAdminRules declared to support ERC165 detection. + * + * _Available since v4.9._ + */ +interface IAccessControlDefaultAdminRules is IAccessControl { + /** + * @dev Emitted when a `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` + * as the next default admin, which will have rights to claim the `DEFAULT_ADMIN_ROLE` + * after `defaultAdminTransferDelayedUntil` has passed. + */ + event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); + + /** + * @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer. + */ + function defaultAdminDelay() external view returns (uint48); + + /** + * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. + */ + function defaultAdmin() external view returns (address); + + /** + * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. + */ + function pendingDefaultAdmin() external view returns (address); + + /** + * @dev Returns the timestamp after which the pending default admin can claim the `DEFAULT_ADMIN_ROLE`. + */ + function defaultAdminTransferDelayedUntil() external view returns (uint48); + + /** + * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin + * and a timer to pass. + * + * Requirements: + * + * - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder. + * + * Emits a {DefaultAdminRoleChangeStarted}. + */ + function beginDefaultAdminTransfer(address newAdmin) external; + + /** + * @dev Completes a `DEFAULT_ADMIN_ROLE` transfer. + * + * Requirements: + * + * - Caller should be the pending default admin. + * - `DEFAULT_ADMIN_ROLE` should be granted to the caller. + * - `DEFAULT_ADMIN_ROLE` should be revoked from the previous holder. + */ + function acceptDefaultAdminTransfer() external; + + /** + * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. + * + * Requirements: + * + * - Can be called even after the timer has passed. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. + */ + function cancelDefaultAdminTransfer() external; +} diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol index 6d4e866f4da..1378ffb4377 100644 --- a/contracts/access/Ownable.sol +++ b/contracts/access/Ownable.sol @@ -53,10 +53,10 @@ abstract contract Ownable is Context { /** * @dev Leaves the contract without owner. It will not be possible to call - * `onlyOwner` functions anymore. Can only be called by the current owner. + * `onlyOwner` functions. Can only be called by the current owner. * * NOTE: Renouncing ownership will leave the contract without an owner, - * thereby removing any functionality that is only available to the owner. + * thereby disabling any functionality that is only available to the owner. */ function renounceOwnership() public virtual onlyOwner { _transferOwnership(address(0)); diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 888d0e99e37..80ca0020f4b 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -23,3 +23,5 @@ This directory provides ways to restrict who can access the functions of a contr {{IAccessControlEnumerable}} {{AccessControlEnumerable}} + +{{AccessControlDefaultAdminRules}} diff --git a/contracts/interfaces/README.adoc b/contracts/interfaces/README.adoc index 710c078e862..4525bc9a2ee 100644 --- a/contracts/interfaces/README.adoc +++ b/contracts/interfaces/README.adoc @@ -64,6 +64,8 @@ are useful to interact with third party contracts that implement them. {{IERC4626}} +{{IERC5313}} + {{IERC5267}} {{IERC5805}} diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index f3ddb6234dd..56d91e1c1c8 100644 --- a/docs/modules/ROOT/pages/access-control.adoc +++ b/docs/modules/ROOT/pages/access-control.adoc @@ -131,6 +131,8 @@ Every role has an associated admin role, which grants permission to call the `gr This mechanism can be used to create complex permissioning structures resembling organizational charts, but it also provides an easy way to manage simpler applications. `AccessControl` includes a special role, called `DEFAULT_ADMIN_ROLE`, which acts as the **default admin role for all roles**. An account with this role will be able to manage any other role, unless `_setRoleAdmin` is used to select a new admin role. +Since it is the admin for all roles by default, and in fact it is also its own admin, this role carries significant risk. To mitigate this risk we provide xref:api:access.adoc#AccessControlDefaultAdminRules[`AccessControlDefaultAdminRules`], a recommended extension of `AccessControl` that adds a number of enforced security measures for this role: the admin is restricted to a single account, with a 2-step transfer procedure with a delay in between steps. + Let's take a look at the ERC20 token example, this time taking advantage of the default admin role: [source,solidity] diff --git a/docs/modules/ROOT/pages/extending-contracts.adoc b/docs/modules/ROOT/pages/extending-contracts.adoc index a440f406746..d12ef758557 100644 --- a/docs/modules/ROOT/pages/extending-contracts.adoc +++ b/docs/modules/ROOT/pages/extending-contracts.adoc @@ -66,6 +66,8 @@ contract ModifiedAccessControl is AccessControl { The `super.revokeRole` statement at the end will invoke ``AccessControl``'s original version of `revokeRole`, the same code that would've run if there were no overrides in place. +NOTE: The same rule is implemented and extended in xref:api:access.adoc#AccessControlDefaultAdminRules[`AccessControlDefaultAdminRules`], an extension that also adds enforced security measures for the `DEFAULT_ADMIN_ROLE`. + [[using-hooks]] == Using Hooks diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index a935609830e..e04c5a16529 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -1,5 +1,7 @@ const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); const { expect } = require('chai'); +const { time } = require('@nomicfoundation/hardhat-network-helpers'); const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior'); @@ -210,8 +212,290 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, }); } +function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defaultAdmin, newDefaultAdmin, other) { + shouldSupportInterfaces(['AccessControlDefaultAdminRules']); + + it('has a default disabled delayed until', async function () { + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + }); + + it('has a default pending default admin', async function () { + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + }); + + it('has a default current owner set to the initial default admin', async function () { + const owner = await this.accessControl.owner(); + expect(owner).to.equal(defaultAdmin); + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, owner)).to.be.true; + }); + + it('should revert if granting default admin role', async function () { + await expectRevert( + this.accessControl.grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }), + `${errorPrefix}: can't directly grant default admin role`, + ); + }); + + it('should revert if revoking default admin role', async function () { + await expectRevert( + this.accessControl.revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }), + `${errorPrefix}: can't directly revoke default admin role`, + ); + }); + + it("should revert if defaultAdmin's admin is changed", async function () { + await expectRevert( + this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, defaultAdmin), + `${errorPrefix}: can't violate default admin rules`, + ); + }); + + it('should not grant the default admin role twice', async function () { + await expectRevert( + this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin), + `${errorPrefix}: default admin already granted`, + ); + }); + + describe('begins transfer of default admin', function () { + let receipt; + let defaultAdminTransferDelayedUntil; + + beforeEach('begins admin transfer', async function () { + receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + defaultAdminTransferDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); + }); + + it('should set pending default admin and delayed until', async function () { + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + defaultAdminTransferDelayedUntil, + ); + expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { + newDefaultAdmin, + defaultAdminTransferDelayedUntil, + }); + }); + + it('should be able to begin a transfer again before delay pass', async function () { + // Time passes just before delay + await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.subn(1)); + + // defaultAdmin changes its mind and begin again to another address + await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); + const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); + }); + + it('should be able to begin a transfer again after delay pass if not accepted', async function () { + // Time passes after delay without acceptance + await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.addn(1)); + + // defaultAdmin changes its mind and begin again to another address + await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); + const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); + }); + + it('should revert if it is called by non-admin accounts', async function () { + await expectRevert( + this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + }); + + describe('accepts transfer admin', function () { + let delayPassed; + + beforeEach(async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); + }); + + describe('caller is pending default admin and delay has passed', function () { + let from; + + beforeEach(async function () { + await time.setNextBlockTimestamp(delayPassed); + from = newDefaultAdmin; + }); + + it('accepts a transfer and changes default admin', async function () { + const receipt = await this.accessControl.acceptDefaultAdminTransfer({ from }); + + // Storage changes + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, newDefaultAdmin)).to.be.true; + expect(await this.accessControl.owner()).to.equal(newDefaultAdmin); + + // Emit events + expectEvent(receipt, 'RoleRevoked', { + role: DEFAULT_ADMIN_ROLE, + account: defaultAdmin, + }); + expectEvent(receipt, 'RoleGranted', { + role: DEFAULT_ADMIN_ROLE, + account: newDefaultAdmin, + }); + + // Resets pending default admin and delayed until + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + }); + }); + + it('should revert if caller is not pending default admin', async function () { + await time.setNextBlockTimestamp(delayPassed); + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: other }), + `${errorPrefix}: pending admin must accept`, + ); + }); + + describe('delayedUntil not passed', function () { + let delayNotPassed; + + beforeEach(function () { + delayNotPassed = delayPassed.subn(1); + }); + + it('should revert if block.timestamp is equal to delayed until', async function () { + await time.setNextBlockTimestamp(delayNotPassed); + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), + `${errorPrefix}: transfer delay not passed`, + ); + }); + + it('should revert if block.timestamp is less than delayed until', async function () { + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), + `${errorPrefix}: transfer delay not passed`, + ); + }); + }); + }); + + describe('cancel transfer default admin', function () { + let delayPassed; + + beforeEach(async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); + }); + + it('resets pending default admin and delayed until', async function () { + await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + + // Advance until passed delay + await time.setNextBlockTimestamp(delayPassed); + + // Previous pending default admin should not be able to accept after cancellation. + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), + `${errorPrefix}: pending admin must accept`, + ); + }); + + it('cancels even after delay has passed', async function () { + await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + await time.setNextBlockTimestamp(delayPassed); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + }); + + it('reverts if called by non default admin accounts', async function () { + await expectRevert( + this.accessControl.cancelDefaultAdminTransfer({ from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + }); + + describe('renouncing admin', function () { + let delayPassed; + let from = defaultAdmin; + + beforeEach(async function () { + await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); + }); + + it('it renounces role', async function () { + await time.setNextBlockTimestamp(delayPassed); + const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); + + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; + expect(await this.accessControl.hasRole(ZERO_ADDRESS, defaultAdmin)).to.be.false; + expectEvent(receipt, 'RoleRevoked', { + role: DEFAULT_ADMIN_ROLE, + account: from, + }); + expect(await this.accessControl.owner()).to.equal(ZERO_ADDRESS); + }); + + it('allows to recover access using the internal _grantRole', async function () { + await time.setNextBlockTimestamp(delayPassed); + await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); + + const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); + expectEvent(grantRoleReceipt, 'RoleGranted', { + role: DEFAULT_ADMIN_ROLE, + account: other, + }); + }); + + it('reverts if caller is not default admin', async function () { + await time.setNextBlockTimestamp(delayPassed); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), + `${errorPrefix}: can only renounce roles for self`, + ); + }); + + describe('delayed until not passed', function () { + let delayNotPassed; + + beforeEach(function () { + delayNotPassed = delayPassed.subn(1); + }); + + it('reverts if block.timestamp is equal to delayed until', async function () { + await time.setNextBlockTimestamp(delayNotPassed); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), + `${errorPrefix}: only can renounce in two delayed steps`, + ); + }); + + it('reverts if block.timestamp is less than delayed until', async function () { + await time.setNextBlockTimestamp(delayNotPassed.subn(1)); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), + `${errorPrefix}: only can renounce in two delayed steps`, + ); + }); + }); + }); +} + module.exports = { DEFAULT_ADMIN_ROLE, shouldBehaveLikeAccessControl, shouldBehaveLikeAccessControlEnumerable, + shouldBehaveLikeAccessControlDefaultAdminRules, }; diff --git a/test/access/AccessControlDefaultAdminRules.test.js b/test/access/AccessControlDefaultAdminRules.test.js new file mode 100644 index 00000000000..2a23d3b6dab --- /dev/null +++ b/test/access/AccessControlDefaultAdminRules.test.js @@ -0,0 +1,18 @@ +const { time } = require('@openzeppelin/test-helpers'); +const { + shouldBehaveLikeAccessControl, + shouldBehaveLikeAccessControlDefaultAdminRules, +} = require('./AccessControl.behavior.js'); + +const AccessControlDefaultAdminRules = artifacts.require('$AccessControlDefaultAdminRules'); + +contract('AccessControlDefaultAdminRules', function (accounts) { + const delay = web3.utils.toBN(time.duration.days(10)); + + beforeEach(async function () { + this.accessControl = await AccessControlDefaultAdminRules.new(delay, accounts[0], { from: accounts[0] }); + }); + + shouldBehaveLikeAccessControl('AccessControl', ...accounts); + shouldBehaveLikeAccessControlDefaultAdminRules('AccessControl', delay, ...accounts); +}); diff --git a/test/access/Ownable.test.js b/test/access/Ownable.test.js index dc308f98f65..109150874c3 100644 --- a/test/access/Ownable.test.js +++ b/test/access/Ownable.test.js @@ -37,7 +37,7 @@ contract('Ownable', function (accounts) { }); describe('renounce ownership', function () { - it('loses owner after renouncement', async function () { + it('loses ownership after renouncement', async function () { const receipt = await this.ownable.renounceOwnership({ from: owner }); expectEvent(receipt, 'OwnershipTransferred'); @@ -47,5 +47,13 @@ contract('Ownable', function (accounts) { it('prevents non-owners from renouncement', async function () { await expectRevert(this.ownable.renounceOwnership({ from: other }), 'Ownable: caller is not the owner'); }); + + it('allows to recover access using the internal _transferOwnership', async function () { + await this.ownable.renounceOwnership({ from: owner }); + const receipt = await this.ownable.$_transferOwnership(other); + expectEvent(receipt, 'OwnershipTransferred'); + + expect(await this.ownable.owner()).to.equal(other); + }); }); }); diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index 64d43276244..ce043057bec 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -27,6 +27,16 @@ contract('Ownable2Step', function (accounts) { expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); }); + it('guards transfer against invalid user', async function () { + await this.ownable2Step.transferOwnership(accountA, { from: owner }); + await expectRevert( + this.ownable2Step.acceptOwnership({ from: accountB }), + 'Ownable2Step: caller is not the new owner', + ); + }); + }); + + it('renouncing ownership', async function () { it('changes owner after renouncing ownership', async function () { await this.ownable2Step.renounceOwnership({ from: owner }); // If renounceOwnership is removed from parent an alternative is needed ... @@ -46,12 +56,12 @@ contract('Ownable2Step', function (accounts) { ); }); - it('guards transfer against invalid user', async function () { - await this.ownable2Step.transferOwnership(accountA, { from: owner }); - await expectRevert( - this.ownable2Step.acceptOwnership({ from: accountB }), - 'Ownable2Step: caller is not the new owner', - ); + it('allows to recover access using the internal _transferOwnership', async function () { + await this.ownable.renounceOwnership({ from: owner }); + const receipt = await this.ownable.$_transferOwnership(accountA); + expectEvent(receipt, 'OwnershipTransferred'); + + expect(await this.ownable.owner()).to.equal(accountA); }); }); }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 02d147884e4..5ffe242edc5 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -37,6 +37,15 @@ const INTERFACES = { 'renounceRole(bytes32,address)', ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], + AccessControlDefaultAdminRules: [ + 'defaultAdminDelay()', + 'defaultAdmin()', + 'defaultAdminTransferDelayedUntil()', + 'pendingDefaultAdmin()', + 'beginDefaultAdminTransfer(address)', + 'acceptDefaultAdminTransfer()', + 'cancelDefaultAdminTransfer()', + ], Governor: [ 'name()', 'version()',