From e0a2b195e4eddc79788577c154838c56495ba286 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 16 Apr 2021 17:15:09 +0200 Subject: [PATCH] Add modifier & internal function with standard revert message in AccessControl (#2609) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 14 +++++++ contracts/access/AccessControl.sol | 40 ++++++++++++++++--- contracts/governance/TimelockController.sol | 10 +++-- .../mocks/AccessControlEnumerableMock.sol | 2 + contracts/mocks/AccessControlMock.sol | 2 + docs/modules/ROOT/pages/access-control.adoc | 16 +++----- test/access/AccessControl.behavior.js | 39 +++++++++++++----- test/governance/TimelockController.test.js | 13 +++--- 8 files changed, 102 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9fb59a4f..e39b92d66 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index eaa072609..5b723def7 100644 --- a/contracts/access/AccessControl.sol +++ b/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); } diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index a39cef008..02fc3dbc5 100644 --- a/contracts/governance/TimelockController.sol +++ b/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"); diff --git a/contracts/mocks/AccessControlEnumerableMock.sol b/contracts/mocks/AccessControlEnumerableMock.sol index ec2cbbb10..7b15e3602 100644 --- a/contracts/mocks/AccessControlEnumerableMock.sol +++ b/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) {} } diff --git a/contracts/mocks/AccessControlMock.sol b/contracts/mocks/AccessControlMock.sol index 340e2080b..86f51477e 100644 --- a/contracts/mocks/AccessControlMock.sol +++ b/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) {} } diff --git a/docs/modules/ROOT/pages/access-control.adoc b/docs/modules/ROOT/pages/access-control.adoc index 257388900..a982983df 100644 --- a/docs/modules/ROOT/pages/access-control.adoc +++ b/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); } } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 4f7fece9b..53dfa3ddb 100644 --- a/test/access/AccessControl.behavior.js +++ b/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}`, ); }); }); diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 800f6ced4..f39b96348 100644 --- a/test/governance/TimelockController.test.js +++ b/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}`, ); }); });