Add modifier & internal function with standard revert message in AccessControl

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,7 +80,7 @@ contract TimelockController is AccessControl {
      * considered. Granting a role to `address(0)` is equivalent to enabling
      * this role for everyone.
      */
-    modifier onlyRole(bytes32 role) {
+    modifier onlyRoleOrOpenRole(bytes32 role) {
         require(hasRole(role, _msgSender()) || hasRole(role, address(0)), "TimelockController: sender requires permission");
         _;
     }
@@ -163,7 +163,7 @@ contract TimelockController is AccessControl {
      *
      * - the caller must have the 'proposer' role.
      */
-    function schedule(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt, uint256 delay) public virtual onlyRole(PROPOSER_ROLE) {
+    function schedule(address target, uint256 value, bytes calldata data, bytes32 predecessor, bytes32 salt, uint256 delay) public virtual onlyRoleOrOpenRole(PROPOSER_ROLE) {
         bytes32 id = hashOperation(target, value, data, predecessor, salt);
         _schedule(id, delay);
         emit CallScheduled(id, 0, target, value, data, predecessor, delay);
@@ -178,7 +178,7 @@ contract TimelockController is AccessControl {
      *
      * - the caller must have the 'proposer' role.
      */
-    function scheduleBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt, uint256 delay) public virtual onlyRole(PROPOSER_ROLE) {
+    function scheduleBatch(address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt, uint256 delay) public virtual onlyRoleOrOpenRole(PROPOSER_ROLE) {
         require(targets.length == values.length, "TimelockController: length mismatch");
         require(targets.length == datas.length, "TimelockController: length mismatch");
 
@@ -206,7 +206,7 @@ contract TimelockController is AccessControl {
      *
      * - the caller must have the 'proposer' role.
      */
-    function cancel(bytes32 id) public virtual onlyRole(PROPOSER_ROLE) {
+    function cancel(bytes32 id) public virtual onlyRoleOrOpenRole(PROPOSER_ROLE) {
         require(isOperationPending(id), "TimelockController: operation cannot be cancelled");
         delete _timestamps[id];
 
@@ -222,7 +222,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 +238,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) {}
 }

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}`,
       );
     });
   });