Add modifier & internal function with standard revert message in AccessControl (#2609)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

CHANGELOG.md

@@ -9,6 +9,20 @@
  * `SignatureChecker`: add a signature verification library that supports both EOA and ERC1271 compliant contracts as signers. ([#2532](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2532))
  * `Multicall`: add abstract contract with `multicall(bytes[] calldata data)` function to bundle multiple calls together ([#2608](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2608))
  * `ECDSA`: add support for ERC2098 short-signatures. ([#2582](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2582))
+ * `AccessControl`: add a `onlyRole` modifier to restrict specific function to callers bearing a specific role. ([#2609](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2609))
+
+### Breaking changes
+
+This release includes two small breaking changes in `TimelockController`.
+
+1. The `onlyRole` modifier in this contract was designed to let anyone through if the role was granted to `address(0)`,
+   allowing the possibility to to make a role "open", which can be used for `EXECUTOR_ROLE`. This modifier is now
+   replaced by `AccessControl.onlyRole`, which does not have this ability. The previous behavior was moved to the
+   modifier `TimelockController.onlyRoleOrOpenRole`.
+2. It was possible to make `PROPOSER_ROLE` an open role (as described in the previous item) if it was granted to
+   `address(0)`. This would affect the `schedule`, `scheduleBatch`, and `cancel` operations in `TimelockController`.
+   This ability was removed as it does not make sense to open up the `PROPOSER_ROLE` in the same way that it does for
+   `EXECUTOR_ROLE`.
 
 ## 4.0.0 (2021-03-23)
 

contracts/access/AccessControl.sol

@@ -3,6 +3,7 @@
 pragma solidity ^0.8.0;
 
 import "../utils/Context.sol";
+import "../utils/Strings.sol";
 import "../utils/introspection/ERC165.sol";
 
 /**
@@ -91,6 +92,19 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
      */
     event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
 
+    /**
+     * @dev Modifier that checks that an account has a specific role. Reverts
+     * with a standardized message including the required role.
+     *
+     * The format of the revert reason is given by the following regular expression:
+     *
+     *  /^AccessControl: account (0x[0-9a-f]{20}) is missing role (0x[0-9a-f]{32})$/
+     */
+    modifier onlyRole(bytes32 role) {
+        _checkRole(role, _msgSender());
+        _;
+    }
+
     /**
      * @dev See {IERC165-supportsInterface}.
      */
@@ -106,6 +120,24 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
         return _roles[role].members[account];
     }
 
+    /**
+     * @dev Revert with a standard message if `account` is missing `role`.
+     *
+     * The format of the revert reason is given by the following regular expression:
+     *
+     *  /^AccessControl: account (0x[0-9a-f]{20}) is missing role (0x[0-9a-f]{32})$/
+     */
+    function _checkRole(bytes32 role, address account) internal view {
+        if(!hasRole(role, account)) {
+            revert(string(abi.encodePacked(
+                "AccessControl: account ",
+                Strings.toHexString(uint160(account), 20),
+                " is missing role ",
+                Strings.toHexString(uint256(role), 32)
+            )));
+        }
+    }
+
     /**
      * @dev Returns the admin role that controls `role`. See {grantRole} and
      * {revokeRole}.
@@ -126,9 +158,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
      *
      * - the caller must have ``role``'s admin role.
      */
-    function grantRole(bytes32 role, address account) public virtual override {
-        require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to grant");
-
+    function grantRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
         _grantRole(role, account);
     }
 
@@ -141,9 +171,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
      *
      * - the caller must have ``role``'s admin role.
      */
-    function revokeRole(bytes32 role, address account) public virtual override {
-        require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to revoke");
-
+    function revokeRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
         _revokeRole(role, account);
     }
 

contracts/governance/TimelockController.sol

@@ -80,8 +80,10 @@ contract TimelockController is AccessControl {
      * considered. Granting a role to `address(0)` is equivalent to enabling
      * this role for everyone.
      */
-    modifier onlyRole(bytes32 role) {
-        require(hasRole(role, _msgSender()) || hasRole(role, address(0)), "TimelockController: sender requires permission");
+    modifier onlyRoleOrOpenRole(bytes32 role) {
+        if (!hasRole(role, address(0))) {
+            _checkRole(role, _msgSender());
+        }
         _;
     }
 
@@ -222,7 +224,7 @@ contract TimelockController is AccessControl {
      *
      * - the caller must have the 'executor' role.
      */
-    function execute(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt) public payable virtual onlyRole(EXECUTOR_ROLE) {
+    function execute(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {
         bytes32 id = hashOperation(target, value, data, predecessor, salt);
         _beforeCall(predecessor);
         _call(id, 0, target, value, data);
@@ -238,7 +240,7 @@ contract TimelockController is AccessControl {
      *
      * - the caller must have the 'executor' role.
      */
-    function executeBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt) public payable virtual onlyRole(EXECUTOR_ROLE) {
+    function executeBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {
         require(targets.length == values.length, "TimelockController: length mismatch");
         require(targets.length == datas.length, "TimelockController: length mismatch");
 

contracts/mocks/AccessControlEnumerableMock.sol

@@ -12,4 +12,6 @@ contract AccessControlEnumerableMock is AccessControlEnumerable {
     function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
         _setRoleAdmin(roleId, adminRoleId);
     }
+
+    function senderProtected(bytes32 roleId) public onlyRole(roleId) {}
 }

contracts/mocks/AccessControlMock.sol

@@ -12,4 +12,6 @@ contract AccessControlMock is AccessControl {
     function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
         _setRoleAdmin(roleId, adminRoleId);
     }
+
+    function senderProtected(bytes32 roleId) public onlyRole(roleId) {}
 }

docs/modules/ROOT/pages/access-control.adoc

@@ -46,7 +46,7 @@ In this way you can use _composability_ to add additional layers of access contr
 
 While the simplicity of _ownership_ can be useful for simple systems or quick prototyping, different levels of authorization are often needed. You may want for an account to have permission to ban users from a system, but not create new tokens. https://en.wikipedia.org/wiki/Role-based_access_control[_Role-Based Access Control (RBAC)_] offers flexibility in this regard.
 
-In essence, we will be defining multiple _roles_, each allowed to perform different sets of actions. An account may have, for example, 'moderator', 'minter' or 'admin' roles, which you will then check for instead of simply using `onlyOwner`. Separately, you will be able to define rules for how accounts can be granted a role, have it revoked, and more.
+In essence, we will be defining multiple _roles_, each allowed to perform different sets of actions. An account may have, for example, 'moderator', 'minter' or 'admin' roles, which you will then check for instead of simply using `onlyOwner`. This check can be enforced through the `onlyRole` modifier. Separately, you will be able to define rules for how accounts can be granted a role, have it revoked, and more.
 
 Most software uses access control systems that are role-based: some users are regular users, some may be supervisors or managers, and a few will often have administrative privileges.
 
@@ -88,7 +88,7 @@ NOTE: Make sure you fully understand how xref:api:access.adoc#AccessControl[`Acc
 
 While clear and explicit, this isn't anything we wouldn't have been able to achieve with `Ownable`. Indeed, where `AccessControl` shines is in scenarios where granular permissions are required, which can be implemented by defining _multiple_ roles.
 
-Let's augment our ERC20 token example by also defining a 'burner' role, which lets accounts destroy tokens:
+Let's augment our ERC20 token example by also defining a 'burner' role, which lets accounts destroy tokens, and by using the `onlyRole` modifier:
 
 [source,solidity]
 ----
@@ -108,13 +108,11 @@ contract MyToken is ERC20, AccessControl {
         _setupRole(BURNER_ROLE, burner);
     }
 
-    function mint(address to, uint256 amount) public {
-        require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
+    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
         _mint(to, amount);
     }
 
-    function burn(address from, uint256 amount) public {
-        require(hasRole(BURNER_ROLE, msg.sender), "Caller is not a burner");
+    function burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) {
         _burn(from, amount);
     }
 }
@@ -154,13 +152,11 @@ contract MyToken is ERC20, AccessControl {
         _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
     }
 
-    function mint(address to, uint256 amount) public {
-        require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
+    function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
         _mint(to, amount);
     }
 
-    function burn(address from, uint256 amount) public {
-        require(hasRole(BURNER_ROLE, msg.sender), "Caller is not a burner");
+    function burn(address from, uint256 amount) public onlyRole(BURNER_ROLE) {
         _burn(from, amount);
     }
 }

test/access/AccessControl.behavior.js

@@ -25,17 +25,14 @@ function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, o
   });
 
   describe('granting', function () {
-    it('admin can grant role to other accounts', async function () {
-      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
-      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, sender: admin });
-
-      expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
+    beforeEach(async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
     });
 
     it('non-admin cannot grant role to other accounts', async function () {
       await expectRevert(
         this.accessControl.grantRole(ROLE, authorized, { from: other }),
-        `${errorPrefix}: sender must be an admin to grant`,
+        `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
       );
     });
 
@@ -69,7 +66,7 @@ function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, o
       it('non-admin cannot revoke role', async function () {
         await expectRevert(
           this.accessControl.revokeRole(ROLE, authorized, { from: other }),
-          `${errorPrefix}: sender must be an admin to revoke`,
+          `${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
         );
       });
 
@@ -146,14 +143,38 @@ function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, o
     it('a role\'s previous admins no longer grant roles', async function () {
       await expectRevert(
         this.accessControl.grantRole(ROLE, authorized, { from: admin }),
-        'AccessControl: sender must be an admin to grant',
+        `${errorPrefix}: account ${admin.toLowerCase()} is missing role ${OTHER_ROLE}`,
       );
     });
 
     it('a role\'s previous admins no longer revoke roles', async function () {
       await expectRevert(
         this.accessControl.revokeRole(ROLE, authorized, { from: admin }),
-        'AccessControl: sender must be an admin to revoke',
+        `${errorPrefix}: account ${admin.toLowerCase()} is missing role ${OTHER_ROLE}`,
+      );
+    });
+  });
+
+  describe('onlyRole modifier', function () {
+    beforeEach(async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+    });
+
+    it('do not revert if sender has role', async function () {
+      await this.accessControl.senderProtected(ROLE, { from: authorized });
+    });
+
+    it('revert if sender doesn\'t have role #1', async function () {
+      await expectRevert(
+        this.accessControl.senderProtected(ROLE, { from: other }),
+        `${errorPrefix}: account ${other.toLowerCase()} is missing role ${ROLE}`,
+      );
+    });
+
+    it('revert if sender doesn\'t have role #2', async function () {
+      await expectRevert(
+        this.accessControl.senderProtected(OTHER_ROLE, { from: authorized }),
+        `${errorPrefix}: account ${authorized.toLowerCase()} is missing role ${OTHER_ROLE}`,
       );
     });
   });

test/governance/TimelockController.test.js

@@ -53,6 +53,9 @@ contract('TimelockController', function (accounts) {
       [ executor ],
       { from: admin },
     );
+    this.TIMELOCK_ADMIN_ROLE = await this.timelock.TIMELOCK_ADMIN_ROLE();
+    this.PROPOSER_ROLE = await this.timelock.PROPOSER_ROLE();
+    this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE();
     // Mocks
     this.callreceivermock = await CallReceiverMock.new({ from: admin });
     this.implementation2 = await Implementation2.new({ from: admin });
@@ -172,7 +175,7 @@ contract('TimelockController', function (accounts) {
               MINDELAY,
               { from: other },
             ),
-            'TimelockController: sender requires permission',
+            `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`,
           );
         });
 
@@ -295,7 +298,7 @@ contract('TimelockController', function (accounts) {
                   this.operation.salt,
                   { from: other },
                 ),
-                'TimelockController: sender requires permission',
+                `AccessControl: account ${other.toLowerCase()} is missing role ${this.EXECUTOR_ROLE}`,
               );
             });
           });
@@ -409,7 +412,7 @@ contract('TimelockController', function (accounts) {
               MINDELAY,
               { from: other },
             ),
-            'TimelockController: sender requires permission',
+            `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`,
           );
         });
 
@@ -534,7 +537,7 @@ contract('TimelockController', function (accounts) {
                   this.operation.salt,
                   { from: other },
                 ),
-                'TimelockController: sender requires permission',
+                `AccessControl: account ${other.toLowerCase()} is missing role ${this.EXECUTOR_ROLE}`,
               );
             });
 
@@ -663,7 +666,7 @@ contract('TimelockController', function (accounts) {
       it('prevent non-proposer from canceling', async function () {
         await expectRevert(
           this.timelock.cancel(this.operation.id, { from: other }),
-          'TimelockController: sender requires permission',
+          `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`,
         );
       });
     });