Improve AccessManager tests (#4613)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>

contracts/access/manager/AccessManager.sol

@@ -13,23 +13,32 @@ import {Time} from "../../utils/types/Time.sol";
 /**
  * @dev AccessManager is a central contract to store the permissions of a system.
  *
- * The smart contracts under the control of an AccessManager instance will have a set of "restricted" functions, and the
+ * A smart contract under the control of an AccessManager instance is known as a target, and will inherit from the
- * exact details of how access is restricted for each of those functions is configurable by the admins of the instance.
+ * {AccessManaged} contract, be connected to this contract as its manager and implement the {AccessManaged-restricted}
- * These restrictions are expressed in terms of "roles".
+ * modifier on a set of functions selected to be permissioned. Note that any function without this setup won't be
+ * effectively restricted.
  *
- * An AccessManager instance will define a set of roles. Accounts can be added into any number of these roles. Each of
+ * The restriction rules for such functions are defined in terms of "roles" identified by an `uint64` and scoped
- * them defines a role, and may confer access to some of the restricted functions in the system, as configured by admins
+ * by target (`address`) and function selectors (`bytes4`). These roles are stored in this contract and can be
- * through the use of {setFunctionAllowedRoles}.
+ * configured by admins (`ADMIN_ROLE` members) after a delay (see {getTargetAdminDelay}).
  *
- * Note that a function in a target contract may become permissioned in this way only when: 1) said contract is
+ * For each target contract, admins can configure the following without any delay:
- * {AccessManaged} and is connected to this contract as its manager, and 2) said function is decorated with the
- * `restricted` modifier.
  *
- * There is a special role defined by default named "public" which all accounts automatically have.
+ * * The target's {AccessManaged-authority} via {updateAuthority}.
+ * * Close or open a target via {setTargetClosed} keeping the permissions intact.
+ * * The roles that are allowed (or disallowed) to call a given function (identified by its selector) through {setTargetFunctionRole}.
  *
- * In addition to the access rules defined by each target's functions being assigned to roles, then entire target can
+ * By default every address is member of the `PUBLIC_ROLE` and every target function is restricted to the `ADMIN_ROLE` until configured otherwise.
- * be "closed". This "closed" mode is set/unset by the admin using {setTargetClosed} and can be used to lock a contract
+ * Additionally, each role has the following configuration options restricted to this manager's admins:
- * while permissions are being (re-)configured.
+ *
+ * * A role's admin role via {setRoleAdmin} who can grant or revoke roles.
+ * * A role's guardian role via {setRoleGuardian} who's allowed to cancel operations.
+ * * A delay in which a role takes effect after being granted through {setGrantDelay}.
+ * * A delay of any target's admin action via {setTargetAdminDelay}.
+ * * A role label for discoverability purposes with {labelRole}.
+ *
+ * Any account can be added and removed into any number of these roles by using the {grantRole} and {revokeRole} functions
+ * restricted to each role's admin (see {getRoleAdmin}).
  *
  * Since all the permissions of the managed system can be modified by the admins of this instance, it is expected that
  * they will be highly secured (e.g., a multisig or a well-configured DAO).
@@ -60,28 +69,30 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     // Structure that stores the details for a role/account pair. This structures fit into a single slot.
     struct Access {
-        // Timepoint at which the user gets the permission. If this is either 0, or in the future, the role
+        // Timepoint at which the user gets the permission.
-        // permission is not available.
+        // If this is either 0 or in the future, then the role permission is not available.
         uint48 since;
         // Delay for execution. Only applies to restricted() / execute() calls.
         Time.Delay delay;
     }
 
-    // Structure that stores the details of a role, including:
+    // Structure that stores the details of a role.
-    // - the members of the role
-    // - the admin role (that can grant or revoke permissions)
-    // - the guardian role (that can cancel operations targeting functions that need this role)
-    // - the grand delay
     struct Role {
+        // Members of the role.
         mapping(address user => Access access) members;
+        // Admin who can grant or revoke permissions.
         uint64 admin;
+        // Guardian who can cancel operations targeting functions that need this role.
         uint64 guardian;
+        // Delay in which the role takes effect after being granted.
         Time.Delay grantDelay;
     }
 
     // Structure that stores the details for a scheduled operation. This structure fits into a single slot.
     struct Schedule {
+        // Moment at which the operation can be executed.
         uint48 timepoint;
+        // Operation nonce to allow third-party contracts to identify the operation.
         uint32 nonce;
     }
 
@@ -92,6 +103,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     mapping(uint64 roleId => Role) private _roles;
     mapping(bytes32 operationId => Schedule) private _schedules;
 
+    // Used to identify operations that are currently being executed via {execute}.
     // This should be transient storage when supported by the EVM.
     bytes32 private _executionId;
 
@@ -120,15 +132,19 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * & {execute} workflow.
      *
      * This function is usually called by the targeted contract to control immediate execution of restricted functions.
-     * Therefore we only return true is the call can be performed without any delay. If the call is subject to a delay,
+     * Therefore we only return true if the call can be performed without any delay. If the call is subject to a
-     * then the function should return false, and the caller should schedule the operation for future execution.
+     * previously set delay (not zero), then the function should return false and the caller should schedule the operation
+     * for future execution.
      *
-     * We may be able to hash the operation, and check if the call was scheduled, but we would not be able to cleanup
+     * If `immediate` is true, the delay can be disregarded and the operation can be immediately executed, otherwise
-     * the schedule, leaving the possibility of multiple executions. Maybe this function should not be view?
+     * the operation can be executed if and only if delay is greater than 0.
      *
      * NOTE: The IAuthority interface does not include the `uint32` delay. This is an extension of that interface that
      * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail
      * to identify the indirect workflow, and will consider calls that require a delay to be forbidden.
+     *
+     * NOTE: This function does not report the permissions of this manager itself. These are defined by the
+     * {_canCallSelf} function instead.
      */
     function canCall(
         address caller,
@@ -150,13 +166,16 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     /**
      * @dev Expiration delay for scheduled proposals. Defaults to 1 week.
+     *
+     * IMPORTANT: Avoid overriding the expiration with 0. Otherwise every contract proposal will be expired immediately,
+     * disabling any scheduling usage.
      */
     function expiration() public view virtual returns (uint32) {
         return 1 weeks;
     }
 
     /**
-     * @dev Minimum setback for all delay updates, with the exception of execution delays, which
+     * @dev Minimum setback for all delay updates, with the exception of execution delays. It
      * can be increased without setback (and in the event of an accidental increase can be reset
      * via {revokeRole}). Defaults to 5 days.
      */
@@ -165,7 +184,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Get the mode under which a contract is operating.
+     * @dev Get whether the contract is closed disabling any access. Otherwise role permissions are applied.
      */
     function isTargetClosed(address target) public view virtual returns (bool) {
         return _targets[target].closed;
@@ -186,7 +205,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Get the id of the role that acts as an admin for given role.
+     * @dev Get the id of the role that acts as an admin for the given role.
      *
      * The admin permission is required to grant the role, revoke the role and update the execution delay to execute
      * an operation that is restricted to this role.
@@ -205,9 +224,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Get the role current grant delay, that value may change at any point, without an event emitted, following
+     * @dev Get the role current grant delay.
-     * a call to {setGrantDelay}. Changes to this value, including effect timepoint are notified by the
+     *
-     * {RoleGrantDelayChanged} event.
+     * Its value may change at any point without an event emitted following a call to {setGrantDelay}.
+     * Changes to this value, including effect timepoint are notified in advance by the {RoleGrantDelayChanged} event.
      */
     function getRoleGrantDelay(uint64 roleId) public view virtual returns (uint32) {
         return _roles[roleId].grantDelay.get();
@@ -237,8 +257,8 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Check if a given account currently had the permission level corresponding to a given role. Note that this
+     * @dev Check if a given account currently has the permission level corresponding to a given role. Note that this
-     * permission might be associated with a delay. {getAccess} can provide more details.
+     * permission might be associated with an execution delay. {getAccess} can provide more details.
      */
     function hasRole(
         uint64 roleId,
@@ -256,6 +276,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Give a label to a role, for improved role discoverabily by UIs.
      *
+     * Requirements:
+     *
+     * - the caller must be a global admin
+     *
      * Emits a {RoleLabel} event.
      */
     function labelRole(uint64 roleId, string calldata label) public virtual onlyAuthorized {
@@ -270,19 +294,20 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * This gives the account the authorization to call any function that is restricted to this role. An optional
      * execution delay (in seconds) can be set. If that delay is non 0, the user is required to schedule any operation
-     * that is restricted to members this role. The user will only be able to execute the operation after the delay has
+     * that is restricted to members of this role. The user will only be able to execute the operation after the delay has
      * passed, before it has expired. During this period, admin and guardians can cancel the operation (see {cancel}).
      *
      * If the account has already been granted this role, the execution delay will be updated. This update is not
-     * immediate and follows the delay rules. For example, If a user currently has a delay of 3 hours, and this is
+     * immediate and follows the delay rules. For example, if a user currently has a delay of 3 hours, and this is
      * called to reduce that delay to 1 hour, the new delay will take some time to take effect, enforcing that any
      * operation executed in the 3 hours that follows this update was indeed scheduled before this update.
      *
      * Requirements:
      *
      * - the caller must be an admin for the role (see {getRoleAdmin})
+     * - granted role must not be the `PUBLIC_ROLE`
      *
-     * Emits a {RoleGranted} event
+     * Emits a {RoleGranted} event.
      */
     function grantRole(uint64 roleId, address account, uint32 executionDelay) public virtual onlyAuthorized {
         _grantRole(roleId, account, getRoleGrantDelay(roleId), executionDelay);
@@ -295,6 +320,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * Requirements:
      *
      * - the caller must be an admin for the role (see {getRoleAdmin})
+     * - revoked role must not be the `PUBLIC_ROLE`
      *
      * Emits a {RoleRevoked} event if the account had the role.
      */
@@ -303,8 +329,8 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Renounce role permissions for the calling account, with immediate effect. If the sender is not in
+     * @dev Renounce role permissions for the calling account with immediate effect. If the sender is not in
-     * the role, this call has no effect.
+     * the role this call has no effect.
      *
      * Requirements:
      *
@@ -416,7 +442,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal version of {setRoleAdmin} without access control.
      *
-     * Emits a {RoleAdminChanged} event
+     * Emits a {RoleAdminChanged} event.
+     *
+     * NOTE: Setting the admin role as the `PUBLIC_ROLE` is allowed, but it will effectively allow
+     * anyone to set grant or revoke such role.
      */
     function _setRoleAdmin(uint64 roleId, uint64 admin) internal virtual {
         if (roleId == ADMIN_ROLE || roleId == PUBLIC_ROLE) {
@@ -431,7 +460,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal version of {setRoleGuardian} without access control.
      *
-     * Emits a {RoleGuardianChanged} event
+     * Emits a {RoleGuardianChanged} event.
+     *
+     * NOTE: Setting the guardian role as the `PUBLIC_ROLE` is allowed, but it will effectively allow
+     * anyone to cancel any scheduled operation for such role.
      */
     function _setRoleGuardian(uint64 roleId, uint64 guardian) internal virtual {
         if (roleId == ADMIN_ROLE || roleId == PUBLIC_ROLE) {
@@ -446,7 +478,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal version of {setGrantDelay} without access control.
      *
-     * Emits a {RoleGrantDelayChanged} event
+     * Emits a {RoleGrantDelayChanged} event.
      */
     function _setGrantDelay(uint64 roleId, uint32 newDelay) internal virtual {
         if (roleId == PUBLIC_ROLE) {
@@ -480,9 +512,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Internal version of {setFunctionAllowedRole} without access control.
+     * @dev Internal version of {setTargetFunctionRole} without access control.
      *
-     * Emits a {TargetFunctionRoleUpdated} event
+     * Emits a {TargetFunctionRoleUpdated} event.
      */
     function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual {
         _targets[target].allowedRoles[selector] = roleId;
@@ -496,7 +528,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * - the caller must be a global admin
      *
-     * Emits a {TargetAdminDelayUpdated} event per selector
+     * Emits a {TargetAdminDelayUpdated} event.
      */
     function setTargetAdminDelay(address target, uint32 newDelay) public virtual onlyAuthorized {
         _setTargetAdminDelay(target, newDelay);
@@ -505,7 +537,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal version of {setTargetAdminDelay} without access control.
      *
-     * Emits a {TargetAdminDelayUpdated} event
+     * Emits a {TargetAdminDelayUpdated} event.
      */
     function _setTargetAdminDelay(address target, uint32 newDelay) internal virtual {
         uint48 effect;
@@ -673,7 +705,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * This is useful for contract that want to enforce that calls targeting them were scheduled on the manager,
      * with all the verifications that it implies.
      *
-     * Emit a {OperationExecuted} event
+     * Emit a {OperationExecuted} event.
      */
     function consumeScheduledOp(address caller, bytes calldata data) public virtual {
         address target = _msgSender();
@@ -788,7 +820,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * Returns:
      * - bool restricted: does this data match a restricted operation
      * - uint64: which role is this operation restricted to
-     * - uint32: minimum delay to enforce for that operation (on top of the admin's execution delay)
+     * - uint32: minimum delay to enforce for that operation (max between operation's delay and admin's execution delay)
      */
     function _getAdminRestrictions(
         bytes calldata data
@@ -834,14 +866,12 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     // =================================================== HELPERS ====================================================
     /**
-     * @dev An extended version of {canCall} for internal use that considers restrictions for admin functions.
+     * @dev An extended version of {canCall} for internal usage that checks {_canCallSelf}
+     * when the target is this contract.
      *
      * Returns:
      * - bool immediate: whether the operation can be executed immediately (with no delay)
      * - uint32 delay: the execution delay
-     *
-     * If immediate is true, the delay can be disregarded and the operation can be immediately executed.
-     * If immediate is false, the operation can be executed if and only if delay is greater than 0.
      */
     function _canCallExtended(
         address caller,

contracts/access/manager/IAccessManager.sol

@@ -29,6 +29,13 @@ interface IAccessManager {
     event OperationCanceled(bytes32 indexed operationId, uint32 indexed nonce);
 
     event RoleLabel(uint64 indexed roleId, string label);
+    /**
+     * @dev Emitted when `account` is granted `roleId`.
+     *
+     * NOTE: The meaning of the `since` argument depends on the `newMember` argument.
+     * If the role is granted to a new member, the `since` argument indicates when the account becomes a member of the role,
+     * otherwise it indicates the execution delay for this account and roleId is updated.
+     */
     event RoleGranted(uint64 indexed roleId, address indexed account, uint32 delay, uint48 since, bool newMember);
     event RoleRevoked(uint64 indexed roleId, address indexed account);
     event RoleAdminChanged(uint64 indexed roleId, uint64 indexed admin);

contracts/mocks/AccessManagedTarget.sol

@@ -3,6 +3,7 @@
 pragma solidity ^0.8.20;
 
 import {AccessManaged} from "../access/manager/AccessManaged.sol";
+import {StorageSlot} from "../utils/StorageSlot.sol";
 
 abstract contract AccessManagedTarget is AccessManaged {
     event CalledRestricted(address caller);
@@ -17,6 +18,16 @@ abstract contract AccessManagedTarget is AccessManaged {
         emit CalledUnrestricted(msg.sender);
     }
 
+    function setIsConsumingScheduledOp(bool isConsuming, bytes32 slot) external {
+        // Memory layout is 0x....<_consumingSchedule (boolean)><authority (address)>
+        bytes32 mask = bytes32(uint256(1 << 160));
+        if (isConsuming) {
+            StorageSlot.getBytes32Slot(slot).value |= mask;
+        } else {
+            StorageSlot.getBytes32Slot(slot).value &= ~mask;
+        }
+    }
+
     fallback() external {
         emit CalledFallback(msg.sender);
     }

contracts/utils/types/Time.sol

@@ -96,22 +96,26 @@ library Time {
      * enforce the old delay at the moment of the update. Returns the updated Delay object and the timestamp when the
      * new delay becomes effective.
      */
-    function withUpdate(Delay self, uint32 newValue, uint32 minSetback) internal view returns (Delay, uint48) {
+    function withUpdate(
+        Delay self,
+        uint32 newValue,
+        uint32 minSetback
+    ) internal view returns (Delay updatedDelay, uint48 effect) {
         uint32 value = self.get();
         uint32 setback = uint32(Math.max(minSetback, value > newValue ? value - newValue : 0));
-        uint48 effect = timestamp() + setback;
+        effect = timestamp() + setback;
         return (pack(value, newValue, effect), effect);
     }
 
     /**
      * @dev Split a delay into its components: valueBefore, valueAfter and effect (transition timepoint).
      */
-    function unpack(Delay self) internal pure returns (uint32, uint32, uint48) {
+    function unpack(Delay self) internal pure returns (uint32 valueBefore, uint32 valueAfter, uint48 effect) {
         uint112 raw = Delay.unwrap(self);
 
-        uint32 valueAfter = uint32(raw);
+        valueAfter = uint32(raw);
-        uint32 valueBefore = uint32(raw >> 32);
+        valueBefore = uint32(raw >> 32);
-        uint48 effect = uint48(raw >> 64);
+        effect = uint48(raw >> 64);
 
         return (valueBefore, valueAfter, effect);
     }

scripts/upgradeable/transpile.sh

@@ -6,7 +6,7 @@ VERSION="$(jq -r .version contracts/package.json)"
 DIRNAME="$(dirname -- "${BASH_SOURCE[0]}")"
 
 bash "$DIRNAME/patch-apply.sh"
-sed -i "s/<package-version>/$VERSION/g" contracts/package.json
+sed -i'' -e "s/<package-version>/$VERSION/g" "contracts/package.json"
 git add contracts/package.json
 
 npm run clean

test/access/manager/AccessManager.behavior.js

@@ -0,0 +1,711 @@
+const { time } = require('@openzeppelin/test-helpers');
+const {
+  time: { setNextBlockTimestamp },
+  setStorageAt,
+  mine,
+} = require('@nomicfoundation/hardhat-network-helpers');
+const { impersonate } = require('../../helpers/account');
+const { expectRevertCustomError } = require('../../helpers/customError');
+const { EXPIRATION, EXECUTION_ID_STORAGE_SLOT } = require('../../helpers/access-manager');
+
+// ============ COMMON PATHS ============
+
+const COMMON_IS_EXECUTING_PATH = {
+  executing() {
+    it('succeeds', async function () {
+      await web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller });
+    });
+  },
+  notExecuting() {
+    it('reverts as AccessManagerUnauthorizedAccount', async function () {
+      await expectRevertCustomError(
+        web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+        'AccessManagerUnauthorizedAccount',
+        [this.caller, this.role.id],
+      );
+    });
+  },
+};
+
+const COMMON_GET_ACCESS_PATH = {
+  requiredRoleIsGranted: {
+    roleGrantingIsDelayed: {
+      callerHasAnExecutionDelay: {
+        beforeGrantDelay() {
+          it('reverts as AccessManagerUnauthorizedAccount', async function () {
+            await expectRevertCustomError(
+              web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+              'AccessManagerUnauthorizedAccount',
+              [this.caller, this.role.id],
+            );
+          });
+        },
+        afterGrantDelay: undefined, // Diverges if there's an operation delay or not
+      },
+      callerHasNoExecutionDelay: {
+        beforeGrantDelay() {
+          it('reverts as AccessManagerUnauthorizedAccount', async function () {
+            await expectRevertCustomError(
+              web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+              'AccessManagerUnauthorizedAccount',
+              [this.caller, this.role.id],
+            );
+          });
+        },
+        afterGrantDelay() {
+          it('succeeds called directly', async function () {
+            await web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller });
+          });
+
+          it('succeeds via execute', async function () {
+            await this.manager.execute(this.target.address, this.calldata, { from: this.caller });
+          });
+        },
+      },
+    },
+    roleGrantingIsNotDelayed: {
+      callerHasAnExecutionDelay: undefined, // Diverges if there's an operation to schedule or not
+      callerHasNoExecutionDelay() {
+        it('succeeds called directly', async function () {
+          await web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller });
+        });
+
+        it('succeeds via execute', async function () {
+          await this.manager.execute(this.target.address, this.calldata, { from: this.caller });
+        });
+      },
+    },
+  },
+  requiredRoleIsNotGranted() {
+    it('reverts as AccessManagerUnauthorizedAccount', async function () {
+      await expectRevertCustomError(
+        web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+        'AccessManagerUnauthorizedAccount',
+        [this.caller, this.role.id],
+      );
+    });
+  },
+};
+
+const COMMON_SCHEDULABLE_PATH = {
+  scheduled: {
+    before() {
+      it('reverts as AccessManagerNotReady', async function () {
+        await expectRevertCustomError(
+          web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+          'AccessManagerNotReady',
+          [this.operationId],
+        );
+      });
+    },
+    after() {
+      it('succeeds called directly', async function () {
+        await web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller });
+      });
+
+      it('succeeds via execute', async function () {
+        await this.manager.execute(this.target.address, this.calldata, { from: this.caller });
+      });
+    },
+    expired() {
+      it('reverts as AccessManagerExpired', async function () {
+        await expectRevertCustomError(
+          web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+          'AccessManagerExpired',
+          [this.operationId],
+        );
+      });
+    },
+  },
+  notScheduled() {
+    it('reverts as AccessManagerNotScheduled', async function () {
+      await expectRevertCustomError(
+        web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+        'AccessManagerNotScheduled',
+        [this.operationId],
+      );
+    });
+  },
+};
+
+const COMMON_SCHEDULABLE_PATH_IF_ZERO_DELAY = {
+  scheduled: {
+    before() {
+      it.skip('is not reachable without a delay');
+    },
+    after() {
+      it.skip('is not reachable without a delay');
+    },
+    expired() {
+      it.skip('is not reachable without a delay');
+    },
+  },
+  notScheduled() {
+    it('succeeds', async function () {
+      await this.manager.execute(this.target.address, this.calldata, { from: this.caller });
+    });
+  },
+};
+
+// ============ MODE HELPERS ============
+
+/**
+ * @requires this.{manager,target}
+ */
+function shouldBehaveLikeClosable({ closed, open }) {
+  describe('when the manager is closed', function () {
+    beforeEach('close', async function () {
+      await this.manager.$_setTargetClosed(this.target.address, true);
+    });
+
+    closed();
+  });
+
+  describe('when the manager is open', function () {
+    beforeEach('open', async function () {
+      await this.manager.$_setTargetClosed(this.target.address, false);
+    });
+
+    open();
+  });
+}
+
+// ============ DELAY HELPERS ============
+
+/**
+ * @requires this.{delay}
+ */
+function shouldBehaveLikeDelay(type, { before, after }) {
+  beforeEach('define timestamp when delay takes effect', async function () {
+    const timestamp = await time.latest();
+    this.delayEffect = timestamp.add(this.delay);
+  });
+
+  describe(`when ${type} delay has not taken effect yet`, function () {
+    beforeEach(`set next block timestamp before ${type} takes effect`, async function () {
+      await setNextBlockTimestamp(this.delayEffect.subn(1));
+    });
+
+    before();
+  });
+
+  describe(`when ${type} delay has taken effect`, function () {
+    beforeEach(`set next block timestamp when ${type} takes effect`, async function () {
+      await setNextBlockTimestamp(this.delayEffect);
+    });
+
+    after();
+  });
+}
+
+// ============ OPERATION HELPERS ============
+
+/**
+ * @requires this.{manager,scheduleIn,caller,target,calldata}
+ */
+function shouldBehaveLikeSchedulableOperation({ scheduled: { before, after, expired }, notScheduled }) {
+  describe('when operation is scheduled', function () {
+    beforeEach('schedule operation', async function () {
+      await impersonate(this.caller); // May be a contract
+      const { operationId } = await scheduleOperation(this.manager, {
+        caller: this.caller,
+        target: this.target.address,
+        calldata: this.calldata,
+        delay: this.scheduleIn,
+      });
+      this.operationId = operationId;
+    });
+
+    describe('when operation is not ready for execution', function () {
+      beforeEach('set next block time before operation is ready', async function () {
+        this.scheduledAt = await time.latest();
+        const schedule = await this.manager.getSchedule(this.operationId);
+        await setNextBlockTimestamp(schedule.subn(1));
+      });
+
+      before();
+    });
+
+    describe('when operation is ready for execution', function () {
+      beforeEach('set next block time when operation is ready for execution', async function () {
+        this.scheduledAt = await time.latest();
+        const schedule = await this.manager.getSchedule(this.operationId);
+        await setNextBlockTimestamp(schedule);
+      });
+
+      after();
+    });
+
+    describe('when operation has expired', function () {
+      beforeEach('set next block time when operation expired', async function () {
+        this.scheduledAt = await time.latest();
+        const schedule = await this.manager.getSchedule(this.operationId);
+        await setNextBlockTimestamp(schedule.add(EXPIRATION));
+      });
+
+      expired();
+    });
+  });
+
+  describe('when operation is not scheduled', function () {
+    beforeEach('set expected operationId', async function () {
+      this.operationId = await this.manager.hashOperation(this.caller, this.target.address, this.calldata);
+
+      // Assert operation is not scheduled
+      expect(await this.manager.getSchedule(this.operationId)).to.be.bignumber.equal(web3.utils.toBN(0));
+    });
+
+    notScheduled();
+  });
+}
+
+/**
+ * @requires this.{manager,roles,target,calldata}
+ */
+function shouldBehaveLikeARestrictedOperation({ callerIsNotTheManager, callerIsTheManager }) {
+  describe('when the call comes from the manager (msg.sender == manager)', function () {
+    beforeEach('define caller as manager', async function () {
+      this.caller = this.manager.address;
+      await impersonate(this.caller);
+    });
+
+    shouldBehaveLikeCanCallExecuting(callerIsTheManager);
+  });
+
+  describe('when the call does not come from the manager (msg.sender != manager)', function () {
+    beforeEach('define non manager caller', function () {
+      this.caller = this.roles.SOME.members[0];
+    });
+
+    callerIsNotTheManager();
+  });
+}
+
+/**
+ * @requires this.{manager,roles,executionDelay,operationDelay,target}
+ */
+function shouldBehaveLikeDelayedOperation() {
+  describe('with operation delay', function () {
+    describe('when operation delay is greater than execution delay', function () {
+      beforeEach('set operation delay', async function () {
+        this.operationDelay = this.executionDelay.add(time.duration.hours(1));
+        await this.manager.$_setTargetAdminDelay(this.target.address, this.operationDelay);
+        this.scheduleIn = this.operationDelay; // For shouldBehaveLikeSchedulableOperation
+      });
+
+      shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+    });
+
+    describe('when operation delay is shorter than execution delay', function () {
+      beforeEach('set operation delay', async function () {
+        this.operationDelay = this.executionDelay.sub(time.duration.hours(1));
+        await this.manager.$_setTargetAdminDelay(this.target.address, this.operationDelay);
+        this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+      });
+
+      shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+    });
+  });
+
+  describe('without operation delay', function () {
+    beforeEach('set operation delay', async function () {
+      this.operationDelay = web3.utils.toBN(0);
+      await this.manager.$_setTargetAdminDelay(this.target.address, this.operationDelay);
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  });
+}
+
+// ============ METHOD HELPERS ============
+
+/**
+ * @requires this.{manager,roles,role,target,calldata}
+ */
+function shouldBehaveLikeCanCall({
+  closed,
+  open: {
+    callerIsTheManager,
+    callerIsNotTheManager: { publicRoleIsRequired, specificRoleIsRequired },
+  },
+}) {
+  shouldBehaveLikeClosable({
+    closed,
+    open() {
+      shouldBehaveLikeARestrictedOperation({
+        callerIsTheManager,
+        callerIsNotTheManager() {
+          shouldBehaveLikeHasRole({
+            publicRoleIsRequired,
+            specificRoleIsRequired,
+          });
+        },
+      });
+    },
+  });
+}
+
+/**
+ * @requires this.{target,calldata}
+ */
+function shouldBehaveLikeCanCallExecuting({ executing, notExecuting }) {
+  describe('when _executionId is in storage for target and selector', function () {
+    beforeEach('set _executionId flag from calldata and target', async function () {
+      const executionId = await web3.utils.keccak256(
+        web3.eth.abi.encodeParameters(['address', 'bytes4'], [this.target.address, this.calldata.substring(0, 10)]),
+      );
+      await setStorageAt(this.manager.address, EXECUTION_ID_STORAGE_SLOT, executionId);
+    });
+
+    executing();
+  });
+
+  describe('when _executionId does not match target and selector', notExecuting);
+}
+
+/**
+ * @requires this.{target,calldata,roles,role}
+ */
+function shouldBehaveLikeHasRole({ publicRoleIsRequired, specificRoleIsRequired }) {
+  describe('when the function requires the caller to be granted with the PUBLIC_ROLE', function () {
+    beforeEach('set target function role as PUBLIC_ROLE', async function () {
+      this.role = this.roles.PUBLIC;
+      await this.manager.$_setTargetFunctionRole(this.target.address, this.calldata.substring(0, 10), this.role.id, {
+        from: this.roles.ADMIN.members[0],
+      });
+    });
+
+    publicRoleIsRequired();
+  });
+
+  describe('when the function requires the caller to be granted with a role other than PUBLIC_ROLE', function () {
+    beforeEach('set target function role as PUBLIC_ROLE', async function () {
+      await this.manager.$_setTargetFunctionRole(this.target.address, this.calldata.substring(0, 10), this.role.id, {
+        from: this.roles.ADMIN.members[0],
+      });
+    });
+
+    shouldBehaveLikeGetAccess(specificRoleIsRequired);
+  });
+}
+
+/**
+ * @requires this.{manager,role,caller}
+ */
+function shouldBehaveLikeGetAccess({
+  requiredRoleIsGranted: {
+    roleGrantingIsDelayed: {
+      // Because both grant and execution delay are set within the same $_grantRole call
+      // it's not possible to create a set of tests that diverge between grant and execution delay.
+      // Therefore, the shouldBehaveLikeDelay arguments are renamed for clarity:
+      // before => beforeGrantDelay
+      // after => afterGrantDelay
+      callerHasAnExecutionDelay: { beforeGrantDelay: case1, afterGrantDelay: case2 },
+      callerHasNoExecutionDelay: { beforeGrantDelay: case3, afterGrantDelay: case4 },
+    },
+    roleGrantingIsNotDelayed: { callerHasAnExecutionDelay: case5, callerHasNoExecutionDelay: case6 },
+  },
+  requiredRoleIsNotGranted,
+}) {
+  describe('when the required role is granted to the caller', function () {
+    describe('when role granting is delayed', function () {
+      beforeEach('define delay', function () {
+        this.grantDelay = time.duration.minutes(3);
+        this.delay = this.grantDelay; // For shouldBehaveLikeDelay
+      });
+
+      describe('when caller has an execution delay', function () {
+        beforeEach('set role and delay', async function () {
+          this.executionDelay = time.duration.hours(10);
+          this.delay = this.grantDelay;
+          await this.manager.$_grantRole(this.role.id, this.caller, this.grantDelay, this.executionDelay);
+        });
+
+        shouldBehaveLikeDelay('grant', { before: case1, after: case2 });
+      });
+
+      describe('when caller has no execution delay', function () {
+        beforeEach('set role and delay', async function () {
+          this.executionDelay = web3.utils.toBN(0);
+          await this.manager.$_grantRole(this.role.id, this.caller, this.grantDelay, this.executionDelay);
+        });
+
+        shouldBehaveLikeDelay('grant', { before: case3, after: case4 });
+      });
+    });
+
+    describe('when role granting is not delayed', function () {
+      beforeEach('define delay', function () {
+        this.grantDelay = web3.utils.toBN(0);
+      });
+
+      describe('when caller has an execution delay', function () {
+        beforeEach('set role and delay', async function () {
+          this.executionDelay = time.duration.hours(10);
+          await this.manager.$_grantRole(this.role.id, this.caller, this.grantDelay, this.executionDelay);
+        });
+
+        case5();
+      });
+
+      describe('when caller has no execution delay', function () {
+        beforeEach('set role and delay', async function () {
+          this.executionDelay = web3.utils.toBN(0);
+          await this.manager.$_grantRole(this.role.id, this.caller, this.grantDelay, this.executionDelay);
+        });
+
+        case6();
+      });
+    });
+  });
+
+  describe('when role is not granted', function () {
+    // Because this helper can be composed with other helpers, it's possible
+    // that role has been set already by another helper.
+    // Although this is highly unlikely, we check for it here to avoid false positives.
+    beforeEach('assert role is unset', async function () {
+      const { since } = await this.manager.getAccess(this.role.id, this.caller);
+      expect(since).to.be.bignumber.equal(web3.utils.toBN(0));
+    });
+
+    requiredRoleIsNotGranted();
+  });
+}
+
+// ============ ADMIN OPERATION HELPERS ============
+
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeDelayedAdminOperation() {
+  const getAccessPath = COMMON_GET_ACCESS_PATH;
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = function () {
+    beforeEach('consume previously set grant delay', async function () {
+      // Consume previously set delay
+      await mine();
+    });
+    shouldBehaveLikeDelayedOperation();
+  };
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () {
+    beforeEach('set execution delay', async function () {
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeDelayedOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+
+  beforeEach('set target as manager', function () {
+    this.target = this.manager;
+  });
+
+  shouldBehaveLikeARestrictedOperation({
+    callerIsTheManager: COMMON_IS_EXECUTING_PATH,
+    callerIsNotTheManager() {
+      shouldBehaveLikeHasRole({
+        publicRoleIsRequired() {
+          it('reverts as AccessManagerUnauthorizedAccount', async function () {
+            await expectRevertCustomError(
+              web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+              'AccessManagerUnauthorizedAccount',
+              [
+                this.caller,
+                this.roles.ADMIN.id, // Although PUBLIC is required, target function role doesn't apply to admin ops
+              ],
+            );
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      });
+    },
+  });
+}
+
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeNotDelayedAdminOperation() {
+  const getAccessPath = COMMON_GET_ACCESS_PATH;
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = function () {
+    beforeEach('set execution delay', async function () {
+      await mine();
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () {
+    beforeEach('set execution delay', async function () {
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+
+  beforeEach('set target as manager', function () {
+    this.target = this.manager;
+  });
+
+  shouldBehaveLikeARestrictedOperation({
+    callerIsTheManager: COMMON_IS_EXECUTING_PATH,
+    callerIsNotTheManager() {
+      shouldBehaveLikeHasRole({
+        publicRoleIsRequired() {
+          it('reverts as AccessManagerUnauthorizedAccount', async function () {
+            await expectRevertCustomError(
+              web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+              'AccessManagerUnauthorizedAccount',
+              [this.caller, this.roles.ADMIN.id], // Although PUBLIC_ROLE is required, admin ops are not subject to target function roles
+            );
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      });
+    },
+  });
+}
+
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeRoleAdminOperation(roleAdmin) {
+  const getAccessPath = COMMON_GET_ACCESS_PATH;
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = function () {
+    beforeEach('set operation delay', async function () {
+      await mine();
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () {
+    beforeEach('set execution delay', async function () {
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+
+  beforeEach('set target as manager', function () {
+    this.target = this.manager;
+  });
+
+  shouldBehaveLikeARestrictedOperation({
+    callerIsTheManager: COMMON_IS_EXECUTING_PATH,
+    callerIsNotTheManager() {
+      shouldBehaveLikeHasRole({
+        publicRoleIsRequired() {
+          it('reverts as AccessManagerUnauthorizedAccount', async function () {
+            await expectRevertCustomError(
+              web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+              'AccessManagerUnauthorizedAccount',
+              [this.caller, roleAdmin], // Role admin ops require the role's admin
+            );
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      });
+    },
+  });
+}
+
+// ============ RESTRICTED OPERATION HELPERS ============
+
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeAManagedRestrictedOperation() {
+  function revertUnauthorized() {
+    it('reverts as AccessManagedUnauthorized', async function () {
+      await expectRevertCustomError(
+        web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller }),
+        'AccessManagedUnauthorized',
+        [this.caller],
+      );
+    });
+  }
+
+  const getAccessPath = COMMON_GET_ACCESS_PATH;
+
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.beforeGrantDelay =
+    revertUnauthorized;
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasNoExecutionDelay.beforeGrantDelay =
+    revertUnauthorized;
+  getAccessPath.requiredRoleIsNotGranted = revertUnauthorized;
+
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay = function () {
+    beforeEach('consume previously set grant delay', async function () {
+      // Consume previously set delay
+      await mine();
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () {
+    beforeEach('consume previously set grant delay', async function () {
+      this.scheduleIn = this.executionDelay; // For shouldBehaveLikeSchedulableOperation
+    });
+    shouldBehaveLikeSchedulableOperation(COMMON_SCHEDULABLE_PATH);
+  };
+
+  const isExecutingPath = COMMON_IS_EXECUTING_PATH;
+  isExecutingPath.notExecuting = revertUnauthorized;
+
+  shouldBehaveLikeCanCall({
+    closed: revertUnauthorized,
+    open: {
+      callerIsTheManager: isExecutingPath,
+      callerIsNotTheManager: {
+        publicRoleIsRequired() {
+          it('succeeds called directly', async function () {
+            await web3.eth.sendTransaction({ to: this.target.address, data: this.calldata, from: this.caller });
+          });
+
+          it('succeeds via execute', async function () {
+            await this.manager.execute(this.target.address, this.calldata, { from: this.caller });
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      },
+    },
+  });
+}
+
+// ============ HELPERS ============
+
+/**
+ * @requires this.{manager, caller, target, calldata}
+ */
+async function scheduleOperation(manager, { caller, target, calldata, delay }) {
+  const timestamp = await time.latest();
+  const scheduledAt = timestamp.addn(1);
+  await setNextBlockTimestamp(scheduledAt); // Fix next block timestamp for predictability
+  const { receipt } = await manager.schedule(target, calldata, scheduledAt.add(delay), {
+    from: caller,
+  });
+
+  return {
+    receipt,
+    scheduledAt,
+    operationId: await manager.hashOperation(caller, target, calldata),
+  };
+}
+
+module.exports = {
+  // COMMON PATHS
+  COMMON_SCHEDULABLE_PATH,
+  COMMON_SCHEDULABLE_PATH_IF_ZERO_DELAY,
+  // MODE HELPERS
+  shouldBehaveLikeClosable,
+  // DELAY HELPERS
+  shouldBehaveLikeDelay,
+  // OPERATION HELPERS
+  shouldBehaveLikeSchedulableOperation,
+  // METHOD HELPERS
+  shouldBehaveLikeCanCall,
+  shouldBehaveLikeGetAccess,
+  shouldBehaveLikeHasRole,
+  // ADMIN OPERATION HELPERS
+  shouldBehaveLikeDelayedAdminOperation,
+  shouldBehaveLikeNotDelayedAdminOperation,
+  shouldBehaveLikeRoleAdminOperation,
+  // RESTRICTED OPERATION HELPERS
+  shouldBehaveLikeAManagedRestrictedOperation,
+  // HELPERS
+  scheduleOperation,
+};

test/helpers/access-manager.js

@@ -0,0 +1,69 @@
+const { time } = require('@openzeppelin/test-helpers');
+const { MAX_UINT64 } = require('./constants');
+const { artifacts } = require('hardhat');
+
+function buildBaseRoles() {
+  const roles = {
+    ADMIN: {
+      id: web3.utils.toBN(0),
+    },
+    SOME_ADMIN: {
+      id: web3.utils.toBN(17),
+    },
+    SOME_GUARDIAN: {
+      id: web3.utils.toBN(35),
+    },
+    SOME: {
+      id: web3.utils.toBN(42),
+    },
+    PUBLIC: {
+      id: MAX_UINT64,
+    },
+  };
+
+  // Names
+  Object.entries(roles).forEach(([name, role]) => (role.name = name));
+
+  // Defaults
+  for (const role of Object.keys(roles)) {
+    roles[role].admin = roles.ADMIN;
+    roles[role].guardian = roles.ADMIN;
+  }
+
+  // Admins
+  roles.SOME.admin = roles.SOME_ADMIN;
+
+  // Guardians
+  roles.SOME.guardian = roles.SOME_GUARDIAN;
+
+  return roles;
+}
+
+const formatAccess = access => [access[0], access[1].toString()];
+
+const MINSETBACK = time.duration.days(5);
+const EXPIRATION = time.duration.weeks(1);
+
+let EXECUTION_ID_STORAGE_SLOT = 3n;
+let CONSUMING_SCHEDULE_STORAGE_SLOT = 0n;
+try {
+  // Try to get the artifact paths, will throw if it doesn't exist
+  artifacts._getArtifactPathSync('AccessManagerUpgradeable');
+  artifacts._getArtifactPathSync('AccessManagedUpgradeable');
+
+  // ERC-7201 namespace location for AccessManager
+  EXECUTION_ID_STORAGE_SLOT += 0x40c6c8c28789853c7efd823ab20824bbd71718a8a5915e855f6f288c9a26ad00n;
+  // ERC-7201 namespace location for AccessManaged
+  CONSUMING_SCHEDULE_STORAGE_SLOT += 0xf3177357ab46d8af007ab3fdb9af81da189e1068fefdc0073dca88a2cab40a00n;
+} catch (_) {
+  // eslint-disable-next-line no-empty
+}
+
+module.exports = {
+  buildBaseRoles,
+  formatAccess,
+  MINSETBACK,
+  EXPIRATION,
+  EXECUTION_ID_STORAGE_SLOT,
+  CONSUMING_SCHEDULE_STORAGE_SLOT,
+};