From 3f610ebc25480bf6145e519c96e2f809996db8ed Mon Sep 17 00:00:00 2001 From: mmqxyz <127844428+mmqxyz@users.noreply.github.com> Date: Tue, 21 Mar 2023 15:33:16 +0100 Subject: [PATCH 01/17] Fix typo in README (#4129) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 094637ed6..9fc95518a 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ The engineering guidelines we follow to promote project quality can be found in Past audits can be found in [`audits/`](./audits). -Smart contracts are a nascent techology and carry a high level of technical risk and uncertainty. Although OpenZeppelin is well known for its security audits, using OpenZeppelin Contracts is not a substitute for a security audit. +Smart contracts are a nascent technology and carry a high level of technical risk and uncertainty. Although OpenZeppelin is well known for its security audits, using OpenZeppelin Contracts is not a substitute for a security audit. OpenZeppelin Contracts is made available under the MIT License, which disclaims all warranties in relation to the project and which limits the liability of those that contribute and maintain the project, including OpenZeppelin. As set out further in the Terms, you acknowledge that you are solely responsible for any use of OpenZeppelin Contracts and you assume all risks associated with any such use. From ca822213f2275a14c26167bd387ac3522da67fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Sun, 26 Mar 2023 11:23:13 -0600 Subject: [PATCH 02/17] Make AccessControlDefaultAdminRules delay configurable (#4079) Co-authored-by: Francisco Co-authored-by: Hadrien Croubois --- .../access/AccessControlDefaultAdminRules.sol | 409 +++++++---- .../IAccessControlDefaultAdminRules.sol | 139 +++- test/access/AccessControl.behavior.js | 651 ++++++++++++++---- .../AccessControlDefaultAdminRules.test.js | 2 +- .../SupportsInterface.behavior.js | 5 +- 5 files changed, 901 insertions(+), 305 deletions(-) diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index e3bce6b75..43fca9350 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,66 +32,31 @@ 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; - - address private _currentDefaultAdmin; + // pending admin pair read/written together frequently address private _pendingDefaultAdmin; + uint48 private _pendingDefaultAdminSchedule; // 0 == unset - uint48 private _defaultAdminTransferDelayedUntil; + uint48 private _currentDelay; + address private _currentDefaultAdmin; + + // 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 {IERC5313-owner}. - */ - function owner() public view virtual returns (address) { - return defaultAdmin(); - } - - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function defaultAdminDelay() public view virtual returns (uint48) { - return _defaultAdminDelay; - } - - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function defaultAdmin() public view virtual returns (address) { - return _currentDefaultAdmin; - } - - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function pendingDefaultAdmin() public view virtual returns (address) { - return _pendingDefaultAdmin; - } - - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function defaultAdminTransferDelayedUntil() public view virtual returns (uint48) { - return _defaultAdminTransferDelayedUntil; - } - /** * @dev See {IERC165-supportsInterface}. */ @@ -100,50 +65,15 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @inheritdoc IAccessControlDefaultAdminRules + * @dev See {IERC5313-owner}. */ - function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _beginDefaultAdminTransfer(newAdmin); + function owner() public view virtual returns (address) { + return defaultAdmin(); } - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function acceptDefaultAdminTransfer() public virtual { - require(_msgSender() == pendingDefaultAdmin(), "AccessControl: pending admin must accept"); - _acceptDefaultAdminTransfer(); - } - - /** - * @inheritdoc IAccessControlDefaultAdminRules - */ - function cancelDefaultAdminTransfer() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _resetDefaultAdminTransfer(); - } - - /** - * @dev Revokes `role` from the calling account. - * - * For `DEFAULT_ADMIN_ROLE`, only allows renouncing in two steps, so it's required - * that the {defaultAdminTransferDelayedUntil} has passed and the pending default admin is the zero address. - * After its execution, it will not be possible to call `onlyRole(DEFAULT_ADMIN_ROLE)` - * functions. - * - * For other roles, see {AccessControl-renounceRole}. - * - * NOTE: Renouncing `DEFAULT_ADMIN_ROLE` will leave the contract without a defaultAdmin, - * thereby disabling any functionality that is only available to the default admin, and the - * possibility of reassigning a non-administrated role. - */ - function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) { - if (role == DEFAULT_ADMIN_ROLE) { - require( - pendingDefaultAdmin() == address(0) && _hasDefaultAdminTransferDelayPassed(), - "AccessControl: only can renounce in two delayed steps" - ); - } - super.renounceRole(role, account); - } + /// + /// Override AccessControl role management + /// /** * @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`. @@ -162,24 +92,37 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. + * @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 _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 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 Grants `role` to `account`. + * @dev See {AccessControl-_grantRole}. * - * 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 `DEFAULT_ADMIN_ROLE`, it only allows granting if there isn't already a {defaultAdmin} 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. + * 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) { @@ -189,29 +132,6 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu super._grantRole(role, account); } - /** - * @dev See {acceptDefaultAdminTransfer}. - * - * Internal function without access restriction. - */ - function _acceptDefaultAdminTransfer() internal virtual { - require(_hasDefaultAdminTransferDelayPassed(), "AccessControl: transfer delay not passed"); - _revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin()); - _grantRole(DEFAULT_ADMIN_ROLE, pendingDefaultAdmin()); - _resetDefaultAdminTransfer(); - } - - /** - * @dev See {beginDefaultAdminTransfer}. - * - * Internal function without access restriction. - */ - function _beginDefaultAdminTransfer(address newAdmin) internal virtual { - _defaultAdminTransferDelayedUntil = SafeCast.toUint48(block.timestamp) + defaultAdminDelay(); - _pendingDefaultAdmin = newAdmin; - emit DefaultAdminRoleChangeStarted(pendingDefaultAdmin(), defaultAdminTransferDelayedUntil()); - } - /** * @dev See {AccessControl-_revokeRole}. */ @@ -223,18 +143,239 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu } /** - * @dev Resets the pending default admin and delayed until. + * @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`. */ - function _resetDefaultAdminTransfer() private { - delete _pendingDefaultAdmin; - delete _defaultAdminTransferDelayedUntil; + 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 + */ + function defaultAdmin() public view virtual returns (address) { + return _currentDefaultAdmin; } /** - * @dev Checks if a {defaultAdminTransferDelayedUntil} has been set and passed. + * @inheritdoc IAccessControlDefaultAdminRules */ - function _hasDefaultAdminTransferDelayPassed() private view returns (bool) { - uint48 delayedUntil = defaultAdminTransferDelayedUntil(); - return delayedUntil > 0 && delayedUntil < block.timestamp; + 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 pendingDefaultAdminDelay() public view virtual returns (uint48 newDelay, uint48 schedule) { + schedule = _pendingDelaySchedule; + return (_isScheduleSet(schedule) && !_hasSchedulePassed(schedule)) ? (_pendingDelay, schedule) : (0, 0); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function defaultAdminDelayIncreaseWait() public view virtual returns (uint48) { + return 5 days; + } + + /// + /// AccessControlDefaultAdminRules public and internal setters for defaultAdmin/pendingDefaultAdmin + /// + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function beginDefaultAdminTransfer(address newAdmin) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _beginDefaultAdminTransfer(newAdmin); + } + + /** + * @dev See {beginDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + 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) { + _cancelDefaultAdminTransfer(); + } + + /** + * @dev See {cancelDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + function _cancelDefaultAdminTransfer() internal virtual { + _setPendingDefaultAdmin(address(0), 0); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function acceptDefaultAdminTransfer() public virtual { + (address newDefaultAdmin, ) = pendingDefaultAdmin(); + require(_msgSender() == newDefaultAdmin, "AccessControl: pending admin must accept"); + _acceptDefaultAdminTransfer(); + } + + /** + * @dev See {acceptDefaultAdminTransfer}. + * + * Internal function without access restriction. + */ + 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 + /// + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function changeDefaultAdminDelay(uint48 newDelay) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _changeDefaultAdminDelay(newDelay); + } + + /** + * @dev See {changeDefaultAdminDelay}. + * + * Internal function without access restriction. + */ + function _changeDefaultAdminDelay(uint48 newDelay) internal virtual { + uint48 newSchedule = SafeCast.toUint48(block.timestamp) + _delayChangeWait(newDelay); + _setPendingDelay(newDelay, newSchedule); + emit DefaultAdminDelayChangeScheduled(newDelay, newSchedule); + } + + /** + * @inheritdoc IAccessControlDefaultAdminRules + */ + function rollbackDefaultAdminDelay() public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + _rollbackDefaultAdminDelay(); + } + + /** + * @dev See {rollbackDefaultAdminDelay}. + * + * Internal function without access restriction. + */ + function _rollbackDefaultAdminDelay() internal virtual { + _setPendingDelay(0, 0); + } + + /** + * @dev Returns the amount of seconds to wait after the `newDelay` will + * become the new {defaultAdminDelay}. + * + * 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 _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; + _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 Setter of the tuple for pending delay and its schedule. + * + * May emit a DefaultAdminDelayChangeCanceled event. + */ + 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(); + } + } + + _pendingDelay = newDelay; + _pendingDelaySchedule = newSchedule; + } + + /// + /// Private helpers + /// + + /** + * @dev Defines if an `schedule` is considered set. For consistency purposes. + */ + function _isScheduleSet(uint48 schedule) private pure returns (bool) { + return schedule != 0; + } + + /** + * @dev Defines if an `schedule` is considered passed. For consistency purposes. + */ + 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 753c7c802..d28c49d95 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 e04c5a165..6c88aa274 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); + }); + }); }); - it('has a default pending default admin', async function () { - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); + 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 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('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); + }); + } + }); + }); + + 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('should set pending default admin and delayed until', async function () { - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(newDefaultAdmin); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal( - defaultAdminTransferDelayedUntil, - ); - expectEvent(receipt, 'DefaultAdminRoleChangeStarted', { - newDefaultAdmin, - defaultAdminTransferDelayedUntil, - }); - }); - - it('should be able to begin a transfer again before delay pass', async function () { - // Time passes just before delay - await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.subn(1)); - - // defaultAdmin changes its mind and begin again to another address - await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); - }); - - it('should be able to begin a transfer again after delay pass if not accepted', async function () { - // Time passes after delay without acceptance - await time.setNextBlockTimestamp(defaultAdminTransferDelayedUntil.addn(1)); - - // defaultAdmin changes its mind and begin again to another address - await this.accessControl.beginDefaultAdminTransfer(other, { from: defaultAdmin }); - const newDelayedUntil = web3.utils.toBN(await time.latest()).add(delay); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(other); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(newDelayedUntil); - }); - - it('should revert if it is called by non-admin accounts', async function () { + 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}`, ); }); + + 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, + }); + }); + }); + + 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); + }); + + 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)); + + // Accept and restart + await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }); + const receipt = await this.accessControl.beginDefaultAdminTransfer(other, { from: newDefaultAdmin }); + + expectNoEvent(receipt, 'DefaultAdminTransferCanceled'); + }); + }); + + 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); - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: other }), - `${errorPrefix}: pending admin must accept`, - ); - }); - - describe('delayedUntil not passed', function () { - let delayNotPassed; - - beforeEach(function () { - delayNotPassed = delayPassed.subn(1); - }); - - it('should revert if block.timestamp is equal to delayed until', async function () { - await time.setNextBlockTimestamp(delayNotPassed); - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not passed`, - ); - }); - - it('should revert if block.timestamp is less than delayed until', async function () { - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: transfer delay not passed`, - ); - }); + describe('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('cancel transfer default admin', function () { - let delayPassed; - - beforeEach(async function () { - await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); - delayPassed = web3.utils - .toBN(await time.latest()) - .add(delay) - .addn(1); - }); - - it('resets pending default admin and delayed until', async function () { - await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); - - // Advance until passed delay - await time.setNextBlockTimestamp(delayPassed); - - // Previous pending default admin should not be able to accept after cancellation. - await expectRevert( - this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }), - `${errorPrefix}: pending admin must accept`, - ); - }); - - it('cancels even after delay has passed', async function () { - await this.accessControl.cancelDefaultAdminTransfer({ from: defaultAdmin }); - await time.setNextBlockTimestamp(delayPassed); - expect(await this.accessControl.defaultAdminTransferDelayedUntil()).to.be.bignumber.equal(web3.utils.toBN(0)); - expect(await this.accessControl.pendingDefaultAdmin()).to.equal(ZERO_ADDRESS); - }); - + describe('cancels a default admin transfer', function () { it('reverts if called by non default admin accounts', async function () { await expectRevert( this.accessControl.cancelDefaultAdminTransfer({ from: other }), `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, ); }); + + describe('when there is a pending default admin transfer', function () { + let acceptSchedule; + + beforeEach(async function () { + await this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: defaultAdmin }); + acceptSchedule = web3.utils.toBN(await time.latest()).add(delay); + }); + + 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}: pending admin must accept`, + ); + }); + }); + + 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('renouncing admin', function () { + describe('renounces admin', function () { let delayPassed; let from = defaultAdmin; beforeEach(async function () { - await this.accessControl.beginDefaultAdminTransfer(ZERO_ADDRESS, { from }); + await this.accessControl.beginDefaultAdminTransfer(constants.ZERO_ADDRESS, { from }); delayPassed = web3.utils .toBN(await time.latest()) .add(delay) .addn(1); }); - it('it renounces role', async function () { + it('reverts if caller is not default admin', async function () { + await time.setNextBlockTimestamp(delayPassed); + await expectRevert( + this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), + `${errorPrefix}: can only renounce roles for self`, + ); + }); + + 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); - await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from }), - `${errorPrefix}: can only renounce roles for self`, - ); - }); - - describe('delayed until not passed', function () { + describe('schedule not passed', function () { let delayNotPassed; beforeEach(function () { delayNotPassed = delayPassed.subn(1); }); - it('reverts if block.timestamp is equal to delayed until', async function () { - await time.setNextBlockTimestamp(delayNotPassed); - await expectRevert( - this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from }), - `${errorPrefix}: only can renounce in two delayed steps`, - ); + 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.changeDefaultAdminDelay(time.duration.hours(4), { + from: other, + }), + `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`, + ); + }); + + 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, + }); + }); + + 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'); + }); + } + }); + }); + } + }); + + 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 2a23d3b6d..4e3167a46 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 5ffe242ed..541f6c611 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()', From 7e7060e00e107460fc57c178859d3cf0c6ac64ef Mon Sep 17 00:00:00 2001 From: Antonio Viggiano Date: Thu, 30 Mar 2023 15:57:09 -0300 Subject: [PATCH 03/17] Update IERC3156FlashBorrower.sol (#4145) --- contracts/interfaces/IERC3156FlashBorrower.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/interfaces/IERC3156FlashBorrower.sol b/contracts/interfaces/IERC3156FlashBorrower.sol index c3b4f1eb1..0428391fc 100644 --- a/contracts/interfaces/IERC3156FlashBorrower.sol +++ b/contracts/interfaces/IERC3156FlashBorrower.sol @@ -17,7 +17,7 @@ interface IERC3156FlashBorrower { * @param amount The amount of tokens lent. * @param fee The additional amount of tokens to repay. * @param data Arbitrary data structure, intended to contain user-defined parameters. - * @return The keccak256 hash of "IERC3156FlashBorrower.onFlashLoan" + * @return The keccak256 hash of "ERC3156FlashBorrower.onFlashLoan" */ function onFlashLoan( address initiator, From ead3bcaccbc0ae02f0aaa1b294b432ac55f758cf Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Apr 2023 23:05:39 -0300 Subject: [PATCH 04/17] Fix spurious CI check failures (#4160) --- .github/workflows/checks.yml | 2 ++ foundry.toml | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index c37bf935b..290c6a943 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -83,6 +83,8 @@ jobs: uses: ./.github/actions/setup - run: rm foundry.toml - uses: crytic/slither-action@v0.3.0 + with: + node-version: 18 codespell: if: github.repository != 'OpenZeppelin/openzeppelin-contracts-upgradeable' diff --git a/foundry.toml b/foundry.toml index c0da48773..d0aa4ea39 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,3 +1,3 @@ [fuzz] runs = 10000 -max_test_rejects = 100000 +max_test_rejects = 150000 From 5523c1482bd0a503e4c4dfe821bd2f6b523f3c86 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Apr 2023 16:57:08 +0200 Subject: [PATCH 05/17] Fix TransparentUpgradeableProxy's transparency (#4154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Francisco Co-authored-by: Ernesto García --- .changeset/thirty-shrimps-mix.md | 5 + contracts/interfaces/IERC1967.sol | 25 +++++ contracts/proxy/ERC1967/ERC1967Upgrade.sol | 18 +-- contracts/proxy/transparent/ProxyAdmin.sol | 10 +- .../TransparentUpgradeableProxy.sol | 103 +++++++++++++----- test/proxy/transparent/ProxyAdmin.test.js | 4 +- .../TransparentUpgradeableProxy.behaviour.js | 22 ++-- .../TransparentUpgradeableProxy.test.js | 4 +- 8 files changed, 132 insertions(+), 59 deletions(-) create mode 100644 .changeset/thirty-shrimps-mix.md create mode 100644 contracts/interfaces/IERC1967.sol diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md new file mode 100644 index 000000000..54256d52c --- /dev/null +++ b/.changeset/thirty-shrimps-mix.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata. diff --git a/contracts/interfaces/IERC1967.sol b/contracts/interfaces/IERC1967.sol new file mode 100644 index 000000000..e5deebee9 --- /dev/null +++ b/contracts/interfaces/IERC1967.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/** + * @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC. + * + * _Available since v4.9._ + */ +interface IERC1967 { + /** + * @dev Emitted when the implementation is upgraded. + */ + event Upgraded(address indexed implementation); + + /** + * @dev Emitted when the admin account has changed. + */ + event AdminChanged(address previousAdmin, address newAdmin); + + /** + * @dev Emitted when the beacon is changed. + */ + event BeaconUpgraded(address indexed beacon); +} diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index 0680f3549..a79c10502 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.2; import "../beacon/IBeacon.sol"; +import "../../interfaces/IERC1967.sol"; import "../../interfaces/draft-IERC1822.sol"; import "../../utils/Address.sol"; import "../../utils/StorageSlot.sol"; @@ -14,7 +15,7 @@ import "../../utils/StorageSlot.sol"; * * _Available since v4.1._ */ -abstract contract ERC1967Upgrade { +abstract contract ERC1967Upgrade is IERC1967 { // This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1 bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143; @@ -25,11 +26,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; - /** - * @dev Emitted when the implementation is upgraded. - */ - event Upgraded(address indexed implementation); - /** * @dev Returns the current implementation address. */ @@ -95,11 +91,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; - /** - * @dev Emitted when the admin account has changed. - */ - event AdminChanged(address previousAdmin, address newAdmin); - /** * @dev Returns the current admin. */ @@ -131,11 +122,6 @@ abstract contract ERC1967Upgrade { */ bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; - /** - * @dev Emitted when the beacon is upgraded. - */ - event BeaconUpgraded(address indexed beacon); - /** * @dev Returns the current beacon. */ diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 839534298..1a9698196 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -18,7 +18,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function getProxyImplementation(TransparentUpgradeableProxy proxy) public view virtual returns (address) { + function getProxyImplementation(ITransparentUpgradeableProxy proxy) public view virtual returns (address) { // We need to manually run the static call since the getter cannot be flagged as view // bytes4(keccak256("implementation()")) == 0x5c60da1b (bool success, bytes memory returndata) = address(proxy).staticcall(hex"5c60da1b"); @@ -33,7 +33,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) { + function getProxyAdmin(ITransparentUpgradeableProxy proxy) public view virtual returns (address) { // We need to manually run the static call since the getter cannot be flagged as view // bytes4(keccak256("admin()")) == 0xf851a440 (bool success, bytes memory returndata) = address(proxy).staticcall(hex"f851a440"); @@ -48,7 +48,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the current admin of `proxy`. */ - function changeProxyAdmin(TransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner { + function changeProxyAdmin(ITransparentUpgradeableProxy proxy, address newAdmin) public virtual onlyOwner { proxy.changeAdmin(newAdmin); } @@ -59,7 +59,7 @@ contract ProxyAdmin is Ownable { * * - This contract must be the admin of `proxy`. */ - function upgrade(TransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { + function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner { proxy.upgradeTo(implementation); } @@ -72,7 +72,7 @@ contract ProxyAdmin is Ownable { * - This contract must be the admin of `proxy`. */ function upgradeAndCall( - TransparentUpgradeableProxy proxy, + ITransparentUpgradeableProxy proxy, address implementation, bytes memory data ) public payable virtual onlyOwner { diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index 155a22e01..a89b233f2 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -5,6 +5,24 @@ pragma solidity ^0.8.0; import "../ERC1967/ERC1967Proxy.sol"; +/** + * @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a + * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also + * {TransparentUpgradeableProxy} does not inherit from this interface because it's implemented in a way that the + * compiler doesn't understand and cannot verify. + */ +interface ITransparentUpgradeableProxy is IERC1967 { + function admin() external view returns (address); + + function implementation() external view returns (address); + + function changeAdmin(address) external; + + function upgradeTo(address) external; + + function upgradeToAndCall(address, bytes memory) external payable; +} + /** * @dev This contract implements a proxy that is upgradeable by an admin. * @@ -25,6 +43,13 @@ import "../ERC1967/ERC1967Proxy.sol"; * * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way, * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy. + * + * WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function is implicitly + * implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an + * ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not + * check that there are no selector conflicts. A selector clash between any new function and the functions declared in + * {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could render the admin operations + * inaccessible, which could prevent upgradeability. */ contract TransparentUpgradeableProxy is ERC1967Proxy { /** @@ -37,6 +62,9 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { /** * @dev Modifier used internally that will delegate the call to the implementation unless the sender is the admin. + * + * CAUTION: This modifier is deprecated, as it could cause issues if the modified function has arguments, and the + * implementation provides a function with the same selector. */ modifier ifAdmin() { if (msg.sender == _getAdmin()) { @@ -46,65 +74,98 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { } } + /** + * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior + */ + function _fallback() internal virtual override { + if (msg.sender == _getAdmin()) { + bytes memory ret; + bytes4 selector = msg.sig; + if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) { + ret = _dispatchUpgradeTo(); + } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { + ret = _dispatchUpgradeToAndCall(); + } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) { + ret = _dispatchChangeAdmin(); + } else if (selector == ITransparentUpgradeableProxy.admin.selector) { + ret = _dispatchAdmin(); + } else if (selector == ITransparentUpgradeableProxy.implementation.selector) { + ret = _dispatchImplementation(); + } else { + revert("TransparentUpgradeableProxy: admin cannot fallback to proxy target"); + } + assembly { + return(add(ret, 0x20), mload(ret)) + } + } else { + super._fallback(); + } + } + /** * @dev Returns the current admin. * - * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}. - * * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103` */ - function admin() external payable ifAdmin returns (address admin_) { + function _dispatchAdmin() private returns (bytes memory) { _requireZeroValue(); - admin_ = _getAdmin(); + + address admin = _getAdmin(); + return abi.encode(admin); } /** * @dev Returns the current implementation. * - * NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}. - * * TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` */ - function implementation() external payable ifAdmin returns (address implementation_) { + function _dispatchImplementation() private returns (bytes memory) { _requireZeroValue(); - implementation_ = _implementation(); + + address implementation = _implementation(); + return abi.encode(implementation); } /** * @dev Changes the admin of the proxy. * * Emits an {AdminChanged} event. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}. */ - function changeAdmin(address newAdmin) external payable virtual ifAdmin { + function _dispatchChangeAdmin() private returns (bytes memory) { _requireZeroValue(); + + address newAdmin = abi.decode(msg.data[4:], (address)); _changeAdmin(newAdmin); + + return ""; } /** * @dev Upgrade the implementation of the proxy. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgrade}. */ - function upgradeTo(address newImplementation) external payable ifAdmin { + function _dispatchUpgradeTo() private returns (bytes memory) { _requireZeroValue(); + + address newImplementation = abi.decode(msg.data[4:], (address)); _upgradeToAndCall(newImplementation, bytes(""), false); + + return ""; } /** * @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified * by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the * proxied contract. - * - * NOTE: Only the admin can call this function. See {ProxyAdmin-upgradeAndCall}. */ - function upgradeToAndCall(address newImplementation, bytes calldata data) external payable ifAdmin { + function _dispatchUpgradeToAndCall() private returns (bytes memory) { + (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); _upgradeToAndCall(newImplementation, data, true); + + return ""; } /** @@ -114,14 +175,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { return _getAdmin(); } - /** - * @dev Makes sure the admin cannot access the fallback function. See {Proxy-_beforeFallback}. - */ - function _beforeFallback() internal virtual override { - require(msg.sender != _getAdmin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target"); - super._beforeFallback(); - } - /** * @dev To keep this contract fully transparent, all `ifAdmin` functions must be payable. This helper is here to * emulate some proxy functions being non-payable while still allowing value to pass through. diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index 811dc5671..6d473d893 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -6,6 +6,7 @@ const ImplV1 = artifacts.require('DummyImplementation'); const ImplV2 = artifacts.require('DummyImplementationV2'); const ProxyAdmin = artifacts.require('ProxyAdmin'); const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy'); +const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('ProxyAdmin', function (accounts) { const [proxyAdminOwner, newAdmin, anotherAccount] = accounts; @@ -18,12 +19,13 @@ contract('ProxyAdmin', function (accounts) { beforeEach(async function () { const initializeData = Buffer.from(''); this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner }); - this.proxy = await TransparentUpgradeableProxy.new( + const proxy = await TransparentUpgradeableProxy.new( this.implementationV1.address, this.proxyAdmin.address, initializeData, { from: proxyAdminOwner }, ); + this.proxy = await ITransparentUpgradeableProxy.at(proxy.address); }); it('has an owner', async function () { diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 3a10357a9..66c0a406c 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -34,7 +34,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('implementation', function () { it('returns the current implementation address', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV0); }); @@ -55,7 +55,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx it('upgrades to the requested implementation', async function () { await this.proxy.upgradeTo(this.implementationV1, { from }); - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.implementationV1); }); @@ -103,7 +103,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested implementation', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behavior.address); }); @@ -168,7 +168,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV1.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address }); }); @@ -196,7 +196,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV2.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address }); }); @@ -227,7 +227,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('upgrades to the requested version and emits an event', async function () { - const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress }); + const implementation = await this.proxy.implementation({ from: proxyAdminAddress }); expect(implementation).to.be.equal(this.behaviorV3.address); expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address }); }); @@ -271,7 +271,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx }); it('assigns new proxy admin', async function () { - const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin }); + const newProxyAdmin = await this.proxy.admin({ from: newAdmin }); expect(newProxyAdmin).to.be.equal(anotherAccount); }); @@ -332,21 +332,21 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx describe('when function names clash', function () { it('when sender is proxy admin should run the proxy function', async function () { - const value = await this.proxy.admin.call({ from: proxyAdminAddress, value: 0 }); + const value = await this.proxy.admin({ from: proxyAdminAddress, value: 0 }); expect(value).to.be.equal(proxyAdminAddress); }); it('when sender is other should delegate to implementation', async function () { - const value = await this.proxy.admin.call({ from: anotherAccount, value: 0 }); + const value = await this.proxy.admin({ from: anotherAccount, value: 0 }); expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); }); it('when sender is proxy admin value should not be accepted', async function () { - await expectRevert.unspecified(this.proxy.admin.call({ from: proxyAdminAddress, value: 1 })); + await expectRevert.unspecified(this.proxy.admin({ from: proxyAdminAddress, value: 1 })); }); it('when sender is other value should be accepted', async function () { - const value = await this.proxy.admin.call({ from: anotherAccount, value: 1 }); + const value = await this.proxy.admin({ from: anotherAccount, value: 1 }); expect(value).to.be.equal('0x0000000000000000000000000000000011111142'); }); }); diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.test.js b/test/proxy/transparent/TransparentUpgradeableProxy.test.js index 86dd55d32..d60a31a21 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.test.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.test.js @@ -2,12 +2,14 @@ const shouldBehaveLikeProxy = require('../Proxy.behaviour'); const shouldBehaveLikeTransparentUpgradeableProxy = require('./TransparentUpgradeableProxy.behaviour'); const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy'); +const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy'); contract('TransparentUpgradeableProxy', function (accounts) { const [proxyAdminAddress, proxyAdminOwner] = accounts; const createProxy = async function (logic, admin, initData, opts) { - return TransparentUpgradeableProxy.new(logic, admin, initData, opts); + const { address } = await TransparentUpgradeableProxy.new(logic, admin, initData, opts); + return ITransparentUpgradeableProxy.at(address); }; shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyAdminOwner); From 31723ed608878c2e30710a9c46b51d529c0ba958 Mon Sep 17 00:00:00 2001 From: Renan Souza Date: Wed, 5 Apr 2023 15:47:18 -0300 Subject: [PATCH 06/17] Reenable skipped TransparentUpgradeableProxy test (#4161) Co-authored-by: Francisco --- .../TransparentUpgradeableProxy.behaviour.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js index 66c0a406c..5e3f561d5 100644 --- a/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js +++ b/test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js @@ -3,8 +3,8 @@ const { ZERO_ADDRESS } = constants; const { getSlot, ImplementationSlot, AdminSlot } = require('../../helpers/erc1967'); const { expect } = require('chai'); +const { web3 } = require('hardhat'); -const Proxy = artifacts.require('Proxy'); const Implementation1 = artifacts.require('Implementation1'); const Implementation2 = artifacts.require('Implementation2'); const Implementation3 = artifacts.require('Implementation3'); @@ -122,13 +122,11 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy(createProx expect(balance.toString()).to.be.bignumber.equal(value.toString()); }); - it.skip('uses the storage of the proxy', async function () { + it('uses the storage of the proxy', async function () { // storage layout should look as follows: - // - 0: Initializable storage - // - 1-50: Initailizable reserved storage (50 slots) - // - 51: initializerRan - // - 52: x - const storedValue = await Proxy.at(this.proxyAddress).getStorageAt(52); + // - 0: Initializable storage ++ initializerRan ++ onlyInitializingRan + // - 1: x + const storedValue = await web3.eth.getStorageAt(this.proxyAddress, 1); expect(parseInt(storedValue)).to.eq(42); }); }); From cf86fd9962701396457e50ab0d6cc78aa29a5ebc Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 5 Apr 2023 17:20:34 -0300 Subject: [PATCH 07/17] Merge changesets for transparency improvements (#4165) --- .changeset/many-panthers-hide.md | 5 ----- .changeset/thirty-shrimps-mix.md | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) delete mode 100644 .changeset/many-panthers-hide.md diff --git a/.changeset/many-panthers-hide.md b/.changeset/many-panthers-hide.md deleted file mode 100644 index 5f04c99df..000000000 --- a/.changeset/many-panthers-hide.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'openzeppelin-solidity': minor ---- - -`TransparentUpgradeableProxy`: support value passthrough for all ifAdmin function. diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md index 54256d52c..656edbf92 100644 --- a/.changeset/thirty-shrimps-mix.md +++ b/.changeset/thirty-shrimps-mix.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': patch --- -`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata. +`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata or payable mutability. From f2346b6749546707af883e2c0acbc8a487e16ae4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 Apr 2023 11:21:53 +0200 Subject: [PATCH 08/17] Add fuzz tests for the Checkpoints library (#4146) Co-authored-by: Francisco --- scripts/generate/run.js | 28 +- scripts/generate/templates/Checkpoints.js | 23 +- .../generate/templates/Checkpoints.opts.js | 22 ++ scripts/generate/templates/Checkpoints.t.js | 252 +++++++++++++ test/utils/Checkpoints.t.sol | 341 ++++++++++++++++++ 5 files changed, 636 insertions(+), 30 deletions(-) create mode 100644 scripts/generate/templates/Checkpoints.opts.js create mode 100644 scripts/generate/templates/Checkpoints.t.js create mode 100644 test/utils/Checkpoints.t.sol diff --git a/scripts/generate/run.js b/scripts/generate/run.js index e68681e9d..368b53ad1 100755 --- a/scripts/generate/run.js +++ b/scripts/generate/run.js @@ -13,16 +13,10 @@ function getVersion(path) { } } -for (const [file, template] of Object.entries({ - 'utils/math/SafeCast.sol': './templates/SafeCast.js', - 'utils/structs/EnumerableSet.sol': './templates/EnumerableSet.js', - 'utils/structs/EnumerableMap.sol': './templates/EnumerableMap.js', - 'utils/Checkpoints.sol': './templates/Checkpoints.js', - 'utils/StorageSlot.sol': './templates/StorageSlot.js', -})) { +function generateFromTemplate(file, template, outputPrefix = '') { const script = path.relative(path.join(__dirname, '../..'), __filename); const input = path.join(path.dirname(script), template); - const output = `./contracts/${file}`; + const output = path.join(outputPrefix, file); const version = getVersion(output); const content = format( '// SPDX-License-Identifier: MIT', @@ -35,3 +29,21 @@ for (const [file, template] of Object.entries({ fs.writeFileSync(output, content); cp.execFileSync('prettier', ['--write', output]); } + +// Contracts +for (const [file, template] of Object.entries({ + 'utils/math/SafeCast.sol': './templates/SafeCast.js', + 'utils/structs/EnumerableSet.sol': './templates/EnumerableSet.js', + 'utils/structs/EnumerableMap.sol': './templates/EnumerableMap.js', + 'utils/Checkpoints.sol': './templates/Checkpoints.js', + 'utils/StorageSlot.sol': './templates/StorageSlot.js', +})) { + generateFromTemplate(file, template, './contracts/'); +} + +// Tests +for (const [file, template] of Object.entries({ + 'utils/Checkpoints.t.sol': './templates/Checkpoints.t.js', +})) { + generateFromTemplate(file, template, './test/'); +} diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index e51e8b8d1..18999682d 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -1,26 +1,5 @@ const format = require('../format-lines'); - -// OPTIONS -const defaultOpts = size => ({ - historyTypeName: `Trace${size}`, - checkpointTypeName: `Checkpoint${size}`, - checkpointFieldName: '_checkpoints', - keyTypeName: `uint${256 - size}`, - keyFieldName: '_key', - valueTypeName: `uint${size}`, - valueFieldName: '_value', -}); - -const VALUE_SIZES = [224, 160]; - -const OPTS = VALUE_SIZES.map(size => defaultOpts(size)); - -const LEGACY_OPTS = { - ...defaultOpts(224), - historyTypeName: 'History', - checkpointTypeName: 'Checkpoint', - keyFieldName: '_blockNumber', -}; +const { OPTS, LEGACY_OPTS } = require('./Checkpoints.opts.js'); // TEMPLATE const header = `\ diff --git a/scripts/generate/templates/Checkpoints.opts.js b/scripts/generate/templates/Checkpoints.opts.js new file mode 100644 index 000000000..03c5a9569 --- /dev/null +++ b/scripts/generate/templates/Checkpoints.opts.js @@ -0,0 +1,22 @@ +// OPTIONS +const VALUE_SIZES = [224, 160]; + +const defaultOpts = size => ({ + historyTypeName: `Trace${size}`, + checkpointTypeName: `Checkpoint${size}`, + checkpointFieldName: '_checkpoints', + keyTypeName: `uint${256 - size}`, + keyFieldName: '_key', + valueTypeName: `uint${size}`, + valueFieldName: '_value', +}); + +module.exports = { + OPTS: VALUE_SIZES.map(size => defaultOpts(size)), + LEGACY_OPTS: { + ...defaultOpts(224), + historyTypeName: 'History', + checkpointTypeName: 'Checkpoint', + keyFieldName: '_blockNumber', + }, +}; diff --git a/scripts/generate/templates/Checkpoints.t.js b/scripts/generate/templates/Checkpoints.t.js new file mode 100644 index 000000000..84b5992ad --- /dev/null +++ b/scripts/generate/templates/Checkpoints.t.js @@ -0,0 +1,252 @@ +const format = require('../format-lines'); +const { capitalize } = require('../../helpers'); +const { OPTS, LEGACY_OPTS } = require('./Checkpoints.opts.js'); + +// TEMPLATE +const header = `\ +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; +import "../../contracts/utils/Checkpoints.sol"; +import "../../contracts/utils/math/SafeCast.sol"; +`; + +/* eslint-disable max-len */ +const common = opts => `\ +using Checkpoints for Checkpoints.${opts.historyTypeName}; + +// Maximum gap between keys used during the fuzzing tests: the \`_prepareKeys\` function with make sure that +// key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range. +uint8 internal constant _KEY_MAX_GAP = 64; + +Checkpoints.${opts.historyTypeName} internal _ckpts; + +// helpers +function _bound${capitalize(opts.keyTypeName)}( + ${opts.keyTypeName} x, + ${opts.keyTypeName} min, + ${opts.keyTypeName} max +) internal view returns (${opts.keyTypeName}) { + return SafeCast.to${capitalize(opts.keyTypeName)}(bound(uint256(x), uint256(min), uint256(max))); +} + +function _prepareKeys( + ${opts.keyTypeName}[] memory keys, + ${opts.keyTypeName} maxSpread +) internal view { + ${opts.keyTypeName} lastKey = 0; + for (uint256 i = 0; i < keys.length; ++i) { + ${opts.keyTypeName} key = _bound${capitalize(opts.keyTypeName)}(keys[i], lastKey, lastKey + maxSpread); + keys[i] = key; + lastKey = key; + } +} + +function _assertLatestCheckpoint( + bool exist, + ${opts.keyTypeName} key, + ${opts.valueTypeName} value +) internal { + (bool _exist, ${opts.keyTypeName} _key, ${opts.valueTypeName} _value) = _ckpts.latestCheckpoint(); + assertEq(_exist, exist); + assertEq(_key, key); + assertEq(_value, value); +} +`; + +const testTrace = opts => `\ +// tests +function testPush( + ${opts.keyTypeName}[] memory keys, + ${opts.valueTypeName}[] memory values, + ${opts.keyTypeName} pastKey +) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + ${opts.keyTypeName} key = keys[i]; + ${opts.valueTypeName} value = values[i % values.length]; + if (i > 0 && key == keys[i-1]) ++duplicates; + + // push + _ckpts.push(key, value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + if (keys.length > 0) { + ${opts.keyTypeName} lastKey = keys[keys.length - 1]; + pastKey = _bound${capitalize(opts.keyTypeName)}(pastKey, 0, lastKey - 1); + + vm.expectRevert(); + this.push(pastKey, values[keys.length % values.length]); + } +} + +// used to test reverts +function push(${opts.keyTypeName} key, ${opts.valueTypeName} value) external { + _ckpts.push(key, value); +} + +function testLookup( + ${opts.keyTypeName}[] memory keys, + ${opts.valueTypeName}[] memory values, + ${opts.keyTypeName} lookup +) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + ${opts.keyTypeName} lastKey = keys.length == 0 ? 0 : keys[keys.length - 1]; + lookup = _bound${capitalize(opts.keyTypeName)}(lookup, 0, lastKey + _KEY_MAX_GAP); + + ${opts.valueTypeName} upper = 0; + ${opts.valueTypeName} lower = 0; + ${opts.keyTypeName} lowerKey = type(${opts.keyTypeName}).max; + for (uint256 i = 0; i < keys.length; ++i) { + ${opts.keyTypeName} key = keys[i]; + ${opts.valueTypeName} value = values[i % values.length]; + + // push + _ckpts.push(key, value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + // find the first key that is not smaller than the lookup key + if (key >= lookup && (i == 0 || keys[i-1] < lookup)) { + lowerKey = key; + } + if (key == lowerKey) { + lower = value; + } + } + + // check lookup + assertEq(_ckpts.lowerLookup(lookup), lower); + assertEq(_ckpts.upperLookup(lookup), upper); + assertEq(_ckpts.upperLookupRecent(lookup), upper); +} +`; + +const testHistory = opts => `\ +// tests +function testPush( + ${opts.keyTypeName}[] memory keys, + ${opts.valueTypeName}[] memory values, + ${opts.keyTypeName} pastKey +) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + ${opts.keyTypeName} key = keys[i]; + ${opts.valueTypeName} value = values[i % values.length]; + if (i > 0 && key == keys[i - 1]) ++duplicates; + + // push + vm.roll(key); + _ckpts.push(value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + // Can't push any key in the past + if (keys.length > 0) { + ${opts.keyTypeName} lastKey = keys[keys.length - 1]; + pastKey = _bound${capitalize(opts.keyTypeName)}(pastKey, 0, lastKey - 1); + + vm.roll(pastKey); + vm.expectRevert(); + this.push(values[keys.length % values.length]); + } +} + +// used to test reverts +function push(${opts.valueTypeName} value) external { + _ckpts.push(value); +} + +function testLookup( + ${opts.keyTypeName}[] memory keys, + ${opts.valueTypeName}[] memory values, + ${opts.keyTypeName} lookup +) public { + vm.assume(keys.length > 0); + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + ${opts.keyTypeName} lastKey = keys[keys.length - 1]; + vm.assume(lastKey > 0); + lookup = _bound${capitalize(opts.keyTypeName)}(lookup, 0, lastKey - 1); + + ${opts.valueTypeName} upper = 0; + for (uint256 i = 0; i < keys.length; ++i) { + ${opts.keyTypeName} key = keys[i]; + ${opts.valueTypeName} value = values[i % values.length]; + + // push + vm.roll(key); + _ckpts.push(value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + } + + // check lookup + assertEq(_ckpts.getAtBlock(lookup), upper); + assertEq(_ckpts.getAtProbablyRecentBlock(lookup), upper); + + vm.expectRevert(); this.getAtBlock(lastKey); + vm.expectRevert(); this.getAtBlock(lastKey + 1); + vm.expectRevert(); this.getAtProbablyRecentBlock(lastKey); + vm.expectRevert(); this.getAtProbablyRecentBlock(lastKey + 1); +} + +// used to test reverts +function getAtBlock(${opts.keyTypeName} key) external view { + _ckpts.getAtBlock(key); +} + +// used to test reverts +function getAtProbablyRecentBlock(${opts.keyTypeName} key) external view { + _ckpts.getAtProbablyRecentBlock(key); +} +`; +/* eslint-enable max-len */ + +// GENERATE +module.exports = format( + header, + // HISTORY + `contract Checkpoints${LEGACY_OPTS.historyTypeName}Test is Test {`, + [common(LEGACY_OPTS), testHistory(LEGACY_OPTS)], + '}', + // TRACEXXX + ...OPTS.flatMap(opts => [ + `contract Checkpoints${opts.historyTypeName}Test is Test {`, + [common(opts), testTrace(opts)], + '}', + ]), +); diff --git a/test/utils/Checkpoints.t.sol b/test/utils/Checkpoints.t.sol new file mode 100644 index 000000000..abdf8b436 --- /dev/null +++ b/test/utils/Checkpoints.t.sol @@ -0,0 +1,341 @@ +// SPDX-License-Identifier: MIT +// This file was procedurally generated from scripts/generate/templates/Checkpoints.t.js. + +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; +import "../../contracts/utils/Checkpoints.sol"; +import "../../contracts/utils/math/SafeCast.sol"; + +contract CheckpointsHistoryTest is Test { + using Checkpoints for Checkpoints.History; + + // Maximum gap between keys used during the fuzzing tests: the `_prepareKeys` function with make sure that + // key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range. + uint8 internal constant _KEY_MAX_GAP = 64; + + Checkpoints.History internal _ckpts; + + // helpers + function _boundUint32(uint32 x, uint32 min, uint32 max) internal view returns (uint32) { + return SafeCast.toUint32(bound(uint256(x), uint256(min), uint256(max))); + } + + function _prepareKeys(uint32[] memory keys, uint32 maxSpread) internal view { + uint32 lastKey = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = _boundUint32(keys[i], lastKey, lastKey + maxSpread); + keys[i] = key; + lastKey = key; + } + } + + function _assertLatestCheckpoint(bool exist, uint32 key, uint224 value) internal { + (bool _exist, uint32 _key, uint224 _value) = _ckpts.latestCheckpoint(); + assertEq(_exist, exist); + assertEq(_key, key); + assertEq(_value, value); + } + + // tests + function testPush(uint32[] memory keys, uint224[] memory values, uint32 pastKey) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = keys[i]; + uint224 value = values[i % values.length]; + if (i > 0 && key == keys[i - 1]) ++duplicates; + + // push + vm.roll(key); + _ckpts.push(value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + // Can't push any key in the past + if (keys.length > 0) { + uint32 lastKey = keys[keys.length - 1]; + pastKey = _boundUint32(pastKey, 0, lastKey - 1); + + vm.roll(pastKey); + vm.expectRevert(); + this.push(values[keys.length % values.length]); + } + } + + // used to test reverts + function push(uint224 value) external { + _ckpts.push(value); + } + + function testLookup(uint32[] memory keys, uint224[] memory values, uint32 lookup) public { + vm.assume(keys.length > 0); + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + uint32 lastKey = keys[keys.length - 1]; + vm.assume(lastKey > 0); + lookup = _boundUint32(lookup, 0, lastKey - 1); + + uint224 upper = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = keys[i]; + uint224 value = values[i % values.length]; + + // push + vm.roll(key); + _ckpts.push(value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + } + + // check lookup + assertEq(_ckpts.getAtBlock(lookup), upper); + assertEq(_ckpts.getAtProbablyRecentBlock(lookup), upper); + + vm.expectRevert(); + this.getAtBlock(lastKey); + vm.expectRevert(); + this.getAtBlock(lastKey + 1); + vm.expectRevert(); + this.getAtProbablyRecentBlock(lastKey); + vm.expectRevert(); + this.getAtProbablyRecentBlock(lastKey + 1); + } + + // used to test reverts + function getAtBlock(uint32 key) external view { + _ckpts.getAtBlock(key); + } + + // used to test reverts + function getAtProbablyRecentBlock(uint32 key) external view { + _ckpts.getAtProbablyRecentBlock(key); + } +} + +contract CheckpointsTrace224Test is Test { + using Checkpoints for Checkpoints.Trace224; + + // Maximum gap between keys used during the fuzzing tests: the `_prepareKeys` function with make sure that + // key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range. + uint8 internal constant _KEY_MAX_GAP = 64; + + Checkpoints.Trace224 internal _ckpts; + + // helpers + function _boundUint32(uint32 x, uint32 min, uint32 max) internal view returns (uint32) { + return SafeCast.toUint32(bound(uint256(x), uint256(min), uint256(max))); + } + + function _prepareKeys(uint32[] memory keys, uint32 maxSpread) internal view { + uint32 lastKey = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = _boundUint32(keys[i], lastKey, lastKey + maxSpread); + keys[i] = key; + lastKey = key; + } + } + + function _assertLatestCheckpoint(bool exist, uint32 key, uint224 value) internal { + (bool _exist, uint32 _key, uint224 _value) = _ckpts.latestCheckpoint(); + assertEq(_exist, exist); + assertEq(_key, key); + assertEq(_value, value); + } + + // tests + function testPush(uint32[] memory keys, uint224[] memory values, uint32 pastKey) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = keys[i]; + uint224 value = values[i % values.length]; + if (i > 0 && key == keys[i - 1]) ++duplicates; + + // push + _ckpts.push(key, value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + if (keys.length > 0) { + uint32 lastKey = keys[keys.length - 1]; + pastKey = _boundUint32(pastKey, 0, lastKey - 1); + + vm.expectRevert(); + this.push(pastKey, values[keys.length % values.length]); + } + } + + // used to test reverts + function push(uint32 key, uint224 value) external { + _ckpts.push(key, value); + } + + function testLookup(uint32[] memory keys, uint224[] memory values, uint32 lookup) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + uint32 lastKey = keys.length == 0 ? 0 : keys[keys.length - 1]; + lookup = _boundUint32(lookup, 0, lastKey + _KEY_MAX_GAP); + + uint224 upper = 0; + uint224 lower = 0; + uint32 lowerKey = type(uint32).max; + for (uint256 i = 0; i < keys.length; ++i) { + uint32 key = keys[i]; + uint224 value = values[i % values.length]; + + // push + _ckpts.push(key, value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + // find the first key that is not smaller than the lookup key + if (key >= lookup && (i == 0 || keys[i - 1] < lookup)) { + lowerKey = key; + } + if (key == lowerKey) { + lower = value; + } + } + + // check lookup + assertEq(_ckpts.lowerLookup(lookup), lower); + assertEq(_ckpts.upperLookup(lookup), upper); + assertEq(_ckpts.upperLookupRecent(lookup), upper); + } +} + +contract CheckpointsTrace160Test is Test { + using Checkpoints for Checkpoints.Trace160; + + // Maximum gap between keys used during the fuzzing tests: the `_prepareKeys` function with make sure that + // key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range. + uint8 internal constant _KEY_MAX_GAP = 64; + + Checkpoints.Trace160 internal _ckpts; + + // helpers + function _boundUint96(uint96 x, uint96 min, uint96 max) internal view returns (uint96) { + return SafeCast.toUint96(bound(uint256(x), uint256(min), uint256(max))); + } + + function _prepareKeys(uint96[] memory keys, uint96 maxSpread) internal view { + uint96 lastKey = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint96 key = _boundUint96(keys[i], lastKey, lastKey + maxSpread); + keys[i] = key; + lastKey = key; + } + } + + function _assertLatestCheckpoint(bool exist, uint96 key, uint160 value) internal { + (bool _exist, uint96 _key, uint160 _value) = _ckpts.latestCheckpoint(); + assertEq(_exist, exist); + assertEq(_key, key); + assertEq(_value, value); + } + + // tests + function testPush(uint96[] memory keys, uint160[] memory values, uint96 pastKey) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint96 key = keys[i]; + uint160 value = values[i % values.length]; + if (i > 0 && key == keys[i - 1]) ++duplicates; + + // push + _ckpts.push(key, value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + if (keys.length > 0) { + uint96 lastKey = keys[keys.length - 1]; + pastKey = _boundUint96(pastKey, 0, lastKey - 1); + + vm.expectRevert(); + this.push(pastKey, values[keys.length % values.length]); + } + } + + // used to test reverts + function push(uint96 key, uint160 value) external { + _ckpts.push(key, value); + } + + function testLookup(uint96[] memory keys, uint160[] memory values, uint96 lookup) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + uint96 lastKey = keys.length == 0 ? 0 : keys[keys.length - 1]; + lookup = _boundUint96(lookup, 0, lastKey + _KEY_MAX_GAP); + + uint160 upper = 0; + uint160 lower = 0; + uint96 lowerKey = type(uint96).max; + for (uint256 i = 0; i < keys.length; ++i) { + uint96 key = keys[i]; + uint160 value = values[i % values.length]; + + // push + _ckpts.push(key, value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + // find the first key that is not smaller than the lookup key + if (key >= lookup && (i == 0 || keys[i - 1] < lookup)) { + lowerKey = key; + } + if (key == lowerKey) { + lower = value; + } + } + + // check lookup + assertEq(_ckpts.lowerLookup(lookup), lower); + assertEq(_ckpts.upperLookup(lookup), upper); + assertEq(_ckpts.upperLookupRecent(lookup), upper); + } +} From 473d0b6884ce538f534a0fc04c97b78e8cc0f48b Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 Apr 2023 20:36:58 -0300 Subject: [PATCH 09/17] Add Codecov token --- .github/workflows/checks.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 290c6a943..c6b42dd82 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -73,6 +73,8 @@ jobs: env: NODE_OPTIONS: --max_old_space_size=4096 - uses: codecov/codecov-action@v3 + with: + token: ${{ secrets.CODECOV_TOKEN }} slither: if: github.repository != 'OpenZeppelin/openzeppelin-contracts-upgradeable' From 661343f74ca798c0ac5037411fe39d8ef15f3377 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 12 Apr 2023 04:17:10 +0200 Subject: [PATCH 10/17] Add DoubleEndedQueue FV (#4147) Co-authored-by: Francisco Co-authored-by: Hadrien Croubois --- certora/harnesses/DoubleEndedQueueHarness.sol | 59 +++ certora/specs.json | 5 + certora/specs/DoubleEndedQueue.spec | 366 ++++++++++++++++++ 3 files changed, 430 insertions(+) create mode 100644 certora/harnesses/DoubleEndedQueueHarness.sol create mode 100644 certora/specs/DoubleEndedQueue.spec diff --git a/certora/harnesses/DoubleEndedQueueHarness.sol b/certora/harnesses/DoubleEndedQueueHarness.sol new file mode 100644 index 000000000..b4f0bc841 --- /dev/null +++ b/certora/harnesses/DoubleEndedQueueHarness.sol @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../patched/utils/structs/DoubleEndedQueue.sol"; + +contract DoubleEndedQueueHarness { + using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; + + DoubleEndedQueue.Bytes32Deque private _deque; + + function pushFront(bytes32 value) external { + _deque.pushFront(value); + } + + function pushBack(bytes32 value) external { + _deque.pushBack(value); + } + + function popFront() external returns (bytes32 value) { + return _deque.popFront(); + } + + function popBack() external returns (bytes32 value) { + return _deque.popBack(); + } + + function clear() external { + _deque.clear(); + } + + function begin() external view returns (int128) { + return _deque._begin; + } + + function end() external view returns (int128) { + return _deque._end; + } + + function length() external view returns (uint256) { + return _deque.length(); + } + + function empty() external view returns (bool) { + return _deque.empty(); + } + + function front() external view returns (bytes32 value) { + return _deque.front(); + } + + function back() external view returns (bytes32 value) { + return _deque.back(); + } + + function at_(uint256 index) external view returns (bytes32 value) { + return _deque.at(index); + } +} diff --git a/certora/specs.json b/certora/specs.json index 80fbf26af..cd68af65e 100644 --- a/certora/specs.json +++ b/certora/specs.json @@ -9,6 +9,11 @@ "contract": "AccessControlHarness", "files": ["certora/harnesses/AccessControlHarness.sol"] }, + { + "spec": "DoubleEndedQueue", + "contract": "DoubleEndedQueueHarness", + "files": ["certora/harnesses/DoubleEndedQueueHarness.sol"] + }, { "spec": "Ownable", "contract": "OwnableHarness", diff --git a/certora/specs/DoubleEndedQueue.spec b/certora/specs/DoubleEndedQueue.spec new file mode 100644 index 000000000..2412b4cc8 --- /dev/null +++ b/certora/specs/DoubleEndedQueue.spec @@ -0,0 +1,366 @@ +import "helpers.spec" + +methods { + pushFront(bytes32) envfree + pushBack(bytes32) envfree + popFront() returns (bytes32) envfree + popBack() returns (bytes32) envfree + clear() envfree + + // exposed for FV + begin() returns (int128) envfree + end() returns (int128) envfree + + // view + length() returns (uint256) envfree + empty() returns (bool) envfree + front() returns (bytes32) envfree + back() returns (bytes32) envfree + at_(uint256) returns (bytes32) envfree // at is a reserved word +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Helpers │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ + +function min_int128() returns mathint { + return -(1 << 127); +} + +function max_int128() returns mathint { + return (1 << 127) - 1; +} + +// Could be broken in theory, but not in practice +function boundedQueue() returns bool { + return + max_int128() > to_mathint(end()) && + min_int128() < to_mathint(begin()); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: end is larger or equal than begin │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant boundariesConsistency() + end() >= begin() + filtered { f -> !f.isView } + { preserved { require boundedQueue(); } } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: length is end minus begin │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant lengthConsistency() + length() == to_mathint(end()) - to_mathint(begin()) + filtered { f -> !f.isView } + { preserved { require boundedQueue(); } } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: empty() is length 0 and no element exists │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant emptiness() + empty() <=> length() == 0 + filtered { f -> !f.isView } + { preserved { require boundedQueue(); } } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: front points to the first index and back points to the last one │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant queueEndings() + at_(length() - 1) == back() && at_(0) == front() + filtered { f -> !f.isView } + { + preserved { + requireInvariant boundariesConsistency(); + require boundedQueue(); + } + } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Function correctness: pushFront adds an element at the beginning of the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule pushFront(bytes32 value) { + require boundedQueue(); + + uint256 lengthBefore = length(); + + pushFront@withrevert(value); + + // liveness + assert !lastReverted, "never reverts"; + + // effect + assert front() == value, "front set to value"; + assert length() == lengthBefore + 1, "queue extended"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: pushFront preserves the previous values in the queue with a +1 offset │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule pushFrontConsistency(uint256 index) { + require boundedQueue(); + + bytes32 beforeAt = at_(index); + + bytes32 value; + pushFront(value); + + // try to read value + bytes32 afterAt = at_@withrevert(index + 1); + + assert !lastReverted, "value still there"; + assert afterAt == beforeAt, "data is preserved"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Function correctness: pushBack adds an element at the end of the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule pushBack(bytes32 value) { + require boundedQueue(); + + uint256 lengthBefore = length(); + + pushBack@withrevert(value); + + // liveness + assert !lastReverted, "never reverts"; + + // effect + assert back() == value, "back set to value"; + assert length() == lengthBefore + 1, "queue increased"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: pushBack preserves the previous values in the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule pushBackConsistency(uint256 index) { + require boundedQueue(); + + bytes32 beforeAt = at_(index); + + bytes32 value; + pushBack(value); + + // try to read value + bytes32 afterAt = at_@withrevert(index); + + assert !lastReverted, "value still there"; + assert afterAt == beforeAt, "data is preserved"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Function correctness: popFront removes an element from the beginning of the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule popFront { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + uint256 lengthBefore = length(); + bytes32 frontBefore = front@withrevert(); + + bytes32 popped = popFront@withrevert(); + bool success = !lastReverted; + + // liveness + assert success <=> lengthBefore != 0, "never reverts if not previously empty"; + + // effect + assert success => frontBefore == popped, "previous front is returned"; + assert success => length() == lengthBefore - 1, "queue decreased"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: at(x) is preserved and offset to at(x - 1) after calling popFront | +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule popFrontConsistency(uint256 index) { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + // Read (any) value that is not the front (this asserts the value exists / the queue is long enough) + require index > 1; + bytes32 before = at_(index); + + popFront(); + + // try to read value + bytes32 after = at_@withrevert(index - 1); + + assert !lastReverted, "value still exists in the queue"; + assert before == after, "values are offset and not modified"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Function correctness: popBack removes an element from the end of the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule popBack { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + uint256 lengthBefore = length(); + bytes32 backBefore = back@withrevert(); + + bytes32 popped = popBack@withrevert(); + bool success = !lastReverted; + + // liveness + assert success <=> lengthBefore != 0, "never reverts if not previously empty"; + + // effect + assert success => backBefore == popped, "previous back is returned"; + assert success => length() == lengthBefore - 1, "queue decreased"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: at(x) is preserved after calling popBack | +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule popBackConsistency(uint256 index) { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + // Read (any) value that is not the back (this asserts the value exists / the queue is long enough) + require index < length() - 1; + bytes32 before = at_(index); + + popBack(); + + // try to read value + bytes32 after = at_@withrevert(index); + + assert !lastReverted, "value still exists in the queue"; + assert before == after, "values are offset and not modified"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Function correctness: clear sets length to 0 │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule clear { + clear@withrevert(); + + // liveness + assert !lastReverted, "never reverts"; + + // effect + assert length() == 0, "sets length to 0"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: front/back access reverts only if the queue is empty or querying out of bounds │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule onlyEmptyRevert(env e) { + require nonpayable(e); + requireInvariant boundariesConsistency(); + require boundedQueue(); + + method f; + calldataarg args; + + bool emptyBefore = empty(); + + f@withrevert(e, args); + + assert lastReverted => ( + (f.selector == front().selector && emptyBefore) || + (f.selector == back().selector && emptyBefore) || + (f.selector == popFront().selector && emptyBefore) || + (f.selector == popBack().selector && emptyBefore) || + f.selector == at_(uint256).selector // revert conditions are verified in onlyOutOfBoundsRevert + ), "only revert if empty or out of bounds"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: at(index) only reverts if index is out of bounds | +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule onlyOutOfBoundsRevert(uint256 index) { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + at_@withrevert(index); + + assert lastReverted <=> index >= length(), "only reverts if index is out of bounds"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: only clear/push/pop operations can change the length of the queue │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule noLengthChange(env e) { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + method f; + calldataarg args; + + uint256 lengthBefore = length(); + f(e, args); + uint256 lengthAfter = length(); + + assert lengthAfter != lengthBefore => ( + (f.selector == pushFront(bytes32).selector && lengthAfter == lengthBefore + 1) || + (f.selector == pushBack(bytes32).selector && lengthAfter == lengthBefore + 1) || + (f.selector == popBack().selector && lengthAfter == lengthBefore - 1) || + (f.selector == popFront().selector && lengthAfter == lengthBefore - 1) || + (f.selector == clear().selector && lengthAfter == 0) + ), "length is only affected by clear/pop/push operations"; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: only push/pop can change values bounded in the queue (outside values aren't cleared) │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule noDataChange(env e) { + requireInvariant boundariesConsistency(); + require boundedQueue(); + + method f; + calldataarg args; + + uint256 index; + bytes32 atBefore = at_(index); + f(e, args); + bytes32 atAfter = at_@withrevert(index); + bool atAfterSuccess = !lastReverted; + + assert !atAfterSuccess <=> ( + f.selector == clear().selector || + (f.selector == popBack().selector && index == length()) || + (f.selector == popFront().selector && index == length()) + ), "indexes of the queue are only removed by clear or pop"; + + assert atAfterSuccess && atAfter != atBefore => ( + f.selector == popFront().selector || + f.selector == pushFront(bytes32).selector + ), "values of the queue are only changed by popFront or pushFront"; +} From 86f6eb2c9c0eb741de18ef75a290ff340acf7148 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Apr 2023 05:29:36 +0200 Subject: [PATCH 11/17] Add FV specification for ERC721 (#4104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- certora/diff/token_ERC721_ERC721.sol.patch | 14 + certora/harnesses/ERC721Harness.sol | 37 ++ certora/harnesses/ERC721ReceiverHarness.sol | 11 + certora/specs.json | 6 + certora/specs/ERC721.spec | 589 ++++++++++++++++++++ certora/specs/methods/IERC721.spec | 20 + 6 files changed, 677 insertions(+) create mode 100644 certora/diff/token_ERC721_ERC721.sol.patch create mode 100644 certora/harnesses/ERC721Harness.sol create mode 100644 certora/harnesses/ERC721ReceiverHarness.sol create mode 100644 certora/specs/ERC721.spec create mode 100644 certora/specs/methods/IERC721.spec diff --git a/certora/diff/token_ERC721_ERC721.sol.patch b/certora/diff/token_ERC721_ERC721.sol.patch new file mode 100644 index 000000000..c3eae357a --- /dev/null +++ b/certora/diff/token_ERC721_ERC721.sol.patch @@ -0,0 +1,14 @@ +--- token/ERC721/ERC721.sol 2023-03-07 10:48:47.736822221 +0100 ++++ token/ERC721/ERC721.sol 2023-03-09 19:49:39.669338673 +0100 +@@ -199,6 +199,11 @@ + return _owners[tokenId]; + } + ++ // FV ++ function _getApproved(uint256 tokenId) internal view returns (address) { ++ return _tokenApprovals[tokenId]; ++ } ++ + /** + * @dev Returns whether `tokenId` exists. + * diff --git a/certora/harnesses/ERC721Harness.sol b/certora/harnesses/ERC721Harness.sol new file mode 100644 index 000000000..3307369a8 --- /dev/null +++ b/certora/harnesses/ERC721Harness.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../patched/token/ERC721/ERC721.sol"; + +contract ERC721Harness is ERC721 { + constructor(string memory name, string memory symbol) ERC721(name, symbol) {} + + function mint(address account, uint256 tokenId) external { + _mint(account, tokenId); + } + + function safeMint(address to, uint256 tokenId) external { + _safeMint(to, tokenId); + } + + function safeMint(address to, uint256 tokenId, bytes memory data) external { + _safeMint(to, tokenId, data); + } + + function burn(uint256 tokenId) external { + _burn(tokenId); + } + + function tokenExists(uint256 tokenId) external view returns (bool) { + return _exists(tokenId); + } + + function unsafeOwnerOf(uint256 tokenId) external view returns (address) { + return _ownerOf(tokenId); + } + + function unsafeGetApproved(uint256 tokenId) external view returns (address) { + return _getApproved(tokenId); + } +} diff --git a/certora/harnesses/ERC721ReceiverHarness.sol b/certora/harnesses/ERC721ReceiverHarness.sol new file mode 100644 index 000000000..7e5739ee3 --- /dev/null +++ b/certora/harnesses/ERC721ReceiverHarness.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../patched/interfaces/IERC721Receiver.sol"; + +contract ERC721ReceiverHarness is IERC721Receiver { + function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) { + return this.onERC721Received.selector; + } +} diff --git a/certora/specs.json b/certora/specs.json index cd68af65e..39ba8c235 100644 --- a/certora/specs.json +++ b/certora/specs.json @@ -51,6 +51,12 @@ "--optimistic_loop" ] }, + { + "spec": "ERC721", + "contract": "ERC721Harness", + "files": ["certora/harnesses/ERC721Harness.sol", "certora/harnesses/ERC721ReceiverHarness.sol"], + "options": ["--optimistic_loop"] + }, { "spec": "Initializable", "contract": "InitializableHarness", diff --git a/certora/specs/ERC721.spec b/certora/specs/ERC721.spec new file mode 100644 index 000000000..48503469b --- /dev/null +++ b/certora/specs/ERC721.spec @@ -0,0 +1,589 @@ +import "helpers.spec" +import "methods/IERC721.spec" + +methods { + // exposed for FV + mint(address,uint256) + safeMint(address,uint256) + safeMint(address,uint256,bytes) + burn(uint256) + + tokenExists(uint256) returns (bool) envfree + unsafeOwnerOf(uint256) returns (address) envfree + unsafeGetApproved(uint256) returns (address) envfree +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Helpers │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ + +// Could be broken in theory, but not in practice +function balanceLimited(address account) returns bool { + return balanceOf(account) < max_uint256; +} + +function helperTransferWithRevert(env e, method f, address from, address to, uint256 tokenId) { + if (f.selector == transferFrom(address,address,uint256).selector) { + transferFrom@withrevert(e, from, to, tokenId); + } else if (f.selector == safeTransferFrom(address,address,uint256).selector) { + safeTransferFrom@withrevert(e, from, to, tokenId); + } else if (f.selector == safeTransferFrom(address,address,uint256,bytes).selector) { + bytes params; + require params.length < 0xffff; + safeTransferFrom@withrevert(e, from, to, tokenId, params); + } else { + calldataarg args; + f@withrevert(e, args); + } +} + +function helperMintWithRevert(env e, method f, address to, uint256 tokenId) { + if (f.selector == mint(address,uint256).selector) { + mint@withrevert(e, to, tokenId); + } else if (f.selector == safeMint(address,uint256).selector) { + safeMint@withrevert(e, to, tokenId); + } else if (f.selector == safeMint(address,uint256,bytes).selector) { + bytes params; + require params.length < 0xffff; + safeMint@withrevert(e, to, tokenId, params); + } else { + require false; + } +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Ghost & hooks: ownership count │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +ghost ownedTotal() returns uint256 { + init_state axiom ownedTotal() == 0; +} + +ghost mapping(address => uint256) ownedByUser { + init_state axiom forall address a. ownedByUser[a] == 0; +} + +hook Sstore _owners[KEY uint256 tokenId] address newOwner (address oldOwner) STORAGE { + ownedByUser[newOwner] = ownedByUser[newOwner] + to_uint256(newOwner != 0 ? 1 : 0); + ownedByUser[oldOwner] = ownedByUser[oldOwner] - to_uint256(oldOwner != 0 ? 1 : 0); + + havoc ownedTotal assuming ownedTotal@new() == ownedTotal@old() + + to_uint256(newOwner != 0 ? 1 : 0) + - to_uint256(oldOwner != 0 ? 1 : 0); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Ghost & hooks: sum of all balances │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +ghost sumOfBalances() returns uint256 { + init_state axiom sumOfBalances() == 0; +} + +hook Sstore _balances[KEY address addr] uint256 newValue (uint256 oldValue) STORAGE { + havoc sumOfBalances assuming sumOfBalances@new() == sumOfBalances@old() + newValue - oldValue; +} + +ghost mapping(address => uint256) ghostBalanceOf { + init_state axiom forall address a. ghostBalanceOf[a] == 0; +} + +hook Sload uint256 value _balances[KEY address user] STORAGE { + require ghostBalanceOf[user] == value; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: ownedTotal is the sum of all balances │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant ownedTotalIsSumOfBalances() + ownedTotal() == sumOfBalances() + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: balanceOf is the number of tokens owned │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant balanceOfConsistency(address user) + balanceOf(user) == ownedByUser[user] && + balanceOf(user) == ghostBalanceOf[user] + { + preserved { + require balanceLimited(user); + } + } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: owner of a token must have some balance │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant ownerHasBalance(uint256 tokenId) + balanceOf(ownerOf(tokenId)) > 0 + { + preserved { + requireInvariant balanceOfConsistency(ownerOf(tokenId)); + require balanceLimited(ownerOf(tokenId)); + } + } + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Invariant: tokens that do not exist are not owned and not approved │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +invariant notMintedUnset(uint256 tokenId) + (!tokenExists(tokenId) <=> unsafeOwnerOf(tokenId) == 0) && + (!tokenExists(tokenId) => unsafeGetApproved(tokenId) == 0) + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: ownerOf and getApproved revert if token does not exist │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule notMintedRevert(uint256 tokenId) { + requireInvariant notMintedUnset(tokenId); + + bool e = tokenExists(tokenId); + + address owner = ownerOf@withrevert(tokenId); + assert e <=> !lastReverted; + assert e => owner == unsafeOwnerOf(tokenId); // notMintedUnset tells us this is non-zero + + address approved = getApproved@withrevert(tokenId); + assert e <=> !lastReverted; + assert e => approved == unsafeGetApproved(tokenId); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: unsafeOwnerOf and unsafeGetApproved don't revert │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule unsafeDontRevert(uint256 tokenId) { + unsafeOwnerOf@withrevert(tokenId); + assert !lastReverted; + + unsafeGetApproved@withrevert(tokenId); + assert !lastReverted; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: balance of address(0) is 0 │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule zeroAddressBalanceRevert() { + balanceOf@withrevert(0); + assert lastReverted; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rules: total supply can only change through mint and burn │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule supplyChange(env e) { + uint256 supplyBefore = ownedTotal(); + method f; calldataarg args; f(e, args); + uint256 supplyAfter = ownedTotal(); + + assert supplyAfter > supplyBefore => ( + supplyAfter == supplyBefore + 1 && + ( + f.selector == mint(address,uint256).selector || + f.selector == safeMint(address,uint256).selector || + f.selector == safeMint(address,uint256,bytes).selector + ) + ); + assert supplyAfter < supplyBefore => ( + supplyAfter == supplyBefore - 1 && + f.selector == burn(uint256).selector + ); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rules: balanceOf can only change through mint, burn or transfers. balanceOf cannot change by more than 1. │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule balanceChange(env e, address account) { + requireInvariant balanceOfConsistency(account); + require balanceLimited(account); + + uint256 balanceBefore = balanceOf(account); + method f; calldataarg args; f(e, args); + uint256 balanceAfter = balanceOf(account); + + // balance can change by at most 1 + assert balanceBefore != balanceAfter => ( + balanceAfter == balanceBefore - 1 || + balanceAfter == balanceBefore + 1 + ); + + // only selected function can change balances + assert balanceBefore != balanceAfter => ( + f.selector == transferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256,bytes).selector || + f.selector == mint(address,uint256).selector || + f.selector == safeMint(address,uint256).selector || + f.selector == safeMint(address,uint256,bytes).selector || + f.selector == burn(uint256).selector + ); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rules: ownership can only change through mint, burn or transfers. │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule ownershipChange(env e, uint256 tokenId) { + address ownerBefore = unsafeOwnerOf(tokenId); + method f; calldataarg args; f(e, args); + address ownerAfter = unsafeOwnerOf(tokenId); + + assert ownerBefore == 0 && ownerAfter != 0 => ( + f.selector == mint(address,uint256).selector || + f.selector == safeMint(address,uint256).selector || + f.selector == safeMint(address,uint256,bytes).selector + ); + + assert ownerBefore != 0 && ownerAfter == 0 => ( + f.selector == burn(uint256).selector + ); + + assert (ownerBefore != ownerAfter && ownerBefore != 0 && ownerAfter != 0) => ( + f.selector == transferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256,bytes).selector + ); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rules: token approval can only change through approve or transfers (implicitly). │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule approvalChange(env e, uint256 tokenId) { + address approvalBefore = unsafeGetApproved(tokenId); + method f; calldataarg args; f(e, args); + address approvalAfter = unsafeGetApproved(tokenId); + + // approve can set any value, other functions reset + assert approvalBefore != approvalAfter => ( + f.selector == approve(address,uint256).selector || + ( + ( + f.selector == transferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256,bytes).selector || + f.selector == burn(uint256).selector + ) && approvalAfter == 0 + ) + ); +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rules: approval for all tokens can only change through isApprovedForAll. │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule approvedForAllChange(env e, address owner, address spender) { + bool approvedForAllBefore = isApprovedForAll(owner, spender); + method f; calldataarg args; f(e, args); + bool approvedForAllAfter = isApprovedForAll(owner, spender); + + assert approvedForAllBefore != approvedForAllAfter => f.selector == setApprovalForAll(address,bool).selector; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: transferFrom behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule transferFrom(env e, address from, address to, uint256 tokenId) { + require nonpayable(e); + + address operator = e.msg.sender; + uint256 otherTokenId; + address otherAccount; + + requireInvariant ownerHasBalance(tokenId); + require balanceLimited(to); + + uint256 balanceOfFromBefore = balanceOf(from); + uint256 balanceOfToBefore = balanceOf(to); + uint256 balanceOfOtherBefore = balanceOf(otherAccount); + address ownerBefore = unsafeOwnerOf(tokenId); + address otherOwnerBefore = unsafeOwnerOf(otherTokenId); + address approvalBefore = unsafeGetApproved(tokenId); + address otherApprovalBefore = unsafeGetApproved(otherTokenId); + + transferFrom@withrevert(e, from, to, tokenId); + bool success = !lastReverted; + + // liveness + assert success <=> ( + from == ownerBefore && + from != 0 && + to != 0 && + (operator == from || operator == approvalBefore || isApprovedForAll(ownerBefore, operator)) + ); + + // effect + assert success => ( + balanceOf(from) == balanceOfFromBefore - to_uint256(from != to ? 1 : 0) && + balanceOf(to) == balanceOfToBefore + to_uint256(from != to ? 1 : 0) && + unsafeOwnerOf(tokenId) == to && + unsafeGetApproved(tokenId) == 0 + ); + + // no side effect + assert balanceOf(otherAccount) != balanceOfOtherBefore => (otherAccount == from || otherAccount == to); + assert unsafeOwnerOf(otherTokenId) != otherOwnerBefore => otherTokenId == tokenId; + assert unsafeGetApproved(otherTokenId) != otherApprovalBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: safeTransferFrom behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule safeTransferFrom(env e, method f, address from, address to, uint256 tokenId) filtered { f -> + f.selector == safeTransferFrom(address,address,uint256).selector || + f.selector == safeTransferFrom(address,address,uint256,bytes).selector +} { + require nonpayable(e); + + address operator = e.msg.sender; + uint256 otherTokenId; + address otherAccount; + + requireInvariant ownerHasBalance(tokenId); + require balanceLimited(to); + + uint256 balanceOfFromBefore = balanceOf(from); + uint256 balanceOfToBefore = balanceOf(to); + uint256 balanceOfOtherBefore = balanceOf(otherAccount); + address ownerBefore = unsafeOwnerOf(tokenId); + address otherOwnerBefore = unsafeOwnerOf(otherTokenId); + address approvalBefore = unsafeGetApproved(tokenId); + address otherApprovalBefore = unsafeGetApproved(otherTokenId); + + helperTransferWithRevert(e, f, from, to, tokenId); + bool success = !lastReverted; + + assert success <=> ( + from == ownerBefore && + from != 0 && + to != 0 && + (operator == from || operator == approvalBefore || isApprovedForAll(ownerBefore, operator)) + ); + + // effect + assert success => ( + balanceOf(from) == balanceOfFromBefore - to_uint256(from != to ? 1: 0) && + balanceOf(to) == balanceOfToBefore + to_uint256(from != to ? 1: 0) && + unsafeOwnerOf(tokenId) == to && + unsafeGetApproved(tokenId) == 0 + ); + + // no side effect + assert balanceOf(otherAccount) != balanceOfOtherBefore => (otherAccount == from || otherAccount == to); + assert unsafeOwnerOf(otherTokenId) != otherOwnerBefore => otherTokenId == tokenId; + assert unsafeGetApproved(otherTokenId) != otherApprovalBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: mint behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule mint(env e, address to, uint256 tokenId) { + require nonpayable(e); + requireInvariant notMintedUnset(tokenId); + + uint256 otherTokenId; + address otherAccount; + + require balanceLimited(to); + + uint256 supplyBefore = ownedTotal(); + uint256 balanceOfToBefore = balanceOf(to); + uint256 balanceOfOtherBefore = balanceOf(otherAccount); + address ownerBefore = unsafeOwnerOf(tokenId); + address otherOwnerBefore = unsafeOwnerOf(otherTokenId); + + mint@withrevert(e, to, tokenId); + bool success = !lastReverted; + + // liveness + assert success <=> ( + ownerBefore == 0 && + to != 0 + ); + + // effect + assert success => ( + ownedTotal() == supplyBefore + 1 && + balanceOf(to) == balanceOfToBefore + 1 && + unsafeOwnerOf(tokenId) == to + ); + + // no side effect + assert balanceOf(otherAccount) != balanceOfOtherBefore => otherAccount == to; + assert unsafeOwnerOf(otherTokenId) != otherOwnerBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: safeMint behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule safeMint(env e, method f, address to, uint256 tokenId) filtered { f -> + f.selector == safeMint(address,uint256).selector || + f.selector == safeMint(address,uint256,bytes).selector +} { + require nonpayable(e); + requireInvariant notMintedUnset(tokenId); + + uint256 otherTokenId; + address otherAccount; + + require balanceLimited(to); + + uint256 supplyBefore = ownedTotal(); + uint256 balanceOfToBefore = balanceOf(to); + uint256 balanceOfOtherBefore = balanceOf(otherAccount); + address ownerBefore = unsafeOwnerOf(tokenId); + address otherOwnerBefore = unsafeOwnerOf(otherTokenId); + + helperMintWithRevert(e, f, to, tokenId); + bool success = !lastReverted; + + assert success <=> ( + ownerBefore == 0 && + to != 0 + ); + + // effect + assert success => ( + ownedTotal() == supplyBefore + 1 && + balanceOf(to) == balanceOfToBefore + 1 && + unsafeOwnerOf(tokenId) == to + ); + + // no side effect + assert balanceOf(otherAccount) != balanceOfOtherBefore => otherAccount == to; + assert unsafeOwnerOf(otherTokenId) != otherOwnerBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: burn behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule burn(env e, uint256 tokenId) { + require nonpayable(e); + + address from = unsafeOwnerOf(tokenId); + uint256 otherTokenId; + address otherAccount; + + requireInvariant ownerHasBalance(tokenId); + + uint256 supplyBefore = ownedTotal(); + uint256 balanceOfFromBefore = balanceOf(from); + uint256 balanceOfOtherBefore = balanceOf(otherAccount); + address ownerBefore = unsafeOwnerOf(tokenId); + address otherOwnerBefore = unsafeOwnerOf(otherTokenId); + address otherApprovalBefore = unsafeGetApproved(otherTokenId); + + burn@withrevert(e, tokenId); + bool success = !lastReverted; + + // liveness + assert success <=> ( + ownerBefore != 0 + ); + + // effect + assert success => ( + ownedTotal() == supplyBefore - 1 && + balanceOf(from) == balanceOfFromBefore - 1 && + unsafeOwnerOf(tokenId) == 0 && + unsafeGetApproved(tokenId) == 0 + ); + + // no side effect + assert balanceOf(otherAccount) != balanceOfOtherBefore => otherAccount == from; + assert unsafeOwnerOf(otherTokenId) != otherOwnerBefore => otherTokenId == tokenId; + assert unsafeGetApproved(otherTokenId) != otherApprovalBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: approve behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule approve(env e, address spender, uint256 tokenId) { + require nonpayable(e); + + address caller = e.msg.sender; + address owner = unsafeOwnerOf(tokenId); + uint256 otherTokenId; + + address otherApprovalBefore = unsafeGetApproved(otherTokenId); + + approve@withrevert(e, spender, tokenId); + bool success = !lastReverted; + + // liveness + assert success <=> ( + owner != 0 && + owner != spender && + (owner == caller || isApprovedForAll(owner, caller)) + ); + + // effect + assert success => unsafeGetApproved(tokenId) == spender; + + // no side effect + assert unsafeGetApproved(otherTokenId) != otherApprovalBefore => otherTokenId == tokenId; +} + +/* +┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ +│ Rule: setApprovalForAll behavior and side effects │ +└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ +*/ +rule setApprovalForAll(env e, address operator, bool approved) { + require nonpayable(e); + + address owner = e.msg.sender; + address otherOwner; + address otherOperator; + + bool otherIsApprovedForAllBefore = isApprovedForAll(otherOwner, otherOperator); + + setApprovalForAll@withrevert(e, operator, approved); + bool success = !lastReverted; + + // liveness + assert success <=> owner != operator; + + // effect + assert success => isApprovedForAll(owner, operator) == approved; + + // no side effect + assert isApprovedForAll(otherOwner, otherOperator) != otherIsApprovedForAllBefore => ( + otherOwner == owner && + otherOperator == operator + ); +} diff --git a/certora/specs/methods/IERC721.spec b/certora/specs/methods/IERC721.spec new file mode 100644 index 000000000..e6d4e1e04 --- /dev/null +++ b/certora/specs/methods/IERC721.spec @@ -0,0 +1,20 @@ +methods { + // IERC721 + balanceOf(address) returns (uint256) envfree => DISPATCHER(true) + ownerOf(uint256) returns (address) envfree => DISPATCHER(true) + getApproved(uint256) returns (address) envfree => DISPATCHER(true) + isApprovedForAll(address,address) returns (bool) envfree => DISPATCHER(true) + safeTransferFrom(address,address,uint256,bytes) => DISPATCHER(true) + safeTransferFrom(address,address,uint256) => DISPATCHER(true) + transferFrom(address,address,uint256) => DISPATCHER(true) + approve(address,uint256) => DISPATCHER(true) + setApprovalForAll(address,bool) => DISPATCHER(true) + + // IERC721Metadata + name() returns (string) => DISPATCHER(true) + symbol() returns (string) => DISPATCHER(true) + tokenURI(uint256) returns (string) => DISPATCHER(true) + + // IERC721Receiver + onERC721Received(address,address,uint256,bytes) returns (bytes4) => DISPATCHER(true) +} From 788d6a129a342d7a23a91ad553479e861845a9b2 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 12 Apr 2023 12:09:30 -0300 Subject: [PATCH 12/17] Add fuzz tests for ShortString (#4175) --- test/utils/ShortStrings.t.sol | 55 +++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 test/utils/ShortStrings.t.sol diff --git a/test/utils/ShortStrings.t.sol b/test/utils/ShortStrings.t.sol new file mode 100644 index 000000000..7c4faa89d --- /dev/null +++ b/test/utils/ShortStrings.t.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "forge-std/Test.sol"; + +import "../../contracts/utils/ShortStrings.sol"; + +contract ShortStringsTest is Test { + string _fallback; + + function testRoundtripShort(string memory input) external { + vm.assume(_isShort(input)); + ShortString short = ShortStrings.toShortString(input); + string memory output = ShortStrings.toString(short); + assertEq(input, output); + } + + function testRoundtripWithFallback(string memory input, string memory fallbackInitial) external { + _fallback = fallbackInitial; // Make sure that the initial value has no effect + ShortString short = ShortStrings.toShortStringWithFallback(input, _fallback); + string memory output = ShortStrings.toStringWithFallback(short, _fallback); + assertEq(input, output); + } + + function testRevertLong(string memory input) external { + vm.assume(!_isShort(input)); + vm.expectRevert(abi.encodeWithSelector(ShortStrings.StringTooLong.selector, input)); + this.toShortString(input); + } + + function testLengthShort(string memory input) external { + vm.assume(_isShort(input)); + uint256 inputLength = bytes(input).length; + ShortString short = ShortStrings.toShortString(input); + uint256 shortLength = ShortStrings.byteLength(short); + assertEq(inputLength, shortLength); + } + + function testLengthWithFallback(string memory input, string memory fallbackInitial) external { + _fallback = fallbackInitial; + uint256 inputLength = bytes(input).length; + ShortString short = ShortStrings.toShortStringWithFallback(input, _fallback); + uint256 shortLength = ShortStrings.byteLengthWithFallback(short, _fallback); + assertEq(inputLength, shortLength); + } + + function toShortString(string memory input) external pure returns (ShortString) { + return ShortStrings.toShortString(input); + } + + function _isShort(string memory input) internal pure returns (bool) { + return bytes(input).length < 32; + } +} From dd1265cb1dc834b129c6daf6aae7bdae28521d9e Mon Sep 17 00:00:00 2001 From: Pascal Marco Caversaccio Date: Wed, 12 Apr 2023 22:33:50 +0200 Subject: [PATCH 13/17] Improve `ERC4626` test coverage (#4134) Signed-off-by: Pascal Marco Caversaccio --- .../mocks/token/ERC20ExcessDecimalsMock.sol | 9 +++++ remappings.txt | 1 + test/token/ERC20/extensions/ERC4626.t.sol | 34 ++++++++++++++++--- test/token/ERC20/extensions/ERC4626.test.js | 23 +++++++++++++ 4 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 contracts/mocks/token/ERC20ExcessDecimalsMock.sol create mode 100644 remappings.txt diff --git a/contracts/mocks/token/ERC20ExcessDecimalsMock.sol b/contracts/mocks/token/ERC20ExcessDecimalsMock.sol new file mode 100644 index 000000000..0fb35a607 --- /dev/null +++ b/contracts/mocks/token/ERC20ExcessDecimalsMock.sol @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +contract ERC20ExcessDecimalsMock { + function decimals() public pure returns (uint256) { + return type(uint256).max; + } +} diff --git a/remappings.txt b/remappings.txt new file mode 100644 index 000000000..2479e3d26 --- /dev/null +++ b/remappings.txt @@ -0,0 +1 @@ +openzeppelin/=contracts/ diff --git a/test/token/ERC20/extensions/ERC4626.t.sol b/test/token/ERC20/extensions/ERC4626.t.sol index 95514531c..f220086d1 100644 --- a/test/token/ERC20/extensions/ERC4626.t.sol +++ b/test/token/ERC20/extensions/ERC4626.t.sol @@ -1,18 +1,42 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import "erc4626-tests/ERC4626.test.sol"; +import {ERC4626Test} from "erc4626-tests/ERC4626.test.sol"; -import {SafeCast} from "../../../../contracts/utils/math/SafeCast.sol"; -import {ERC20Mock} from "../../../../contracts/mocks/ERC20Mock.sol"; -import {ERC4626Mock} from "../../../../contracts/mocks/ERC4626Mock.sol"; +import {SafeCast} from "openzeppelin/utils/math/SafeCast.sol"; +import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol"; +import {ERC4626} from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; + +import {ERC20Mock} from "openzeppelin/mocks/ERC20Mock.sol"; +import {ERC4626Mock} from "openzeppelin/mocks/ERC4626Mock.sol"; +import {ERC4626OffsetMock} from "openzeppelin/mocks/token/ERC4626OffsetMock.sol"; + +contract ERC4626VaultOffsetMock is ERC4626OffsetMock { + constructor( + ERC20 underlying_, + uint8 offset_ + ) ERC20("My Token Vault", "MTKNV") ERC4626(underlying_) ERC4626OffsetMock(offset_) {} +} contract ERC4626StdTest is ERC4626Test { + ERC20 private _underlying = new ERC20Mock(); + function setUp() public override { - _underlying_ = address(new ERC20Mock()); + _underlying_ = address(_underlying); _vault_ = address(new ERC4626Mock(_underlying_)); _delta_ = 0; _vaultMayBeEmpty = true; _unlimitedAmount = true; } + + /** + * @dev Check the case where calculated `decimals` value overflows the `uint8` type. + */ + function testFuzzDecimalsOverflow(uint8 offset) public { + /// @dev Remember that the `_underlying` exhibits a `decimals` value of 18. + offset = uint8(bound(uint256(offset), 238, uint256(type(uint8).max))); + ERC4626VaultOffsetMock erc4626VaultOffsetMock = new ERC4626VaultOffsetMock(_underlying, offset); + vm.expectRevert(); + erc4626VaultOffsetMock.decimals(); + } } diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index c9e5a4098..ee0998717 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -5,6 +5,7 @@ const ERC20Decimals = artifacts.require('$ERC20DecimalsMock'); const ERC4626 = artifacts.require('$ERC4626'); const ERC4626OffsetMock = artifacts.require('$ERC4626OffsetMock'); const ERC4626FeesMock = artifacts.require('$ERC4626FeesMock'); +const ERC20ExcessDecimalsMock = artifacts.require('ERC20ExcessDecimalsMock'); contract('ERC4626', function (accounts) { const [holder, recipient, spender, other, user1, user2] = accounts; @@ -21,6 +22,28 @@ contract('ERC4626', function (accounts) { } }); + it('asset has not yet been created', async function () { + const vault = await ERC4626.new('', '', other); + expect(await vault.decimals()).to.be.bignumber.equal(decimals); + }); + + it('underlying excess decimals', async function () { + const token = await ERC20ExcessDecimalsMock.new(); + const vault = await ERC4626.new('', '', token.address); + expect(await vault.decimals()).to.be.bignumber.equal(decimals); + }); + + it('decimals overflow', async function () { + for (const offset of [243, 250, 255].map(web3.utils.toBN)) { + const token = await ERC20Decimals.new('', '', decimals); + const vault = await ERC4626OffsetMock.new(name + ' Vault', symbol + 'V', token.address, offset); + await expectRevert( + vault.decimals(), + 'reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)', + ); + } + }); + for (const offset of [0, 6, 18].map(web3.utils.toBN)) { const parseToken = token => web3.utils.toBN(10).pow(decimals).muln(token); const parseShare = share => web3.utils.toBN(10).pow(decimals.add(offset)).muln(share); From 3b117992e17ae67fcd19ea2bd988f64520813cd1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 13 Apr 2023 11:04:04 -0300 Subject: [PATCH 14/17] Improve docs for transparent proxy (#4181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/proxy/README.adoc | 2 ++ .../TransparentUpgradeableProxy.sol | 24 +++++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/contracts/proxy/README.adoc b/contracts/proxy/README.adoc index 5ada16e38..89717a7bf 100644 --- a/contracts/proxy/README.adoc +++ b/contracts/proxy/README.adoc @@ -56,6 +56,8 @@ The current implementation of this security mechanism uses https://eips.ethereum == ERC1967 +{{IERC1967}} + {{ERC1967Proxy}} {{ERC1967Upgrade}} diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index a89b233f2..d8765fc9e 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -6,10 +6,10 @@ pragma solidity ^0.8.0; import "../ERC1967/ERC1967Proxy.sol"; /** - * @dev Interface for the {TransparentUpgradeableProxy}. This is useful because {TransparentUpgradeableProxy} uses a - * custom call-routing mechanism, the compiler is unaware of the functions being exposed, and cannot list them. Also - * {TransparentUpgradeableProxy} does not inherit from this interface because it's implemented in a way that the - * compiler doesn't understand and cannot verify. + * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy} + * does not implement this interface directly, and some of its functions are implemented by an internal dispatch + * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not + * include them in the ABI so this interface must be used to interact with it. */ interface ITransparentUpgradeableProxy is IERC1967 { function admin() external view returns (address); @@ -44,12 +44,16 @@ interface ITransparentUpgradeableProxy is IERC1967 { * Our recommendation is for the dedicated account to be an instance of the {ProxyAdmin} contract. If set up this way, * you should think of the `ProxyAdmin` instance as the real administrative interface of your proxy. * - * WARNING: This contract does not inherit from {ITransparentUpgradeableProxy}, and the admin function is implicitly - * implemented using a custom call-routing mechanism in `_fallback`. Consequently, the compiler will not produce an - * ABI for this contract. Also, if you inherit from this contract and add additional functions, the compiler will not - * check that there are no selector conflicts. A selector clash between any new function and the functions declared in - * {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could render the admin operations - * inaccessible, which could prevent upgradeability. + * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not + * inherit from that interface, and instead the admin functions are implicitly implemented using a custom dispatch + * mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to + * fully implement transparency without decoding reverts caused by selector clashes between the proxy and the + * implementation. + * + * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler + * will not check that there are no selector conflicts, due to the note above. A selector clash between any new function + * and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could + * render the admin operations inaccessible, which could prevent upgradeability. Transparency may also be compromised. */ contract TransparentUpgradeableProxy is ERC1967Proxy { /** From 8d633cb7d169f2f8595b273660b00b69e845c2fe Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Apr 2023 20:47:51 +0200 Subject: [PATCH 15/17] Merge pull request from GHSA-93hq-5wgc-jc82 Co-authored-by: Francisco --- .changeset/silent-pugs-scream.md | 5 +++++ .../compatibility/GovernorCompatibilityBravo.sol | 4 ++-- .../GovernorCompatibilityBravo.test.js | 15 +++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 .changeset/silent-pugs-scream.md diff --git a/.changeset/silent-pugs-scream.md b/.changeset/silent-pugs-scream.md new file mode 100644 index 000000000..c92d12486 --- /dev/null +++ b/.changeset/silent-pugs-scream.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`GovernorCompatibilityBravo`: Fix encoding of proposal data when signatures are missing. diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 25f404403..7fd8ce0c4 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -70,6 +70,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { + require(signatures.length == calldatas.length, "GovernorBravo: invalid signatures length"); // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code @@ -149,8 +150,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp bytes[] memory calldatas ) private pure returns (bytes[] memory) { bytes[] memory fullcalldatas = new bytes[](calldatas.length); - - for (uint256 i = 0; i < signatures.length; ++i) { + for (uint256 i = 0; i < fullcalldatas.length; ++i) { fullcalldatas[i] = bytes(signatures[i]).length == 0 ? calldatas[i] : abi.encodePacked(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 9e551949f..5a8104a98 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -224,6 +224,21 @@ contract('GovernorCompatibilityBravo', function (accounts) { }); }); + it('with inconsistent array size for selector and arguments', async function () { + const target = this.receiver.address; + this.helper.setProposal( + { + targets: [target, target], + values: [0, 0], + signatures: ['mockFunction()'], // One signature + data: ['0x', this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI()], // Two data entries + }, + '', + ); + + await expectRevert(this.helper.propose({ from: proposer }), 'GovernorBravo: invalid signatures length'); + }); + describe('should revert', function () { describe('on propose', function () { it('if proposal does not meet proposalThreshold', async function () { From 91df66c4a9dfd0425ff923cbeb3a20155f1355ea Mon Sep 17 00:00:00 2001 From: Francisco Date: Fri, 21 Apr 2023 12:35:07 +0100 Subject: [PATCH 16/17] Implement suggestions from audit of 4.9 (#4176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/governance/Governor.sol | 54 +++++++++++-------- contracts/governance/IGovernor.sol | 8 ++- contracts/governance/TimelockController.sol | 25 +++++---- .../GovernorCompatibilityBravo.sol | 10 ++-- .../IGovernorCompatibilityBravo.sol | 5 ++ .../extensions/GovernorPreventLateQuorum.sol | 12 ++--- .../extensions/GovernorTimelockCompound.sol | 14 +++-- .../extensions/GovernorTimelockControl.sol | 10 ++-- .../GovernorVotesQuorumFraction.sol | 7 ++- contracts/governance/utils/IVotes.sol | 4 +- contracts/governance/utils/Votes.sol | 6 +-- .../token/ERC20/extensions/ERC20Votes.sol | 2 +- .../token/ERC721/extensions/ERC721Votes.sol | 2 + test/governance/Governor.test.js | 6 +-- .../SupportsInterface.behavior.js | 8 ++- 15 files changed, 97 insertions(+), 76 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index a2911bf98..241d6139b 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -17,7 +17,7 @@ import "./IGovernor.sol"; /** * @dev Core of the governance system, designed to be extended though various modules. * - * This contract is abstract and requires several function to be implemented in various modules: + * This contract is abstract and requires several functions to be implemented in various modules: * * - A counting module must implement {quorum}, {_quorumReached}, {_voteSucceeded} and {_countVote} * - A voting module must implement {_getVotes} @@ -27,7 +27,6 @@ import "./IGovernor.sol"; */ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver { using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; - using SafeCast for uint256; bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); bytes32 public constant EXTENDED_BALLOT_TYPEHASH = @@ -90,25 +89,34 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive * @dev Function to receive ETH that will be handled by the governor (disabled if executor is a third party contract) */ receive() external payable virtual { - require(_executor() == address(this)); + require(_executor() == address(this), "Governor: must send to executor"); } /** * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { - // In addition to the current interfaceId, also support previous version of the interfaceId that did not - // include the castVoteWithReasonAndParams() function as standard + bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector; + + bytes4 governorParamsId = this.castVoteWithReasonAndParams.selector ^ + this.castVoteWithReasonAndParamsBySig.selector ^ + this.getVotesWithParams.selector; + + // The original interface id in v4.3. + bytes4 governor43Id = type(IGovernor).interfaceId ^ + type(IERC6372).interfaceId ^ + governorCancelId ^ + governorParamsId; + + // An updated interface id in v4.6, with params added. + bytes4 governor46Id = type(IGovernor).interfaceId ^ type(IERC6372).interfaceId ^ governorCancelId; + + // For the updated interface id in v4.9, we use governorCancelId directly. + return - interfaceId == - (type(IGovernor).interfaceId ^ - type(IERC6372).interfaceId ^ - this.cancel.selector ^ - this.castVoteWithReasonAndParams.selector ^ - this.castVoteWithReasonAndParamsBySig.selector ^ - this.getVotesWithParams.selector) || - // Previous interface for backwards compatibility - interfaceId == (type(IGovernor).interfaceId ^ type(IERC6372).interfaceId ^ this.cancel.selector) || + interfaceId == governor43Id || + interfaceId == governor46Id || + interfaceId == governorCancelId || interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); } @@ -210,9 +218,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive } /** - * @dev Address of the proposer + * @dev Returns the account that created a given proposal. */ - function _proposalProposer(uint256 proposalId) internal view virtual returns (address) { + function proposalProposer(uint256 proposalId) public view virtual override returns (address) { return _proposals[proposalId].proposer; } @@ -283,8 +291,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive _proposals[proposalId] = ProposalCore({ proposer: proposer, - voteStart: snapshot.toUint64(), - voteEnd: deadline.toUint64(), + voteStart: SafeCast.toUint64(snapshot), + voteEnd: SafeCast.toUint64(deadline), executed: false, canceled: false, __gap_unused0: 0, @@ -317,9 +325,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive ) public payable virtual override returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - ProposalState status = state(proposalId); + ProposalState currentState = state(proposalId); require( - status == ProposalState.Succeeded || status == ProposalState.Queued, + currentState == ProposalState.Succeeded || currentState == ProposalState.Queued, "Governor: proposal not successful" ); _proposals[proposalId].executed = true; @@ -415,10 +423,12 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive ) internal virtual returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - ProposalState status = state(proposalId); + ProposalState currentState = state(proposalId); require( - status != ProposalState.Canceled && status != ProposalState.Expired && status != ProposalState.Executed, + currentState != ProposalState.Canceled && + currentState != ProposalState.Expired && + currentState != ProposalState.Executed, "Governor: proposal not active" ); _proposals[proposalId].canceled = true; diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 70f81efe8..e4ad83e87 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -152,6 +152,12 @@ abstract contract IGovernor is IERC165, IERC6372 { */ function proposalDeadline(uint256 proposalId) public view virtual returns (uint256); + /** + * @notice module:core + * @dev The account that created a proposal. + */ + function proposalProposer(uint256 proposalId) public view virtual returns (address); + /** * @notice module:user-config * @dev Delay, between the proposal is created and the vote starts. The unit this duration is expressed in depends @@ -164,7 +170,7 @@ abstract contract IGovernor is IERC165, IERC6372 { /** * @notice module:user-config - * @dev Delay, between the vote start and vote ends. The unit this duration is expressed in depends on the clock + * @dev Delay between the vote start and vote end. The unit this duration is expressed in depends on the clock * (see EIP-6372) this contract uses. * * NOTE: The {votingDelay} can delay the start of the vote. This must be considered when setting the voting diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 18ca81e5f..e330cca06 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -6,7 +6,6 @@ pragma solidity ^0.8.0; import "../access/AccessControl.sol"; import "../token/ERC721/IERC721Receiver.sol"; import "../token/ERC1155/IERC1155Receiver.sol"; -import "../utils/Address.sol"; /** * @dev Contract module which acts as a timelocked controller. When set as the @@ -137,21 +136,21 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * @dev Returns whether an id correspond to a registered operation. This * includes both Pending, Ready and Done operations. */ - function isOperation(bytes32 id) public view virtual returns (bool registered) { + function isOperation(bytes32 id) public view virtual returns (bool) { return getTimestamp(id) > 0; } /** - * @dev Returns whether an operation is pending or not. + * @dev Returns whether an operation is pending or not. Note that a "pending" operation may also be "ready". */ - function isOperationPending(bytes32 id) public view virtual returns (bool pending) { + function isOperationPending(bytes32 id) public view virtual returns (bool) { return getTimestamp(id) > _DONE_TIMESTAMP; } /** - * @dev Returns whether an operation is ready or not. + * @dev Returns whether an operation is ready for execution. Note that a "ready" operation is also "pending". */ - function isOperationReady(bytes32 id) public view virtual returns (bool ready) { + function isOperationReady(bytes32 id) public view virtual returns (bool) { uint256 timestamp = getTimestamp(id); return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp; } @@ -159,7 +158,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver /** * @dev Returns whether an operation is done or not. */ - function isOperationDone(bytes32 id) public view virtual returns (bool done) { + function isOperationDone(bytes32 id) public view virtual returns (bool) { return getTimestamp(id) == _DONE_TIMESTAMP; } @@ -167,7 +166,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * @dev Returns the timestamp at which an operation becomes ready (0 for * unset operations, 1 for done operations). */ - function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) { + function getTimestamp(bytes32 id) public view virtual returns (uint256) { return _timestamps[id]; } @@ -176,7 +175,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * * This value can be changed by executing an operation that calls `updateDelay`. */ - function getMinDelay() public view virtual returns (uint256 duration) { + function getMinDelay() public view virtual returns (uint256) { return _minDelay; } @@ -190,7 +189,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver bytes calldata data, bytes32 predecessor, bytes32 salt - ) public pure virtual returns (bytes32 hash) { + ) public pure virtual returns (bytes32) { return keccak256(abi.encode(target, value, data, predecessor, salt)); } @@ -204,14 +203,14 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver bytes[] calldata payloads, bytes32 predecessor, bytes32 salt - ) public pure virtual returns (bytes32 hash) { + ) public pure virtual returns (bytes32) { return keccak256(abi.encode(targets, values, payloads, predecessor, salt)); } /** * @dev Schedule an operation containing a single transaction. * - * Emits events {CallScheduled} and {CallSalt}. + * Emits {CallSalt} if salt is nonzero, and {CallScheduled}. * * Requirements: * @@ -236,7 +235,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver /** * @dev Schedule an operation containing a batch of transactions. * - * Emits a {CallSalt} event and one {CallScheduled} event per transaction in the batch. + * Emits {CallSalt} if salt is nonzero, and one {CallScheduled} event per transaction in the batch. * * Requirements: * diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 7fd8ce0c4..0fbb4e17c 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -74,7 +74,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code - // is added their is also executed when calling this alternative interface. + // is added there is also executed when calling this alternative interface. _storeProposal(_msgSender(), targets, values, signatures, calldatas, description); return propose(targets, values, _encodeCalldata(signatures, calldatas), description); } @@ -110,7 +110,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp /** * @dev Cancel a proposal with GovernorBravo logic. */ - function cancel(uint256 proposalId) public virtual { + function cancel(uint256 proposalId) public virtual override { ( address[] memory targets, uint256[] memory values, @@ -238,9 +238,9 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp againstVotes = details.againstVotes; abstainVotes = details.abstainVotes; - ProposalState status = state(proposalId); - canceled = status == ProposalState.Canceled; - executed = status == ProposalState.Executed; + ProposalState currentState = state(proposalId); + canceled = currentState == ProposalState.Canceled; + executed = currentState == ProposalState.Executed; } /** diff --git a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol index 159c55100..7aa806a18 100644 --- a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol @@ -90,6 +90,11 @@ abstract contract IGovernorCompatibilityBravo is IGovernor { */ function execute(uint256 proposalId) public payable virtual; + /** + * @dev Cancels a proposal only if the sender is the proposer or the proposer delegates' voting power dropped below the proposal threshold. + */ + function cancel(uint256 proposalId) public virtual; + /** * @dev Part of the Governor Bravo's interface: _"Gets actions of a proposal"_. */ diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index 676d2b13e..68496ca1e 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -12,14 +12,12 @@ import "../../utils/math/Math.sol"; * and try to oppose the decision. * * If a vote causes quorum to be reached, the proposal's voting period may be extended so that it does not end before at - * least a given number of blocks have passed (the "vote extension" parameter). This parameter can be set by the - * governance executor (e.g. through a governance proposal). + * least a specified time has passed (the "vote extension" parameter). This parameter can be set through a governance + * proposal. * * _Available since v4.5._ */ abstract contract GovernorPreventLateQuorum is Governor { - using SafeCast for uint256; - uint64 private _voteExtension; /// @custom:oz-retyped-from mapping(uint256 => Timers.BlockNumber) @@ -32,9 +30,9 @@ abstract contract GovernorPreventLateQuorum is Governor { event LateQuorumVoteExtensionSet(uint64 oldVoteExtension, uint64 newVoteExtension); /** - * @dev Initializes the vote extension parameter: the number of blocks that are required to pass since a proposal - * reaches quorum until its voting period ends. If necessary the voting period will be extended beyond the one set - * at proposal creation. + * @dev Initializes the vote extension parameter: the time in either number of blocks or seconds (depending on the governor + * clock mode) that is required to pass since the moment a proposal reaches quorum until its voting period ends. If + * necessary the voting period will be extended beyond the one set during proposal creation. */ constructor(uint64 initialVoteExtension) { _setLateQuorumVoteExtension(initialVoteExtension); diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 629f8f800..912171cc3 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -21,8 +21,6 @@ import "../../vendor/compound/ICompoundTimelock.sol"; * _Available since v4.3._ */ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { - using SafeCast for uint256; - ICompoundTimelock private _timelock; /// @custom:oz-retyped-from mapping(uint256 => GovernorTimelockCompound.ProposalTimelock) @@ -48,18 +46,18 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { } /** - * @dev Overridden version of the {Governor-state} function with added support for the `Queued` and `Expired` status. + * @dev Overridden version of the {Governor-state} function with added support for the `Queued` and `Expired` state. */ function state(uint256 proposalId) public view virtual override(IGovernor, Governor) returns (ProposalState) { - ProposalState status = super.state(proposalId); + ProposalState currentState = super.state(proposalId); - if (status != ProposalState.Succeeded) { - return status; + if (currentState != ProposalState.Succeeded) { + return currentState; } uint256 eta = proposalEta(proposalId); if (eta == 0) { - return status; + return currentState; } else if (block.timestamp >= eta + _timelock.GRACE_PERIOD()) { return ProposalState.Expired; } else { @@ -95,7 +93,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { require(state(proposalId) == ProposalState.Succeeded, "Governor: proposal not successful"); uint256 eta = block.timestamp + _timelock.delay(); - _proposalTimelocks[proposalId] = eta.toUint64(); + _proposalTimelocks[proposalId] = SafeCast.toUint64(eta); for (uint256 i = 0; i < targets.length; ++i) { require( diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 6aa2556ab..0cf2ea5f0 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -47,19 +47,19 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { } /** - * @dev Overridden version of the {Governor-state} function with added support for the `Queued` status. + * @dev Overridden version of the {Governor-state} function with added support for the `Queued` state. */ function state(uint256 proposalId) public view virtual override(IGovernor, Governor) returns (ProposalState) { - ProposalState status = super.state(proposalId); + ProposalState currentState = super.state(proposalId); - if (status != ProposalState.Succeeded) { - return status; + if (currentState != ProposalState.Succeeded) { + return currentState; } // core tracks execution, so we just have to check if successful proposal have been queued. bytes32 queueid = _timelockIds[proposalId]; if (queueid == bytes32(0)) { - return status; + return currentState; } else if (_timelock.isOperationDone(queueid)) { return ProposalState.Executed; } else if (_timelock.isOperationPending(queueid)) { diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 19c5b194d..403570260 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -14,7 +14,6 @@ import "../../utils/math/SafeCast.sol"; * _Available since v4.3._ */ abstract contract GovernorVotesQuorumFraction is GovernorVotes { - using SafeCast for *; using Checkpoints for Checkpoints.Trace224; uint256 private _quorumNumerator; // DEPRECATED in favor of _quorumNumeratorHistory @@ -59,7 +58,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } // Otherwise, do the binary search - return _quorumNumeratorHistory.upperLookupRecent(timepoint.toUint32()); + return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint)); } /** @@ -110,12 +109,12 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { _quorumNumeratorHistory._checkpoints.push( - Checkpoints.Checkpoint224({_key: 0, _value: oldQuorumNumerator.toUint224()}) + Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)}) ); } // Set new quorum for future proposals - _quorumNumeratorHistory.push(clock().toUint32(), newQuorumNumerator.toUint224()); + _quorumNumeratorHistory.push(SafeCast.toUint32(clock()), SafeCast.toUint224(newQuorumNumerator)); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/contracts/governance/utils/IVotes.sol b/contracts/governance/utils/IVotes.sol index c73d0732b..4f2b7eb85 100644 --- a/contracts/governance/utils/IVotes.sol +++ b/contracts/governance/utils/IVotes.sol @@ -25,13 +25,13 @@ interface IVotes { /** * @dev Returns the amount of votes that `account` had at a specific moment in the past. If the `clock()` is - * configured to use block numbers, this will return the value the end of the corresponding block. + * configured to use block numbers, this will return the value at the end of the corresponding block. */ function getPastVotes(address account, uint256 timepoint) external view returns (uint256); /** * @dev Returns the total supply of votes available at a specific moment in the past. If the `clock()` is - * configured to use block numbers, this will return the value the end of the corresponding block. + * configured to use block numbers, this will return the value at the end of the corresponding block. * * NOTE: This value is the sum of all available votes, which is not necessarily the sum of all delegated votes. * Votes that have not been delegated are still part of total supply, even though they would not participate in a diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index f70cf3831..b24ce824a 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -59,7 +59,7 @@ abstract contract Votes is Context, EIP712, IERC5805 { // solhint-disable-next-line func-name-mixedcase function CLOCK_MODE() public view virtual override returns (string memory) { // Check that the clock was not modified - require(clock() == block.number); + require(clock() == block.number, "Votes: broken clock mode"); return "mode=blocknumber&from=default"; } @@ -72,7 +72,7 @@ abstract contract Votes is Context, EIP712, IERC5805 { /** * @dev Returns the amount of votes that `account` had at a specific moment in the past. If the `clock()` is - * configured to use block numbers, this will return the value the end of the corresponding block. + * configured to use block numbers, this will return the value at the end of the corresponding block. * * Requirements: * @@ -85,7 +85,7 @@ abstract contract Votes is Context, EIP712, IERC5805 { /** * @dev Returns the total supply of votes available at a specific moment in the past. If the `clock()` is - * configured to use block numbers, this will return the value the end of the corresponding block. + * configured to use block numbers, this will return the value at the end of the corresponding block. * * NOTE: This value is the sum of all available votes, which is not necessarily the sum of all delegated votes. * Votes that have not been delegated are still part of total supply, even though they would not participate in a diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index f78938e6d..d72d34f81 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -50,7 +50,7 @@ abstract contract ERC20Votes is ERC20Permit, IERC5805 { // solhint-disable-next-line func-name-mixedcase function CLOCK_MODE() public view virtual override returns (string memory) { // Check that the clock was not modified - require(clock() == block.number); + require(clock() == block.number, "ERC20Votes: broken clock mode"); return "mode=blocknumber&from=default"; } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 8e6500ec3..31397f107 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -34,6 +34,8 @@ abstract contract ERC721Votes is ERC721, Votes { /** * @dev Returns the balance of `account`. + * + * WARNING: Overriding this function will likely result in incorrect vote tracking. */ function _getVotingUnits(address account) internal view virtual override returns (uint256) { return balanceOf(account); diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 1eabaccf0..f867958b5 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -70,7 +70,7 @@ contract('Governor', function (accounts) { ); }); - shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor', 'GovernorWithParams']); + shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor', 'GovernorWithParams', 'GovernorCancel']); shouldBehaveLikeEIP6372(mode); it('deployment check', async function () { @@ -84,7 +84,7 @@ contract('Governor', function (accounts) { it('nominal workflow', async function () { // Before - expect(await this.mock.$_proposalProposer(this.proposal.id)).to.be.equal(constants.ZERO_ADDRESS); + expect(await this.mock.proposalProposer(this.proposal.id)).to.be.equal(constants.ZERO_ADDRESS); expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false); expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false); expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false); @@ -149,7 +149,7 @@ contract('Governor', function (accounts) { await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalled'); // After - expect(await this.mock.$_proposalProposer(this.proposal.id)).to.be.equal(proposer); + expect(await this.mock.proposalProposer(this.proposal.id)).to.be.equal(proposer); expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false); expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(true); expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(true); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 541f6c611..201a55f47 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -90,6 +90,7 @@ const INTERFACES = { 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', 'castVoteWithReasonAndParamsBySig(uint256,uint8,string,bytes,uint8,bytes32,bytes32)', ], + GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'], GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'], ERC2981: ['royaltyInfo(uint256,uint256)'], }; @@ -120,7 +121,7 @@ function shouldSupportInterfaces(interfaces = []) { it('all interfaces are reported as supported', async function () { for (const k of interfaces) { const interfaceId = INTERFACE_IDS[k] ?? k; - expect(await this.contractUnderTest.supportsInterface(interfaceId)).to.equal(true); + expect(await this.contractUnderTest.supportsInterface(interfaceId)).to.equal(true, `does not support ${k}`); } }); @@ -130,7 +131,10 @@ function shouldSupportInterfaces(interfaces = []) { if (INTERFACES[k] === undefined) continue; for (const fnName of INTERFACES[k]) { const fnSig = FN_SIGNATURES[fnName]; - expect(this.contractUnderTest.abi.filter(fn => fn.signature === fnSig).length).to.equal(1); + expect(this.contractUnderTest.abi.filter(fn => fn.signature === fnSig).length).to.equal( + 1, + `did not find ${fnName}`, + ); } } }); From 6aac66d065dc39795fbad1f680f80e3a3d70f2d1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 24 Apr 2023 13:18:27 +0100 Subject: [PATCH 17/17] Merge release-v4.8 (#4188) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Hadrien Croubois Co-authored-by: Benjamin Co-authored-by: Owen Co-authored-by: Hadrien Croubois Co-authored-by: JulissaDantes Co-authored-by: Ernesto García Co-authored-by: Yamen Merhi Co-authored-by: Pascal Marco Caversaccio Co-authored-by: alpharush <0xalpharush@protonmail.com> Co-authored-by: Paul Razvan Berg --- .changeset/silent-pugs-scream.md | 5 ----- .changeset/thirty-shrimps-mix.md | 5 ----- CHANGELOG.md | 5 +++++ .../governance/compatibility/GovernorCompatibilityBravo.sol | 2 +- contracts/proxy/ERC1967/ERC1967Upgrade.sol | 2 +- contracts/proxy/transparent/ProxyAdmin.sol | 2 +- contracts/proxy/transparent/TransparentUpgradeableProxy.sol | 2 +- 7 files changed, 9 insertions(+), 14 deletions(-) delete mode 100644 .changeset/silent-pugs-scream.md delete mode 100644 .changeset/thirty-shrimps-mix.md diff --git a/.changeset/silent-pugs-scream.md b/.changeset/silent-pugs-scream.md deleted file mode 100644 index c92d12486..000000000 --- a/.changeset/silent-pugs-scream.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'openzeppelin-solidity': patch ---- - -`GovernorCompatibilityBravo`: Fix encoding of proposal data when signatures are missing. diff --git a/.changeset/thirty-shrimps-mix.md b/.changeset/thirty-shrimps-mix.md deleted file mode 100644 index 656edbf92..000000000 --- a/.changeset/thirty-shrimps-mix.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'openzeppelin-solidity': patch ---- - -`TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata or payable mutability. diff --git a/CHANGELOG.md b/CHANGELOG.md index 333b6d4a3..ffbd3acea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ - `ERC777`: The `ERC777` token standard is no longer supported by OpenZeppelin. Our implementation is now deprecated and will be removed in the next major release. The corresponding standard interfaces remain available. ([#4066](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4066)) - `ERC1820Implementer`: The `ERC1820` pseudo-introspection mechanism is no longer supported by OpenZeppelin. Our implementation is now deprecated and will be removed in the next major release. The corresponding standard interfaces remain available. ([#4066](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4066)) +## 4.8.3 (2023-04-13) + +- `GovernorCompatibilityBravo`: Fix encoding of proposal data when signatures are missing. +- `TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata or payable mutability. ([#4154](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4154)) + ## 4.8.2 (2023-03-02) - `ERC721Consecutive`: Fixed a bug when `_mintConsecutive` is used for batches of size 1 that could lead to balance overflow. Refer to the breaking changes section in the changelog for a note on the behavior of `ERC721._beforeTokenTransfer`. diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 0fbb4e17c..1332ac79d 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.8.0) (governance/compatibility/GovernorCompatibilityBravo.sol) +// OpenZeppelin Contracts (last updated v4.8.3) (governance/compatibility/GovernorCompatibilityBravo.sol) pragma solidity ^0.8.0; diff --git a/contracts/proxy/ERC1967/ERC1967Upgrade.sol b/contracts/proxy/ERC1967/ERC1967Upgrade.sol index a79c10502..3942ca699 100644 --- a/contracts/proxy/ERC1967/ERC1967Upgrade.sol +++ b/contracts/proxy/ERC1967/ERC1967Upgrade.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.5.0) (proxy/ERC1967/ERC1967Upgrade.sol) +// OpenZeppelin Contracts (last updated v4.8.3) (proxy/ERC1967/ERC1967Upgrade.sol) pragma solidity ^0.8.2; diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 1a9698196..571530595 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.1 (proxy/transparent/ProxyAdmin.sol) +// OpenZeppelin Contracts (last updated v4.8.3) (proxy/transparent/ProxyAdmin.sol) pragma solidity ^0.8.0; diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index d8765fc9e..4e2b0c759 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.7.0) (proxy/transparent/TransparentUpgradeableProxy.sol) +// OpenZeppelin Contracts (last updated v4.8.3) (proxy/transparent/TransparentUpgradeableProxy.sol) pragma solidity ^0.8.0;