diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index e3bce6b7505..43fca935055 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -19,9 +19,9 @@ import "../interfaces/IERC5313.sol"; * This contract implements the following risk mitigations on top of {AccessControl}: * * * Only one account holds the `DEFAULT_ADMIN_ROLE` since deployment until it's potentially renounced. - * * Enforce a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. - * * Enforce a configurable delay between the two steps, with the ability to cancel in between. - * - Even after the timer has passed to avoid locking it forever. + * * Enforces a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account. + * * Enforces a configurable delay between the two steps, with the ability to cancel before the transfer is accepted. + * * The delay can be changed by scheduling, see {changeDefaultAdminDelay}. * * It is not possible to use another role to manage the `DEFAULT_ADMIN_ROLE`. * * Example usage: @@ -32,31 +32,38 @@ import "../interfaces/IERC5313.sol"; * 3 days, * msg.sender // Explicit initial `DEFAULT_ADMIN_ROLE` holder * ) {} - *} + * } * ``` * - * NOTE: The `delay` can only be set in the constructor and is fixed thereafter. - * * _Available since v4.9._ */ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRules, IERC5313, AccessControl { - uint48 private immutable _defaultAdminDelay; + // pending admin pair read/written together frequently + address private _pendingDefaultAdmin; + uint48 private _pendingDefaultAdminSchedule; // 0 == unset + uint48 private _currentDelay; address private _currentDefaultAdmin; - address private _pendingDefaultAdmin; - uint48 private _defaultAdminTransferDelayedUntil; + // pending delay pair read/written together frequently + uint48 private _pendingDelay; + uint48 private _pendingDelaySchedule; // 0 == unset /** - * @dev Sets the initial values for {defaultAdminDelay} in seconds and {defaultAdmin}. - * - * The `defaultAdminDelay` value is immutable. It can only be set at the constructor. + * @dev Sets the initial values for {defaultAdminDelay} and {defaultAdmin} address. */ - constructor(uint48 defaultAdminDelay_, address initialDefaultAdmin) { - _defaultAdminDelay = defaultAdminDelay_; + constructor(uint48 initialDelay, address initialDefaultAdmin) { + _currentDelay = initialDelay; _grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin); } + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId); + } + /** * @dev See {IERC5313-owner}. */ @@ -64,13 +71,89 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu return defaultAdmin(); } + /// + /// Override AccessControl role management + /// + /** - * @inheritdoc IAccessControlDefaultAdminRules + * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function defaultAdminDelay() public view virtual returns (uint48) { - return _defaultAdminDelay; + function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role"); + super.grantRole(role, account); + } + + /** + * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role"); + super.revokeRole(role, account); + } + + /** + * @dev See {AccessControl-renounceRole}. + * + * For the `DEFAULT_ADMIN_ROLE`, it only allows renouncing in two steps by first calling + * {beginDefaultAdminTransfer} to the `address(0)`, so it's required that the {pendingDefaultAdmin} schedule + * has also passed when calling this function. + * + * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` functions. + * + * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without a {defaultAdmin}, + * thereby disabling any functionality that is only available for it, and the possibility of reassigning a + * non-administrated role. + */ + function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { + if (role == DEFAULT_ADMIN_ROLE) { + (address newDefaultAdmin, uint48 schedule) = pendingDefaultAdmin(); + require( + newDefaultAdmin == address(0) && _isScheduleSet(schedule) && _hasSchedulePassed(schedule), + "AccessControl: only can renounce in two delayed steps" + ); + } + super.renounceRole(role, account); + } + + /** + * @dev See {AccessControl-_grantRole}. + * + * For `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a {defaultAdmin} or if the + * role has been previously renounced. + * + * NOTE: Exposing this function through another mechanism may make the `DEFAULT_ADMIN_ROLE` + * assignable again. Make sure to guarantee this is the expected behavior in your implementation. + */ + function _grantRole(bytes32 role, address account) internal virtual override { + if (role == DEFAULT_ADMIN_ROLE) { + require(defaultAdmin() == address(0), "AccessControl: default admin already granted"); + _currentDefaultAdmin = account; + } + super._grantRole(role, account); + } + + /** + * @dev See {AccessControl-_revokeRole}. + */ + function _revokeRole(bytes32 role, address account) internal virtual override { + if (role == DEFAULT_ADMIN_ROLE) { + delete _currentDefaultAdmin; + } + super._revokeRole(role, account); } + /** + * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. + */ + function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override { + require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules"); + super._setRoleAdmin(role, adminRole); + } + + /// + /// AccessControlDefaultAdminRules accessors + /// + /** * @inheritdoc IAccessControlDefaultAdminRules */ @@ -81,24 +164,37 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu /** * @inheritdoc IAccessControlDefaultAdminRules */ - function pendingDefaultAdmin() public view virtual returns (address) { - return _pendingDefaultAdmin; + function pendingDefaultAdmin() public view virtual returns (address newAdmin, uint48 schedule) { + return (_pendingDefaultAdmin, _pendingDefaultAdminSchedule); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function defaultAdminDelay() public view virtual returns (uint48) { + uint48 schedule = _pendingDelaySchedule; + return (_isScheduleSet(schedule) && _hasSchedulePassed(schedule)) ? _pendingDelay : _currentDelay; } /** * @inheritdoc IAccessControlDefaultAdminRules */ - function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { - return _defaultAdminTransferDelayedUntil; + function pendingDefaultAdminDelay() public view virtual returns (uint48 newDelay, uint48 schedule) { + schedule = _pendingDelaySchedule; + return (_isScheduleSet(schedule) && !_hasSchedulePassed(schedule)) ? (_pendingDelay, schedule) : (0, 0); } /** - * @dev See {IERC165-supportsInterface}. + * @inheritdoc IAccessControlDefaultAdminRules */ - function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { - return interfaceId == type(IAccessControlDefaultAdminRules).interfaceId || super.supportsInterface(interfaceId); + function defaultAdminDelayIncreaseWait() public view virtual returns (uint48) { + return 5 days; } + /// + /// AccessControlDefaultAdminRules public and internal setters for defaultAdmin/pendingDefaultAdmin + /// + /** * @inheritdoc IAccessControlDefaultAdminRules */ @@ -107,134 +203,179 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @inheritdoc IAccessControlDefaultAdminRules + * @dev See {beginDefaultAdminTransfer}. + * + * Internal function without access restriction. */ - function acceptDefaultAdminTransfer() public virtual { - require(_msgSender() == pendingDefaultAdmin(), "AccessControl: pending admin must accept"); - _acceptDefaultAdminTransfer(); + function _beginDefaultAdminTransfer(address newAdmin) internal virtual { + uint48 newSchedule = SafeCast.toUint48(block.timestamp) + defaultAdminDelay(); + _setPendingDefaultAdmin(newAdmin, newSchedule); + emit DefaultAdminTransferScheduled(newAdmin, newSchedule); } /** * @inheritdoc IAccessControlDefaultAdminRules */ function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _resetDefaultAdminTransfer(); + _cancelDefaultAdminTransfer(); } /** - * @dev Revokes `role` from the calling account. - * - * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required - * that the {defaultAdminTransferDelayedUntil} has passed and the pending default admin is the zero address. - * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` - * functions. - * - * For other roles, see {AccessControl-renounceRole}. + * @dev See {cancelDefaultAdminTransfer}. * - * 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. + * Internal function without access restriction. */ - function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { - if (role == DEFAULT_ADMIN_ROLE) { - require( - pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), - "AccessControl: only can renounce in two delayed steps" - ); - } - super.renounceRole(role, account); + function _cancelDefaultAdminTransfer() internal virtual { + _setPendingDefaultAdmin(address(0), 0); } /** - * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + * @inheritdoc IAccessControlDefaultAdminRules */ - function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { - require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role"); - super.grantRole(role, account); + function acceptDefaultAdminTransfer() public virtual { + (address newDefaultAdmin, ) = pendingDefaultAdmin(); + require(_msgSender() == newDefaultAdmin, "AccessControl: pending admin must accept"); + _acceptDefaultAdminTransfer(); } /** - * @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`. + * @dev See {acceptDefaultAdminTransfer}. + * + * Internal function without access restriction. */ - function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { - require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role"); - super.revokeRole(role, account); + function _acceptDefaultAdminTransfer() internal virtual { + (address newAdmin, uint48 schedule) = pendingDefaultAdmin(); + require(_isScheduleSet(schedule) && _hasSchedulePassed(schedule), "AccessControl: transfer delay not passed"); + _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); + _grantRole(DEFAULT_ADMIN_ROLE, newAdmin); + delete _pendingDefaultAdmin; + delete _pendingDefaultAdminSchedule; } + /// + /// AccessControlDefaultAdminRules public and internal setters for defaultAdminDelay/pendingDefaultAdminDelay + /// + /** - * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. + * @inheritdoc IAccessControlDefaultAdminRules */ - function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override { - require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules"); - super._setRoleAdmin(role, adminRole); + function changeDefaultAdminDelay(uint48 newDelay) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _changeDefaultAdminDelay(newDelay); } /** - * @dev Grants `role` to `account`. - * - * For `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a role's holder - * or if the role has been previously renounced. + * @dev See {changeDefaultAdminDelay}. * - * 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. + * Internal function without access restriction. */ - function _grantRole(bytes32 role, address account) internal virtual override { - if (role == DEFAULT_ADMIN_ROLE) { - require(defaultAdmin() == address(0), "AccessControl: default admin already granted"); - _currentDefaultAdmin = account; - } - super._grantRole(role, account); + function _changeDefaultAdminDelay(uint48 newDelay) internal virtual { + uint48 newSchedule = SafeCast.toUint48(block.timestamp) + _delayChangeWait(newDelay); + _setPendingDelay(newDelay, newSchedule); + emit DefaultAdminDelayChangeScheduled(newDelay, newSchedule); } /** - * @dev See {acceptDefaultAdminTransfer}. + * @inheritdoc IAccessControlDefaultAdminRules + */ + function rollbackDefaultAdminDelay() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _rollbackDefaultAdminDelay(); + } + + /** + * @dev See {rollbackDefaultAdminDelay}. * * Internal function without access restriction. */ - function _acceptDefaultAdminTransfer() internal virtual { - require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not passed"); - _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); - _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin()); - _resetDefaultAdminTransfer(); + function _rollbackDefaultAdminDelay() internal virtual { + _setPendingDelay(0, 0); } /** - * @dev See {beginDefaultAdminTransfer}. + * @dev Returns the amount of seconds to wait after the `newDelay` will + * become the new {defaultAdminDelay}. * - * Internal function without access restriction. + * The value returned guarantees that if the delay is reduced, it will go into effect + * after a wait that honors the previously set delay. + * + * See {defaultAdminDelayIncreaseWait}. */ - function _beginDefaultAdminTransfer(address newAdmin) internal virtual { - _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + defaultAdminDelay(); + function _delayChangeWait(uint48 newDelay) internal view virtual returns (uint48) { + uint48 currentDelay = defaultAdminDelay(); + + // When increasing the delay, we schedule the delay change to occur after a period of "new delay" has passed, up + // to a maximum given by defaultAdminDelayIncreaseWait, by default 5 days. For example, if increasing from 1 day + // to 3 days, the new delay will come into effect after 3 days. If increasing from 1 day to 10 days, the new + // delay will come into effect after 5 days. The 5 day wait period is intended to be able to fix an error like + // using milliseconds instead of seconds. + // + // When decreasing the delay, we wait the difference between "current delay" and "new delay". This guarantees + // that an admin transfer cannot be made faster than "current delay" at the time the delay change is scheduled. + // For example, if decreasing from 10 days to 3 days, the new delay will come into effect after 7 days. + return + newDelay > currentDelay + ? uint48(Math.min(newDelay, defaultAdminDelayIncreaseWait())) // no need to safecast, both inputs are uint48 + : currentDelay - newDelay; + } + + /// + /// Private setters + /// + + /** + * @dev Setter of the tuple for pending admin and its schedule. + * + * May emit a DefaultAdminTransferCanceled event. + */ + function _setPendingDefaultAdmin(address newAdmin, uint48 newSchedule) private { + (, uint48 oldSchedule) = pendingDefaultAdmin(); + _pendingDefaultAdmin = newAdmin; - emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); + _pendingDefaultAdminSchedule = newSchedule; + + // An `oldSchedule` from `pendingDefaultAdmin()` is only set if it hasn't been accepted. + if (_isScheduleSet(oldSchedule)) { + // Emit for implicit cancellations when another default admin was scheduled. + emit DefaultAdminTransferCanceled(); + } } /** - * @dev See {AccessControl-_revokeRole}. + * @dev Setter of the tuple for pending delay and its schedule. + * + * May emit a DefaultAdminDelayChangeCanceled event. */ - function _revokeRole(bytes32 role, address account) internal virtual override { - if (role == DEFAULT_ADMIN_ROLE) { - delete _currentDefaultAdmin; + function _setPendingDelay(uint48 newDelay, uint48 newSchedule) private { + uint48 oldSchedule = _pendingDelaySchedule; + + if (_isScheduleSet(oldSchedule)) { + if (_hasSchedulePassed(oldSchedule)) { + // Materialize a virtual delay + _currentDelay = _pendingDelay; + } else { + // Emit for implicit cancellations when another delay was scheduled. + emit DefaultAdminDelayChangeCanceled(); + } } - super._revokeRole(role, account); + + _pendingDelay = newDelay; + _pendingDelaySchedule = newSchedule; } + /// + /// Private helpers + /// + /** - * @dev Resets the pending default admin and delayed until. + * @dev Defines if an `schedule` is considered set. For consistency purposes. */ - function _resetDefaultAdminTransfer() private { - delete _pendingDefaultAdmin; - delete _defaultAdminTransferDelayedUntil; + function _isScheduleSet(uint48 schedule) private pure returns (bool) { + return schedule != 0; } /** - * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and passed. + * @dev Defines if an `schedule` is considered passed. For consistency purposes. */ - function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { - uint48 delayedUntil = defaultAdminTransferDelayedUntil(); - return delayedUntil > 0 && delayedUntil < block.timestamp; + function _hasSchedulePassed(uint48 schedule) private view returns (bool) { + return schedule < block.timestamp; } } diff --git a/contracts/access/IAccessControlDefaultAdminRules.sol b/contracts/access/IAccessControlDefaultAdminRules.sol index 753c7c80274..d28c49d9542 100644 --- a/contracts/access/IAccessControlDefaultAdminRules.sol +++ b/contracts/access/IAccessControlDefaultAdminRules.sol @@ -12,16 +12,27 @@ import "./IAccessControl.sol"; */ interface IAccessControlDefaultAdminRules is IAccessControl { /** - * @dev Emitted when a `DEFAULT_ADMIN_ROLE` transfer is started, setting `newDefaultAdmin` - * as the next default admin, which will have rights to claim the `DEFAULT_ADMIN_ROLE` - * after `defaultAdminTransferDelayedUntil` has passed. + * @dev Emitted when a {defaultAdmin} transfer is started, setting `newAdmin` as the next + * address to become the {defaultAdmin} by calling {acceptDefaultAdminTransfer} only after `acceptSchedule` + * passes. */ - event DefaultAdminRoleChangeStarted(address indexed newDefaultAdmin, uint48 defaultAdminTransferDelayedUntil); + event DefaultAdminTransferScheduled(address indexed newAdmin, uint48 acceptSchedule); /** - * @dev Returns the delay between each `DEFAULT_ADMIN_ROLE` transfer. + * @dev Emitted when a {pendingDefaultAdmin} is reset if it was never accepted, regardless of its schedule. */ - function defaultAdminDelay() external view returns (uint48); + event DefaultAdminTransferCanceled(); + + /** + * @dev Emitted when a {defaultAdminDelay} change is started, setting `newDelay` as the next + * delay to be applied between default admin transfer after `effectSchedule` has passed. + */ + event DefaultAdminDelayChangeScheduled(uint48 newDelay, uint48 effectSchedule); + + /** + * @dev Emitted when a {pendingDefaultAdminDelay} is reset if its schedule didn't pass. + */ + event DefaultAdminDelayChangeCanceled(); /** * @dev Returns the address of the current `DEFAULT_ADMIN_ROLE` holder. @@ -29,45 +40,133 @@ interface IAccessControlDefaultAdminRules is IAccessControl { function defaultAdmin() external view returns (address); /** - * @dev Returns the address of the pending `DEFAULT_ADMIN_ROLE` holder. + * @dev Returns a tuple of a `newAdmin` and an accept schedule. + * + * After the `schedule` passes, the `newAdmin` will be able to accept the {defaultAdmin} role + * by calling {acceptDefaultAdminTransfer}, completing the role transfer. + * + * A zero value only in `acceptSchedule` indicates no pending admin transfer. + * + * NOTE: A zero address `newAdmin` means that {defaultAdmin} is being renounced. */ - function pendingDefaultAdmin() external view returns (address); + function pendingDefaultAdmin() external view returns (address newAdmin, uint48 acceptSchedule); /** - * @dev Returns the timestamp after which the pending default admin can claim the `DEFAULT_ADMIN_ROLE`. + * @dev Returns the delay required to schedule the acceptance of a {defaultAdmin} transfer started. + * + * This delay will be added to the current timestamp when calling {beginDefaultAdminTransfer} to set + * the acceptance schedule. + * + * NOTE: If a delay change has been scheduled, it will take effect as soon as the schedule passes, making this + * function returns the new delay. See {changeDefaultAdminDelay}. */ - function defaultAdminTransferDelayedUntil() external view returns (uint48); + function defaultAdminDelay() external view returns (uint48); /** - * @dev Starts a `DEFAULT_ADMIN_ROLE` transfer by setting a pending default admin - * and a timer to pass. + * @dev Returns a tuple of `newDelay` and an effect schedule. + * + * After the `schedule` passes, the `newDelay` will get into effect immediately for every + * new {defaultAdmin} transfer started with {beginDefaultAdminTransfer}. + * + * A zero value only in `effectSchedule` indicates no pending delay change. + * + * NOTE: A zero value only for `newDelay` means that the next {defaultAdminDelay} + * will be zero after the effect schedule. + */ + function pendingDefaultAdminDelay() external view returns (uint48 newDelay, uint48 effectSchedule); + + /** + * @dev Starts a {defaultAdmin} transfer by setting a {pendingDefaultAdmin} scheduled for acceptance + * after the current timestamp plus a {defaultAdminDelay}. * * Requirements: * - * - Only can be called by the current `DEFAULT_ADMIN_ROLE` holder. + * - Only can be called by the current {defaultAdmin}. * - * Emits a {DefaultAdminRoleChangeStarted}. + * Emits a DefaultAdminRoleChangeStarted event. */ function beginDefaultAdminTransfer(address newAdmin) external; /** - * @dev Completes a `DEFAULT_ADMIN_ROLE` transfer. + * @dev Cancels a {defaultAdmin} transfer previously started with {beginDefaultAdminTransfer}. + * + * A {pendingDefaultAdmin} not yet accepted can also be cancelled with this function. * * Requirements: * - * - Caller should be the pending default admin. + * - Only can be called by the current {defaultAdmin}. + * + * May emit a DefaultAdminTransferCanceled event. + */ + function cancelDefaultAdminTransfer() external; + + /** + * @dev Completes a {defaultAdmin} transfer previously started with {beginDefaultAdminTransfer}. + * + * After calling the function: + * * - `DEFAULT_ADMIN_ROLE` should be granted to the caller. * - `DEFAULT_ADMIN_ROLE` should be revoked from the previous holder. + * - {pendingDefaultAdmin} should be reset to zero values. + * + * Requirements: + * + * - Only can be called by the {pendingDefaultAdmin}'s `newAdmin`. + * - The {pendingDefaultAdmin}'s `acceptSchedule` should've passed. */ function acceptDefaultAdminTransfer() external; /** - * @dev Cancels a `DEFAULT_ADMIN_ROLE` transfer. + * @dev Initiates a {defaultAdminDelay} update by setting a {pendingDefaultAdminDelay} scheduled for getting + * into effect after the current timestamp plus a {defaultAdminDelay}. + * + * This function guarantees that any call to {beginDefaultAdminTransfer} done between the timestamp this + * method is called and the {pendingDefaultAdminDelay} effect schedule will use the current {defaultAdminDelay} + * set before calling. + * + * The {pendingDefaultAdminDelay}'s effect schedule is defined in a way that waiting until the schedule and then + * calling {beginDefaultAdminTransfer} with the new delay will take at least the same as another {defaultAdmin} + * complete transfer (including acceptance). + * + * The schedule is designed for two scenarios: + * + * - When the delay is changed for a larger one the schedule is `block.timestamp + newDelay` capped by + * {defaultAdminDelayIncreaseWait}. + * - When the delay is changed for a shorter one, the schedule is `block.timestamp + (current delay - new delay)`. + * + * A {pendingDefaultAdminDelay} that never got into effect will be canceled in favor of a new scheduled change. * * Requirements: * - * - Can be called even after the timer has passed. - * - Can only be called by the current `DEFAULT_ADMIN_ROLE` holder. + * - Only can be called by the current {defaultAdmin}. + * + * Emits a DefaultAdminDelayChangeScheduled event and may emit a DefaultAdminDelayChangeCanceled event. */ - function cancelDefaultAdminTransfer() external; + function changeDefaultAdminDelay(uint48 newDelay) external; + + /** + * @dev Cancels a scheduled {defaultAdminDelay} change. + * + * Requirements: + * + * - Only can be called by the current {defaultAdmin}. + * + * May emit a DefaultAdminDelayChangeCanceled event. + */ + function rollbackDefaultAdminDelay() external; + + /** + * @dev Maximum time in seconds for an increase to {defaultAdminDelay} (that is scheduled using {changeDefaultAdminDelay}) + * to take effect. Default to 5 days. + * + * When the {defaultAdminDelay} is scheduled to be increased, it goes into effect after the new delay has passed with + * the purpose of giving enough time for reverting any accidental change (i.e. using milliseconds instead of seconds) + * that may lock the contract. However, to avoid excessive schedules, the wait is capped by this function and it can + * be overrode for a custom {defaultAdminDelay} increase scheduling. + * + * IMPORTANT: Make sure to add a reasonable amount of time while overriding this value, otherwise, + * there's a risk of setting a high new delay that goes into effect almost immediately without the + * possibility of human intervention in the case of an input error (eg. set milliseconds instead of seconds). + */ + function defaultAdminDelayIncreaseWait() external view returns (uint48); } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index e04c5a16529..6c88aa274fd 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -1,13 +1,16 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); -const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); +const { expectEvent, expectRevert, constants, BN } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); + const { time } = require('@nomicfoundation/hardhat-network-helpers'); const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior'); +const { network } = require('hardhat'); +const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants'); const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; const ROLE = web3.utils.soliditySha3('ROLE'); const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE'); +const ZERO = web3.utils.toBN(0); function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, otherAdmin) { shouldSupportInterfaces(['AccessControl']); @@ -215,18 +218,151 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defaultAdmin, newDefaultAdmin, other) { shouldSupportInterfaces(['AccessControlDefaultAdminRules']); - it('has a default disabled delayed until', async function () { - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); + function expectNoEvent(receipt, eventName) { + try { + expectEvent(receipt, eventName); + throw new Error(`${eventName} event found`); + } catch (err) { + expect(err.message).to.eq(`No '${eventName}' events found: expected false to equal true`); + } + } + + for (const getter of ['owner', 'defaultAdmin']) { + describe(`${getter}()`, function () { + it('has a default set to the initial default admin', async function () { + const value = await this.accessControl[getter](); + expect(value).to.equal(defaultAdmin); + expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, value)).to.be.true; + }); + + it('changes if the default admin changes', async function () { + // Starts an admin transfer + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + + // Wait for acceptance + const acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); + await time.setNextBlockTimestamp(acceptSchedule.addn(1)); + await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }); + + const value = await this.accessControl[getter](); + expect(value).to.equal(newDefaultAdmin); + }); + }); + } + + describe('pendingDefaultAdmin()', function () { + it('returns 0 if no pending default admin transfer', async function () { + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.eq(ZERO_ADDRESS); + expect(schedule).to.be.bignumber.eq(ZERO); + }); + + describe('when there is a scheduled default admin transfer', function () { + beforeEach('begins admin transfer', async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + }); + + for (const [fromSchedule, tag] of [ + [-1, 'before'], + [0, 'exactly when'], + [1, 'after'], + ]) { + it(`returns pending admin and delay ${tag} delay schedule passes if not accepted`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdmin(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + await network.provider.send('evm_mine'); // Mine a block to force the timestamp + + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.eq(newDefaultAdmin); + expect(schedule).to.be.bignumber.eq(firstSchedule); + }); + } + + it('returns 0 after delay schedule passes and the transfer was accepted', async function () { + // Wait after schedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdmin(); + await time.setNextBlockTimestamp(firstSchedule.addn(1)); + + // Accepts + await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }); + + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.eq(ZERO_ADDRESS); + expect(schedule).to.be.bignumber.eq(ZERO); + }); + }); + }); + + describe('defaultAdminDelay()', function () { + it('returns the current delay', async function () { + expect(await this.accessControl.defaultAdminDelay()).to.be.bignumber.eq(delay); + }); + + describe('when there is a scheduled delay change', function () { + const newDelay = web3.utils.toBN(0xdead); // Any change + + beforeEach('begins delay change', async function () { + await this.accessControl.changeDefaultAdminDelay(newDelay, { from: defaultAdmin }); + }); + + for (const [fromSchedule, tag, expectedDelay, delayTag] of [ + [-1, 'before', delay, 'old'], + [0, 'exactly when', delay, 'old'], + [1, 'after', newDelay, 'new'], + ]) { + it(`returns ${delayTag} delay ${tag} delay schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(schedule.toNumber() + fromSchedule); + await network.provider.send('evm_mine'); // Mine a block to force the timestamp + + const currentDelay = await this.accessControl.defaultAdminDelay(); + expect(currentDelay).to.be.bignumber.eq(expectedDelay); + }); + } + }); }); - it('has a default pending default admin', async function () { - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + describe('pendingDefaultAdminDelay()', function () { + it('returns 0 if not set', async function () { + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(ZERO); + expect(schedule).to.be.bignumber.eq(ZERO); + }); + + describe('when there is a scheduled delay change', function () { + const newDelay = web3.utils.toBN(0xdead); // Any change + + beforeEach('begins admin transfer', async function () { + await this.accessControl.changeDefaultAdminDelay(newDelay, { from: defaultAdmin }); + }); + + for (const [fromSchedule, tag, expectedDelay, delayTag, expectZeroSchedule] of [ + [-1, 'before', newDelay, 'new'], + [0, 'exactly when', newDelay, 'new'], + [1, 'after', ZERO, 'zero', true], + ]) { + it(`returns ${delayTag} delay ${tag} delay schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + await network.provider.send('evm_mine'); // Mine a block to force the timestamp + + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(expectedDelay); + expect(schedule).to.be.bignumber.eq(expectZeroSchedule ? ZERO : firstSchedule); + }); + } + }); }); - it('has a default current owner set to the initial default admin', async function () { - const owner = await this.accessControl.owner(); - expect(owner).to.equal(defaultAdmin); - expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, owner)).to.be.true; + describe('defaultAdminDelayIncreaseWait()', function () { + it('should return 5 days (default)', async function () { + expect(await this.accessControl.defaultAdminDelayIncreaseWait()).to.be.bignumber.eq( + web3.utils.toBN(time.duration.days(5)), + ); + }); }); it('should revert if granting default admin role', async function () { @@ -257,72 +393,130 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa ); }); - describe('begins transfer of default admin', function () { + describe('begins a default admin transfer', function () { let receipt; - let defaultAdminTransferDelayedUntil; + let acceptSchedule; - beforeEach('begins admin transfer', async function () { - receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - defaultAdminTransferDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); + it('reverts if called by non default admin accounts', async function () { + await expectRevert( + this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); }); - it('should set pending default admin and delayed until', async function () { - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - defaultAdminTransferDelayedUntil, - ); - expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { - newDefaultAdmin, - defaultAdminTransferDelayedUntil, + describe('when there is no pending delay nor pending admin transfer', function () { + beforeEach('begins admin transfer', async function () { + receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); + }); + + it('should set pending default admin and schedule', async function () { + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(newDefaultAdmin); + expect(schedule).to.be.bignumber.equal(acceptSchedule); + expectEvent(receipt, 'DefaultAdminTransferScheduled', { + newAdmin, + acceptSchedule, + }); }); }); - it('should be able to begin a transfer again before delay pass', async function () { - // Time passes just before delay - await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.subn(1)); + describe('when there is a pending admin transfer', function () { + beforeEach('sets a pending default admin transfer', async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); + }); - // defaultAdmin changes its mind and begin again to another address - await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); - }); + for (const [fromSchedule, tag] of [ + [-1, 'before'], + [0, 'exactly when'], + [1, 'after'], + ]) { + it(`should be able to begin a transfer again ${tag} acceptSchedule passes`, async function () { + // Wait until schedule + fromSchedule + await time.setNextBlockTimestamp(acceptSchedule.toNumber() + fromSchedule); + + // defaultAdmin changes its mind and begin again to another address + const receipt = await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); + const newSchedule = web3.utils.toBN(await time.latest()).add(delay); + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(other); + expect(schedule).to.be.bignumber.equal(newSchedule); + + // Cancellation is always emitted since it was never accepted + expectEvent(receipt, 'DefaultAdminTransferCanceled'); + }); + } + + it('should not emit a cancellation event if the new default admin accepted', async function () { + // Wait until the acceptSchedule has passed + await time.setNextBlockTimestamp(acceptSchedule.addn(1)); - it('should be able to begin a transfer again after delay pass if not accepted', async function () { - // Time passes after delay without acceptance - await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.addn(1)); + // Accept and restart + await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }); + const receipt = await this.accessControl.beginDefaultAdminTransfer(other, { from: newDefaultAdmin }); - // defaultAdmin changes its mind and begin again to another address - await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); + expectNoEvent(receipt, 'DefaultAdminTransferCanceled'); + }); }); - it('should revert if it is called by non-admin accounts', async function () { - await expectRevert( - this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }), - `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, - ); + describe('when there is a pending delay', function () { + const newDelay = web3.utils.toBN(time.duration.hours(3)); + + beforeEach('schedule a delay change', async function () { + await this.accessControl.changeDefaultAdminDelay(newDelay, { from: defaultAdmin }); + const pendingDefaultAdminDelay = await this.accessControl.pendingDefaultAdminDelay(); + acceptSchedule = pendingDefaultAdminDelay.schedule; + }); + + for (const [fromSchedule, schedulePassed, expectedDelay, delayTag] of [ + [-1, 'before', delay, 'old'], + [0, 'exactly when', delay, 'old'], + [1, 'after', newDelay, 'new'], + ]) { + it(`should set the ${delayTag} delay and apply it to next default admin transfer schedule ${schedulePassed} acceptSchedule passed`, async function () { + // Wait until the expected fromSchedule time + await time.setNextBlockTimestamp(acceptSchedule.toNumber() + fromSchedule); + + // Start the new default admin transfer and get its schedule + const receipt = await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + const expectedAcceptSchedule = web3.utils.toBN(await time.latest()).add(expectedDelay); + + // Check that the schedule corresponds with the new delay + const { newAdmin, schedule: transferSchedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(newDefaultAdmin); + expect(transferSchedule).to.be.bignumber.equal(expectedAcceptSchedule); + + expectEvent(receipt, 'DefaultAdminTransferScheduled', { + newAdmin, + acceptSchedule: expectedAcceptSchedule, + }); + }); + } }); }); describe('accepts transfer admin', function () { - let delayPassed; + let acceptSchedule; beforeEach(async function () { await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - delayPassed = web3.utils - .toBN(await time.latest()) - .add(delay) - .addn(1); + acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); }); - describe('caller is pending default admin and delay has passed', function () { + it('should revert if caller is not pending default admin', async function () { + await time.setNextBlockTimestamp(acceptSchedule.addn(1)); + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: other }), + `${errorPrefix}: pending admin must accept`, + ); + }); + + describe('when caller is pending default admin and delay has passed', function () { let from; beforeEach(async function () { - await time.setNextBlockTimestamp(delayPassed); + await time.setNextBlockTimestamp(acceptSchedule.addn(1)); from = newDefaultAdmin; }); @@ -344,108 +538,122 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa account: newDefaultAdmin, }); - // Resets pending default admin and delayed until - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + // Resets pending default admin and schedule + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(constants.ZERO_ADDRESS); + expect(schedule).to.be.bignumber.equal(ZERO); }); }); - it('should revert if caller is not pending default admin', async function () { - await time.setNextBlockTimestamp(delayPassed); + describe('schedule not passed', function () { + for (const [fromSchedule, tag] of [ + [-1, 'less'], + [0, 'equal'], + ]) { + it(`should revert if block.timestamp is ${tag} to schedule`, async function () { + await time.setNextBlockTimestamp(acceptSchedule.toNumber() + fromSchedule); + await expectRevert( + this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), + `${errorPrefix}: transfer delay not passed`, + ); + }); + } + }); + }); + + describe('cancels a default admin transfer', function () { + it('reverts if called by non default admin accounts', async function () { await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: other }), - `${errorPrefix}: pending admin must accept`, + this.accessControl.cancelDefaultAdminTransfer({ from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); - describe('delayedUntil not passed', function () { - let delayNotPassed; + describe('when there is a pending default admin transfer', function () { + let acceptSchedule; - beforeEach(function () { - delayNotPassed = delayPassed.subn(1); + beforeEach(async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); }); - it('should revert if block.timestamp is equal to delayed until', async function () { - await time.setNextBlockTimestamp(delayNotPassed); + for (const [fromSchedule, tag] of [ + [-1, 'before'], + [0, 'exactly when'], + [1, 'after'], + ]) { + it(`resets pending default admin and schedule ${tag} transfer schedule passes`, async function () { + // Advance until passed delay + await time.setNextBlockTimestamp(acceptSchedule.toNumber() + fromSchedule); + + const receipt = await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(constants.ZERO_ADDRESS); + expect(schedule).to.be.bignumber.equal(ZERO); + + expectEvent(receipt, 'DefaultAdminTransferCanceled'); + }); + } + + it('should revert if the previous default admin tries to accept', async function () { + await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + + // Advance until passed delay + await time.setNextBlockTimestamp(acceptSchedule.addn(1)); + + // Previous pending default admin should not be able to accept after cancellation. await expectRevert( this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not passed`, + `${errorPrefix}: pending admin must accept`, ); }); + }); - it('should revert if block.timestamp is less than delayed until', async function () { - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not passed`, - ); + describe('when there is no pending default admin transfer', async function () { + it('should succeed without changes', async function () { + const receipt = await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + + const { newAdmin, schedule } = await this.accessControl.pendingDefaultAdmin(); + expect(newAdmin).to.equal(constants.ZERO_ADDRESS); + expect(schedule).to.be.bignumber.equal(ZERO); + + expectNoEvent(receipt, 'DefaultAdminTransferCanceled'); }); }); }); - describe('cancel transfer default admin', function () { + describe('renounces admin', function () { let delayPassed; + let from = defaultAdmin; beforeEach(async function () { - await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + await this.accessControl.beginDefaultAdminTransfer(constants.ZERO_ADDRESS, { from }); delayPassed = web3.utils .toBN(await time.latest()) .add(delay) .addn(1); }); - it('resets pending default admin and delayed until', async function () { - await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); - - // Advance until passed delay - await time.setNextBlockTimestamp(delayPassed); - - // Previous pending default admin should not be able to accept after cancellation. - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: pending admin must accept`, - ); - }); - - it('cancels even after delay has passed', async function () { - await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); + it('reverts if caller is not default admin', async function () { await time.setNextBlockTimestamp(delayPassed); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); - }); - - it('reverts if called by non default admin accounts', async function () { await expectRevert( - this.accessControl.cancelDefaultAdminTransfer({ from: other }), - `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), + `${errorPrefix}: can only renounce roles for self`, ); }); - }); - - describe('renouncing admin', function () { - let delayPassed; - let from = defaultAdmin; - - beforeEach(async function () { - await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); - delayPassed = web3.utils - .toBN(await time.latest()) - .add(delay) - .addn(1); - }); - it('it renounces role', async function () { + it('renounces role', async function () { await time.setNextBlockTimestamp(delayPassed); const receipt = await this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, from, { from }); expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, defaultAdmin)).to.be.false; - expect(await this.accessControl.hasRole(ZERO_ADDRESS, defaultAdmin)).to.be.false; + expect(await this.accessControl.hasRole(constants.ZERO_ADDRESS, defaultAdmin)).to.be.false; expectEvent(receipt, 'RoleRevoked', { role: DEFAULT_ADMIN_ROLE, account: from, }); - expect(await this.accessControl.owner()).to.equal(ZERO_ADDRESS); + expect(await this.accessControl.owner()).to.equal(constants.ZERO_ADDRESS); }); it('allows to recover access using the internal _grantRole', async function () { @@ -459,35 +667,180 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa }); }); - it('reverts if caller is not default admin', async function () { - await time.setNextBlockTimestamp(delayPassed); + describe('schedule not passed', function () { + let delayNotPassed; + + beforeEach(function () { + delayNotPassed = delayPassed.subn(1); + }); + + for (const [fromSchedule, tag] of [ + [-1, 'less'], + [0, 'equal'], + ]) { + it(`reverts if block.timestamp is ${tag} to schedule`, async function () { + await time.setNextBlockTimestamp(delayNotPassed.toNumber() + fromSchedule); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), + `${errorPrefix}: only can renounce in two delayed steps`, + ); + }); + } + }); + }); + + describe('changes delay', function () { + it('reverts if called by non default admin accounts', async function () { await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), - `${errorPrefix}: can only renounce roles for self`, + this.accessControl.changeDefaultAdminDelay(time.duration.hours(4), { + from: other, + }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); - describe('delayed until not passed', function () { - let delayNotPassed; + for (const [newDefaultAdminDelay, delayChangeType] of [ + [web3.utils.toBN(delay).subn(time.duration.hours(1)), 'decreased'], + [web3.utils.toBN(delay).addn(time.duration.hours(1)), 'increased'], + [web3.utils.toBN(delay).addn(time.duration.days(5)), 'increased to more than 5 days'], + ]) { + describe(`when the delay is ${delayChangeType}`, function () { + it('begins the delay change to the new delay', async function () { + // Begins the change + const receipt = await this.accessControl.changeDefaultAdminDelay(newDefaultAdminDelay, { + from: defaultAdmin, + }); + + // Calculate expected values + const cap = await this.accessControl.defaultAdminDelayIncreaseWait(); + const changeDelay = newDefaultAdminDelay.lte(delay) + ? delay.sub(newDefaultAdminDelay) + : BN.min(newDefaultAdminDelay, cap); + const timestamp = web3.utils.toBN(await time.latest()); + const effectSchedule = timestamp.add(changeDelay); + + // Assert + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(newDefaultAdminDelay); + expect(schedule).to.be.bignumber.eq(effectSchedule); + expectEvent(receipt, 'DefaultAdminDelayChangeScheduled', { + newDelay, + effectSchedule, + }); + }); - beforeEach(function () { - delayNotPassed = delayPassed.subn(1); + describe('scheduling again', function () { + beforeEach('schedule once', async function () { + await this.accessControl.changeDefaultAdminDelay(newDefaultAdminDelay, { from: defaultAdmin }); + }); + + for (const [fromSchedule, tag] of [ + [-1, 'before'], + [0, 'exactly when'], + [1, 'after'], + ]) { + const passed = fromSchedule > 0; + + it(`succeeds ${tag} the delay schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + + // Default admin changes its mind and begins another delay change + const anotherNewDefaultAdminDelay = newDefaultAdminDelay.addn(time.duration.hours(2)); + const receipt = await this.accessControl.changeDefaultAdminDelay(anotherNewDefaultAdminDelay, { + from: defaultAdmin, + }); + + // Calculate expected values + const cap = await this.accessControl.defaultAdminDelayIncreaseWait(); + const timestamp = web3.utils.toBN(await time.latest()); + const effectSchedule = timestamp.add(BN.min(cap, anotherNewDefaultAdminDelay)); + + // Assert + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(anotherNewDefaultAdminDelay); + expect(schedule).to.be.bignumber.eq(effectSchedule); + expectEvent(receipt, 'DefaultAdminDelayChangeScheduled', { + newDelay, + effectSchedule, + }); + }); + + const emit = passed ? 'not emit' : 'emit'; + it(`should ${emit} a cancellation event ${tag} the delay schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + + // Default admin changes its mind and begins another delay change + const anotherNewDefaultAdminDelay = newDefaultAdminDelay.addn(time.duration.hours(2)); + const receipt = await this.accessControl.changeDefaultAdminDelay(anotherNewDefaultAdminDelay, { + from: defaultAdmin, + }); + + const eventMatcher = passed ? expectNoEvent : expectEvent; + eventMatcher(receipt, 'DefaultAdminDelayChangeCanceled'); + }); + } + }); }); + } + }); - it('reverts if block.timestamp is equal to delayed until', async function () { - await time.setNextBlockTimestamp(delayNotPassed); - await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), - `${errorPrefix}: only can renounce in two delayed steps`, - ); + describe('rollbacks a delay change', function () { + it('reverts if called by non default admin accounts', async function () { + await expectRevert( + this.accessControl.rollbackDefaultAdminDelay({ from: other }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + + describe('when there is a pending delay', function () { + beforeEach('set pending delay', async function () { + await this.accessControl.changeDefaultAdminDelay(time.duration.hours(12), { from: defaultAdmin }); }); - it('reverts if block.timestamp is less than delayed until', async function () { - await time.setNextBlockTimestamp(delayNotPassed.subn(1)); - await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), - `${errorPrefix}: only can renounce in two delayed steps`, - ); + for (const [fromSchedule, tag] of [ + [-1, 'before'], + [0, 'exactly when'], + [1, 'after'], + ]) { + const passed = fromSchedule > 0; + + it(`resets pending delay and schedule ${tag} delay change schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + + await this.accessControl.rollbackDefaultAdminDelay({ from: defaultAdmin }); + + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(ZERO); + expect(schedule).to.be.bignumber.eq(ZERO); + }); + + const emit = passed ? 'not emit' : 'emit'; + it(`should ${emit} a cancellation event ${tag} the delay schedule passes`, async function () { + // Wait until schedule + fromSchedule + const { schedule: firstSchedule } = await this.accessControl.pendingDefaultAdminDelay(); + await time.setNextBlockTimestamp(firstSchedule.toNumber() + fromSchedule); + + const receipt = await this.accessControl.rollbackDefaultAdminDelay({ from: defaultAdmin }); + + const eventMatcher = passed ? expectNoEvent : expectEvent; + eventMatcher(receipt, 'DefaultAdminDelayChangeCanceled'); + }); + } + }); + + describe('when there is no pending delay', function () { + it('succeeds without changes', async function () { + await this.accessControl.rollbackDefaultAdminDelay({ from: defaultAdmin }); + + const { newDelay, schedule } = await this.accessControl.pendingDefaultAdminDelay(); + expect(newDelay).to.be.bignumber.eq(ZERO); + expect(schedule).to.be.bignumber.eq(ZERO); }); }); }); diff --git a/test/access/AccessControlDefaultAdminRules.test.js b/test/access/AccessControlDefaultAdminRules.test.js index 2a23d3b6dab..4e3167a468b 100644 --- a/test/access/AccessControlDefaultAdminRules.test.js +++ b/test/access/AccessControlDefaultAdminRules.test.js @@ -7,7 +7,7 @@ const { const AccessControlDefaultAdminRules = artifacts.require('$AccessControlDefaultAdminRules'); contract('AccessControlDefaultAdminRules', function (accounts) { - const delay = web3.utils.toBN(time.duration.days(10)); + const delay = web3.utils.toBN(time.duration.hours(10)); beforeEach(async function () { this.accessControl = await AccessControlDefaultAdminRules.new(delay, accounts[0], { from: accounts[0] }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 5ffe242edc5..541f6c61130 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -39,9 +39,12 @@ const INTERFACES = { AccessControlEnumerable: ['getRoleMember(bytes32,uint256)', 'getRoleMemberCount(bytes32)'], AccessControlDefaultAdminRules: [ 'defaultAdminDelay()', + 'pendingDefaultAdminDelay()', 'defaultAdmin()', - 'defaultAdminTransferDelayedUntil()', 'pendingDefaultAdmin()', + 'defaultAdminDelayIncreaseWait()', + 'changeDefaultAdminDelay(uint48)', + 'rollbackDefaultAdminDelay()', 'beginDefaultAdminTransfer(address)', 'acceptDefaultAdminTransfer()', 'cancelDefaultAdminTransfer()',