From 57066ec59e6afba7c94da4143e16d393a69b9c15 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 4 Mar 2021 17:45:06 -0300 Subject: [PATCH 1/4] Add ERC165 interface detection to AccessControl --- contracts/access/AccessControl.sol | 32 +++++++++++++++---- contracts/access/AccessControlEnumerable.sol | 22 +++++++++++-- .../presets/ERC1155PresetMinterPauser.sol | 7 ++++ .../ERC721PresetMinterPauserAutoId.sol | 2 +- test/access/AccessControl.behavior.js | 6 ++++ .../SupportsInterface.behavior.js | 13 +++++++- 6 files changed, 71 insertions(+), 11 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 708ec9adac2..eaa07260964 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -3,6 +3,18 @@ pragma solidity ^0.8.0; import "../utils/Context.sol"; +import "../utils/introspection/ERC165.sol"; + +/** + * @dev External interface of AccessControl declared to support ERC165 detection. + */ +interface IAccessControl { + function hasRole(bytes32 role, address account) external view returns (bool); + function getRoleAdmin(bytes32 role) external view returns (bytes32); + function grantRole(bytes32 role, address account) external; + function revokeRole(bytes32 role, address account) external; + function renounceRole(bytes32 role, address account) external; +} /** * @dev Contract module that allows children to implement role-based access @@ -42,7 +54,7 @@ import "../utils/Context.sol"; * grant and revoke this role. Extra precautions should be taken to secure * accounts that have been granted it. */ -abstract contract AccessControl is Context { +abstract contract AccessControl is Context, IAccessControl, ERC165 { struct RoleData { mapping (address => bool) members; bytes32 adminRole; @@ -79,10 +91,18 @@ abstract contract AccessControl is Context { */ event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender); + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IAccessControl).interfaceId + || super.supportsInterface(interfaceId); + } + /** * @dev Returns `true` if `account` has been granted `role`. */ - function hasRole(bytes32 role, address account) public view returns (bool) { + function hasRole(bytes32 role, address account) public view override returns (bool) { return _roles[role].members[account]; } @@ -92,7 +112,7 @@ abstract contract AccessControl is Context { * * To change a role's admin, use {_setRoleAdmin}. */ - function getRoleAdmin(bytes32 role) public view returns (bytes32) { + function getRoleAdmin(bytes32 role) public view override returns (bytes32) { return _roles[role].adminRole; } @@ -106,7 +126,7 @@ abstract contract AccessControl is Context { * * - the caller must have ``role``'s admin role. */ - function grantRole(bytes32 role, address account) public virtual { + function grantRole(bytes32 role, address account) public virtual override { require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to grant"); _grantRole(role, account); @@ -121,7 +141,7 @@ abstract contract AccessControl is Context { * * - the caller must have ``role``'s admin role. */ - function revokeRole(bytes32 role, address account) public virtual { + function revokeRole(bytes32 role, address account) public virtual override { require(hasRole(getRoleAdmin(role), _msgSender()), "AccessControl: sender must be an admin to revoke"); _revokeRole(role, account); @@ -141,7 +161,7 @@ abstract contract AccessControl is Context { * * - the caller must be `account`. */ - function renounceRole(bytes32 role, address account) public virtual { + function renounceRole(bytes32 role, address account) public virtual override { require(account == _msgSender(), "AccessControl: can only renounce roles for self"); _revokeRole(role, account); diff --git a/contracts/access/AccessControlEnumerable.sol b/contracts/access/AccessControlEnumerable.sol index b68ad1ad474..8fe8f03324d 100644 --- a/contracts/access/AccessControlEnumerable.sol +++ b/contracts/access/AccessControlEnumerable.sol @@ -5,14 +5,30 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; import "../utils/structs/EnumerableSet.sol"; +/** + * @dev External interface of AccessControlEnumerable declared to support ERC165 detection. + */ +interface IAccessControlEnumerable { + function getRoleMember(bytes32 role, uint256 index) external view returns (address); + function getRoleMemberCount(bytes32 role) external view returns (uint256); +} + /** * @dev Extension of {AccessControl} that allows enumerating the members of each role. */ -abstract contract AccessControlEnumerable is AccessControl { +abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessControl { using EnumerableSet for EnumerableSet.AddressSet; mapping (bytes32 => EnumerableSet.AddressSet) private _roleMembers; + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IAccessControlEnumerable).interfaceId + || super.supportsInterface(interfaceId); + } + /** * @dev Returns one of the accounts that have `role`. `index` must be a * value between 0 and {getRoleMemberCount}, non-inclusive. @@ -25,7 +41,7 @@ abstract contract AccessControlEnumerable is AccessControl { * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post] * for more information. */ - function getRoleMember(bytes32 role, uint256 index) public view returns (address) { + function getRoleMember(bytes32 role, uint256 index) public view override returns (address) { return _roleMembers[role].at(index); } @@ -33,7 +49,7 @@ abstract contract AccessControlEnumerable is AccessControl { * @dev Returns the number of accounts that have `role`. Can be used * together with {getRoleMember} to enumerate all bearers of a role. */ - function getRoleMemberCount(bytes32 role) public view returns (uint256) { + function getRoleMemberCount(bytes32 role) public view override returns (uint256) { return _roleMembers[role].length(); } diff --git a/contracts/token/ERC1155/presets/ERC1155PresetMinterPauser.sol b/contracts/token/ERC1155/presets/ERC1155PresetMinterPauser.sol index 55e62c555be..3ff5e02f857 100644 --- a/contracts/token/ERC1155/presets/ERC1155PresetMinterPauser.sol +++ b/contracts/token/ERC1155/presets/ERC1155PresetMinterPauser.sol @@ -89,6 +89,13 @@ contract ERC1155PresetMinterPauser is Context, AccessControlEnumerable, ERC1155B _unpause(); } + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override(AccessControlEnumerable, ERC1155) returns (bool) { + return super.supportsInterface(interfaceId); + } + function _beforeTokenTransfer( address operator, address from, diff --git a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol index 4d1fbe9e6f7..77b3fd17e19 100644 --- a/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol +++ b/contracts/token/ERC721/presets/ERC721PresetMinterPauserAutoId.sol @@ -110,7 +110,7 @@ contract ERC721PresetMinterPauserAutoId is Context, AccessControlEnumerable, ERC /** * @dev See {IERC165-supportsInterface}. */ - function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721, ERC721Enumerable) returns (bool) { + function supportsInterface(bytes4 interfaceId) public view virtual override(AccessControlEnumerable, ERC721, ERC721Enumerable) returns (bool) { return super.supportsInterface(interfaceId); } } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 5b7c32fa3c2..4cb4f7fc85c 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -1,11 +1,15 @@ const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); +const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior'); + const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; const ROLE = web3.utils.soliditySha3('ROLE'); const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE'); function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, otherAdmin, otherAuthorized) { + shouldSupportInterfaces(['AccessControl']); + describe('default admin', function () { it('deployer has default admin role', async function () { expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.equal(true); @@ -156,6 +160,8 @@ function shouldBehaveLikeAccessControl (errorPrefix, admin, authorized, other, o } function shouldBehaveLikeAccessControlEnumerable (errorPrefix, admin, authorized, other, otherAdmin, otherAuthorized) { + shouldSupportInterfaces(['AccessControlEnumerable']); + describe('enumerating', function () { it('role bearers can be enumerated', async function () { await this.accessControl.grantRole(ROLE, authorized, { from: admin }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index e1256cefa4e..1be01c42634 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -39,6 +39,17 @@ const INTERFACES = { 'onERC1155Received(address,address,uint256,uint256,bytes)', 'onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)', ], + AccessControl: [ + 'hasRole(bytes32,address)', + 'getRoleAdmin(bytes32)', + 'grantRole(bytes32,address)', + 'revokeRole(bytes32,address)', + 'renounceRole(bytes32,address)', + ], + AccessControlEnumerable: [ + 'getRoleMember(bytes32,uint256)', + 'getRoleMemberCount(bytes32)', + ], }; const INTERFACE_IDS = {}; @@ -54,7 +65,7 @@ for (const k of Object.getOwnPropertyNames(INTERFACES)) { function shouldSupportInterfaces (interfaces = []) { describe('Contract interface', function () { beforeEach(function () { - this.contractUnderTest = this.mock || this.token || this.holder; + this.contractUnderTest = this.mock || this.token || this.holder || this.accessControl; }); for (const k of interfaces) { From 1a0e16c9e38d7967248420d19fc76ea26073ceba Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 4 Mar 2021 17:53:26 -0300 Subject: [PATCH 2/4] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86993639d92..4406f5bd0b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ * Rename `UpgradeableProxy` to `ERC1967Proxy`. ([#2547](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2547)) * `ERC777`: Optimize the gas costs of the constructor. ([#2551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2551)) * `ERC721URIStorage`: Add a new extension that implements the `_setTokenURI` behavior as it was available in 3.4.0. ([#2555](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2555)) + * `AccessControl`: Added ERC165 interface detection. ([#2562](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2562)) ### How to upgrade from 3.x From 8dc32cfd8d5e45132bc88b3daa7a7e3e7bd11042 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 4 Mar 2021 22:25:53 +0100 Subject: [PATCH 3/4] check introspection on presets to improve coverage --- test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js | 3 +++ .../ERC721/presets/ERC721PresetMinterPauserAutoId.test.js | 3 +++ 2 files changed, 6 insertions(+) diff --git a/test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js b/test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js index 56baea83355..fe833435560 100644 --- a/test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js +++ b/test/token/ERC1155/presets/ERC1155PresetMinterPauser.test.js @@ -1,5 +1,6 @@ const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { ZERO_ADDRESS } = constants; +const { shouldSupportInterfaces } = require('../../../utils/introspection/SupportsInterface.behavior'); const { expect } = require('chai'); @@ -24,6 +25,8 @@ contract('ERC1155PresetMinterPauser', function (accounts) { this.token = await ERC1155PresetMinterPauser.new(uri, { from: deployer }); }); + shouldSupportInterfaces(['ERC1155', 'AccessControl', 'AccessControlEnumerable']); + it('deployer has the default admin role', async function () { expect(await this.token.getRoleMemberCount(DEFAULT_ADMIN_ROLE)).to.be.bignumber.equal('1'); expect(await this.token.getRoleMember(DEFAULT_ADMIN_ROLE, 0)).to.equal(deployer); diff --git a/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js b/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js index 38216d6a26a..c7ef4a0fed4 100644 --- a/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js +++ b/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js @@ -1,5 +1,6 @@ const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { ZERO_ADDRESS } = constants; +const { shouldSupportInterfaces } = require('../../../utils/introspection/SupportsInterface.behavior'); const { expect } = require('chai'); @@ -19,6 +20,8 @@ contract('ERC721PresetMinterPauserAutoId', function (accounts) { this.token = await ERC721PresetMinterPauserAutoId.new(name, symbol, baseURI, { from: deployer }); }); + shouldSupportInterfaces(['ERC721', 'ERC721Enumerable', 'AccessControl', 'AccessControlEnumerable']); + it('token has correct name', async function () { expect(await this.token.name()).to.equal(name); }); From b0b98563d795acc6ffbd27a5000df80b12769ef4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 4 Mar 2021 22:36:55 +0100 Subject: [PATCH 4/4] fix lint --- .../token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js b/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js index c7ef4a0fed4..ed80e92fd82 100644 --- a/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js +++ b/test/token/ERC721/presets/ERC721PresetMinterPauserAutoId.test.js @@ -20,7 +20,7 @@ contract('ERC721PresetMinterPauserAutoId', function (accounts) { this.token = await ERC721PresetMinterPauserAutoId.new(name, symbol, baseURI, { from: deployer }); }); - shouldSupportInterfaces(['ERC721', 'ERC721Enumerable', 'AccessControl', 'AccessControlEnumerable']); + shouldSupportInterfaces(['ERC721', 'ERC721Enumerable', 'AccessControl', 'AccessControlEnumerable']); it('token has correct name', async function () { expect(await this.token.name()).to.equal(name);