From 3989054a3042037c5c4c24ad298a2be35676d85b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 14:22:51 -0600 Subject: [PATCH 01/69] Create `AccessControlAdminRules` and its corresponding interface --- contracts/access/AccessControlAdminRules.sol | 224 ++++++++++++++++++ contracts/access/IAccessControlAdminRules.sol | 77 ++++++ 2 files changed, 301 insertions(+) create mode 100644 contracts/access/AccessControlAdminRules.sol create mode 100644 contracts/access/IAccessControlAdminRules.sol diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol new file mode 100644 index 00000000000..a9df5c59345 --- /dev/null +++ b/contracts/access/AccessControlAdminRules.sol @@ -0,0 +1,224 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.8.0) (access/AccessControlAdminRules.sol) + +pragma solidity ^0.8.0; + +import "./AccessControl.sol"; +import "./IAccessControlAdminRules.sol"; +import "../utils/math/SafeCast.sol"; + +/** + * @dev Extension of {AccessControl} that allows to specify special rules to manage + * the `DEFAULT_ADMIN_ROLE`´s owner, which is a sensitive role with special permissions + * over other valid roles that may potentially have rights. + * + * 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}'s default value (`address(0)`). + * + * This contract implements the following risk mitigations on top of the AccessControl implementation: + * + * - Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when renounced. + * - Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. Even when it's been renounced. + * - 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. + * - The `DEFAULT_ADMIN_ROLE`'s admin can be only held by itself. + * + * NOTE: `delay` is only configurable in the constructor to avoid issues related with handling delay management during the transfer is pending to be completed. + * + * _Available since v4.9._ + */ +abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessControl { + uint48 private immutable _delay; + + bool private _hasAdmin; + address private _currentAdmin; + uint48 private _delayedUntil; + + address private _pendingAdmin; + + /** + * @dev Sets the values for {name} and {symbol}. + * + * All two of these values are immutable: they can only be set once during + * construction. + */ + constructor(uint48 initialDelay, address initialAdmin) { + _delay = initialDelay; + _grantRole(DEFAULT_ADMIN_ROLE, initialAdmin); + } + + /** + * @dev See {IAccessControlAdminRules-hasAdmin} + */ + function hasAdmin() public view virtual returns (bool) { + return _hasAdmin; + } + + /** + * @dev See {IAccessControlAdminRules-owner} + */ + function owner() public view virtual returns (address) { + return _currentAdmin; + } + + /** + * @dev See {IAccessControlAdminRules-delayedUntil} + */ + function delayedUntil() public view virtual returns (uint48) { + return _delayedUntil; + } + + /** + * @dev See {IAccessControlAdminRules-pendingAdmin} + */ + function pendingAdmin() public view virtual returns (address) { + return _pendingAdmin; + } + + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IAccessControlAdminRules).interfaceId || super.supportsInterface(interfaceId); + } + + /** + * @dev See {IAccessControlAdminRules-beginAdminTransfer} + */ + function beginAdminTransfer(address newAdmin) public override onlyRole(DEFAULT_ADMIN_ROLE) { + uint48 delayedUntilTimestamp = delayedUntil(); + require(delayedUntilTimestamp == 0, "AdminRules: pending admin already set"); + _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; + _pendingAdmin = newAdmin; + emit AdminRoleChangeStarted(pendingAdmin(), delayedUntilTimestamp); + } + + /** + * @dev See {IAccessControlAdminRules-acceptAdminTransfer} + */ + function acceptAdminTransfer() public { + address pendingAdminOwner = pendingAdmin(); + require( + _adminTransferIsUnlocked() && _msgSender() == pendingAdminOwner, + "AdminRules: delay must be met and caller must be pending admin" + ); + _revokeRole(DEFAULT_ADMIN_ROLE, owner()); + _grantRole(DEFAULT_ADMIN_ROLE, pendingAdminOwner); + } + + /** + * @dev See {IAccessControlAdminRules-cancelAdminTransfer} + */ + function cancelAdminTransfer() external override onlyRole(DEFAULT_ADMIN_ROLE) { + _resetAdminTransfer(); + } + + /** + * @dev Revokes `role` from the calling account. + * + * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required + * that the {delayedUntil} is met and the pending admin is the zero address. + * + * For other roles, see {AccessControl-renounceRole}. + * + * May emit a {RoleRevoked} event. + * + * NOTE: {AccessControl-renounceRole} already checks that caller is `account`. + * + * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without an owner, + * thereby removing 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 override(IAccessControl, AccessControl) { + if (role == DEFAULT_ADMIN_ROLE) { + require( + pendingAdmin() == address(0) && _adminTransferIsUnlocked(), + "AdminRules: admin can only renounce in two steps" + ); + } + super.renounceRole(role, account); + _hasAdmin = true; // Force locking forever + } + + /** + * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function grantRole( + bytes32 role, + address account + ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't directly grant admin role"); + super.grantRole(role, account); + } + + /** + * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function revokeRole( + bytes32 role, + address account + ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't directly revoke admin role"); + super.revokeRole(role, account); + } + + /** + * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal override { + require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't override admin's admin"); + 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 owner + * 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. + * + * May emit a {RoleGranted} event. + */ + function _grantRole(bytes32 role, address account) internal override { + if (role == DEFAULT_ADMIN_ROLE) { + require(!hasAdmin(), "AdminRules: admin already granted"); + _hasAdmin = true; + } + super._grantRole(role, account); + } + + /** + * @dev Revokes `role` from `account`. + * + * Internal function without access restriction. + * + * May emit a {RoleRevoked} event. + */ + function _revokeRole(bytes32 role, address account) internal override { + if (role == DEFAULT_ADMIN_ROLE) { + _hasAdmin = false; + } + super._revokeRole(role, account); + } + + /** + * @dev Resets the pending admin and delayed until. + */ + function _resetAdminTransfer() private { + delete _pendingAdmin; + delete _delayedUntil; + } + + /** + * @dev Checks if a {delayUntil} has been set and met. + */ + function _adminTransferIsUnlocked() private view returns (bool) { + uint48 delayedUntilTimestamp = delayedUntil(); + return delayedUntilTimestamp > 0 && delayedUntilTimestamp > block.timestamp; + } +} diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol new file mode 100644 index 00000000000..d9a371f44e8 --- /dev/null +++ b/contracts/access/IAccessControlAdminRules.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts v4.9.0 (access/IAccessControlAdminRules.sol) + +pragma solidity ^0.8.0; + +import "./IAccessControl.sol"; + +/** + * @dev External interface of AccessControladminRules declared to support ERC165 detection. + * + * _Available since v4.9._ + */ +interface IAccessControlAdminRules is IAccessControl { + /** + * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newAdmin` + * as the next owner to be claimed after `delayedUntil` is met. + */ + event AdminRoleChangeStarted(address indexed newAdmin, uint48 delayedUntil); + + /** + * @dev Returns a boolean indicating if there's a `DEFAULT_ADMIN_ROLE`'s owner. + * `address(0)` shouldn't be considered an owner. + */ + function hasAdmin() external view returns (bool); + + /** + * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` owner. + */ + function owner() external view returns (address); + + /** + * @dev Returns the timestamp in which the pending admin can claim the + * `DEFAULT_ADMIN_ROLE` role. + */ + function delayedUntil() external view returns (uint48); + + /** + * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` owner. + */ + function pendingAdmin() external view returns (address); + + /** + * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending admin and + * a timer to be met. + * + * Requirements: + * + * - There shouldn't be another admin transfer in progress. See {cancelAdminTransfer}. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` owner. + * + * Emits an {AdminRoleChangeStarted}. + */ + function beginAdminTransfer(address newAdmin) external; + + /** + * @dev Completes a `DEFAULT_ADMIN_ROLE` transfer. + * + * Requirements: + * + * - Caller should be the pending admin. + * - `DEFAULT_ADMIN_ROLE` should be granted to the caller. + * - `DEFAULT_ADMIN_ROLE` should be revoked from the previous owner. + * - Should allow to call {beginAdminTransfer} again. + */ + function acceptAdminTransfer() external; + + /** + * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. + * + * Requirements: + * + * - Should allow to call {beginAdminTransfer} again. + * - Can be called even after the timer is met. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` owner. + */ + function cancelAdminTransfer() external; +} From da5267839c4a7583f792546505079e80b10f423b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 18:13:49 -0600 Subject: [PATCH 02/69] Add tests --- contracts/access/AccessControlAdminRules.sol | 47 ++-- contracts/access/IAccessControlAdminRules.sol | 6 - contracts/access/Ownable.sol | 4 +- test/access/AccessControl.behavior.js | 251 +++++++++++++++++- test/access/AccessControlAdminRules.test.js | 18 ++ test/access/Ownable.test.js | 10 +- test/access/Ownable2Step.test.js | 22 +- .../SupportsInterface.behavior.js | 8 + 8 files changed, 318 insertions(+), 48 deletions(-) create mode 100644 test/access/AccessControlAdminRules.test.js diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index a9df5c59345..dc5d113d8cf 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -30,7 +30,6 @@ import "../utils/math/SafeCast.sol"; abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessControl { uint48 private immutable _delay; - bool private _hasAdmin; address private _currentAdmin; uint48 private _delayedUntil; @@ -47,13 +46,6 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon _grantRole(DEFAULT_ADMIN_ROLE, initialAdmin); } - /** - * @dev See {IAccessControlAdminRules-hasAdmin} - */ - function hasAdmin() public view virtual returns (bool) { - return _hasAdmin; - } - /** * @dev See {IAccessControlAdminRules-owner} */ @@ -86,11 +78,10 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * @dev See {IAccessControlAdminRules-beginAdminTransfer} */ function beginAdminTransfer(address newAdmin) public override onlyRole(DEFAULT_ADMIN_ROLE) { - uint48 delayedUntilTimestamp = delayedUntil(); - require(delayedUntilTimestamp == 0, "AdminRules: pending admin already set"); + require(delayedUntil() == 0, "AccessControl: pending admin already set"); _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingAdmin = newAdmin; - emit AdminRoleChangeStarted(pendingAdmin(), delayedUntilTimestamp); + emit AdminRoleChangeStarted(pendingAdmin(), delayedUntil()); } /** @@ -100,10 +91,11 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon address pendingAdminOwner = pendingAdmin(); require( _adminTransferIsUnlocked() && _msgSender() == pendingAdminOwner, - "AdminRules: delay must be met and caller must be pending admin" + "AccessControl: delay must be met and caller must be pending admin" ); _revokeRole(DEFAULT_ADMIN_ROLE, owner()); _grantRole(DEFAULT_ADMIN_ROLE, pendingAdminOwner); + _resetAdminTransfer(); } /** @@ -118,26 +110,23 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required * that the {delayedUntil} is met and the pending 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}. * - * May emit a {RoleRevoked} event. - * - * NOTE: {AccessControl-renounceRole} already checks that caller is `account`. - * * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without an owner, - * thereby removing any functionality that is only available to the default admin, and the + * 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 override(IAccessControl, AccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( pendingAdmin() == address(0) && _adminTransferIsUnlocked(), - "AdminRules: admin can only renounce in two steps" + "AccessControl: admin can only renounce in two delayed steps" ); } super.renounceRole(role, account); - _hasAdmin = true; // Force locking forever } /** @@ -147,7 +136,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon bytes32 role, address account ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { - require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't directly grant admin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant admin role"); super.grantRole(role, account); } @@ -158,7 +147,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon bytes32 role, address account ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { - require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't directly revoke admin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke admin role"); super.revokeRole(role, account); } @@ -166,7 +155,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. */ function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal override { - require(role != DEFAULT_ADMIN_ROLE, "AdminRules: can't override admin's admin"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't override admin's admin"); super._setRoleAdmin(role, adminRole); } @@ -181,27 +170,21 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * 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. - * - * May emit a {RoleGranted} event. */ function _grantRole(bytes32 role, address account) internal override { if (role == DEFAULT_ADMIN_ROLE) { - require(!hasAdmin(), "AdminRules: admin already granted"); - _hasAdmin = true; + require(owner() == address(0), "AccessControl: admin already granted"); + _currentAdmin = account; } super._grantRole(role, account); } /** - * @dev Revokes `role` from `account`. - * - * Internal function without access restriction. - * - * May emit a {RoleRevoked} event. + * @dev See {AccessControl-_revokeRole}. */ function _revokeRole(bytes32 role, address account) internal override { if (role == DEFAULT_ADMIN_ROLE) { - _hasAdmin = false; + delete _currentAdmin; } super._revokeRole(role, account); } diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol index d9a371f44e8..4da58587c1a 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlAdminRules.sol @@ -17,12 +17,6 @@ interface IAccessControlAdminRules is IAccessControl { */ event AdminRoleChangeStarted(address indexed newAdmin, uint48 delayedUntil); - /** - * @dev Returns a boolean indicating if there's a `DEFAULT_ADMIN_ROLE`'s owner. - * `address(0)` shouldn't be considered an owner. - */ - function hasAdmin() external view returns (bool); - /** * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` owner. */ 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/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index a935609830e..5b05bb9c5c2 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -1,4 +1,5 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); const { expect } = require('chai'); const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior'); @@ -210,8 +211,256 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, }); } +function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newAdmin, other) { + shouldSupportInterfaces(['AccessControlAdminRules']); + + it('has a default disabled delayed until', async function () { + expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + }); + + it('has a default pending admin', async function () { + expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + }); + + it('has a default current owner set to the initial owner', async function () { + const owner = await this.accessControl.owner(); + expect(owner).to.equal(admin); + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, owner)).to.be.true; + }); + + it('should revert if granting admin role', async function () { + await expectRevert( + this.accessControl.grantRole(DEFAULT_ADMIN_ROLE, admin, { from: admin }), + `${errorPrefix}: can't directly grant admin role`, + ); + }); + + it('should revert if revoking admin role', async function () { + await expectRevert( + this.accessControl.revokeRole(DEFAULT_ADMIN_ROLE, admin, { from: admin }), + `${errorPrefix}: can't directly revoke admin role`, + ); + }); + + it("should revert if admin's is changed", async function () { + await expectRevert( + this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, admin), + `${errorPrefix}: can't override admin's admin`, + ); + }); + + it('should not grant the admin role twice', async function () { + await expectRevert( + this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, admin), + `${errorPrefix}: admin already granted`, + ); + }); + + describe('begins transfer admin', async function () { + it('should set pending admin and delayed until', async function () { + const receipt = await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + const delayedUntil = (await time.latest()).add(delay); + expect(await this.accessControl.pendingAdmin()).to.equal(newAdmin); + expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal((await time.latest()).add(delay)); + expectEvent(receipt, 'AdminRoleChangeStarted', { newAdmin, delayedUntil }); + }); + + it('should revert if it called by non-admin accounts', async function () { + await expectRevert( + this.accessControl.beginAdminTransfer(newAdmin, { from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + + it('should revert if another transfer has started', async function () { + await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + await expectRevert( + this.accessControl.beginAdminTransfer(other, { from: admin }), + `${errorPrefix}: pending admin already set`, + ); + }); + }); + + describe('accepts transfer admin', async function () { + let correctPendingAdmin; + let correctIncreaseTo; + + beforeEach(async function () { + // Set as correct so they're explicitly needed in revert tests, since conditions + // are not mutually exclusive and accidentally an incorrect value will make it revert + // possiblty creating false positives (eg. expect revert for incorrect caller but getting it + // from a badly expected delayed until) + correctPendingAdmin = newAdmin; + correctIncreaseTo = (await time.latest()).add(delay).subn(1); + + await this.accessControl.beginAdminTransfer(correctPendingAdmin, { from: admin }); + }); + + describe('caller is pending admin and delayed until is met', async function () { + let from; + let receipt; + + beforeEach(async function () { + await time.increaseTo(correctIncreaseTo); + from = correctPendingAdmin; + receipt = await this.accessControl.acceptAdminTransfer({ from }); + }); + + it('accepts a transfer and changes admin', async function () { + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.be.false; + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, newAdmin)).to.be.true; + expect(await this.accessControl.owner()).to.equal(newAdmin); + }); + + it('accepts a transfer and emit events', async function () { + expectEvent(receipt, 'RoleRevoked', { + role: DEFAULT_ADMIN_ROLE, + account: admin, + }); + expectEvent(receipt, 'RoleGranted', { + role: DEFAULT_ADMIN_ROLE, + account: newAdmin, + }); + }); + + it('accepts a transfer resetting pending admin and delayed until', async function () { + expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + }); + }); + + it('should revert if caller is not pending admin', async function () { + await time.increaseTo(correctIncreaseTo); + await expectRevert( + this.accessControl.acceptAdminTransfer({ from: other }), + `${errorPrefix}: delay must be met and caller must be pending admin`, + ); + }); + + describe('delayed until not met', async function () { + let incorrectIncreaseTo; + + beforeEach(async function () { + incorrectIncreaseTo = correctIncreaseTo.addn(1); + }); + + it('should revert if block.timestamp is equal to delayed until', async function () { + await time.increaseTo(incorrectIncreaseTo); + await expectRevert( + this.accessControl.acceptAdminTransfer({ from: correctPendingAdmin }), + `${errorPrefix}: delay must be met and caller must be pending admin`, + ); + }); + + it('should revert if block.timestamp is less to delayed until', async function () { + await time.increaseTo(incorrectIncreaseTo.addn(1)); + await expectRevert( + this.accessControl.acceptAdminTransfer({ from: correctPendingAdmin }), + `${errorPrefix}: delay must be met and caller must be pending admin`, + ); + }); + }); + }); + + describe('cancel transfer admin', async function () { + beforeEach(async function () { + await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + }); + + it('resets pending admin and delayed until', async function () { + await this.accessControl.cancelAdminTransfer({ from: admin }); + expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + }); + + it('reverts if called by non-admin accounts', async function () { + await expectRevert( + this.accessControl.cancelAdminTransfer({ from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + }); + + describe('renouncing admin', async function () { + let correctIncreaseTo; + let from = admin; + + beforeEach(async function () { + correctIncreaseTo = (await time.latest()).add(delay).subn(1); + await this.accessControl.beginAdminTransfer(ZERO_ADDRESS, { from }); + }); + + describe('caller is admin and delayed until is met', async function () { + let receipt; + + beforeEach(async function () { + await time.increaseTo(correctIncreaseTo); + from = admin; + receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); + }); + + it('renounces role and does not grant it to the ZERO ADDRESS', async function () { + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.be.false; + expect(await this.accessControl.hasRole(ZERO_ADDRESS, admin)).to.be.false; + }); + + it('emits events', async function () { + expectEvent(receipt, 'RoleRevoked', { + role: DEFAULT_ADMIN_ROLE, + account: from, + }); + }); + + it('marks the contract as not owned', async function () { + expect(await this.accessControl.owner()).to.equal(ZERO_ADDRESS); + }); + + it('allows to recover access using the internal _grantRole', async function () { + 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 admin', async function () { + await time.increaseTo(correctIncreaseTo); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), + `${errorPrefix}: can only renounce roles for self`, + ); + }); + + describe('delayed until not met', async function () { + let incorrectIncreaseTo; + + beforeEach(async function () { + incorrectIncreaseTo = correctIncreaseTo.addn(1); + }); + + it('reverts if block.timestamp is equal to delayed until', async function () { + await time.increaseTo(incorrectIncreaseTo); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, admin, { from }), + `${errorPrefix}: admin can only renounce in two delayed steps`, + ); + }); + + it('reverts if block.timestamp is less to delayed until', async function () { + await time.increaseTo(incorrectIncreaseTo.addn(1)); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, admin, { from }), + `${errorPrefix}: admin can only renounce in two delayed steps`, + ); + }); + }); + }); +} + module.exports = { DEFAULT_ADMIN_ROLE, shouldBehaveLikeAccessControl, shouldBehaveLikeAccessControlEnumerable, + shouldBehaveLikeAccessControlAdminRules, }; diff --git a/test/access/AccessControlAdminRules.test.js b/test/access/AccessControlAdminRules.test.js new file mode 100644 index 00000000000..338a4dedb66 --- /dev/null +++ b/test/access/AccessControlAdminRules.test.js @@ -0,0 +1,18 @@ +const { time } = require('@openzeppelin/test-helpers'); +const { + shouldBehaveLikeAccessControl, + shouldBehaveLikeAccessControlAdminRules, +} = require('./AccessControl.behavior.js'); + +const AccessControlAdminRules = artifacts.require('$AccessControlAdminRules'); + +contract('AccessControlAdminRules', function (accounts) { + const delay = web3.utils.toBN(time.duration.days(10)); + + beforeEach(async function () { + this.accessControl = await AccessControlAdminRules.new(delay, accounts[0], { from: accounts[0] }); + }); + + shouldBehaveLikeAccessControl('AccessControl', ...accounts); + shouldBehaveLikeAccessControlAdminRules('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 ba5aca2fc06..f0a46fc45e0 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -37,6 +37,14 @@ const INTERFACES = { 'renounceRole(bytes32,address)', ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], + AccessControlAdminRules: [ + 'owner()', + 'delayedUntil()', + 'pendingAdmin()', + 'beginAdminTransfer(address)', + 'acceptAdminTransfer()', + 'cancelAdminTransfer()', + ], Governor: [ 'name()', 'version()', From f7738a63b30e1de11f33cae7138db8c2402481c9 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 18:16:59 -0600 Subject: [PATCH 03/69] Add changeset --- .changeset/silent-dancers-type.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/silent-dancers-type.md diff --git a/.changeset/silent-dancers-type.md b/.changeset/silent-dancers-type.md new file mode 100644 index 00000000000..c49a19fb2b9 --- /dev/null +++ b/.changeset/silent-dancers-type.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessControlAdminRules`: Add extension to `AccessControl` to add extra conditions to manage the `DEFAULT_ADMIN_ROLE` From 68adaf5b91a72e556067e2dacc829d0c3884d853 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 19:49:06 -0600 Subject: [PATCH 04/69] Correctly set unlocked crite --- contracts/access/AccessControlAdminRules.sol | 4 ++-- test/access/AccessControl.behavior.js | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index dc5d113d8cf..67c0e8daf21 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -198,10 +198,10 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon } /** - * @dev Checks if a {delayUntil} has been set and met. + * @dev Checks if a {delayedUntil} has been set and met. */ function _adminTransferIsUnlocked() private view returns (bool) { uint48 delayedUntilTimestamp = delayedUntil(); - return delayedUntilTimestamp > 0 && delayedUntilTimestamp > block.timestamp; + return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 5b05bb9c5c2..8bff29d5c14 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -291,7 +291,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA // possiblty creating false positives (eg. expect revert for incorrect caller but getting it // from a badly expected delayed until) correctPendingAdmin = newAdmin; - correctIncreaseTo = (await time.latest()).add(delay).subn(1); + correctIncreaseTo = (await time.latest()).add(delay).addn(1); await this.accessControl.beginAdminTransfer(correctPendingAdmin, { from: admin }); }); @@ -341,7 +341,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA let incorrectIncreaseTo; beforeEach(async function () { - incorrectIncreaseTo = correctIncreaseTo.addn(1); + incorrectIncreaseTo = correctIncreaseTo.subn(1); }); it('should revert if block.timestamp is equal to delayed until', async function () { @@ -353,7 +353,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA }); it('should revert if block.timestamp is less to delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo.addn(1)); + await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.acceptAdminTransfer({ from: correctPendingAdmin }), `${errorPrefix}: delay must be met and caller must be pending admin`, @@ -386,7 +386,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA let from = admin; beforeEach(async function () { - correctIncreaseTo = (await time.latest()).add(delay).subn(1); + correctIncreaseTo = (await time.latest()).add(delay).addn(1); await this.accessControl.beginAdminTransfer(ZERO_ADDRESS, { from }); }); @@ -436,7 +436,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA let incorrectIncreaseTo; beforeEach(async function () { - incorrectIncreaseTo = correctIncreaseTo.addn(1); + incorrectIncreaseTo = correctIncreaseTo.subn(1); }); it('reverts if block.timestamp is equal to delayed until', async function () { @@ -448,7 +448,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA }); it('reverts if block.timestamp is less to delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo.addn(1)); + await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, admin, { from }), `${errorPrefix}: admin can only renounce in two delayed steps`, From 0aff3ff68e76cd0f6df251ed2c6557d44f79ba4f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 21:02:28 -0600 Subject: [PATCH 05/69] Completed docs --- contracts/access/AccessControl.sol | 2 +- contracts/access/AccessControlAdminRules.sol | 37 +++++++++++++------- contracts/access/README.adoc | 4 +++ docs/modules/ROOT/pages/access-control.adoc | 2 ++ 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 5f2829e74fb..04b8c16a7b5 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -44,7 +44,7 @@ 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. See {AccessControlAdminRules} */ abstract contract AccessControl is Context, IAccessControl, ERC165 { struct RoleData { diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 67c0e8daf21..72f4f78c2c1 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -9,7 +9,7 @@ import "../utils/math/SafeCast.sol"; /** * @dev Extension of {AccessControl} that allows to specify special rules to manage - * the `DEFAULT_ADMIN_ROLE`´s owner, which is a sensitive role with special permissions + * the `DEFAULT_ADMIN_ROLE` 's owner, which is a sensitive role with special permissions * over other valid roles that may potentially have rights. * * If a specific role doesn't have an `adminRole` assigned, the holder of the @@ -18,12 +18,26 @@ import "../utils/math/SafeCast.sol"; * * This contract implements the following risk mitigations on top of the AccessControl implementation: * - * - Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when renounced. - * - Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. Even when it's been renounced. - * - 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. - * - The `DEFAULT_ADMIN_ROLE`'s admin can be only held by itself. + * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when renounced. + * * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. + * - Even when it's been renounced. + * * 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. + * * The `DEFAULT_ADMIN_ROLE` 's admin can be only held by itself. * - * NOTE: `delay` is only configurable in the constructor to avoid issues related with handling delay management during the transfer is pending to be completed. + * Once you understand what the {constructor} parameters, you can use this reference implementation: + * + * ```solidity + * contract MyToken is AccessControlAdminRules { + * constructor() AccessControlAdminRules( + * 60 * 60 * 24, // 3 days + * _msgSender() // Explicit initial `DEFAULT_ADMIN_ROLE` holder + * ) {} + *} + * ``` + * + * NOTE: The `delay` is only configurable in the constructor to avoid issues related with handling + * delay management during the transfer is pending to be completed. * * _Available since v4.9._ */ @@ -36,13 +50,12 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon address private _pendingAdmin; /** - * @dev Sets the values for {name} and {symbol}. - * - * All two of these values are immutable: they can only be set once during - * construction. + * @dev Sets the initial values the internal delay and {owner}. + * + * The `delay` value is immutable. It can only be set at the constructor. */ - constructor(uint48 initialDelay, address initialAdmin) { - _delay = initialDelay; + constructor(uint48 delay, address initialAdmin) { + _delay = delay; _grantRole(DEFAULT_ADMIN_ROLE, initialAdmin); } diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 888d0e99e37..45d12117a9b 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -23,3 +23,7 @@ This directory provides ways to restrict who can access the functions of a contr {{IAccessControlEnumerable}} {{AccessControlEnumerable}} + +{{IAccessControlAdminRules}} + +{{AccessControlAdminRules}} diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index f3ddb6234dd..c28ec51304d 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. +NOTE: For risk mitigations related to `DEFAULT_ADMIN_ROLE` management. See xref:api:access.adoc#AccessControlAdminRules[`AccessControlAdminRules`] as an alternative with restrictions such as 2-step admin role change, single-holder enforcing and configurable minimum delay. + Let's take a look at the ERC20 token example, this time taking advantage of the default admin role: [source,solidity] From 4090a3e8b9a8a5faa153196e8d36d1c6ce43b3dd Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 21:05:03 -0600 Subject: [PATCH 06/69] Fix linter --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 72f4f78c2c1..467252e6eaa 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -51,7 +51,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev Sets the initial values the internal delay and {owner}. - * + * * The `delay` value is immutable. It can only be set at the constructor. */ constructor(uint48 delay, address initialAdmin) { From 21aacf9abe9a7cb676fd41d74c6cbd6a094eb0d5 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 27 Jan 2023 21:12:41 -0600 Subject: [PATCH 07/69] Refactor tests so coverage is not reduced --- test/access/AccessControl.behavior.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 8bff29d5c14..fea3c81b906 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -298,21 +298,21 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA describe('caller is pending admin and delayed until is met', async function () { let from; - let receipt; beforeEach(async function () { await time.increaseTo(correctIncreaseTo); from = correctPendingAdmin; - receipt = await this.accessControl.acceptAdminTransfer({ from }); }); it('accepts a transfer and changes admin', async function () { + await this.accessControl.acceptAdminTransfer({ from }); expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.be.false; expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, newAdmin)).to.be.true; expect(await this.accessControl.owner()).to.equal(newAdmin); }); it('accepts a transfer and emit events', async function () { + const receipt = await this.accessControl.acceptAdminTransfer({ from }); expectEvent(receipt, 'RoleRevoked', { role: DEFAULT_ADMIN_ROLE, account: admin, @@ -324,6 +324,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA }); it('accepts a transfer resetting pending admin and delayed until', async function () { + await this.accessControl.acceptAdminTransfer({ from }); expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); }); From 2863047d078a3600ef34c3840dcce0de3030d736 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 11:29:12 -0600 Subject: [PATCH 08/69] Address PR review comments --- .changeset/silent-dancers-type.md | 2 +- contracts/access/AccessControlAdminRules.sol | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/silent-dancers-type.md b/.changeset/silent-dancers-type.md index c49a19fb2b9..02fe13f7831 100644 --- a/.changeset/silent-dancers-type.md +++ b/.changeset/silent-dancers-type.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`AccessControlAdminRules`: Add extension to `AccessControl` to add extra conditions to manage the `DEFAULT_ADMIN_ROLE` +`AccessControlAdminRules`: Add extension to `AccessControl` for adding extra rules to handle the `DEFAULT_ADMIN_ROLE` diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 467252e6eaa..bab7f579cac 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -45,10 +45,10 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon uint48 private immutable _delay; address private _currentAdmin; - uint48 private _delayedUntil; - address private _pendingAdmin; + uint48 private _delayedUntil; + /** * @dev Sets the initial values the internal delay and {owner}. * From a9a349565bf54b62c359a0f525270a15df03c4b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Mon, 30 Jan 2023 12:13:55 -0600 Subject: [PATCH 09/69] Update contracts/access/AccessControlAdminRules.sol Co-authored-by: Hadrien Croubois --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index bab7f579cac..5c7e93c3a9b 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -94,7 +94,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon require(delayedUntil() == 0, "AccessControl: pending admin already set"); _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingAdmin = newAdmin; - emit AdminRoleChangeStarted(pendingAdmin(), delayedUntil()); + emit AdminRoleChangeStarted(_pendingAdmin, _delayedUntil); } /** From 59f3b62acda48c3cbf76002fdd1f44a185856abf Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 16:50:47 -0600 Subject: [PATCH 10/69] Add draft-ERC5313 --- contracts/access/AccessControlAdminRules.sol | 36 +++++++++++-------- contracts/access/IAccessControlAdminRules.sol | 2 +- contracts/interfaces/draft-IERC5313.sol | 13 +++++++ test/access/AccessControlAdminRules.t.sol | 28 +++++++++++++++ .../SupportsInterface.behavior.js | 2 +- 5 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 contracts/interfaces/draft-IERC5313.sol create mode 100644 test/access/AccessControlAdminRules.t.sol diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 5c7e93c3a9b..67a6517ab76 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -6,6 +6,7 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; import "./IAccessControlAdminRules.sol"; import "../utils/math/SafeCast.sol"; +import "../interfaces/draft-IERC5313.sol"; /** * @dev Extension of {AccessControl} that allows to specify special rules to manage @@ -41,7 +42,7 @@ import "../utils/math/SafeCast.sol"; * * _Available since v4.9._ */ -abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessControl { +abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, AccessControl { uint48 private immutable _delay; address private _currentAdmin; @@ -60,9 +61,16 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon } /** - * @dev See {IAccessControlAdminRules-owner} + * @dev See {draft-IERC5313-owner} */ function owner() public view virtual returns (address) { + return admin(); + } + + /** + * @dev See {IAccessControlAdminRules-admin} + */ + function admin() public view virtual returns (address) { return _currentAdmin; } @@ -90,7 +98,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev See {IAccessControlAdminRules-beginAdminTransfer} */ - function beginAdminTransfer(address newAdmin) public override onlyRole(DEFAULT_ADMIN_ROLE) { + function beginAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { require(delayedUntil() == 0, "AccessControl: pending admin already set"); _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingAdmin = newAdmin; @@ -100,13 +108,13 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev See {IAccessControlAdminRules-acceptAdminTransfer} */ - function acceptAdminTransfer() public { + function acceptAdminTransfer() public virtual { address pendingAdminOwner = pendingAdmin(); require( _adminTransferIsUnlocked() && _msgSender() == pendingAdminOwner, "AccessControl: delay must be met and caller must be pending admin" ); - _revokeRole(DEFAULT_ADMIN_ROLE, owner()); + _revokeRole(DEFAULT_ADMIN_ROLE, admin()); _grantRole(DEFAULT_ADMIN_ROLE, pendingAdminOwner); _resetAdminTransfer(); } @@ -114,7 +122,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev See {IAccessControlAdminRules-cancelAdminTransfer} */ - function cancelAdminTransfer() external override onlyRole(DEFAULT_ADMIN_ROLE) { + function cancelAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _resetAdminTransfer(); } @@ -132,7 +140,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * 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 override(IAccessControl, AccessControl) { + function renounceRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( pendingAdmin() == address(0) && _adminTransferIsUnlocked(), @@ -148,7 +156,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon function grantRole( bytes32 role, address account - ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant admin role"); super.grantRole(role, account); } @@ -159,7 +167,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon function revokeRole( bytes32 role, address account - ) public override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke admin role"); super.revokeRole(role, account); } @@ -167,7 +175,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal override { + function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't override admin's admin"); super._setRoleAdmin(role, adminRole); } @@ -184,9 +192,9 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * `DEFAULT_ADMIN_ROLE` assignable again. Make sure to guarantee this is * the expected behavior in your implementation. */ - function _grantRole(bytes32 role, address account) internal override { + function _grantRole(bytes32 role, address account) internal virtual override { if (role == DEFAULT_ADMIN_ROLE) { - require(owner() == address(0), "AccessControl: admin already granted"); + require(admin() == address(0), "AccessControl: admin already granted"); _currentAdmin = account; } super._grantRole(role, account); @@ -195,7 +203,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon /** * @dev See {AccessControl-_revokeRole}. */ - function _revokeRole(bytes32 role, address account) internal override { + function _revokeRole(bytes32 role, address account) internal virtual override { if (role == DEFAULT_ADMIN_ROLE) { delete _currentAdmin; } @@ -214,7 +222,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, AccessCon * @dev Checks if a {delayedUntil} has been set and met. */ function _adminTransferIsUnlocked() private view returns (bool) { - uint48 delayedUntilTimestamp = delayedUntil(); + uint48 delayedUntilTimestamp = _delayedUntil; return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } } diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol index 4da58587c1a..f8d2965ea58 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlAdminRules.sol @@ -20,7 +20,7 @@ interface IAccessControlAdminRules is IAccessControl { /** * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` owner. */ - function owner() external view returns (address); + function admin() external view returns (address); /** * @dev Returns the timestamp in which the pending admin can claim the diff --git a/contracts/interfaces/draft-IERC5313.sol b/contracts/interfaces/draft-IERC5313.sol new file mode 100644 index 00000000000..d56582f6053 --- /dev/null +++ b/contracts/interfaces/draft-IERC5313.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: CC0-1.0 +// OpenZeppelin Contracts (last updated v4.5.0) (interfaces/draft-IERC5313.sol) +pragma solidity ^0.8.0; + +/** + * @title EIP-5313 Light Contract Ownership Standard + */ +interface IERC5313 { + /** + * @dev Gets the address of the owner. + */ + function owner() external view returns (address); +} diff --git a/test/access/AccessControlAdminRules.t.sol b/test/access/AccessControlAdminRules.t.sol new file mode 100644 index 00000000000..f2150007b81 --- /dev/null +++ b/test/access/AccessControlAdminRules.t.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../../../contracts/access/AccessControlAdminRules.sol"; + +contract MyToken is AccessControlAdminRules { + constructor(uint48 delay) AccessControlAdminRules(delay, _msgSender()) {} +} + +contract AdminRules is Test { + MyToken token; + + uint48 constant DELAY = 60 * 60 * 24; + + function setUp() public { + token = new MyToken(DELAY); + } + + function testAccept(address newAdmin) public { + token.beginAdminTransfer(newAdmin); + vm.warp(block.timestamp + DELAY + 1); + vm.prank(newAdmin); + token.acceptAdminTransfer(); + } +} diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index f0a46fc45e0..be1212de2c3 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -38,7 +38,7 @@ const INTERFACES = { ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], AccessControlAdminRules: [ - 'owner()', + 'admin()', 'delayedUntil()', 'pendingAdmin()', 'beginAdminTransfer(address)', From 2bfb0282352c399ad76ec3a6d24ef8a0ac1cebe7 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 16:51:43 -0600 Subject: [PATCH 11/69] Remove property-based test --- test/access/AccessControlAdminRules.t.sol | 28 ----------------------- 1 file changed, 28 deletions(-) delete mode 100644 test/access/AccessControlAdminRules.t.sol diff --git a/test/access/AccessControlAdminRules.t.sol b/test/access/AccessControlAdminRules.t.sol deleted file mode 100644 index f2150007b81..00000000000 --- a/test/access/AccessControlAdminRules.t.sol +++ /dev/null @@ -1,28 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "forge-std/Test.sol"; - -import "../../../contracts/access/AccessControlAdminRules.sol"; - -contract MyToken is AccessControlAdminRules { - constructor(uint48 delay) AccessControlAdminRules(delay, _msgSender()) {} -} - -contract AdminRules is Test { - MyToken token; - - uint48 constant DELAY = 60 * 60 * 24; - - function setUp() public { - token = new MyToken(DELAY); - } - - function testAccept(address newAdmin) public { - token.beginAdminTransfer(newAdmin); - vm.warp(block.timestamp + DELAY + 1); - vm.prank(newAdmin); - token.acceptAdminTransfer(); - } -} From 7ae4aa73fcc9cc2fac0a002ce2b13940b0d04bb1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 16:52:22 -0600 Subject: [PATCH 12/69] Fix typo --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 67a6517ab76..059f47f3611 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -26,7 +26,7 @@ import "../interfaces/draft-IERC5313.sol"; * - Even after the timer has passed to avoid locking it forever. * * The `DEFAULT_ADMIN_ROLE` 's admin can be only held by itself. * - * Once you understand what the {constructor} parameters, you can use this reference implementation: + * Once you understand what the {constructor} parameters are, you can use this reference implementation: * * ```solidity * contract MyToken is AccessControlAdminRules { From 2d630c39b3450ff07ca1c707b92a772a8a9f2289 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 17:01:51 -0600 Subject: [PATCH 13/69] Add IERC5313 docs --- contracts/access/AccessControlAdminRules.sol | 2 +- contracts/access/IAccessControlAdminRules.sol | 12 ++++++------ contracts/interfaces/README.adoc | 5 ++++- contracts/interfaces/draft-IERC5313.sol | 6 +++++- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 059f47f3611..06ed7421b84 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -61,7 +61,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {draft-IERC5313-owner} + * @dev See {IERC5313-owner} */ function owner() public view virtual returns (address) { return admin(); diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol index f8d2965ea58..d145a27073c 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlAdminRules.sol @@ -13,12 +13,12 @@ import "./IAccessControl.sol"; interface IAccessControlAdminRules is IAccessControl { /** * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newAdmin` - * as the next owner to be claimed after `delayedUntil` is met. + * as the next admin to be claimed after `delayedUntil` is met. */ event AdminRoleChangeStarted(address indexed newAdmin, uint48 delayedUntil); /** - * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` owner. + * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. */ function admin() external view returns (address); @@ -29,7 +29,7 @@ interface IAccessControlAdminRules is IAccessControl { function delayedUntil() external view returns (uint48); /** - * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` owner. + * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. */ function pendingAdmin() external view returns (address); @@ -40,7 +40,7 @@ interface IAccessControlAdminRules is IAccessControl { * Requirements: * * - There shouldn't be another admin transfer in progress. See {cancelAdminTransfer}. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` owner. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. * * Emits an {AdminRoleChangeStarted}. */ @@ -53,7 +53,7 @@ interface IAccessControlAdminRules is IAccessControl { * * - Caller should be the pending admin. * - `DEFAULT_ADMIN_ROLE` should be granted to the caller. - * - `DEFAULT_ADMIN_ROLE` should be revoked from the previous owner. + * - `DEFAULT_ADMIN_ROLE` should be revoked from the previous holder. * - Should allow to call {beginAdminTransfer} again. */ function acceptAdminTransfer() external; @@ -65,7 +65,7 @@ interface IAccessControlAdminRules is IAccessControl { * * - Should allow to call {beginAdminTransfer} again. * - Can be called even after the timer is met. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` owner. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. */ function cancelAdminTransfer() external; } diff --git a/contracts/interfaces/README.adoc b/contracts/interfaces/README.adoc index 5b4cedf9543..4e2bb0ee102 100644 --- a/contracts/interfaces/README.adoc +++ b/contracts/interfaces/README.adoc @@ -30,6 +30,7 @@ are useful to interact with third party contracts that implement them. - {IERC3156FlashLender} - {IERC3156FlashBorrower} - {IERC4626} +- {IERC5313} == Detailed ABI @@ -53,4 +54,6 @@ are useful to interact with third party contracts that implement them. {{IERC3156FlashBorrower}} -{{IERC4626}} \ No newline at end of file +{{IERC4626}} + +{{IERC5313}} diff --git a/contracts/interfaces/draft-IERC5313.sol b/contracts/interfaces/draft-IERC5313.sol index d56582f6053..8372a9f5797 100644 --- a/contracts/interfaces/draft-IERC5313.sol +++ b/contracts/interfaces/draft-IERC5313.sol @@ -3,7 +3,11 @@ pragma solidity ^0.8.0; /** - * @title EIP-5313 Light Contract Ownership Standard + * @dev Interface for the Light Contract Ownership Standard. + * + * A standardized minimal interface required to identify an account that controls a contract + * + * _Available since v4.9._ */ interface IERC5313 { /** From 192c5bcb6f5c0e95fe63785fc1a5fb887cb9e27f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 17:12:40 -0600 Subject: [PATCH 14/69] Change owner for admin --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 06ed7421b84..84fdccc75b9 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -51,7 +51,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, uint48 private _delayedUntil; /** - * @dev Sets the initial values the internal delay and {owner}. + * @dev Sets the initial values the internal delay and {admin}. * * The `delay` value is immutable. It can only be set at the constructor. */ From 021ee376c2aaac93fe908901ec9160ee14cd42f9 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 17:47:56 -0600 Subject: [PATCH 15/69] Call overrideable functions --- contracts/access/AccessControlAdminRules.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 84fdccc75b9..7a38c5f3854 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -102,7 +102,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, require(delayedUntil() == 0, "AccessControl: pending admin already set"); _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingAdmin = newAdmin; - emit AdminRoleChangeStarted(_pendingAdmin, _delayedUntil); + emit AdminRoleChangeStarted(pendingAdmin(), delayedUntil()); } /** @@ -222,7 +222,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev Checks if a {delayedUntil} has been set and met. */ function _adminTransferIsUnlocked() private view returns (bool) { - uint48 delayedUntilTimestamp = _delayedUntil; + uint48 delayedUntilTimestamp = delayedUntil(); return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } } From a54dd2b34f3957c1e5c209291fbe329f5652a73f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 30 Jan 2023 18:01:00 -0600 Subject: [PATCH 16/69] Update IERC5313 to final --- contracts/access/AccessControlAdminRules.sol | 2 +- contracts/interfaces/{draft-IERC5313.sol => IERC5313.sol} | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename contracts/interfaces/{draft-IERC5313.sol => IERC5313.sol} (75%) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 7a38c5f3854..8cb2852c7c1 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -6,7 +6,7 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; import "./IAccessControlAdminRules.sol"; import "../utils/math/SafeCast.sol"; -import "../interfaces/draft-IERC5313.sol"; +import "../interfaces/IERC5313.sol"; /** * @dev Extension of {AccessControl} that allows to specify special rules to manage diff --git a/contracts/interfaces/draft-IERC5313.sol b/contracts/interfaces/IERC5313.sol similarity index 75% rename from contracts/interfaces/draft-IERC5313.sol rename to contracts/interfaces/IERC5313.sol index 8372a9f5797..d494d7e77c6 100644 --- a/contracts/interfaces/draft-IERC5313.sol +++ b/contracts/interfaces/IERC5313.sol @@ -1,5 +1,5 @@ -// SPDX-License-Identifier: CC0-1.0 -// OpenZeppelin Contracts (last updated v4.5.0) (interfaces/draft-IERC5313.sol) +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (interfaces/IERC5313.sol) pragma solidity ^0.8.0; /** From 85cff3676d97b3588bfa9ad6024dca90b9aef3d8 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 31 Jan 2023 13:20:04 -0600 Subject: [PATCH 17/69] Reword private function --- contracts/access/AccessControlAdminRules.sol | 6 +++--- test/access/AccessControlAdminRules.t.sol | 21 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 test/access/AccessControlAdminRules.t.sol diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 8cb2852c7c1..c73643d33cb 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -111,7 +111,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function acceptAdminTransfer() public virtual { address pendingAdminOwner = pendingAdmin(); require( - _adminTransferIsUnlocked() && _msgSender() == pendingAdminOwner, + _hasTransferDelayPassed() && _msgSender() == pendingAdminOwner, "AccessControl: delay must be met and caller must be pending admin" ); _revokeRole(DEFAULT_ADMIN_ROLE, admin()); @@ -143,7 +143,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function renounceRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( - pendingAdmin() == address(0) && _adminTransferIsUnlocked(), + pendingAdmin() == address(0) && _hasTransferDelayPassed(), "AccessControl: admin can only renounce in two delayed steps" ); } @@ -221,7 +221,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, /** * @dev Checks if a {delayedUntil} has been set and met. */ - function _adminTransferIsUnlocked() private view returns (bool) { + function _hasTransferDelayPassed() private view returns (bool) { uint48 delayedUntilTimestamp = delayedUntil(); return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } diff --git a/test/access/AccessControlAdminRules.t.sol b/test/access/AccessControlAdminRules.t.sol new file mode 100644 index 00000000000..ded8f31d6c5 --- /dev/null +++ b/test/access/AccessControlAdminRules.t.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../../contracts/access/AccessControlAdminRules.sol"; + +contract MyToken is AccessControlAdminRules { + constructor(uint48 delay) AccessControlAdminRules(delay, _msgSender()) {} +} + +contract AdminRules is Test { + MyToken token; + + uint48 constant DELAY = 60 * 60 * 24; + + function testDeploy() public { + token = new MyToken(DELAY); + } +} From 8baa13b06bc89ce01e44079b15058c45d4210e3e Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 31 Jan 2023 13:21:05 -0600 Subject: [PATCH 18/69] Remove property test again --- test/access/AccessControlAdminRules.t.sol | 21 --------------------- 1 file changed, 21 deletions(-) delete mode 100644 test/access/AccessControlAdminRules.t.sol diff --git a/test/access/AccessControlAdminRules.t.sol b/test/access/AccessControlAdminRules.t.sol deleted file mode 100644 index ded8f31d6c5..00000000000 --- a/test/access/AccessControlAdminRules.t.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "forge-std/Test.sol"; - -import "../../contracts/access/AccessControlAdminRules.sol"; - -contract MyToken is AccessControlAdminRules { - constructor(uint48 delay) AccessControlAdminRules(delay, _msgSender()) {} -} - -contract AdminRules is Test { - MyToken token; - - uint48 constant DELAY = 60 * 60 * 24; - - function testDeploy() public { - token = new MyToken(DELAY); - } -} From fb895d2001707e4d90ed48257a9a75ef797f24b5 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 31 Jan 2023 13:23:48 -0600 Subject: [PATCH 19/69] Rename again --- contracts/access/AccessControlAdminRules.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index c73643d33cb..b8f20fbc64a 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -111,7 +111,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function acceptAdminTransfer() public virtual { address pendingAdminOwner = pendingAdmin(); require( - _hasTransferDelayPassed() && _msgSender() == pendingAdminOwner, + _hasAdminTransferDelayPassed() && _msgSender() == pendingAdminOwner, "AccessControl: delay must be met and caller must be pending admin" ); _revokeRole(DEFAULT_ADMIN_ROLE, admin()); @@ -143,7 +143,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function renounceRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( - pendingAdmin() == address(0) && _hasTransferDelayPassed(), + pendingAdmin() == address(0) && _hasAdminTransferDelayPassed(), "AccessControl: admin can only renounce in two delayed steps" ); } @@ -221,7 +221,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, /** * @dev Checks if a {delayedUntil} has been set and met. */ - function _hasTransferDelayPassed() private view returns (bool) { + function _hasAdminTransferDelayPassed() private view returns (bool) { uint48 delayedUntilTimestamp = delayedUntil(); return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } From d120c1d4b7ff1810fc5bd2cd774e104a9909fe8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 15 Feb 2023 21:15:14 -0600 Subject: [PATCH 20/69] Update .changeset/silent-dancers-type.md Co-authored-by: Francisco --- .changeset/silent-dancers-type.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/silent-dancers-type.md b/.changeset/silent-dancers-type.md index 02fe13f7831..ca7bb49e948 100644 --- a/.changeset/silent-dancers-type.md +++ b/.changeset/silent-dancers-type.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`AccessControlAdminRules`: Add extension to `AccessControl` for adding extra rules to handle the `DEFAULT_ADMIN_ROLE` +`AccessControlAdminRules`: Add an extension of `AccessControl` with additional security rules for the `DEFAULT_ADMIN_ROLE`. From 4322ed7b900648097dc9967952bd73d9c547b65a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 15 Feb 2023 21:15:25 -0600 Subject: [PATCH 21/69] Update contracts/access/AccessControl.sol Co-authored-by: Francisco --- contracts/access/AccessControl.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 04b8c16a7b5..6baa35a9900 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. See {AccessControlAdminRules} + * accounts that have been granted it. We recommend using {AccessControlAdminRules} + * to enforce additional security measures for this role. */ abstract contract AccessControl is Context, IAccessControl, ERC165 { struct RoleData { From d52b37145675623a5e11fbf1d7ca9b15dc0b0470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 16 Feb 2023 16:38:11 -0600 Subject: [PATCH 22/69] Update docs/modules/ROOT/pages/access-control.adoc Co-authored-by: Francisco --- docs/modules/ROOT/pages/access-control.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index c28ec51304d..2681df87fbf 100644 --- a/docs/modules/ROOT/pages/access-control.adoc +++ b/docs/modules/ROOT/pages/access-control.adoc @@ -131,7 +131,7 @@ 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. -NOTE: For risk mitigations related to `DEFAULT_ADMIN_ROLE` management. See xref:api:access.adoc#AccessControlAdminRules[`AccessControlAdminRules`] as an alternative with restrictions such as 2-step admin role change, single-holder enforcing and configurable minimum delay. +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#AccessControlAdminRules[`AccessControlAdminRules`], 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: From 54184af4be5e98ec30af11e1698f4cfe60f7be5a Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 17:20:51 -0600 Subject: [PATCH 23/69] Rename to defaultAdmin --- contracts/access/AccessControlAdminRules.sol | 101 ++++++++------- contracts/access/IAccessControlAdminRules.sol | 38 +++--- test/access/AccessControl.behavior.js | 122 +++++++++--------- .../SupportsInterface.behavior.js | 10 +- 4 files changed, 139 insertions(+), 132 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index b8f20fbc64a..fda646cb91a 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -9,24 +9,24 @@ import "../utils/math/SafeCast.sol"; import "../interfaces/IERC5313.sol"; /** - * @dev Extension of {AccessControl} that allows to specify special rules to manage - * the `DEFAULT_ADMIN_ROLE` 's owner, which is a sensitive role with special permissions + * @dev Extension of {AccessControl} that allows specifying special rules to manage + * the `DEFAULT_ADMIN_ROLE` 's holder, which is a sensitive role with special permissions * over other valid roles that may potentially have rights. * * 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}'s default value (`address(0)`). * - * This contract implements the following risk mitigations on top of the AccessControl implementation: + * This contract implements the following risk mitigations on top of the {AccessControl} implementation: * * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when renounced. * * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. * - Even when it's been renounced. * * 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. - * * The `DEFAULT_ADMIN_ROLE` 's admin can be only held by itself. + * * The `DEFAULT_ADMIN_ROLE` 's admin can be only itself. * - * Once you understand what the {constructor} parameters are, you can use this reference implementation: + * Example usage: * * ```solidity * contract MyToken is AccessControlAdminRules { @@ -45,47 +45,54 @@ import "../interfaces/IERC5313.sol"; abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, AccessControl { uint48 private immutable _delay; - address private _currentAdmin; - address private _pendingAdmin; + address private _currentDefaultAdmin; + address private _pendingDefaultAdmin; uint48 private _delayedUntil; /** - * @dev Sets the initial values the internal delay and {admin}. + * @dev Sets the initial values for {delay} and {defaultAdmin}. * * The `delay` value is immutable. It can only be set at the constructor. */ - constructor(uint48 delay, address initialAdmin) { - _delay = delay; - _grantRole(DEFAULT_ADMIN_ROLE, initialAdmin); + constructor(uint48 delay_, address initialDefaultAdmin) { + _delay = delay_; + _grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin); } /** - * @dev See {IERC5313-owner} + * @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer. + */ + function delay() public view returns (uint48) { + return _delay; + } + + /** + * @dev See {IERC5313-owner}. */ function owner() public view virtual returns (address) { - return admin(); + return defaultAdmin(); } /** - * @dev See {IAccessControlAdminRules-admin} + * @dev See {IAccessControlAdminRules-defaultAdmin}. */ - function admin() public view virtual returns (address) { - return _currentAdmin; + function defaultAdmin() public view virtual returns (address) { + return _currentDefaultAdmin; } /** - * @dev See {IAccessControlAdminRules-delayedUntil} + * @dev See {IAccessControlAdminRules-delayedUntil}. */ function delayedUntil() public view virtual returns (uint48) { return _delayedUntil; } /** - * @dev See {IAccessControlAdminRules-pendingAdmin} + * @dev See {IAccessControlAdminRules-pendingDefaultAdmin}. */ - function pendingAdmin() public view virtual returns (address) { - return _pendingAdmin; + function pendingDefaultAdmin() public view virtual returns (address) { + return _pendingDefaultAdmin; } /** @@ -96,41 +103,41 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-beginAdminTransfer} + * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer} */ - function beginAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { + function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { require(delayedUntil() == 0, "AccessControl: pending admin already set"); _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; - _pendingAdmin = newAdmin; - emit AdminRoleChangeStarted(pendingAdmin(), delayedUntil()); + _pendingDefaultAdmin = newAdmin; + emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), delayedUntil()); } /** - * @dev See {IAccessControlAdminRules-acceptAdminTransfer} + * @dev See {IAccessControlAdminRules-acceptDefaultAdminTransfer} */ - function acceptAdminTransfer() public virtual { - address pendingAdminOwner = pendingAdmin(); + function acceptDefaultAdminTransfer() public virtual { + address pendingDefaultAdminHolder = pendingDefaultAdmin(); require( - _hasAdminTransferDelayPassed() && _msgSender() == pendingAdminOwner, + _hasDefaultAdminTransferDelayPassed() && _msgSender() == pendingDefaultAdminHolder, "AccessControl: delay must be met and caller must be pending admin" ); - _revokeRole(DEFAULT_ADMIN_ROLE, admin()); - _grantRole(DEFAULT_ADMIN_ROLE, pendingAdminOwner); - _resetAdminTransfer(); + _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); + _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdminHolder); + _resetDefaultAdminTransfer(); } /** - * @dev See {IAccessControlAdminRules-cancelAdminTransfer} + * @dev See {IAccessControlAdminRules-cancelDefaultAdminTransfer} */ - function cancelAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _resetAdminTransfer(); + 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 {delayedUntil} is met and the pending admin is the zero address. + * that the {delayedUntil} is met and the pending default admin is the zero address. * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` * functions. * @@ -143,8 +150,8 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function renounceRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( - pendingAdmin() == address(0) && _hasAdminTransferDelayPassed(), - "AccessControl: admin can only renounce in two delayed steps" + pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), + "AccessControl: only can renounce in two delayed steps" ); } super.renounceRole(role, account); @@ -157,7 +164,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, bytes32 role, address account ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { - require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant admin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant defaultAdmin role"); super.grantRole(role, account); } @@ -168,7 +175,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, bytes32 role, address account ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { - require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke admin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke defaultAdmin role"); super.revokeRole(role, account); } @@ -176,7 +183,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @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 override admin's admin"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate defaultAdmin rules"); super._setRoleAdmin(role, adminRole); } @@ -194,8 +201,8 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, */ function _grantRole(bytes32 role, address account) internal virtual override { if (role == DEFAULT_ADMIN_ROLE) { - require(admin() == address(0), "AccessControl: admin already granted"); - _currentAdmin = account; + require(defaultAdmin() == address(0), "AccessControl: defaultAdmin already granted"); + _currentDefaultAdmin = account; } super._grantRole(role, account); } @@ -205,23 +212,23 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, */ function _revokeRole(bytes32 role, address account) internal virtual override { if (role == DEFAULT_ADMIN_ROLE) { - delete _currentAdmin; + delete _currentDefaultAdmin; } super._revokeRole(role, account); } /** - * @dev Resets the pending admin and delayed until. + * @dev Resets the pending default admin and delayed until. */ - function _resetAdminTransfer() private { - delete _pendingAdmin; + function _resetDefaultAdminTransfer() private { + delete _pendingDefaultAdmin; delete _delayedUntil; } /** * @dev Checks if a {delayedUntil} has been set and met. */ - function _hasAdminTransferDelayPassed() private view returns (bool) { + function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { uint48 delayedUntilTimestamp = delayedUntil(); return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; } diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol index d145a27073c..8d74f9cab24 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlAdminRules.sol @@ -6,66 +6,66 @@ pragma solidity ^0.8.0; import "./IAccessControl.sol"; /** - * @dev External interface of AccessControladminRules declared to support ERC165 detection. + * @dev External interface of AccessControlAdminRules declared to support ERC165 detection. * * _Available since v4.9._ */ interface IAccessControlAdminRules is IAccessControl { /** - * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newAdmin` - * as the next admin to be claimed after `delayedUntil` is met. + * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` + * as the next default admin to be claimed after `delayedUntil` is met. */ - event AdminRoleChangeStarted(address indexed newAdmin, uint48 delayedUntil); + event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 delayedUntil); /** * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. */ - function admin() external view returns (address); + function defaultAdmin() external view returns (address); /** - * @dev Returns the timestamp in which the pending admin can claim the - * `DEFAULT_ADMIN_ROLE` role. + * @dev Returns the timestamp in which the pending default admin can claim the + * `DEFAULT_ADMIN_ROLE`. */ function delayedUntil() external view returns (uint48); /** * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. */ - function pendingAdmin() external view returns (address); + function pendingDefaultAdmin() external view returns (address); /** - * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending admin and - * a timer to be met. + * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin + * and a timer to be met. * * Requirements: * - * - There shouldn't be another admin transfer in progress. See {cancelAdminTransfer}. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. + * - There shouldn't be another default admin transfer in progress. See {cancelDefaultAdminTransfer}. + * - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder. * - * Emits an {AdminRoleChangeStarted}. + * Emits a {DefaultAdminRoleChangeStarted}. */ - function beginAdminTransfer(address newAdmin) external; + function beginDefaultAdminTransfer(address newAdmin) external; /** * @dev Completes a `DEFAULT_ADMIN_ROLE` transfer. * * Requirements: * - * - Caller should be the pending admin. + * - 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. - * - Should allow to call {beginAdminTransfer} again. + * - Should allow to call {beginDefaultAdminTransfer} again. */ - function acceptAdminTransfer() external; + function acceptDefaultAdminTransfer() external; /** * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. * * Requirements: * - * - Should allow to call {beginAdminTransfer} again. + * - Should allow to call {beginDefaultAdminTransfer} again. * - Can be called even after the timer is met. * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. */ - function cancelAdminTransfer() external; + function cancelDefaultAdminTransfer() external; } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index fea3c81b906..fcff7ea25e8 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -211,78 +211,78 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, }); } -function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newAdmin, other) { +function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmin, newDefaultAdmin, other) { shouldSupportInterfaces(['AccessControlAdminRules']); it('has a default disabled delayed until', async function () { expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); }); - it('has a default pending admin', async function () { - expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + 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 owner', async function () { + it('has a default current owner set to the initial default admin', async function () { const owner = await this.accessControl.owner(); - expect(owner).to.equal(admin); + expect(owner).to.equal(defaultAdmin); expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, owner)).to.be.true; }); - it('should revert if granting admin role', async function () { + it('should revert if granting default admin role', async function () { await expectRevert( - this.accessControl.grantRole(DEFAULT_ADMIN_ROLE, admin, { from: admin }), - `${errorPrefix}: can't directly grant admin role`, + this.accessControl.grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }), + `${errorPrefix}: can't directly grant defaultAdmin role`, ); }); - it('should revert if revoking admin role', async function () { + it('should revert if revoking defaultAdmin role', async function () { await expectRevert( - this.accessControl.revokeRole(DEFAULT_ADMIN_ROLE, admin, { from: admin }), - `${errorPrefix}: can't directly revoke admin role`, + this.accessControl.revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }), + `${errorPrefix}: can't directly revoke defaultAdmin role`, ); }); - it("should revert if admin's is changed", async function () { + it("should revert if defaultAdmin's is changed", async function () { await expectRevert( - this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, admin), - `${errorPrefix}: can't override admin's admin`, + this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, defaultAdmin), + `${errorPrefix}: can't violate defaultAdmin rules`, ); }); - it('should not grant the admin role twice', async function () { + it('should not grant the default admin role twice', async function () { await expectRevert( - this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, admin), - `${errorPrefix}: admin already granted`, + this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin), + `${errorPrefix}: defaultAdmin already granted`, ); }); - describe('begins transfer admin', async function () { - it('should set pending admin and delayed until', async function () { - const receipt = await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + describe('begins transfer default admin', async function () { + it('should set pending default admin and delayed until', async function () { + const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); const delayedUntil = (await time.latest()).add(delay); - expect(await this.accessControl.pendingAdmin()).to.equal(newAdmin); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal((await time.latest()).add(delay)); - expectEvent(receipt, 'AdminRoleChangeStarted', { newAdmin, delayedUntil }); + expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, delayedUntil }); }); it('should revert if it called by non-admin accounts', async function () { await expectRevert( - this.accessControl.beginAdminTransfer(newAdmin, { from: other }), + this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); it('should revert if another transfer has started', async function () { - await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); await expectRevert( - this.accessControl.beginAdminTransfer(other, { from: admin }), + this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }), `${errorPrefix}: pending admin already set`, ); }); }); describe('accepts transfer admin', async function () { - let correctPendingAdmin; + let correctPendingDefaultAdmin; let correctIncreaseTo; beforeEach(async function () { @@ -290,50 +290,50 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA // are not mutually exclusive and accidentally an incorrect value will make it revert // possiblty creating false positives (eg. expect revert for incorrect caller but getting it // from a badly expected delayed until) - correctPendingAdmin = newAdmin; + correctPendingDefaultAdmin = newDefaultAdmin; correctIncreaseTo = (await time.latest()).add(delay).addn(1); - await this.accessControl.beginAdminTransfer(correctPendingAdmin, { from: admin }); + await this.accessControl.beginDefaultAdminTransfer(correctPendingDefaultAdmin, { from: defaultAdmin }); }); - describe('caller is pending admin and delayed until is met', async function () { + describe('caller is pending default admin and delayed until is met', async function () { let from; beforeEach(async function () { await time.increaseTo(correctIncreaseTo); - from = correctPendingAdmin; + from = correctPendingDefaultAdmin; }); - it('accepts a transfer and changes admin', async function () { - await this.accessControl.acceptAdminTransfer({ from }); - expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.be.false; - expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, newAdmin)).to.be.true; - expect(await this.accessControl.owner()).to.equal(newAdmin); + it('accepts a transfer and changes default admin', async function () { + await this.accessControl.acceptDefaultAdminTransfer({ from }); + 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); }); it('accepts a transfer and emit events', async function () { - const receipt = await this.accessControl.acceptAdminTransfer({ from }); + const receipt = await this.accessControl.acceptDefaultAdminTransfer({ from }); expectEvent(receipt, 'RoleRevoked', { role: DEFAULT_ADMIN_ROLE, - account: admin, + account: defaultAdmin, }); expectEvent(receipt, 'RoleGranted', { role: DEFAULT_ADMIN_ROLE, - account: newAdmin, + account: newDefaultAdmin, }); }); it('accepts a transfer resetting pending admin and delayed until', async function () { - await this.accessControl.acceptAdminTransfer({ from }); + await this.accessControl.acceptDefaultAdminTransfer({ from }); expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); }); }); it('should revert if caller is not pending admin', async function () { await time.increaseTo(correctIncreaseTo); await expectRevert( - this.accessControl.acceptAdminTransfer({ from: other }), + this.accessControl.acceptDefaultAdminTransfer({ from: other }), `${errorPrefix}: delay must be met and caller must be pending admin`, ); }); @@ -348,7 +348,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA it('should revert if block.timestamp is equal to delayed until', async function () { await time.increaseTo(incorrectIncreaseTo); await expectRevert( - this.accessControl.acceptAdminTransfer({ from: correctPendingAdmin }), + this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), `${errorPrefix}: delay must be met and caller must be pending admin`, ); }); @@ -356,27 +356,27 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA it('should revert if block.timestamp is less to delayed until', async function () { await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( - this.accessControl.acceptAdminTransfer({ from: correctPendingAdmin }), + this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), `${errorPrefix}: delay must be met and caller must be pending admin`, ); }); }); }); - describe('cancel transfer admin', async function () { + describe('cancel transfer default admin', async function () { beforeEach(async function () { - await this.accessControl.beginAdminTransfer(newAdmin, { from: admin }); + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); }); - it('resets pending admin and delayed until', async function () { - await this.accessControl.cancelAdminTransfer({ from: admin }); + it('resets pending default admin and delayed until', async function () { + await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingAdmin()).to.equal(ZERO_ADDRESS); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); }); - it('reverts if called by non-admin accounts', async function () { + it('reverts if called by non default admin accounts', async function () { await expectRevert( - this.accessControl.cancelAdminTransfer({ from: other }), + this.accessControl.cancelDefaultAdminTransfer({ from: other }), `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); @@ -384,25 +384,25 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA describe('renouncing admin', async function () { let correctIncreaseTo; - let from = admin; + let from = defaultAdmin; beforeEach(async function () { correctIncreaseTo = (await time.latest()).add(delay).addn(1); - await this.accessControl.beginAdminTransfer(ZERO_ADDRESS, { from }); + await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); }); - describe('caller is admin and delayed until is met', async function () { + describe('caller is default admin and delayed until is met', async function () { let receipt; beforeEach(async function () { await time.increaseTo(correctIncreaseTo); - from = admin; + from = defaultAdmin; receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); }); it('renounces role and does not grant it to the ZERO ADDRESS', async function () { - expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.be.false; - expect(await this.accessControl.hasRole(ZERO_ADDRESS, admin)).to.be.false; + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; + expect(await this.accessControl.hasRole(ZERO_ADDRESS, defaultAdmin)).to.be.false; }); it('emits events', async function () { @@ -425,7 +425,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA }); }); - it('reverts if caller is not admin', async function () { + it('reverts if caller is not default admin', async function () { await time.increaseTo(correctIncreaseTo); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), @@ -443,16 +443,16 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, admin, newA it('reverts if block.timestamp is equal to delayed until', async function () { await time.increaseTo(incorrectIncreaseTo); await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, admin, { from }), - `${errorPrefix}: admin can only renounce in two delayed steps`, + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), + `${errorPrefix}: only can renounce in two delayed steps`, ); }); it('reverts if block.timestamp is less to delayed until', async function () { await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, admin, { from }), - `${errorPrefix}: admin can only renounce in two delayed steps`, + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), + `${errorPrefix}: only can renounce in two delayed steps`, ); }); }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index be1212de2c3..50a4d3c0518 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -38,12 +38,12 @@ const INTERFACES = { ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], AccessControlAdminRules: [ - 'admin()', + 'defaultAdmin()', 'delayedUntil()', - 'pendingAdmin()', - 'beginAdminTransfer(address)', - 'acceptAdminTransfer()', - 'cancelAdminTransfer()', + 'pendingDefaultAdmin()', + 'beginDefaultAdminTransfer(address)', + 'acceptDefaultAdminTransfer()', + 'cancelDefaultAdminTransfer()', ], Governor: [ 'name()', From d1064f4ee498be92609aad1ab265856e8c177ffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 16 Feb 2023 17:24:14 -0600 Subject: [PATCH 24/69] Update contracts/access/AccessControlAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index fda646cb91a..c3f330bf99e 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -163,7 +163,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function grantRole( bytes32 role, address account - ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + ) public virtual override(IAccessControl, AccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant defaultAdmin role"); super.grantRole(role, account); } From d3526941c74c12cc6513a630c738ed9f7345c434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 16 Feb 2023 17:24:31 -0600 Subject: [PATCH 25/69] Update contracts/access/AccessControlAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index c3f330bf99e..1006b30b126 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -174,7 +174,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, function revokeRole( bytes32 role, address account - ) public virtual override(IAccessControl, AccessControl) onlyRole(getRoleAdmin(role)) { + ) public virtual override(IAccessControl, AccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke defaultAdmin role"); super.revokeRole(role, account); } From 53e8087d7b5c6973627617e584512000a24a58a7 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 17:25:03 -0600 Subject: [PATCH 26/69] Rename delayedUntil --- contracts/access/AccessControlAdminRules.sol | 36 +++++++++---------- contracts/access/IAccessControlAdminRules.sol | 6 ++-- test/access/AccessControl.behavior.js | 12 +++---- .../SupportsInterface.behavior.js | 2 +- 4 files changed, 26 insertions(+), 30 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 1006b30b126..522e79b00a1 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -48,7 +48,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, address private _currentDefaultAdmin; address private _pendingDefaultAdmin; - uint48 private _delayedUntil; + uint48 private defaultAadminTransferDelayedUntil; /** * @dev Sets the initial values for {delay} and {defaultAdmin}. @@ -82,10 +82,10 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-delayedUntil}. + * @dev See {IAccessControlAdminRules-defaultAdminTransferDelayedUntil}. */ - function delayedUntil() public view virtual returns (uint48) { - return _delayedUntil; + function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { + return defaultAadminTransferDelayedUntil; } /** @@ -106,10 +106,10 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer} */ function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { - require(delayedUntil() == 0, "AccessControl: pending admin already set"); - _delayedUntil = SafeCast.toUint48(block.timestamp) + _delay; + require(defaultAdminTransferDelayedUntil() == 0, "AccessControl: pending admin already set"); + defaultAadminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingDefaultAdmin = newAdmin; - emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), delayedUntil()); + emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); } /** @@ -137,7 +137,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev Revokes `role` from the calling account. * * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required - * that the {delayedUntil} is met and the pending default admin is the zero address. + * that the {defaultAdminTransferDelayedUntil} is met and the pending default admin is the zero address. * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` * functions. * @@ -160,10 +160,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, /** * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function grantRole( - bytes32 role, - address account - ) public virtual override(IAccessControl, AccessControl) { + function grantRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant defaultAdmin role"); super.grantRole(role, account); } @@ -171,10 +168,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, /** * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function revokeRole( - bytes32 role, - address account - ) public virtual override(IAccessControl, AccessControl) { + function revokeRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke defaultAdmin role"); super.revokeRole(role, account); } @@ -222,14 +216,16 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, */ function _resetDefaultAdminTransfer() private { delete _pendingDefaultAdmin; - delete _delayedUntil; + delete defaultAadminTransferDelayedUntil; } /** - * @dev Checks if a {delayedUntil} has been set and met. + * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and met. */ function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { - uint48 delayedUntilTimestamp = delayedUntil(); - return delayedUntilTimestamp > 0 && delayedUntilTimestamp < block.timestamp; + uint48 defaultAdminTransferDelayedUntilTimestamp = defaultAdminTransferDelayedUntil(); + return + defaultAdminTransferDelayedUntilTimestamp > 0 && + defaultAdminTransferDelayedUntilTimestamp < block.timestamp; } } diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlAdminRules.sol index 8d74f9cab24..52d5df9c59c 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlAdminRules.sol @@ -13,9 +13,9 @@ import "./IAccessControl.sol"; interface IAccessControlAdminRules is IAccessControl { /** * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` - * as the next default admin to be claimed after `delayedUntil` is met. + * as the next default admin to be claimed after `defaultAdminTransferDelayedUntil` is met. */ - event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 delayedUntil); + event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); /** * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. @@ -26,7 +26,7 @@ interface IAccessControlAdminRules is IAccessControl { * @dev Returns the timestamp in which the pending default admin can claim the * `DEFAULT_ADMIN_ROLE`. */ - function delayedUntil() external view returns (uint48); + function defaultAdminTransferDelayedUntil() external view returns (uint48); /** * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index fcff7ea25e8..f865413034e 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -215,7 +215,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi shouldSupportInterfaces(['AccessControlAdminRules']); it('has a default disabled delayed until', async function () { - expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); }); it('has a default pending default admin', async function () { @@ -259,10 +259,10 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi describe('begins transfer default admin', async function () { it('should set pending default admin and delayed until', async function () { const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - const delayedUntil = (await time.latest()).add(delay); + const defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal((await time.latest()).add(delay)); - expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, delayedUntil }); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal((await time.latest()).add(delay)); + expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); }); it('should revert if it called by non-admin accounts', async function () { @@ -325,7 +325,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi it('accepts a transfer resetting pending admin and delayed until', async function () { await this.accessControl.acceptDefaultAdminTransfer({ from }); - expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); }); }); @@ -370,7 +370,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi it('resets pending default admin and delayed until', async function () { await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - expect(await this.accessControl.delayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 50a4d3c0518..ad8fefdc026 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -39,7 +39,7 @@ const INTERFACES = { AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], AccessControlAdminRules: [ 'defaultAdmin()', - 'delayedUntil()', + 'defaultAdminTransferDelayedUntil()', 'pendingDefaultAdmin()', 'beginDefaultAdminTransfer(address)', 'acceptDefaultAdminTransfer()', From 56b5df6f0ff39444052a22abeef0905a1ff8cfa3 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 17:29:06 -0600 Subject: [PATCH 27/69] Lint --- test/access/AccessControl.behavior.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index f865413034e..9f0dc9f0c3b 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -261,7 +261,9 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); const defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal((await time.latest()).add(delay)); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + (await time.latest()).add(delay), + ); expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); }); From 701d3415a1c840acac7832c49ed89c4d2427bb1f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 18:08:54 -0600 Subject: [PATCH 28/69] Small fixes --- contracts/access/AccessControlAdminRules.sol | 12 ++++++------ test/access/AccessControl.behavior.js | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 522e79b00a1..a1db24b66e0 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -48,7 +48,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, address private _currentDefaultAdmin; address private _pendingDefaultAdmin; - uint48 private defaultAadminTransferDelayedUntil; + uint48 private _defaultAdminTransferDelayedUntil; /** * @dev Sets the initial values for {delay} and {defaultAdmin}. @@ -85,7 +85,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev See {IAccessControlAdminRules-defaultAdminTransferDelayedUntil}. */ function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { - return defaultAadminTransferDelayedUntil; + return _defaultAdminTransferDelayedUntil; } /** @@ -106,8 +106,8 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer} */ function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { - require(defaultAdminTransferDelayedUntil() == 0, "AccessControl: pending admin already set"); - defaultAadminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; + require(defaultAdminTransferDelayedUntil() == 0, "AccessControl: pending defaultAdmin already set"); + _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingDefaultAdmin = newAdmin; emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); } @@ -119,7 +119,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, address pendingDefaultAdminHolder = pendingDefaultAdmin(); require( _hasDefaultAdminTransferDelayPassed() && _msgSender() == pendingDefaultAdminHolder, - "AccessControl: delay must be met and caller must be pending admin" + "AccessControl: can't accept defaultAdmin" ); _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdminHolder); @@ -216,7 +216,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, */ function _resetDefaultAdminTransfer() private { delete _pendingDefaultAdmin; - delete defaultAadminTransferDelayedUntil; + delete _defaultAdminTransferDelayedUntil; } /** diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 9f0dc9f0c3b..023cea8d8ec 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -278,7 +278,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); await expectRevert( this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }), - `${errorPrefix}: pending admin already set`, + `${errorPrefix}: pending defaultAdmin already set`, ); }); }); @@ -336,7 +336,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi await time.increaseTo(correctIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: other }), - `${errorPrefix}: delay must be met and caller must be pending admin`, + `${errorPrefix}: can't accept defaultAdmin`, ); }); @@ -351,7 +351,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi await time.increaseTo(incorrectIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: delay must be met and caller must be pending admin`, + `${errorPrefix}: can't accept defaultAdmin`, ); }); @@ -359,7 +359,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: delay must be met and caller must be pending admin`, + `${errorPrefix}: can't accept defaultAdmin`, ); }); }); From 8ebb27beea2f4bbf89c791b23987489c6731bc20 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 18:22:45 -0600 Subject: [PATCH 29/69] More typos and minor corrections --- contracts/access/AccessControlAdminRules.sol | 4 ++-- test/access/AccessControl.behavior.js | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index a1db24b66e0..5ed2f805268 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -143,7 +143,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * * For other roles, see {AccessControl-renounceRole}. * - * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without an owner, + * 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. */ @@ -184,7 +184,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, /** * @dev Grants `role` to `account`. * - * For `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a role's owner + * 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}. diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 023cea8d8ec..772492f901c 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -242,7 +242,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi ); }); - it("should revert if defaultAdmin's is changed", async function () { + it("should revert if defaultAdmin's admin is changed", async function () { await expectRevert( this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, defaultAdmin), `${errorPrefix}: can't violate defaultAdmin rules`, @@ -267,7 +267,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); }); - it('should revert if it called by non-admin accounts', async function () { + 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}`, @@ -325,14 +325,14 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi }); }); - it('accepts a transfer resetting pending admin and delayed until', async function () { + it('accepts a transfer resetting pending default admin and delayed until', async function () { await this.accessControl.acceptDefaultAdminTransfer({ from }); 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 admin', async function () { + it('should revert if caller is not pending default admin', async function () { await time.increaseTo(correctIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: other }), @@ -340,7 +340,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi ); }); - describe('delayed until not met', async function () { + describe('delayedUntil not met', async function () { let incorrectIncreaseTo; beforeEach(async function () { From f7c6596064704d98055ae3a4c532c83415d102d3 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 18:41:59 -0600 Subject: [PATCH 30/69] Allow defaultAdmin to start transfer again --- contracts/access/AccessControlAdminRules.sol | 3 +- test/access/AccessControl.behavior.js | 31 ++++++++++++++------ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlAdminRules.sol index 5ed2f805268..0b42ce0d36b 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlAdminRules.sol @@ -103,10 +103,9 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer} + * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer}. */ function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { - require(defaultAdminTransferDelayedUntil() == 0, "AccessControl: pending defaultAdmin already set"); _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingDefaultAdmin = newAdmin; emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 772492f901c..6de12861254 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -262,23 +262,36 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi const defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - (await time.latest()).add(delay), + defaultAdminTransferDelayedUntil, ); expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); }); - 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}`, + it('should be able to begin a transfer again', async function () { + // First defaultAdmin transfer started + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + const firstDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + firstDefaultAdminTransferDelayedUntil, + ); + + // Time passes to last minute + await time.increaseTo(firstDefaultAdminTransferDelayedUntil.subn(1)); + + // defaultAdmin changes its mind and begin again to another address + await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); + const secondDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + secondDefaultAdminTransferDelayedUntil, ); }); - it('should revert if another transfer has started', async function () { - await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + it('should revert if it is called by non-admin accounts', async function () { await expectRevert( - this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }), - `${errorPrefix}: pending defaultAdmin already set`, + this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); }); From 5092d7093a4928cb221f2725265967fcb5281570 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 16 Feb 2023 18:45:06 -0600 Subject: [PATCH 31/69] Add extra test --- test/access/AccessControl.behavior.js | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 6de12861254..63ef7c50b03 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -267,7 +267,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); }); - it('should be able to begin a transfer again', async function () { + it('should be able to begin a transfer again before delay pass', async function () { // First defaultAdmin transfer started await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); const firstDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); @@ -276,7 +276,7 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi firstDefaultAdminTransferDelayedUntil, ); - // Time passes to last minute + // Time passes just before delay await time.increaseTo(firstDefaultAdminTransferDelayedUntil.subn(1)); // defaultAdmin changes its mind and begin again to another address @@ -288,6 +288,27 @@ function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmi ); }); + it('should be able to begin a transfer again after delay pass if not accepted', async function () { + // First defaultAdmin transfer started + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + const firstDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + firstDefaultAdminTransferDelayedUntil, + ); + + // Time passes after delay without acceptance + await time.increaseTo(firstDefaultAdminTransferDelayedUntil.addn(1)); + + // defaultAdmin changes its mind and begin again to another address + await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); + const secondDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( + secondDefaultAdminTransferDelayedUntil, + ); + }); + it('should revert if it is called by non-admin accounts', async function () { await expectRevert( this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), From 630df0d7978e01037fc332ccbf959bb4a862caac Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 17 Feb 2023 15:38:30 -0600 Subject: [PATCH 32/69] Rename to AccessControlDefaultAdminRules --- .changeset/silent-dancers-type.md | 2 +- contracts/access/AccessControl.sol | 2 +- ...sol => AccessControlDefaultAdminRules.sol} | 24 +++++++++---------- ...ol => IAccessControlDefaultAdminRules.sol} | 6 ++--- contracts/access/README.adoc | 4 ++-- docs/modules/ROOT/pages/access-control.adoc | 2 +- test/access/AccessControl.behavior.js | 6 ++--- test/access/AccessControlAdminRules.test.js | 18 -------------- .../AccessControlDefaultAdminRules.test.js | 18 ++++++++++++++ .../SupportsInterface.behavior.js | 2 +- 10 files changed, 42 insertions(+), 42 deletions(-) rename contracts/access/{AccessControlAdminRules.sol => AccessControlDefaultAdminRules.sol} (90%) rename contracts/access/{IAccessControlAdminRules.sol => IAccessControlDefaultAdminRules.sol} (90%) delete mode 100644 test/access/AccessControlAdminRules.test.js create mode 100644 test/access/AccessControlDefaultAdminRules.test.js diff --git a/.changeset/silent-dancers-type.md b/.changeset/silent-dancers-type.md index ca7bb49e948..74ecf500d01 100644 --- a/.changeset/silent-dancers-type.md +++ b/.changeset/silent-dancers-type.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`AccessControlAdminRules`: Add an extension of `AccessControl` with additional security rules for the `DEFAULT_ADMIN_ROLE`. +`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 6baa35a9900..3a73de78b55 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -44,7 +44,7 @@ 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. We recommend using {AccessControlAdminRules} + * 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 { diff --git a/contracts/access/AccessControlAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol similarity index 90% rename from contracts/access/AccessControlAdminRules.sol rename to contracts/access/AccessControlDefaultAdminRules.sol index 0b42ce0d36b..be3fa925de6 100644 --- a/contracts/access/AccessControlAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.8.0) (access/AccessControlAdminRules.sol) +// OpenZeppelin Contracts (last updated v4.8.0) (access/AccessControlDefaultAdminRules.sol) pragma solidity ^0.8.0; import "./AccessControl.sol"; -import "./IAccessControlAdminRules.sol"; +import "./IAccessControlDefaultAdminRules.sol"; import "../utils/math/SafeCast.sol"; import "../interfaces/IERC5313.sol"; @@ -29,8 +29,8 @@ import "../interfaces/IERC5313.sol"; * Example usage: * * ```solidity - * contract MyToken is AccessControlAdminRules { - * constructor() AccessControlAdminRules( + * contract MyToken is AccessControlDefaultAdminRules { + * constructor() AccessControlDefaultAdminRules( * 60 * 60 * 24, // 3 days * _msgSender() // Explicit initial `DEFAULT_ADMIN_ROLE` holder * ) {} @@ -42,7 +42,7 @@ import "../interfaces/IERC5313.sol"; * * _Available since v4.9._ */ -abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, AccessControl { +abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { uint48 private immutable _delay; address private _currentDefaultAdmin; @@ -75,21 +75,21 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-defaultAdmin}. + * @dev See {IAccessControlDefaultAdminRules-defaultAdmin}. */ function defaultAdmin() public view virtual returns (address) { return _currentDefaultAdmin; } /** - * @dev See {IAccessControlAdminRules-defaultAdminTransferDelayedUntil}. + * @dev See {IAccessControlDefaultAdminRules-defaultAdminTransferDelayedUntil}. */ function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { return _defaultAdminTransferDelayedUntil; } /** - * @dev See {IAccessControlAdminRules-pendingDefaultAdmin}. + * @dev See {IAccessControlDefaultAdminRules-pendingDefaultAdmin}. */ function pendingDefaultAdmin() public view virtual returns (address) { return _pendingDefaultAdmin; @@ -99,11 +99,11 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { - return interfaceId == type(IAccessControlAdminRules).interfaceId || super.supportsInterface(interfaceId); + return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId); } /** - * @dev See {IAccessControlAdminRules-beginDefaultAdminTransfer}. + * @dev See {IAccessControlDefaultAdminRules-beginDefaultAdminTransfer}. */ function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; @@ -112,7 +112,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-acceptDefaultAdminTransfer} + * @dev See {IAccessControlDefaultAdminRules-acceptDefaultAdminTransfer} */ function acceptDefaultAdminTransfer() public virtual { address pendingDefaultAdminHolder = pendingDefaultAdmin(); @@ -126,7 +126,7 @@ abstract contract AccessControlAdminRules is IAccessControlAdminRules, IERC5313, } /** - * @dev See {IAccessControlAdminRules-cancelDefaultAdminTransfer} + * @dev See {IAccessControlDefaultAdminRules-cancelDefaultAdminTransfer} */ function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _resetDefaultAdminTransfer(); diff --git a/contracts/access/IAccessControlAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol similarity index 90% rename from contracts/access/IAccessControlAdminRules.sol rename to contracts/access/IAccessControlDefaultAdminRules.sol index 52d5df9c59c..2caea892288 100644 --- a/contracts/access/IAccessControlAdminRules.sol +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -1,16 +1,16 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.9.0 (access/IAccessControlAdminRules.sol) +// OpenZeppelin Contracts v4.9.0 (access/IAccessControlDefaultAdminRules.sol) pragma solidity ^0.8.0; import "./IAccessControl.sol"; /** - * @dev External interface of AccessControlAdminRules declared to support ERC165 detection. + * @dev External interface of AccessControlDefaultAdminRules declared to support ERC165 detection. * * _Available since v4.9._ */ -interface IAccessControlAdminRules is IAccessControl { +interface IAccessControlDefaultAdminRules is IAccessControl { /** * @dev Emitted when an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` * as the next default admin to be claimed after `defaultAdminTransferDelayedUntil` is met. diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 45d12117a9b..324d0b2f389 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -24,6 +24,6 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControlEnumerable}} -{{IAccessControlAdminRules}} +{{IAccessControlDefaultAdminRules}} -{{AccessControlAdminRules}} +{{AccessControlDefaultAdminRules}} diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index 2681df87fbf..56d91e1c1c8 100644 --- a/docs/modules/ROOT/pages/access-control.adoc +++ b/docs/modules/ROOT/pages/access-control.adoc @@ -131,7 +131,7 @@ 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#AccessControlAdminRules[`AccessControlAdminRules`], 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. +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: diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 63ef7c50b03..241ce6147a4 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -211,8 +211,8 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, }); } -function shouldBehaveLikeAccessControlAdminRules(errorPrefix, delay, defaultAdmin, newDefaultAdmin, other) { - shouldSupportInterfaces(['AccessControlAdminRules']); +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)); @@ -499,5 +499,5 @@ module.exports = { DEFAULT_ADMIN_ROLE, shouldBehaveLikeAccessControl, shouldBehaveLikeAccessControlEnumerable, - shouldBehaveLikeAccessControlAdminRules, + shouldBehaveLikeAccessControlDefaultAdminRules, }; diff --git a/test/access/AccessControlAdminRules.test.js b/test/access/AccessControlAdminRules.test.js deleted file mode 100644 index 338a4dedb66..00000000000 --- a/test/access/AccessControlAdminRules.test.js +++ /dev/null @@ -1,18 +0,0 @@ -const { time } = require('@openzeppelin/test-helpers'); -const { - shouldBehaveLikeAccessControl, - shouldBehaveLikeAccessControlAdminRules, -} = require('./AccessControl.behavior.js'); - -const AccessControlAdminRules = artifacts.require('$AccessControlAdminRules'); - -contract('AccessControlAdminRules', function (accounts) { - const delay = web3.utils.toBN(time.duration.days(10)); - - beforeEach(async function () { - this.accessControl = await AccessControlAdminRules.new(delay, accounts[0], { from: accounts[0] }); - }); - - shouldBehaveLikeAccessControl('AccessControl', ...accounts); - shouldBehaveLikeAccessControlAdminRules('AccessControl', delay, ...accounts); -}); 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/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 0172f376830..4e8b34784e8 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -37,7 +37,7 @@ const INTERFACES = { 'renounceRole(bytes32,address)', ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], - AccessControlAdminRules: [ + AccessControlDefaultAdminRules: [ 'defaultAdmin()', 'defaultAdminTransferDelayedUntil()', 'pendingDefaultAdmin()', From 07d12a2d87639a2b968f7a36517174a228a9185f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 17 Feb 2023 16:47:15 -0600 Subject: [PATCH 33/69] Removed IAccessControlDefaultAdminRules --- .../access/AccessControlDefaultAdminRules.sol | 69 ++++++++++++------ .../IAccessControlDefaultAdminRules.sol | 71 ------------------- 2 files changed, 47 insertions(+), 93 deletions(-) delete mode 100644 contracts/access/IAccessControlDefaultAdminRules.sol diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index be3fa925de6..424a5461638 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -4,14 +4,13 @@ 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` 's holder, which is a sensitive role with special permissions - * over other valid roles that may potentially have rights. + * 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 `adminRole` assigned, the holder of the * `DEFAULT_ADMIN_ROLE` will have the ability to manage it, as determined by the @@ -19,9 +18,8 @@ import "../interfaces/IERC5313.sol"; * * This contract implements the following risk mitigations on top of the {AccessControl} implementation: * - * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when renounced. + * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when it's renounced. * * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. - * - Even when it's been renounced. * * 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. * * The `DEFAULT_ADMIN_ROLE` 's admin can be only itself. @@ -42,7 +40,7 @@ import "../interfaces/IERC5313.sol"; * * _Available since v4.9._ */ -abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { +abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { uint48 private immutable _delay; address private _currentDefaultAdmin; @@ -51,7 +49,14 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu uint48 private _defaultAdminTransferDelayedUntil; /** - * @dev Sets the initial values for {delay} and {defaultAdmin}. + * @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` is met. + */ + event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); + + /** + * @dev Sets the initial values for {delay} in seconds and {defaultAdmin}. * * The `delay` value is immutable. It can only be set at the constructor. */ @@ -75,44 +80,59 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @dev See {IAccessControlDefaultAdminRules-defaultAdmin}. + * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. */ function defaultAdmin() public view virtual returns (address) { return _currentDefaultAdmin; } /** - * @dev See {IAccessControlDefaultAdminRules-defaultAdminTransferDelayedUntil}. + * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. */ - function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { - return _defaultAdminTransferDelayedUntil; + function pendingDefaultAdmin() public view virtual returns (address) { + return _pendingDefaultAdmin; } /** - * @dev See {IAccessControlDefaultAdminRules-pendingDefaultAdmin}. + * @dev Returns the timestamp after which the pending default admin can claim the `DEFAULT_ADMIN_ROLE`. */ - function pendingDefaultAdmin() public view virtual returns (address) { - return _pendingDefaultAdmin; + 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); + return + interfaceId == 0x4a02b518 || // type(IAccessControlDefaultAdminRules).interfaceId + super.supportsInterface(interfaceId); } /** - * @dev See {IAccessControlDefaultAdminRules-beginDefaultAdminTransfer}. + * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin + * and a timer to be met. + * + * Requirements: + * + * - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder. + * + * Emits a {DefaultAdminRoleChangeStarted}. */ - function beginDefaultAdminTransfer(address newAdmin) public virtual override onlyRole(DEFAULT_ADMIN_ROLE) { + function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; _pendingDefaultAdmin = newAdmin; emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); } /** - * @dev See {IAccessControlDefaultAdminRules-acceptDefaultAdminTransfer} + * @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() public virtual { address pendingDefaultAdminHolder = pendingDefaultAdmin(); @@ -126,7 +146,12 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @dev See {IAccessControlDefaultAdminRules-cancelDefaultAdminTransfer} + * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. + * + * Requirements: + * + * - Can be called even after the timer is met. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. */ function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _resetDefaultAdminTransfer(); @@ -146,7 +171,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * 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(IAccessControl, AccessControl) { + function renounceRole(bytes32 role, address account) public virtual override { if (role == DEFAULT_ADMIN_ROLE) { require( pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), @@ -159,7 +184,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu /** * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function grantRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { + function grantRole(bytes32 role, address account) public virtual override { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant defaultAdmin role"); super.grantRole(role, account); } @@ -167,7 +192,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu /** * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function revokeRole(bytes32 role, address account) public virtual override(IAccessControl, AccessControl) { + function revokeRole(bytes32 role, address account) public virtual override { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke defaultAdmin role"); super.revokeRole(role, account); } diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol deleted file mode 100644 index 2caea892288..00000000000 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ /dev/null @@ -1,71 +0,0 @@ -// 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 an `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` - * as the next default admin to be claimed after `defaultAdminTransferDelayedUntil` is met. - */ - event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); - - /** - * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. - */ - function defaultAdmin() external view returns (address); - - /** - * @dev Returns the timestamp in which the pending default admin can claim the - * `DEFAULT_ADMIN_ROLE`. - */ - function defaultAdminTransferDelayedUntil() external view returns (uint48); - - /** - * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. - */ - function pendingDefaultAdmin() external view returns (address); - - /** - * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin - * and a timer to be met. - * - * Requirements: - * - * - There shouldn't be another default admin transfer in progress. See {cancelDefaultAdminTransfer}. - * - 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. - * - Should allow to call {beginDefaultAdminTransfer} again. - */ - function acceptDefaultAdminTransfer() external; - - /** - * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. - * - * Requirements: - * - * - Should allow to call {beginDefaultAdminTransfer} again. - * - Can be called even after the timer is met. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. - */ - function cancelDefaultAdminTransfer() external; -} From 41f4ba3d530823ec6f43a7f397a8711b16d4efdd Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 17 Feb 2023 19:52:54 -0600 Subject: [PATCH 34/69] Add note in Extending Contracts section --- docs/modules/ROOT/pages/extending-contracts.adoc | 2 ++ 1 file changed, 2 insertions(+) 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 From 6da27add6078f50535a82df4fe34b70e4deb17cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 09:45:47 -0600 Subject: [PATCH 35/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Hadrien Croubois --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 424a5461638..e68908811fb 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -22,7 +22,7 @@ import "../interfaces/IERC5313.sol"; * * 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. - * * The `DEFAULT_ADMIN_ROLE` 's admin can be only itself. + * * It is not possible to use another role to manage the `DEFAULT_ADMIN_ROLE`. * * Example usage: * From 4b39c30b84d95ecfeb3cddbeb40721ba7d80b0f1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 09:48:52 -0600 Subject: [PATCH 36/69] Fix comments example --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index e68908811fb..45bccc08c8d 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -29,7 +29,7 @@ import "../interfaces/IERC5313.sol"; * ```solidity * contract MyToken is AccessControlDefaultAdminRules { * constructor() AccessControlDefaultAdminRules( - * 60 * 60 * 24, // 3 days + * 3 days, * _msgSender() // Explicit initial `DEFAULT_ADMIN_ROLE` holder * ) {} *} From da8227f606d6a9c5f76224991eca0e181390f537 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 09:50:51 -0600 Subject: [PATCH 37/69] Add virtual to delay() --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 45bccc08c8d..5a95b48f8e1 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -68,7 +68,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { /** * @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer. */ - function delay() public view returns (uint48) { + function delay() public view virtual returns (uint48) { return _delay; } From 6139905b6758c9e412fc114a1d85dff7c50885b8 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 09:53:46 -0600 Subject: [PATCH 38/69] Replace _delay with getter --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 5a95b48f8e1..6b8e4740075 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -120,7 +120,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { * Emits a {DefaultAdminRoleChangeStarted}. */ function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + _delay; + _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + delay(); _pendingDefaultAdmin = newAdmin; emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); } From f420162c0eafad65187bdc219a67d5a4854beb64 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 12:28:19 -0600 Subject: [PATCH 39/69] Add IAccessControlDefaultAdmin back --- .../access/AccessControlDefaultAdminRules.sol | 50 ++++---------- .../IAccessControlDefaultAdminRules.sol | 68 +++++++++++++++++++ 2 files changed, 80 insertions(+), 38 deletions(-) create mode 100644 contracts/access/IAccessControlDefaultAdminRules.sol diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 6b8e4740075..8f3b4b90049 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; +import "./IAccessControlDefaultAdminRules.sol"; import "../utils/math/SafeCast.sol"; import "../interfaces/IERC5313.sol"; @@ -40,7 +41,7 @@ import "../interfaces/IERC5313.sol"; * * _Available since v4.9._ */ -abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { +abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { uint48 private immutable _delay; address private _currentDefaultAdmin; @@ -48,13 +49,6 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { uint48 private _defaultAdminTransferDelayedUntil; - /** - * @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` is met. - */ - event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); - /** * @dev Sets the initial values for {delay} in seconds and {defaultAdmin}. * @@ -80,21 +74,21 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { } /** - * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. + * @inheritdoc IAccessControlDefaultAdminRules */ function defaultAdmin() public view virtual returns (address) { return _currentDefaultAdmin; } /** - * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. + * @inheritdoc IAccessControlDefaultAdminRules */ function pendingDefaultAdmin() public view virtual returns (address) { return _pendingDefaultAdmin; } /** - * @dev Returns the timestamp after which the pending default admin can claim the `DEFAULT_ADMIN_ROLE`. + * @inheritdoc IAccessControlDefaultAdminRules */ function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { return _defaultAdminTransferDelayedUntil; @@ -104,20 +98,11 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { - return - interfaceId == 0x4a02b518 || // type(IAccessControlDefaultAdminRules).interfaceId - super.supportsInterface(interfaceId); + return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId); } /** - * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin - * and a timer to be met. - * - * Requirements: - * - * - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder. - * - * Emits a {DefaultAdminRoleChangeStarted}. + * @inheritdoc IAccessControlDefaultAdminRules */ function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + delay(); @@ -126,13 +111,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { } /** - * @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. + * @inheritdoc IAccessControlDefaultAdminRules */ function acceptDefaultAdminTransfer() public virtual { address pendingDefaultAdminHolder = pendingDefaultAdmin(); @@ -146,12 +125,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { } /** - * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. - * - * Requirements: - * - * - Can be called even after the timer is met. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. + * @inheritdoc IAccessControlDefaultAdminRules */ function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _resetDefaultAdminTransfer(); @@ -171,7 +145,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { * 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 { + function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { if (role == DEFAULT_ADMIN_ROLE) { require( pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), @@ -184,7 +158,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { /** * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function grantRole(bytes32 role, address account) public virtual override { + function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant defaultAdmin role"); super.grantRole(role, account); } @@ -192,7 +166,7 @@ abstract contract AccessControlDefaultAdminRules is IERC5313, AccessControl { /** * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function revokeRole(bytes32 role, address account) public virtual override { + function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke defaultAdmin role"); super.revokeRole(role, account); } diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol new file mode 100644 index 00000000000..8c7eee223a4 --- /dev/null +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -0,0 +1,68 @@ +// 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` is met. + */ + event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); + + /** + * @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 be met. + * + * 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 is met. + * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. + */ + function cancelDefaultAdminTransfer() external; +} From 8747bf4863fe4dd3cdee0904888315d6f5552fed Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 12:37:01 -0600 Subject: [PATCH 40/69] Created interal functions for beginning and accepting transfers --- .../access/AccessControlDefaultAdminRules.sol | 37 +++++++++++++------ test/access/AccessControl.behavior.js | 4 +- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 8f3b4b90049..07411716646 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -105,23 +105,15 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @inheritdoc IAccessControlDefaultAdminRules */ function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + delay(); - _pendingDefaultAdmin = newAdmin; - emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); + _beginDefaultAdminTransfer(newAdmin); } /** * @inheritdoc IAccessControlDefaultAdminRules */ function acceptDefaultAdminTransfer() public virtual { - address pendingDefaultAdminHolder = pendingDefaultAdmin(); - require( - _hasDefaultAdminTransferDelayPassed() && _msgSender() == pendingDefaultAdminHolder, - "AccessControl: can't accept defaultAdmin" - ); - _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); - _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdminHolder); - _resetDefaultAdminTransfer(); + require(_msgSender() == pendingDefaultAdmin(), "AccessControl: can't accept defaultAdmin"); + _acceptDefaultAdminTransfer(); } /** @@ -199,6 +191,29 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu super._grantRole(role, account); } + /** + * @dev See {acceptDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + function _acceptDefaultAdminTransfer() internal virtual { + require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: delayedUntil not met"); + _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) + delay(); + _pendingDefaultAdmin = newAdmin; + emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); + } + /** * @dev See {AccessControl-_revokeRole}. */ diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 241ce6147a4..da2f3c9b4bb 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -385,7 +385,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(incorrectIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: can't accept defaultAdmin`, + `${errorPrefix}: delayedUntil not met`, ); }); @@ -393,7 +393,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: can't accept defaultAdmin`, + `${errorPrefix}: delayedUntil not met`, ); }); }); From 5743d7a165c7ef339b31a5dc1becde5220a9f5ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 17:56:23 -0600 Subject: [PATCH 41/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 07411716646..40745dea198 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -31,7 +31,7 @@ import "../interfaces/IERC5313.sol"; * contract MyToken is AccessControlDefaultAdminRules { * constructor() AccessControlDefaultAdminRules( * 3 days, - * _msgSender() // Explicit initial `DEFAULT_ADMIN_ROLE` holder + * msg.sender // Explicit initial `DEFAULT_ADMIN_ROLE` holder * ) {} *} * ``` From 0b9ea0b0dcc62247523c371fc5a341e73befb35a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 17:59:55 -0600 Subject: [PATCH 42/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 40745dea198..d1978db0c2c 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -112,7 +112,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @inheritdoc IAccessControlDefaultAdminRules */ function acceptDefaultAdminTransfer() public virtual { - require(_msgSender() == pendingDefaultAdmin(), "AccessControl: can't accept defaultAdmin"); + require(_msgSender() == pendingDefaultAdmin(), "AccessControl: pending admin must accept"); _acceptDefaultAdminTransfer(); } From 780e901d1abfc4887e62ec4cff1b6cb648cbbae6 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:02:06 -0600 Subject: [PATCH 43/69] Fixed test error message --- contracts/access/README.adoc | 2 -- test/access/AccessControl.behavior.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 324d0b2f389..80ca0020f4b 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -24,6 +24,4 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControlEnumerable}} -{{IAccessControlDefaultAdminRules}} - {{AccessControlDefaultAdminRules}} diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index da2f3c9b4bb..fb1715752eb 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -370,7 +370,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(correctIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: other }), - `${errorPrefix}: can't accept defaultAdmin`, + `${errorPrefix}: pending admin must accept`, ); }); From 24ce7fa5850617ee3a396bcf0ed2659e5fac90cb Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:11:26 -0600 Subject: [PATCH 44/69] Change delay to defaultAdminDelay --- .../access/AccessControlDefaultAdminRules.sol | 28 +++++++++---------- .../IAccessControlDefaultAdminRules.sol | 5 ++++ .../SupportsInterface.behavior.js | 1 + 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index d1978db0c2c..6fde6f9c53e 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -36,13 +36,13 @@ import "../interfaces/IERC5313.sol"; *} * ``` * - * NOTE: The `delay` is only configurable in the constructor to avoid issues related with handling - * delay management during the transfer is pending to be completed. + * NOTE: The `defaultAdminDelay` is only configurable in the constructor to avoid issues related with handling + * defaultAdminDelay management during the transfer is pending to be completed. * * _Available since v4.9._ */ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { - uint48 private immutable _delay; + uint48 private immutable _defaultAdminDelay; address private _currentDefaultAdmin; address private _pendingDefaultAdmin; @@ -50,27 +50,27 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu uint48 private _defaultAdminTransferDelayedUntil; /** - * @dev Sets the initial values for {delay} in seconds and {defaultAdmin}. + * @dev Sets the initial values for {defaultAdminDelay} in seconds and {defaultAdmin}. * - * The `delay` value is immutable. It can only be set at the constructor. + * The `defaultAdminDelay` value is immutable. It can only be set at the constructor. */ - constructor(uint48 delay_, address initialDefaultAdmin) { - _delay = delay_; + constructor(uint48 defaultAdminDelay_, address initialDefaultAdmin) { + _defaultAdminDelay = defaultAdminDelay_; _grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin); } /** - * @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer. + * @dev See {IERC5313-owner}. */ - function delay() public view virtual returns (uint48) { - return _delay; + function owner() public view virtual returns (address) { + return defaultAdmin(); } /** - * @dev See {IERC5313-owner}. + * @inheritdoc IAccessControlDefaultAdminRules */ - function owner() public view virtual returns (address) { - return defaultAdmin(); + function defaultAdminDelay() public view virtual returns (uint48) { + return _defaultAdminDelay; } /** @@ -209,7 +209,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * Internal function without access restriction. */ function _beginDefaultAdminTransfer(address newAdmin) internal virtual { - _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + delay(); + _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + defaultAdminDelay(); _pendingDefaultAdmin = newAdmin; emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); } diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol index 8c7eee223a4..a8c17deb376 100644 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -18,6 +18,11 @@ interface IAccessControlDefaultAdminRules is IAccessControl { */ 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. */ diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 4e8b34784e8..5ffe242edc5 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -38,6 +38,7 @@ const INTERFACES = { ], AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], AccessControlDefaultAdminRules: [ + 'defaultAdminDelay()', 'defaultAdmin()', 'defaultAdminTransferDelayedUntil()', 'pendingDefaultAdmin()', From 2c4e15eb6ccbc5832c13ae360defd3bc53c078b1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:14:15 -0600 Subject: [PATCH 45/69] Change long variable name --- contracts/access/AccessControlDefaultAdminRules.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 6fde6f9c53e..07082521800 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -236,9 +236,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and met. */ function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { - uint48 defaultAdminTransferDelayedUntilTimestamp = defaultAdminTransferDelayedUntil(); + uint48 delayedUntil = defaultAdminTransferDelayedUntil(); return - defaultAdminTransferDelayedUntilTimestamp > 0 && - defaultAdminTransferDelayedUntilTimestamp < block.timestamp; + delayedUntil > 0 && + delayedUntil < block.timestamp; } } From d171299b78c41d4212a46bb0b3e11618ae7a0fe8 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:14:52 -0600 Subject: [PATCH 46/69] Run prettier --- contracts/access/AccessControlDefaultAdminRules.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 07082521800..c8d0a204901 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -237,8 +237,6 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu */ function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { uint48 delayedUntil = defaultAdminTransferDelayedUntil(); - return - delayedUntil > 0 && - delayedUntil < block.timestamp; + return delayedUntil > 0 && delayedUntil < block.timestamp; } } From 281209c1ef0c9df275c6d6e04de5a51ac95ba696 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:16:30 -0600 Subject: [PATCH 47/69] Remove IAccessControlDefaultAdminRules from docs --- contracts/access/README.adoc | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 80ca0020f4b..1abc37cccb8 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -20,8 +20,6 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControlCrossChain}} -{{IAccessControlEnumerable}} - {{AccessControlEnumerable}} {{AccessControlDefaultAdminRules}} From 2c49a5dd5b58f9d07b9bc5dd470d0b1481ad3e88 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:24:02 -0600 Subject: [PATCH 48/69] Remove camel case from error messages --- .../access/AccessControlDefaultAdminRules.sol | 10 +++++----- test/access/AccessControl.behavior.js | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index c8d0a204901..32bb1767a90 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -151,7 +151,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @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 defaultAdmin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role"); super.grantRole(role, account); } @@ -159,7 +159,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @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 defaultAdmin role"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role"); super.revokeRole(role, account); } @@ -167,7 +167,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @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 defaultAdmin rules"); + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules"); super._setRoleAdmin(role, adminRole); } @@ -185,7 +185,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu */ function _grantRole(bytes32 role, address account) internal virtual override { if (role == DEFAULT_ADMIN_ROLE) { - require(defaultAdmin() == address(0), "AccessControl: defaultAdmin already granted"); + require(defaultAdmin() == address(0), "AccessControl: default admin already granted"); _currentDefaultAdmin = account; } super._grantRole(role, account); @@ -197,7 +197,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * Internal function without access restriction. */ function _acceptDefaultAdminTransfer() internal virtual { - require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: delayedUntil not met"); + require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not met"); _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin()); _resetDefaultAdminTransfer(); diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index fb1715752eb..8d63a0a265b 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -231,28 +231,28 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa 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 defaultAdmin role`, + `${errorPrefix}: can't directly grant default admin role`, ); }); - it('should revert if revoking defaultAdmin role', async function () { + 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 defaultAdmin role`, + `${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 defaultAdmin rules`, + `${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}: defaultAdmin already granted`, + `${errorPrefix}: default admin already granted`, ); }); @@ -385,7 +385,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(incorrectIncreaseTo); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: delayedUntil not met`, + `${errorPrefix}: transfer delay not met`, ); }); @@ -393,7 +393,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), - `${errorPrefix}: delayedUntil not met`, + `${errorPrefix}: transfer delay not met`, ); }); }); From cb98ad9fbbdaffe4d753363cf7cf27e65a8c099c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:26:23 -0600 Subject: [PATCH 49/69] Update test/access/AccessControl.behavior.js Co-authored-by: Francisco --- test/access/AccessControl.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 8d63a0a265b..3706ab19a99 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -256,7 +256,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('begins transfer default admin', async function () { + describe('begins transfer of default admin', async function () { it('should set pending default admin and delayed until', async function () { const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); const defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); From 88168be08f3dac806c1988813dfa3d89761444a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:26:55 -0600 Subject: [PATCH 50/69] Update test/access/AccessControl.behavior.js Co-authored-by: Francisco --- test/access/AccessControl.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 3706ab19a99..d2754c126df 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -332,7 +332,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await this.accessControl.beginDefaultAdminTransfer(correctPendingDefaultAdmin, { from: defaultAdmin }); }); - describe('caller is pending default admin and delayed until is met', async function () { + describe('caller is pending default admin and delay has passed', async function () { let from; beforeEach(async function () { From 39d176ad8cfde6e062711e361bcc997b8887cb44 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:35:26 -0600 Subject: [PATCH 51/69] Use beforeEach in begin transfer tests --- test/access/AccessControl.behavior.js | 47 ++++++++++----------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index d2754c126df..94c126fb0a8 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -257,56 +257,45 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('begins transfer of default admin', async function () { + let receipt; + let defaultAdminTransferDelayedUntil; + + beforeEach('begins admin transfer', async function () { + receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + }); + it('should set pending default admin and delayed until', async function () { - const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - const defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( defaultAdminTransferDelayedUntil, ); - expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { newDefaultAdmin, defaultAdminTransferDelayedUntil }); + expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { + newDefaultAdmin, + defaultAdminTransferDelayedUntil, + }); }); it('should be able to begin a transfer again before delay pass', async function () { - // First defaultAdmin transfer started - await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - const firstDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - firstDefaultAdminTransferDelayedUntil, - ); - // Time passes just before delay - await time.increaseTo(firstDefaultAdminTransferDelayedUntil.subn(1)); + await time.increaseTo(defaultAdminTransferDelayedUntil.subn(1)); // defaultAdmin changes its mind and begin again to another address await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const secondDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + const newDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - secondDefaultAdminTransferDelayedUntil, - ); + 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 () { - // First defaultAdmin transfer started - await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - const firstDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - firstDefaultAdminTransferDelayedUntil, - ); - // Time passes after delay without acceptance - await time.increaseTo(firstDefaultAdminTransferDelayedUntil.addn(1)); + await time.increaseTo(defaultAdminTransferDelayedUntil.addn(1)); // defaultAdmin changes its mind and begin again to another address await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const secondDefaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + const newDelayedUntil = (await time.latest()).add(delay); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - secondDefaultAdminTransferDelayedUntil, - ); + expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); }); it('should revert if it is called by non-admin accounts', async function () { From 9e52530b31803fb81ab6086684a58f82ef7d64be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:36:48 -0600 Subject: [PATCH 52/69] Update test/access/AccessControl.behavior.js Co-authored-by: Francisco --- test/access/AccessControl.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 94c126fb0a8..e709024bf46 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -378,7 +378,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - it('should revert if block.timestamp is less to delayed until', async function () { + it('should revert if block.timestamp is less than delayed until', async function () { await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), From 4721d63fa0ab8520bd39a414d29ac804be7247f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:38:29 -0600 Subject: [PATCH 53/69] Update test/access/AccessControl.behavior.js Co-authored-by: Francisco --- test/access/AccessControl.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index e709024bf46..49bf3ddfedb 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -473,7 +473,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - it('reverts if block.timestamp is less to delayed until', async function () { + it('reverts if block.timestamp is less than delayed until', async function () { await time.increaseTo(incorrectIncreaseTo.subn(1)); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), From 0e532c79ae1d957bea01acc9562f711464222fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:39:55 -0600 Subject: [PATCH 54/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 32bb1767a90..6d35cd4d82d 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -17,7 +17,7 @@ import "../interfaces/IERC5313.sol"; * `DEFAULT_ADMIN_ROLE` will have the ability to manage it, as determined by the * function {getRoleAdmin}'s default value (`address(0)`). * - * This contract implements the following risk mitigations on top of the {AccessControl} implementation: + * This contract implements the following risk mitigations on top of {AccessControl}: * * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when it's renounced. * * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. From 70c5cae2ab316603809197d76b989fbab8271c74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:40:13 -0600 Subject: [PATCH 55/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 6d35cd4d82d..7a5cae8d15d 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -14,8 +14,7 @@ import "../interfaces/IERC5313.sol"; * over other roles that may potentially have privileged rights in the system. * * 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}'s default value (`address(0)`). + * `DEFAULT_ADMIN_ROLE` will have the ability to grant it and revoke it. * * This contract implements the following risk mitigations on top of {AccessControl}: * From 667e3daac93e2f53fbc86a9b440428b11ce2d58c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:40:19 -0600 Subject: [PATCH 56/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 7a5cae8d15d..5e58b86582e 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -13,7 +13,7 @@ import "../interfaces/IERC5313.sol"; * 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 `adminRole` assigned, the holder of the + * 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}: From 5a31431a59746687d655ff35ab0264e533513380 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:42:41 -0600 Subject: [PATCH 57/69] Remove unnecessary async --- test/access/AccessControl.behavior.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 49bf3ddfedb..86a0afad4d9 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -256,7 +256,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('begins transfer of default admin', async function () { + describe('begins transfer of default admin', function () { let receipt; let defaultAdminTransferDelayedUntil; @@ -306,7 +306,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); }); - describe('accepts transfer admin', async function () { + describe('accepts transfer admin', function () { let correctPendingDefaultAdmin; let correctIncreaseTo; @@ -321,7 +321,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await this.accessControl.beginDefaultAdminTransfer(correctPendingDefaultAdmin, { from: defaultAdmin }); }); - describe('caller is pending default admin and delay has passed', async function () { + describe('caller is pending default admin and delay has passed', function () { let from; beforeEach(async function () { @@ -363,10 +363,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('delayedUntil not met', async function () { + describe('delayedUntil not met', function () { let incorrectIncreaseTo; - beforeEach(async function () { + beforeEach(function () { incorrectIncreaseTo = correctIncreaseTo.subn(1); }); @@ -388,7 +388,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); }); - describe('cancel transfer default admin', async function () { + describe('cancel transfer default admin', function () { beforeEach(async function () { await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); }); @@ -407,7 +407,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); }); - describe('renouncing admin', async function () { + describe('renouncing admin', function () { let correctIncreaseTo; let from = defaultAdmin; @@ -416,7 +416,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); }); - describe('caller is default admin and delayed until is met', async function () { + describe('caller is default admin and delayed until is met', function () { let receipt; beforeEach(async function () { @@ -430,7 +430,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa expect(await this.accessControl.hasRole(ZERO_ADDRESS, defaultAdmin)).to.be.false; }); - it('emits events', async function () { + it('emits events', function () { expectEvent(receipt, 'RoleRevoked', { role: DEFAULT_ADMIN_ROLE, account: from, @@ -458,10 +458,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('delayed until not met', async function () { + describe('delayed until not met', function () { let incorrectIncreaseTo; - beforeEach(async function () { + beforeEach(function () { incorrectIncreaseTo = correctIncreaseTo.subn(1); }); From 14994be0e20da5b1a19046f0f435cdd304583dc1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:46:46 -0600 Subject: [PATCH 58/69] Change `is met` for `has passed` --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- contracts/access/IAccessControlDefaultAdminRules.sol | 4 ++-- test/access/AccessControl.behavior.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 5e58b86582e..8cc5e8ff6c4 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -126,7 +126,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * @dev Revokes `role` from the calling account. * * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required - * that the {defaultAdminTransferDelayedUntil} is met and the pending default admin is the zero address. + * 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. * diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol index a8c17deb376..da92f1057fd 100644 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -14,7 +14,7 @@ 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` is met. + * after `defaultAdminTransferDelayedUntil` has passed. */ event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); @@ -66,7 +66,7 @@ interface IAccessControlDefaultAdminRules is IAccessControl { * * Requirements: * - * - Can be called even after the timer is met. + * - 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/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 86a0afad4d9..263595cfc2b 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -416,7 +416,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); }); - describe('caller is default admin and delayed until is met', function () { + describe('caller is default admin and delayed until has passed', function () { let receipt; beforeEach(async function () { From fd64f5d4bb5038a3e037afb3a4a22c7c7bbf1dba Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:48:35 -0600 Subject: [PATCH 59/69] Change note in docs --- contracts/access/AccessControlDefaultAdminRules.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 8cc5e8ff6c4..bfffb16097b 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -35,8 +35,7 @@ import "../interfaces/IERC5313.sol"; *} * ``` * - * NOTE: The `defaultAdminDelay` is only configurable in the constructor to avoid issues related with handling - * defaultAdminDelay management during the transfer is pending to be completed. + * NOTE: The `delay` can only be set in the constructor and is fixed thereafter. * * _Available since v4.9._ */ From 9e04336c085c9cb1826b517a43fdc863639b1951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 21 Feb 2023 18:50:48 -0600 Subject: [PATCH 60/69] Update contracts/access/AccessControlDefaultAdminRules.sol Co-authored-by: Francisco --- contracts/access/AccessControlDefaultAdminRules.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index bfffb16097b..6b06011271e 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -18,7 +18,7 @@ import "../interfaces/IERC5313.sol"; * * This contract implements the following risk mitigations on top of {AccessControl}: * - * * Only one account holds the `DEFAULT_ADMIN_ROLE` at every time after construction except when it's renounced. + * * 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. From ac093f42f13bbb6b52386c5c16212e3207d51ace Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 18:55:03 -0600 Subject: [PATCH 61/69] Simplify renouncing tests --- test/access/AccessControl.behavior.js | 44 +++++++++++---------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 263595cfc2b..9ed9e7e1024 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -416,37 +416,27 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); }); - describe('caller is default admin and delayed until has passed', function () { - let receipt; - - beforeEach(async function () { - await time.increaseTo(correctIncreaseTo); - from = defaultAdmin; - receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); - }); - - it('renounces role and does not grant it to the ZERO ADDRESS', async function () { - expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; - expect(await this.accessControl.hasRole(ZERO_ADDRESS, defaultAdmin)).to.be.false; - }); + it('it renounces role', async function () { + await time.increaseTo(correctIncreaseTo); + const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); - it('emits events', function () { - expectEvent(receipt, 'RoleRevoked', { - role: DEFAULT_ADMIN_ROLE, - account: 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('marks the contract as not owned', async function () { - expect(await this.accessControl.owner()).to.equal(ZERO_ADDRESS); - }); + it('allows to recover access using the internal _grantRole', async function () { + await time.increaseTo(correctIncreaseTo); + await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); - it('allows to recover access using the internal _grantRole', async function () { - const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); - expectEvent(grantRoleReceipt, 'RoleGranted', { - role: DEFAULT_ADMIN_ROLE, - account: other, - }); + const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); + expectEvent(grantRoleReceipt, 'RoleGranted', { + role: DEFAULT_ADMIN_ROLE, + account: other, }); }); From 394824665fe7bb4f15bb8b899111cff0b02a6dfe Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:06:16 -0600 Subject: [PATCH 62/69] Extend cancel tests --- test/access/AccessControl.behavior.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 9ed9e7e1024..da9840f9b05 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -397,6 +397,24 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa 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 + const increaseTo = (await time.latest()).add(delay).addn(1); + await time.increaseTo(increaseTo); + + // 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 }); + const increaseTo = (await time.latest()).add(delay).addn(1); + await time.increaseTo(increaseTo); + 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 () { From 34ed74f554691c0b660552683b14bd2e5ce7de4d Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:12:02 -0600 Subject: [PATCH 63/69] Rename delay test variables --- test/access/AccessControl.behavior.js | 45 ++++++++++++++------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index da9840f9b05..fa0838eb57f 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -308,7 +308,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa describe('accepts transfer admin', function () { let correctPendingDefaultAdmin; - let correctIncreaseTo; + let delayPassed; beforeEach(async function () { // Set as correct so they're explicitly needed in revert tests, since conditions @@ -316,7 +316,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa // possiblty creating false positives (eg. expect revert for incorrect caller but getting it // from a badly expected delayed until) correctPendingDefaultAdmin = newDefaultAdmin; - correctIncreaseTo = (await time.latest()).add(delay).addn(1); + delayPassed = (await time.latest()).add(delay).addn(1); await this.accessControl.beginDefaultAdminTransfer(correctPendingDefaultAdmin, { from: defaultAdmin }); }); @@ -325,7 +325,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa let from; beforeEach(async function () { - await time.increaseTo(correctIncreaseTo); + await time.increaseTo(delayPassed); from = correctPendingDefaultAdmin; }); @@ -356,7 +356,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('should revert if caller is not pending default admin', async function () { - await time.increaseTo(correctIncreaseTo); + await time.increaseTo(delayPassed); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: other }), `${errorPrefix}: pending admin must accept`, @@ -364,14 +364,14 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('delayedUntil not met', function () { - let incorrectIncreaseTo; + let delayNotPassed; beforeEach(function () { - incorrectIncreaseTo = correctIncreaseTo.subn(1); + delayNotPassed = delayPassed.subn(1); }); it('should revert if block.timestamp is equal to delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo); + await time.increaseTo(delayNotPassed); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), `${errorPrefix}: transfer delay not met`, @@ -379,7 +379,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('should revert if block.timestamp is less than delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo.subn(1)); + await time.increaseTo(delayNotPassed.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), `${errorPrefix}: transfer delay not met`, @@ -389,7 +389,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('cancel transfer default admin', function () { + let delayPassed; + beforeEach(async function () { + delayPassed = (await time.latest()).add(delay).addn(1); await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); }); @@ -399,9 +402,8 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); // Advance until passed delay - const increaseTo = (await time.latest()).add(delay).addn(1); - await time.increaseTo(increaseTo); - + await time.increaseTo(delayPassed); + // Previous pending default admin should not be able to accept after cancellation. await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), @@ -411,8 +413,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa it('cancels even after delay has passed', async function () { await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - const increaseTo = (await time.latest()).add(delay).addn(1); - await time.increaseTo(increaseTo); + await time.increaseTo(delayPassed); expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); }); @@ -426,16 +427,16 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('renouncing admin', function () { - let correctIncreaseTo; + let delayPassed; let from = defaultAdmin; beforeEach(async function () { - correctIncreaseTo = (await time.latest()).add(delay).addn(1); + delayPassed = (await time.latest()).add(delay).addn(1); await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); }); it('it renounces role', async function () { - await time.increaseTo(correctIncreaseTo); + await time.increaseTo(delayPassed); const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; @@ -448,7 +449,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('allows to recover access using the internal _grantRole', async function () { - await time.increaseTo(correctIncreaseTo); + await time.increaseTo(delayPassed); await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); @@ -459,7 +460,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('reverts if caller is not default admin', async function () { - await time.increaseTo(correctIncreaseTo); + await time.increaseTo(delayPassed); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), `${errorPrefix}: can only renounce roles for self`, @@ -467,14 +468,14 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('delayed until not met', function () { - let incorrectIncreaseTo; + let delayNotPassed; beforeEach(function () { - incorrectIncreaseTo = correctIncreaseTo.subn(1); + delayNotPassed = delayPassed.subn(1); }); it('reverts if block.timestamp is equal to delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo); + await time.increaseTo(delayNotPassed); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), `${errorPrefix}: only can renounce in two delayed steps`, @@ -482,7 +483,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('reverts if block.timestamp is less than delayed until', async function () { - await time.increaseTo(incorrectIncreaseTo.subn(1)); + await time.increaseTo(delayNotPassed.subn(1)); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), `${errorPrefix}: only can renounce in two delayed steps`, From 880ac0b876437d1618b81ff8785f0233901517c2 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:14:57 -0600 Subject: [PATCH 64/69] Remove confusing comment --- test/access/AccessControl.behavior.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index fa0838eb57f..3f0c1d9d641 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -307,18 +307,12 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); describe('accepts transfer admin', function () { - let correctPendingDefaultAdmin; let delayPassed; beforeEach(async function () { - // Set as correct so they're explicitly needed in revert tests, since conditions - // are not mutually exclusive and accidentally an incorrect value will make it revert - // possiblty creating false positives (eg. expect revert for incorrect caller but getting it - // from a badly expected delayed until) - correctPendingDefaultAdmin = newDefaultAdmin; delayPassed = (await time.latest()).add(delay).addn(1); - await this.accessControl.beginDefaultAdminTransfer(correctPendingDefaultAdmin, { from: defaultAdmin }); + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); }); describe('caller is pending default admin and delay has passed', function () { @@ -326,7 +320,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa beforeEach(async function () { await time.increaseTo(delayPassed); - from = correctPendingDefaultAdmin; + from = newDefaultAdmin; }); it('accepts a transfer and changes default admin', async function () { @@ -373,7 +367,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa it('should revert if block.timestamp is equal to delayed until', async function () { await time.increaseTo(delayNotPassed); await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), `${errorPrefix}: transfer delay not met`, ); }); @@ -381,7 +375,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa it('should revert if block.timestamp is less than delayed until', async function () { await time.increaseTo(delayNotPassed.subn(1)); await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: correctPendingDefaultAdmin }), + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), `${errorPrefix}: transfer delay not met`, ); }); From 3337f613c0d4659f4b342d3c1d5a6b7451f0b2a2 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:19:40 -0600 Subject: [PATCH 65/69] Replace met with passed --- contracts/access/AccessControlDefaultAdminRules.sol | 4 ++-- contracts/access/IAccessControlDefaultAdminRules.sol | 2 +- test/access/AccessControl.behavior.js | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 6b06011271e..e3bce6b7505 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -195,7 +195,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * Internal function without access restriction. */ function _acceptDefaultAdminTransfer() internal virtual { - require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not met"); + require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not passed"); _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin()); _resetDefaultAdminTransfer(); @@ -231,7 +231,7 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and met. + * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and passed. */ function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { uint48 delayedUntil = defaultAdminTransferDelayedUntil(); diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol index da92f1057fd..753c7c80274 100644 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -40,7 +40,7 @@ interface IAccessControlDefaultAdminRules is IAccessControl { /** * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin - * and a timer to be met. + * and a timer to pass. * * Requirements: * diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 3f0c1d9d641..28792ac7b5a 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -357,7 +357,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('delayedUntil not met', function () { + describe('delayedUntil not passed', function () { let delayNotPassed; beforeEach(function () { @@ -368,7 +368,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(delayNotPassed); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not met`, + `${errorPrefix}: transfer delay not passed`, ); }); @@ -376,7 +376,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa await time.increaseTo(delayNotPassed.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not met`, + `${errorPrefix}: transfer delay not passed`, ); }); }); @@ -461,7 +461,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('delayed until not met', function () { + describe('delayed until not passed', function () { let delayNotPassed; beforeEach(function () { From a43abe7e27f82755ba2d3d47546a7bf6286cb4c7 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:36:45 -0600 Subject: [PATCH 66/69] Simplified acceptance tests --- test/access/AccessControl.behavior.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 28792ac7b5a..f17acf9deaf 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -324,14 +324,14 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('accepts a transfer and changes default admin', async function () { - await this.accessControl.acceptDefaultAdminTransfer({ from }); + 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); - }); - it('accepts a transfer and emit events', async function () { - const receipt = await this.accessControl.acceptDefaultAdminTransfer({ from }); + // Emit events expectEvent(receipt, 'RoleRevoked', { role: DEFAULT_ADMIN_ROLE, account: defaultAdmin, @@ -340,10 +340,8 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa role: DEFAULT_ADMIN_ROLE, account: newDefaultAdmin, }); - }); - it('accepts a transfer resetting pending default admin and delayed until', async function () { - await this.accessControl.acceptDefaultAdminTransfer({ from }); + // 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); }); From b4ee08a9ddbb796f877d62958ace463d2ac7ad18 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 21 Feb 2023 19:50:32 -0600 Subject: [PATCH 67/69] Add IAccessControlEnumerable interface removed accidentally from docs --- contracts/access/README.adoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 1abc37cccb8..80ca0020f4b 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -20,6 +20,8 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControlCrossChain}} +{{IAccessControlEnumerable}} + {{AccessControlEnumerable}} {{AccessControlDefaultAdminRules}} From 7d37c81cadc8b74c842300887c9de05f630f60d9 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 24 Feb 2023 18:19:56 -0300 Subject: [PATCH 68/69] fix flaky tests --- test/access/AccessControl.behavior.js | 41 +++++++++++++-------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index f17acf9deaf..a97b86228cb 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -1,6 +1,7 @@ -const { expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +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'); @@ -262,7 +263,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa beforeEach('begins admin transfer', async function () { receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - defaultAdminTransferDelayedUntil = (await time.latest()).add(delay); + defaultAdminTransferDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); }); it('should set pending default admin and delayed until', async function () { @@ -278,22 +279,22 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa it('should be able to begin a transfer again before delay pass', async function () { // Time passes just before delay - await time.increaseTo(defaultAdminTransferDelayedUntil.subn(1)); + 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 = (await time.latest()).add(delay); + 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.increaseTo(defaultAdminTransferDelayedUntil.addn(1)); + 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 = (await time.latest()).add(delay); + 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); }); @@ -310,16 +311,15 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa let delayPassed; beforeEach(async function () { - delayPassed = (await time.latest()).add(delay).addn(1); - 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.increaseTo(delayPassed); + await time.setNextBlockTimestamp(delayPassed); from = newDefaultAdmin; }); @@ -348,7 +348,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('should revert if caller is not pending default admin', async function () { - await time.increaseTo(delayPassed); + await time.setNextBlockTimestamp(delayPassed); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: other }), `${errorPrefix}: pending admin must accept`, @@ -363,7 +363,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('should revert if block.timestamp is equal to delayed until', async function () { - await time.increaseTo(delayNotPassed); + await time.setNextBlockTimestamp(delayNotPassed); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), `${errorPrefix}: transfer delay not passed`, @@ -371,7 +371,6 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('should revert if block.timestamp is less than delayed until', async function () { - await time.increaseTo(delayNotPassed.subn(1)); await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), `${errorPrefix}: transfer delay not passed`, @@ -384,8 +383,8 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa let delayPassed; beforeEach(async function () { - delayPassed = (await time.latest()).add(delay).addn(1); 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 () { @@ -394,7 +393,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); // Advance until passed delay - await time.increaseTo(delayPassed); + await time.setNextBlockTimestamp(delayPassed); // Previous pending default admin should not be able to accept after cancellation. await expectRevert( @@ -405,7 +404,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa it('cancels even after delay has passed', async function () { await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - await time.increaseTo(delayPassed); + 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); }); @@ -423,12 +422,12 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa let from = defaultAdmin; beforeEach(async function () { - delayPassed = (await time.latest()).add(delay).addn(1); 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.increaseTo(delayPassed); + 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; @@ -441,7 +440,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('allows to recover access using the internal _grantRole', async function () { - await time.increaseTo(delayPassed); + await time.setNextBlockTimestamp(delayPassed); await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); const grantRoleReceipt = await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, other); @@ -452,7 +451,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('reverts if caller is not default admin', async function () { - await time.increaseTo(delayPassed); + await time.setNextBlockTimestamp(delayPassed); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), `${errorPrefix}: can only renounce roles for self`, @@ -467,7 +466,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('reverts if block.timestamp is equal to delayed until', async function () { - await time.increaseTo(delayNotPassed); + await time.setNextBlockTimestamp(delayNotPassed); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), `${errorPrefix}: only can renounce in two delayed steps`, @@ -475,7 +474,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); it('reverts if block.timestamp is less than delayed until', async function () { - await time.increaseTo(delayNotPassed.subn(1)); + await time.setNextBlockTimestamp(delayNotPassed.subn(1)); await expectRevert( this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), `${errorPrefix}: only can renounce in two delayed steps`, From b164929b1521ea9c97573e688cab3311276899ad Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 24 Feb 2023 18:24:34 -0300 Subject: [PATCH 69/69] lint --- test/access/AccessControl.behavior.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index a97b86228cb..e04c5a16529 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -312,7 +312,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa beforeEach(async function () { await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - delayPassed = web3.utils.toBN(await time.latest()).add(delay).addn(1); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); }); describe('caller is pending default admin and delay has passed', function () { @@ -384,7 +387,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa beforeEach(async function () { await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - delayPassed = web3.utils.toBN(await time.latest()).add(delay).addn(1); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); }); it('resets pending default admin and delayed until', async function () { @@ -423,7 +429,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa beforeEach(async function () { await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); - delayPassed = web3.utils.toBN(await time.latest()).add(delay).addn(1); + delayPassed = web3.utils + .toBN(await time.latest()) + .add(delay) + .addn(1); }); it('it renounces role', async function () {