From 1327b8f0d488203b9ee07ba168f5aa70b4fa472c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 1 Mar 2023 00:55:59 -0300 Subject: [PATCH 01/78] add AccessManager.sol --- contracts/access/AccessControl.sol | 5 + contracts/access/AccessManager.sol | 205 +++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) create mode 100644 contracts/access/AccessManager.sol diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 3a73de78b55..d32c4da5314 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -72,6 +72,11 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { _; } + modifier onlyDefaultAdmin { + _checkRole(DEFAULT_ADMIN_ROLE); + _; + } + /** * @dev See {IERC165-supportsInterface}. */ diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol new file mode 100644 index 00000000000..c70d448ef1c --- /dev/null +++ b/contracts/access/AccessManager.sol @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./AccessControl.sol"; +import "./AccessControlDefaultAdminRules.sol"; + +/// AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, i.e. +/// it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set of +/// roles, with a particular structure. +/// +/// Users are grouped in "teams". Teams must be created before users can be assigned into them, up to a maximum of 256 +/// teams. A user can be assigned to multiple teams. Each team defines an AccessControl role, identified by a role id of +/// the form [specific format TBD, something like " ... "]. +/// +/// Contracts in the system are also grouped. These are simply called "contract groups". There can be an arbitrary +/// number of groups. Each group has a group admin role that is allowed to add new contracts in the system into that +/// contract group. +/// +/// All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between +/// functions and allowed teams. Each function can be allowed for multiple teams, meaning that if a user is in at least +/// one of the allowed teams they can call that funcion. +/// +/// TODO: Implement AccessMode (from zkSync AllowList) in terms of teams and groups. +/// - Define the built-in "all" team (#255?) of which everyone is a member. +/// - Define a contract group where every function is allowed to the "all" group. (-> AccessMode = Open) +/// - Define a contract group where no function is allowed to any group. (-> AccessMode = Closed) +interface IAccessManager { + function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); + + function createTeam(uint8 team, string calldata name) external; + + function updateTeam(uint8 team, string calldata name) external; + + function getTeamName(uint8 team) external view returns (string calldata name); + + // The result is a bit mask. + function getUserTeams(address user) external view returns (bytes32 teams); + + // The result is a bit mask. + function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); + + function addFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) external; + + function removeFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) external; + + function getContractGroup(address target) external view returns (bytes32 group); + + function setContractGroup(address target, bytes32 group) external; + + function setInitialContractGroup(address target, bytes32 group) external; + + function getContractGroupAdmin(bytes32 group) external view returns (bytes32 adminRole); + + function setContractGroupAdmin(bytes32 group, bytes32 adminRole) external; + + function setRoleAdmin(bytes32 role, bytes32 adminRole) external; + + struct EnumeratedTeam { + uint8 id; + string name; + } + + /// Decodes a bit mask into array of team ids and names. + /// Meant for user interface purposes. + /// Can be used with the outputs of `getUserTeams` and `getFunctionAllowedTeams`. + function enumerateTeams(bytes32 teams) external view returns (EnumeratedTeam[] memory); +} + +contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefaultAdminRules */ { + mapping (uint8 => string) private _teamName; + mapping (address => bytes32) private _userTeams; + mapping (bytes32 => mapping (bytes4 => bytes32)) private _allowedTeams; + mapping (address => bytes32) private _contractGroup; + mapping (bytes32 => bytes32) private _groupAdmin; + + event TeamUpdated(uint8 indexed team, string name); + event ContractGroupAdminUpdated(bytes32 indexed group, bytes32 indexed adminRole); + event TargetGroupUpdated(address indexed target, bytes32 indexed group); + + function canCall(address caller, address target, bytes4 selector) public view returns (bool) { + bytes32 group = getContractGroup(target); + bytes32 allowedTeams = getFunctionAllowedTeams(group, selector); + bytes32 callerTeams = getUserTeams(caller); + return callerTeams & allowedTeams != 0; + } + + function createTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { + require(bytes(_teamName[team]).length == 0); + _updateTeam(team, name); + } + + function updateTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { + require(bytes(_teamName[team]).length > 0); + _updateTeam(team, name); + } + + function _updateTeam(uint8 team, string calldata name) internal virtual { + require(bytes(name).length > 0); + _teamName[team] = name; + emit TeamUpdated(team, name); + } + + function getTeamName(uint8 team) public view virtual returns (string memory) { + return _teamName[team]; + } + + function getUserTeams(address user) public view virtual returns (bytes32) { + return _userTeams[user]; + } + + function _grantRole(bytes32 role, address user) internal virtual override { + super._grantRole(role, user); + (bool isTeam, uint8 team) = _parseTeamRole(role); + if (isTeam) { + bytes32 mask = bytes32(1 << team); + _userTeams[user] |= mask; + } + } + + function _revokeRole(bytes32 role, address user) internal virtual override { + super._revokeRole(role, user); + (bool isTeam, uint8 team) = _parseTeamRole(role); + if (isTeam) { + bytes32 mask = bytes32(1 << team); + _userTeams[user] &= ~mask; + } + } + + function _parseTeamRole(bytes32 role) internal virtual returns (bool, uint8) { + bool isTeam = bytes1(role) == hex"01"; + uint8 team = uint8(uint256(role & hex"ff")); + return (isTeam, team); + } + + function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { + return _allowedTeams[group][selector]; + } + + function addFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { + bytes32 mask = bytes32(1 << uint8(team)); + _allowedTeams[group][selector] |= mask; + } + + function removeFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { + bytes32 mask = bytes32(1 << uint8(team)); + _allowedTeams[group][selector] &= ~mask; + } + + function getContractGroup(address target) public view virtual returns (bytes32) { + return _contractGroup[target]; + } + + function setContractGroup(address target, bytes32 group) public virtual onlyDefaultAdmin { + _contractGroup[target] = group; + } + + function setInitialContractGroup(address target, bytes32 group) public virtual { + _checkRole(getContractGroupAdmin(group)); + + require(group != 0); // todo: make group optional, default to target-specific group + require(_contractGroup[target] == 0); + _contractGroup[target] = group; + + emit TargetGroupUpdated(target, group); + } + + function getContractGroupAdmin(bytes32 group) public view virtual returns (bytes32) { + bytes32 admin = _groupAdmin[group]; + if (admin != 0) { + return admin; + } else { + return DEFAULT_ADMIN_ROLE; + } + } + + function setContractGroupAdmin(bytes32 group, bytes32 adminRole) public virtual onlyDefaultAdmin { + _groupAdmin[group] = adminRole; + emit ContractGroupAdminUpdated(group, adminRole); + } + + function setRoleAdmin(bytes32 role, bytes32 adminRole) public virtual onlyDefaultAdmin { + // todo: validate that the roles "exist"? + _setRoleAdmin(role, adminRole); + } + + function enumerateTeams(bytes32 roles) public view virtual returns (EnumeratedTeam[] memory) { + EnumeratedTeam[] memory roleList = new EnumeratedTeam[](256); + + uint nextPos = 0; + + for (uint256 id = 0; id < 256; id += 1) { + bytes32 mask = bytes32(1 << id); + if (roles & mask != 0) { + roleList[nextPos].id = uint8(id); + roleList[nextPos].name = getTeamName(uint8(id)); + nextPos += 1; + } + } + + // todo: shrink array to length = nextPos and reset free pointer, if safe to do so + + return roleList; + } +} From b54b9222b32b6a8a1ac2b7e693d785169f2ae212 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 1 Mar 2023 01:06:02 -0300 Subject: [PATCH 02/78] docs --- contracts/access/AccessManager.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index c70d448ef1c..f6918781b5a 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -14,11 +14,12 @@ import "./AccessControlDefaultAdminRules.sol"; /// the form [specific format TBD, something like " ... "]. /// /// Contracts in the system are also grouped. These are simply called "contract groups". There can be an arbitrary -/// number of groups. Each group has a group admin role that is allowed to add new contracts in the system into that -/// contract group. +/// number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned +/// to its own separate group, but groups can also be shared among similar contracts. Each group has a group admin role +/// whose members are allowed to add new contracts in the system into that contract group. /// /// All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between -/// functions and allowed teams. Each function can be allowed for multiple teams, meaning that if a user is in at least +/// functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least /// one of the allowed teams they can call that funcion. /// /// TODO: Implement AccessMode (from zkSync AllowList) in terms of teams and groups. From 5a0fd3337deff3507a52ca8896f564f8119056d8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 1 Mar 2023 01:11:02 -0300 Subject: [PATCH 03/78] enforce team existence --- contracts/access/AccessManager.sol | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index f6918781b5a..72381238e3e 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -87,21 +87,25 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault } function createTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { - require(bytes(_teamName[team]).length == 0); + require(!_teamExists(team)); _updateTeam(team, name); } function updateTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { - require(bytes(_teamName[team]).length > 0); + require(_teamExists(team)); _updateTeam(team, name); } function _updateTeam(uint8 team, string calldata name) internal virtual { - require(bytes(name).length > 0); + require(_teamExists(team)); _teamName[team] = name; emit TeamUpdated(team, name); } + function _teamExists(uint8 team) internal virtual returns (bool) { + return bytes(_teamName[team]).length > 0; + } + function getTeamName(uint8 team) public view virtual returns (string memory) { return _teamName[team]; } @@ -114,6 +118,7 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault super._grantRole(role, user); (bool isTeam, uint8 team) = _parseTeamRole(role); if (isTeam) { + require(_teamExists(team)); bytes32 mask = bytes32(1 << team); _userTeams[user] |= mask; } @@ -123,6 +128,7 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault super._revokeRole(role, user); (bool isTeam, uint8 team) = _parseTeamRole(role); if (isTeam) { + require(_teamExists(team)); bytes32 mask = bytes32(1 << team); _userTeams[user] &= ~mask; } From ee5de96bac7495bd6b18a183cf32c7ec4038b5a4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 15 Mar 2023 11:21:11 -0300 Subject: [PATCH 04/78] remove on-chain enumerability --- contracts/access/AccessManager.sol | 65 ++++++++---------------------- 1 file changed, 17 insertions(+), 48 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index 72381238e3e..b88d352b6fe 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -33,8 +33,6 @@ interface IAccessManager { function updateTeam(uint8 team, string calldata name) external; - function getTeamName(uint8 team) external view returns (string calldata name); - // The result is a bit mask. function getUserTeams(address user) external view returns (bytes32 teams); @@ -56,20 +54,10 @@ interface IAccessManager { function setContractGroupAdmin(bytes32 group, bytes32 adminRole) external; function setRoleAdmin(bytes32 role, bytes32 adminRole) external; - - struct EnumeratedTeam { - uint8 id; - string name; - } - - /// Decodes a bit mask into array of team ids and names. - /// Meant for user interface purposes. - /// Can be used with the outputs of `getUserTeams` and `getFunctionAllowedTeams`. - function enumerateTeams(bytes32 teams) external view returns (EnumeratedTeam[] memory); } contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefaultAdminRules */ { - mapping (uint8 => string) private _teamName; + bytes32 _createdTeams; mapping (address => bytes32) private _userTeams; mapping (bytes32 => mapping (bytes4 => bytes32)) private _allowedTeams; mapping (address => bytes32) private _contractGroup; @@ -88,26 +76,16 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault function createTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { require(!_teamExists(team)); - _updateTeam(team, name); + emit TeamUpdated(team, name); } function updateTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { require(_teamExists(team)); - _updateTeam(team, name); - } - - function _updateTeam(uint8 team, string calldata name) internal virtual { - require(_teamExists(team)); - _teamName[team] = name; emit TeamUpdated(team, name); } function _teamExists(uint8 team) internal virtual returns (bool) { - return bytes(_teamName[team]).length > 0; - } - - function getTeamName(uint8 team) public view virtual returns (string memory) { - return _teamName[team]; + return _getBit(_createdTeams, team); } function getUserTeams(address user) public view virtual returns (bytes32) { @@ -119,8 +97,7 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault (bool isTeam, uint8 team) = _parseTeamRole(role); if (isTeam) { require(_teamExists(team)); - bytes32 mask = bytes32(1 << team); - _userTeams[user] |= mask; + _userTeams[user] = _setBit(_userTeams[user], team, true); } } @@ -129,8 +106,7 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault (bool isTeam, uint8 team) = _parseTeamRole(role); if (isTeam) { require(_teamExists(team)); - bytes32 mask = bytes32(1 << team); - _userTeams[user] &= ~mask; + _userTeams[user] = _setBit(_userTeams[user], team, false); } } @@ -145,13 +121,11 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault } function addFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { - bytes32 mask = bytes32(1 << uint8(team)); - _allowedTeams[group][selector] |= mask; + _allowedTeams[group][selector] = _setBit(_allowedTeams[group][selector], team, true); } function removeFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { - bytes32 mask = bytes32(1 << uint8(team)); - _allowedTeams[group][selector] &= ~mask; + _allowedTeams[group][selector] = _setBit(_allowedTeams[group][selector], team, false); } function getContractGroup(address target) public view virtual returns (bytes32) { @@ -191,22 +165,17 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault _setRoleAdmin(role, adminRole); } - function enumerateTeams(bytes32 roles) public view virtual returns (EnumeratedTeam[] memory) { - EnumeratedTeam[] memory roleList = new EnumeratedTeam[](256); - - uint nextPos = 0; + function _getBit(bytes32 bitmap, uint8 pos) private pure returns (bool) { + bytes32 mask = bytes32(1 << pos); + return bitmap & mask > 0; + } - for (uint256 id = 0; id < 256; id += 1) { - bytes32 mask = bytes32(1 << id); - if (roles & mask != 0) { - roleList[nextPos].id = uint8(id); - roleList[nextPos].name = getTeamName(uint8(id)); - nextPos += 1; - } + function _setBit(bytes32 bitmap, uint8 pos, bool val) private pure returns (bytes32) { + bytes32 mask = bytes32(1 << pos); + if (val) { + return bitmap | mask; + } else { + return bitmap & ~mask; } - - // todo: shrink array to length = nextPos and reset free pointer, if safe to do so - - return roleList; } } From 6284768cad477350bd7d886a3f8862fcb3e51563 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 15 Mar 2023 11:24:54 -0300 Subject: [PATCH 05/78] add Authority interface --- contracts/access/AccessManager.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index b88d352b6fe..24ca48f4c8d 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -5,6 +5,10 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; import "./AccessControlDefaultAdminRules.sol"; +interface IAuthority { + function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); +} + /// AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, i.e. /// it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set of /// roles, with a particular structure. @@ -26,9 +30,7 @@ import "./AccessControlDefaultAdminRules.sol"; /// - Define the built-in "all" team (#255?) of which everyone is a member. /// - Define a contract group where every function is allowed to the "all" group. (-> AccessMode = Open) /// - Define a contract group where no function is allowed to any group. (-> AccessMode = Closed) -interface IAccessManager { - function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); - +interface IAccessManager is IAuthority { function createTeam(uint8 team, string calldata name) external; function updateTeam(uint8 team, string calldata name) external; From bce3641e62d33de7a54928f375a3222e9ccd26a8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 15 Mar 2023 11:25:13 -0300 Subject: [PATCH 06/78] remove redundancy --- contracts/access/AccessManager.sol | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index 24ca48f4c8d..cbe1978360e 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -149,12 +149,7 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault } function getContractGroupAdmin(bytes32 group) public view virtual returns (bytes32) { - bytes32 admin = _groupAdmin[group]; - if (admin != 0) { - return admin; - } else { - return DEFAULT_ADMIN_ROLE; - } + return _groupAdmin[group]; } function setContractGroupAdmin(bytes32 group, bytes32 adminRole) public virtual onlyDefaultAdmin { From 223e6af9dfda390f6438579185145ab130587435 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 00:36:08 -0300 Subject: [PATCH 07/78] simplify and complete features --- contracts/access/AccessManager.sol | 222 ++++++++++++++++++++--------- 1 file changed, 152 insertions(+), 70 deletions(-) diff --git a/contracts/access/AccessManager.sol b/contracts/access/AccessManager.sol index cbe1978360e..c57ed430281 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/AccessManager.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.0; import "./AccessControl.sol"; import "./AccessControlDefaultAdminRules.sol"; +import "../utils/Create2.sol"; +import "../utils/Address.sol"; interface IAuthority { function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); @@ -25,49 +27,58 @@ interface IAuthority { /// All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between /// functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least /// one of the allowed teams they can call that funcion. -/// -/// TODO: Implement AccessMode (from zkSync AllowList) in terms of teams and groups. -/// - Define the built-in "all" team (#255?) of which everyone is a member. -/// - Define a contract group where every function is allowed to the "all" group. (-> AccessMode = Open) -/// - Define a contract group where no function is allowed to any group. (-> AccessMode = Closed) interface IAccessManager is IAuthority { + event TeamNameUpdated(uint8 indexed team, string name); + + event TeamAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed team, bool allowed); + + event ContractGroupUpdated(address indexed target, bytes32 indexed group); + function createTeam(uint8 team, string calldata name) external; - function updateTeam(uint8 team, string calldata name) external; + function updateTeamName(uint8 team, string calldata name) external; // The result is a bit mask. function getUserTeams(address user) external view returns (bytes32 teams); + function grantTeam(address user, uint8 team) external; + + function revokeTeam(address user, uint8 team) external; + + function renounceTeam(address user, uint8 team) external; + // The result is a bit mask. function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); - function addFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) external; - - function removeFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) external; + function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) external; function getContractGroup(address target) external view returns (bytes32 group); function setContractGroup(address target, bytes32 group) external; - function setInitialContractGroup(address target, bytes32 group) external; + function setContractOpen(address target) external; - function getContractGroupAdmin(bytes32 group) external view returns (bytes32 adminRole); - - function setContractGroupAdmin(bytes32 group, bytes32 adminRole) external; - - function setRoleAdmin(bytes32 role, bytes32 adminRole) external; + function setContractClosed(address target) external; } -contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefaultAdminRules */ { +contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdTeams; mapping (address => bytes32) private _userTeams; mapping (bytes32 => mapping (bytes4 => bytes32)) private _allowedTeams; mapping (address => bytes32) private _contractGroup; - mapping (bytes32 => bytes32) private _groupAdmin; - event TeamUpdated(uint8 indexed team, string name); - event ContractGroupAdminUpdated(bytes32 indexed group, bytes32 indexed adminRole); - event TargetGroupUpdated(address indexed target, bytes32 indexed group); + uint8 private constant _TEAM_ALL = 255; + bytes32 private constant _GROUP_OPEN = "group:open"; + bytes32 private constant _GROUP_CLOSED = "group:closed"; + + constructor( + uint48 initialDefaultAdminDelay, + address initialDefaultAdmin + ) + AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) + { + createTeam(_TEAM_ALL, "all"); + } function canCall(address caller, address target, bytes4 selector) public view returns (bool) { bytes32 group = getContractGroup(target); @@ -76,103 +87,174 @@ contract AccessManager is IAccessManager, AccessControl /*, AccessControlDefault return callerTeams & allowedTeams != 0; } - function createTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { + function createTeam(uint8 team, string memory name) public virtual onlyDefaultAdmin { require(!_teamExists(team)); - emit TeamUpdated(team, name); + _setTeam(_createdTeams, team, true); + emit TeamNameUpdated(team, name); } - function updateTeam(uint8 team, string calldata name) public virtual onlyDefaultAdmin { - require(_teamExists(team)); - emit TeamUpdated(team, name); + function updateTeamName(uint8 team, string calldata name) public virtual onlyDefaultAdmin { + require(team != _TEAM_ALL && _teamExists(team)); + emit TeamNameUpdated(team, name); } function _teamExists(uint8 team) internal virtual returns (bool) { - return _getBit(_createdTeams, team); + return _getTeam(_createdTeams, team); } function getUserTeams(address user) public view virtual returns (bytes32) { - return _userTeams[user]; + return _userTeams[user] | _teamMask(_TEAM_ALL); + } + + function grantTeam(address user, uint8 team) public virtual { + grantRole(_encodeTeamRole(team), user); + } + + function revokeTeam(address user, uint8 team) public virtual { + revokeRole(_encodeTeamRole(team), user); + } + + function renounceTeam(address user, uint8 team) public virtual { + renounceRole(_encodeTeamRole(team), user); + } + + function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { + if (group == _GROUP_OPEN) { + return _teamMask(_TEAM_ALL); + } else if (group == _GROUP_CLOSED) { + return 0; + } else { + return _allowedTeams[group][selector]; + } + } + + function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) public virtual onlyDefaultAdmin { + _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); + emit TeamAllowed(group, selector, team, allowed); + } + + function getContractGroup(address target) public view virtual returns (bytes32) { + return _contractGroup[target]; + } + + function setContractGroup(address target, bytes32 group) public virtual onlyDefaultAdmin { + require(!_isSpecialGroup(group)); + _contractGroup[target] = group; + emit ContractGroupUpdated(target, group); + } + + function setContractOpen(address target) public virtual onlyDefaultAdmin { + bytes32 group = _GROUP_OPEN; + _contractGroup[target] = group; + emit ContractGroupUpdated(target, group); + } + + function setContractClosed(address target) public virtual onlyDefaultAdmin { + bytes32 group = _GROUP_CLOSED; + _contractGroup[target] = group; + emit ContractGroupUpdated(target, group); } function _grantRole(bytes32 role, address user) internal virtual override { super._grantRole(role, user); - (bool isTeam, uint8 team) = _parseTeamRole(role); + (bool isTeam, uint8 team) = _decodeTeamRole(role); if (isTeam) { require(_teamExists(team)); - _userTeams[user] = _setBit(_userTeams[user], team, true); + _userTeams[user] = _setTeam(_userTeams[user], team, true); } } function _revokeRole(bytes32 role, address user) internal virtual override { super._revokeRole(role, user); - (bool isTeam, uint8 team) = _parseTeamRole(role); + (bool isTeam, uint8 team) = _decodeTeamRole(role); if (isTeam) { require(_teamExists(team)); - _userTeams[user] = _setBit(_userTeams[user], team, false); + require(team != _TEAM_ALL); + _userTeams[user] = _setTeam(_userTeams[user], team, false); } } - function _parseTeamRole(bytes32 role) internal virtual returns (bool, uint8) { - bool isTeam = bytes1(role) == hex"01"; - uint8 team = uint8(uint256(role & hex"ff")); - return (isTeam, team); + function _encodeTeamRole(uint8 role) internal virtual returns (bytes32) { + return bytes32("team:") | bytes32(uint256(role)); } - function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { - return _allowedTeams[group][selector]; + function _decodeTeamRole(bytes32 role) internal virtual returns (bool, uint8) { + bytes32 tagMask = hex"ffffffff"; + bytes32 teamMask = hex"ff"; + bool isTeam = (role & tagMask) == bytes32("team:"); + uint8 team = uint8(uint256(role & teamMask)); + return (isTeam, team); } - function addFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { - _allowedTeams[group][selector] = _setBit(_allowedTeams[group][selector], team, true); + function _isSpecialGroup(bytes32 group) private pure returns (bool) { + return group == _GROUP_OPEN || group == _GROUP_CLOSED; } - function removeFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team) public virtual onlyDefaultAdmin { - _allowedTeams[group][selector] = _setBit(_allowedTeams[group][selector], team, false); + function _teamMask(uint8 team) private pure returns (bytes32) { + return bytes32(1 << team); } - function getContractGroup(address target) public view virtual returns (bytes32) { - return _contractGroup[target]; + function _getTeam(bytes32 bitmap, uint8 team) private pure returns (bool) { + return bitmap & _teamMask(team) > 0; } - function setContractGroup(address target, bytes32 group) public virtual onlyDefaultAdmin { - _contractGroup[target] = group; + function _setTeam(bytes32 bitmap, uint8 team, bool value) private pure returns (bytes32) { + bytes32 mask = _teamMask(team); + if (value) { + return bitmap | mask; + } else { + return bitmap & ~mask; + } } +} - function setInitialContractGroup(address target, bytes32 group) public virtual { - _checkRole(getContractGroupAdmin(group)); - - require(group != 0); // todo: make group optional, default to target-specific group - require(_contractGroup[target] == 0); - _contractGroup[target] = group; +contract AccessManaged { + event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); - emit TargetGroupUpdated(target, group); - } + IAuthority private _authority; - function getContractGroupAdmin(bytes32 group) public view virtual returns (bytes32) { - return _groupAdmin[group]; + modifier restricted { + require(_authority.canCall(msg.sender, address(this), msg.sig)); + _; } - function setContractGroupAdmin(bytes32 group, bytes32 adminRole) public virtual onlyDefaultAdmin { - _groupAdmin[group] = adminRole; - emit ContractGroupAdminUpdated(group, adminRole); + constructor(IAuthority initialAuthority) { + _authority = initialAuthority; } - function setRoleAdmin(bytes32 role, bytes32 adminRole) public virtual onlyDefaultAdmin { - // todo: validate that the roles "exist"? - _setRoleAdmin(role, adminRole); + function authority() public virtual view returns (IAuthority) { + return _authority; } - function _getBit(bytes32 bitmap, uint8 pos) private pure returns (bool) { - bytes32 mask = bytes32(1 << pos); - return bitmap & mask > 0; + function setAuthority(IAuthority newAuthority) public virtual { + require(msg.sender == address(_authority)); + IAuthority oldAuthority = _authority; + _authority = newAuthority; + emit AuthorityUpdated(oldAuthority, newAuthority); } +} - function _setBit(bytes32 bitmap, uint8 pos, bool val) private pure returns (bytes32) { - bytes32 mask = bytes32(1 << pos); - if (val) { - return bitmap | mask; - } else { - return bitmap & ~mask; +contract AccessManagerAdapter { + using Address for address; + + AccessManager private _manager; + + bytes32 private _DEFAULT_ADMIN_ROLE = 0; + + function relay(address target, bytes memory data) external payable { + bytes4 sig = bytes4(data); + require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); + (bool ok, bytes memory result) = target.call{value: msg.value}(data); + assembly { + let result_pointer := add(32, result) + let result_size := mload(result) + switch ok + case true { + return(result_pointer, result_size) + } + default { + revert(result_pointer, result_size) + } } } } From eafb571db1362b5bd24775cdb405996d6638bd3d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 00:38:40 -0300 Subject: [PATCH 08/78] split in separate files --- contracts/access/manager/AccessManaged.sol | 31 +++++++++ .../access/{ => manager}/AccessManager.sol | 63 ++----------------- .../access/manager/AccessManagerAdapter.sol | 31 +++++++++ contracts/access/manager/IAuthority.sol | 7 +++ 4 files changed, 73 insertions(+), 59 deletions(-) create mode 100644 contracts/access/manager/AccessManaged.sol rename contracts/access/{ => manager}/AccessManager.sol (81%) create mode 100644 contracts/access/manager/AccessManagerAdapter.sol create mode 100644 contracts/access/manager/IAuthority.sol diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol new file mode 100644 index 00000000000..553a5a56c44 --- /dev/null +++ b/contracts/access/manager/AccessManaged.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./IAuthority.sol"; + +contract AccessManaged { + event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); + + IAuthority private _authority; + + modifier restricted { + require(_authority.canCall(msg.sender, address(this), msg.sig)); + _; + } + + constructor(IAuthority initialAuthority) { + _authority = initialAuthority; + } + + function authority() public virtual view returns (IAuthority) { + return _authority; + } + + function setAuthority(IAuthority newAuthority) public virtual { + require(msg.sender == address(_authority)); + IAuthority oldAuthority = _authority; + _authority = newAuthority; + emit AuthorityUpdated(oldAuthority, newAuthority); + } +} diff --git a/contracts/access/AccessManager.sol b/contracts/access/manager/AccessManager.sol similarity index 81% rename from contracts/access/AccessManager.sol rename to contracts/access/manager/AccessManager.sol index c57ed430281..f23f8c99f5b 100644 --- a/contracts/access/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -2,14 +2,10 @@ pragma solidity ^0.8.0; -import "./AccessControl.sol"; -import "./AccessControlDefaultAdminRules.sol"; -import "../utils/Create2.sol"; -import "../utils/Address.sol"; - -interface IAuthority { - function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); -} +import "../AccessControl.sol"; +import "../AccessControlDefaultAdminRules.sol"; +import "../../utils/Create2.sol"; +import "./IAuthority.sol"; /// AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, i.e. /// it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set of @@ -207,54 +203,3 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } } - -contract AccessManaged { - event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); - - IAuthority private _authority; - - modifier restricted { - require(_authority.canCall(msg.sender, address(this), msg.sig)); - _; - } - - constructor(IAuthority initialAuthority) { - _authority = initialAuthority; - } - - function authority() public virtual view returns (IAuthority) { - return _authority; - } - - function setAuthority(IAuthority newAuthority) public virtual { - require(msg.sender == address(_authority)); - IAuthority oldAuthority = _authority; - _authority = newAuthority; - emit AuthorityUpdated(oldAuthority, newAuthority); - } -} - -contract AccessManagerAdapter { - using Address for address; - - AccessManager private _manager; - - bytes32 private _DEFAULT_ADMIN_ROLE = 0; - - function relay(address target, bytes memory data) external payable { - bytes4 sig = bytes4(data); - require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); - (bool ok, bytes memory result) = target.call{value: msg.value}(data); - assembly { - let result_pointer := add(32, result) - let result_size := mload(result) - switch ok - case true { - return(result_pointer, result_size) - } - default { - revert(result_pointer, result_size) - } - } - } -} diff --git a/contracts/access/manager/AccessManagerAdapter.sol b/contracts/access/manager/AccessManagerAdapter.sol new file mode 100644 index 00000000000..501178ba0ff --- /dev/null +++ b/contracts/access/manager/AccessManagerAdapter.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./AccessManager.sol"; +import "../../utils/Address.sol"; + +contract AccessManagerAdapter { + using Address for address; + + AccessManager private _manager; + + bytes32 private _DEFAULT_ADMIN_ROLE = 0; + + function relay(address target, bytes memory data) external payable { + bytes4 sig = bytes4(data); + require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); + (bool ok, bytes memory result) = target.call{value: msg.value}(data); + assembly { + let result_pointer := add(32, result) + let result_size := mload(result) + switch ok + case true { + return(result_pointer, result_size) + } + default { + revert(result_pointer, result_size) + } + } + } +} diff --git a/contracts/access/manager/IAuthority.sol b/contracts/access/manager/IAuthority.sol new file mode 100644 index 00000000000..6546ee2d356 --- /dev/null +++ b/contracts/access/manager/IAuthority.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +interface IAuthority { + function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); +} From 24af15962b76844c98c67ddc353aa64ff33674bf Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 01:38:32 -0300 Subject: [PATCH 09/78] write AccessManager docs --- contracts/access/manager/AccessManager.sol | 177 ++++++++++++++++----- 1 file changed, 136 insertions(+), 41 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index f23f8c99f5b..1b8be8ad0fe 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -7,56 +7,55 @@ import "../AccessControlDefaultAdminRules.sol"; import "../../utils/Create2.sol"; import "./IAuthority.sol"; -/// AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, i.e. -/// it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set of -/// roles, with a particular structure. -/// -/// Users are grouped in "teams". Teams must be created before users can be assigned into them, up to a maximum of 256 -/// teams. A user can be assigned to multiple teams. Each team defines an AccessControl role, identified by a role id of -/// the form [specific format TBD, something like " ... "]. -/// -/// Contracts in the system are also grouped. These are simply called "contract groups". There can be an arbitrary -/// number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned -/// to its own separate group, but groups can also be shared among similar contracts. Each group has a group admin role -/// whose members are allowed to add new contracts in the system into that contract group. -/// -/// All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between -/// functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least -/// one of the allowed teams they can call that funcion. interface IAccessManager is IAuthority { - event TeamNameUpdated(uint8 indexed team, string name); - + event TeamUpdated(uint8 indexed team, string name); event TeamAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed team, bool allowed); - event ContractGroupUpdated(address indexed target, bytes32 indexed group); function createTeam(uint8 team, string calldata name) external; - function updateTeamName(uint8 team, string calldata name) external; - - // The result is a bit mask. function getUserTeams(address user) external view returns (bytes32 teams); - function grantTeam(address user, uint8 team) external; - function revokeTeam(address user, uint8 team) external; - function renounceTeam(address user, uint8 team) external; - - // The result is a bit mask. function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); - function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) external; - function getContractGroup(address target) external view returns (bytes32 group); - function setContractGroup(address target, bytes32 group) external; - function setContractOpen(address target) external; - function setContractClosed(address target) external; } +/** + * @dev AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, + * i.e. it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set + * of roles, with a particular structure. + * + * Users are grouped in "teams". Teams must be created before users can be assigned into them, up to a maximum of 255 + * teams. A user can be assigned to multiple teams. Each team defines an AccessControl role, identified by a role id + * that starts with the ASCII characters `team:`, followed by zeroes, and ending with the single byte corresponding to + * the team number. + * + * Contracts in the system are grouped as well. These are simply called "contract groups". There can be an arbitrary + * number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned + * to its own separate group, but groups can also be shared among similar contracts. + * + * All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between + * functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least + * one of the allowed teams they can call that funcion. + * + * Note that a function in a target contract may become permissioned only when: 1) said contract is {AccessManaged} and + * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. + * + * There is a special team defined by default named "all" of which all accounts are considered members, and two special + * contract grups: 1) the "open" team, where all functions are allowed to the "all" team, and 2) the "closed" team, + * where no function is allowed to any team. + * + * Permissioning schemes and team and contract group assignments can be configured by the default admin. The contract + * includes {AccessControlDefaultAdminRules} by default to enforce security rules on this account. Additionally, it is + * expected that the account will be highly secured (e.g., a multisig or a well-configured DAO) as all the permissions + * of the managed system can be modified by it. + */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdTeams; mapping (address => bytes32) private _userTeams; @@ -64,9 +63,13 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { mapping (address => bytes32) private _contractGroup; uint8 private constant _TEAM_ALL = 255; + bytes32 private constant _GROUP_UNSET = 0; bytes32 private constant _GROUP_OPEN = "group:open"; bytes32 private constant _GROUP_CLOSED = "group:closed"; + /** + * @dev Initializes an AccessManager with initial default admin and transfer delay. + */ constructor( uint48 initialDefaultAdminDelay, address initialDefaultAdmin @@ -76,6 +79,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { createTeam(_TEAM_ALL, "all"); } + /** + * @dev Returns true if the caller can call on the target the function identified by a function selector. + * Entrypoint for {AccessManaged} contracts. + */ function canCall(address caller, address target, bytes4 selector) public view returns (bool) { bytes32 group = getContractGroup(target); bytes32 allowedTeams = getFunctionAllowedTeams(group, selector); @@ -83,37 +90,68 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return callerTeams & allowedTeams != 0; } + /** + * @dev Creates a new team with a team number that can be chosen arbitarily but must be unused, and gives it a + * human-readable name. The caller must be the default admin. + * + * Emits {TeamUpdated}. + */ function createTeam(uint8 team, string memory name) public virtual onlyDefaultAdmin { require(!_teamExists(team)); _setTeam(_createdTeams, team, true); - emit TeamNameUpdated(team, name); + emit TeamUpdated(team, name); } + /** + * @dev Updates an existing team's name. The caller must be the default admin. + */ function updateTeamName(uint8 team, string calldata name) public virtual onlyDefaultAdmin { require(team != _TEAM_ALL && _teamExists(team)); - emit TeamNameUpdated(team, name); - } - - function _teamExists(uint8 team) internal virtual returns (bool) { - return _getTeam(_createdTeams, team); + emit TeamUpdated(team, name); } + /** + * @dev Returns a bitmap of the teams the user is a member of. Bit `n` is set if the user is in team `n`, + * counting from least significant bit. + */ function getUserTeams(address user) public view virtual returns (bytes32) { return _userTeams[user] | _teamMask(_TEAM_ALL); } + /** + * @dev Adds a user to a team. + * + * Emits {RoleGranted} with the role id of the team, if not previously a member. + */ function grantTeam(address user, uint8 team) public virtual { + // grantRole checks that msg.sender is admin for the role grantRole(_encodeTeamRole(team), user); } + /** + * @dev Removes a user from a team. + * + * Emits {RoleRevoked} with the role id of the team, if previously a member. + */ function revokeTeam(address user, uint8 team) public virtual { + // revokeRole checks that msg.sender is admin for the role revokeRole(_encodeTeamRole(team), user); } + /** + * @dev Renounces a user from a team. + * + * Emits {RoleRevoked} with the role id of the team, if previously a member. + */ function renounceTeam(address user, uint8 team) public virtual { + // renounceRole checks that msg.sender is user renounceRole(_encodeTeamRole(team), user); } + /** + * @dev Returns a bitmap of the teams that are allowed to call a function selector on contracts belonging to a + * group. Bit `n` is set if team `n` is allowed, counting from least significant bit. + */ function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { if (group == _GROUP_OPEN) { return _teamMask(_TEAM_ALL); @@ -124,33 +162,66 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } + /** + * @dev Changes whether a team is allowed to call a function selector on contracts belonging to a group, according + * to the `allowed` argument. The caller must be the default admin. + */ function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) public virtual onlyDefaultAdmin { + require(group != 0); _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); emit TeamAllowed(group, selector, team, allowed); } + /** + * @dev Returns the contract group that the target contract currently belongs to. May be 0 if not set. + */ function getContractGroup(address target) public view virtual returns (bytes32) { return _contractGroup[target]; } + /** + * @dev Sets the contract group that the target belongs to. The caller must be the default admin. + * + * CAUTION: This is a batch operation that will immediately switch the mapping of functions to allowed teams. + * Accounts that were previously able to call permissioned functions on the target contract may no longer be + * allowed, and new sets of account may be allowed after this operation. + */ function setContractGroup(address target, bytes32 group) public virtual onlyDefaultAdmin { require(!_isSpecialGroup(group)); _contractGroup[target] = group; emit ContractGroupUpdated(target, group); } + /** + * @dev Sets the target contract to be in the "open" group. All restricted functions in the target contract will + * become callable by anyone. The caller must be the default admin. + */ function setContractOpen(address target) public virtual onlyDefaultAdmin { bytes32 group = _GROUP_OPEN; _contractGroup[target] = group; emit ContractGroupUpdated(target, group); } + /** + * @dev Sets the target contract to be in the "closed" group. All restricted functions in the target contract will + * be closed down and disallowed to all. The caller must be the default admin. + */ function setContractClosed(address target) public virtual onlyDefaultAdmin { bytes32 group = _GROUP_CLOSED; _contractGroup[target] = group; emit ContractGroupUpdated(target, group); } + /** + * @dev Returns true if the team has already been created via {createTeam}. + */ + function _teamExists(uint8 team) internal virtual returns (bool) { + return _getTeam(_createdTeams, team); + } + + /** + * @dev Augmented version of {AccessControl-_grantRole} that keeps track of user team bitmaps. + */ function _grantRole(bytes32 role, address user) internal virtual override { super._grantRole(role, user); (bool isTeam, uint8 team) = _decodeTeamRole(role); @@ -160,6 +231,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } + /** + * @dev Augmented version of {AccessControl-_revokeRole} that keeps track of user team bitmaps. + */ function _revokeRole(bytes32 role, address user) internal virtual override { super._revokeRole(role, user); (bool isTeam, uint8 team) = _decodeTeamRole(role); @@ -170,10 +244,19 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } - function _encodeTeamRole(uint8 role) internal virtual returns (bytes32) { - return bytes32("team:") | bytes32(uint256(role)); + /** + * @dev Returns the {AccessControl} role id that corresponds to a team. + * + * This role id starts with the ASCII characters `team:`, followed by zeroes, and ends with the single byte + * corresponding to the team number. + */ + function _encodeTeamRole(uint8 team) internal virtual returns (bytes32) { + return bytes32("team:") | bytes32(uint256(team)); } + /** + * @dev Decodes a role id into a team, if it is a role id of the kind returned by {_encodeTeamRole}. + */ function _decodeTeamRole(bytes32 role) internal virtual returns (bool, uint8) { bytes32 tagMask = hex"ffffffff"; bytes32 teamMask = hex"ff"; @@ -182,18 +265,30 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return (isTeam, team); } + /** + * @dev Returns true if the group is one of "open", "closed", or unset (zero). + */ function _isSpecialGroup(bytes32 group) private pure returns (bool) { - return group == _GROUP_OPEN || group == _GROUP_CLOSED; + return group == _GROUP_UNSET || group == _GROUP_OPEN || group == _GROUP_CLOSED; } + /** + * @dev Returns a bit mask where the only non-zero bit is the team number bit. + */ function _teamMask(uint8 team) private pure returns (bytes32) { return bytes32(1 << team); } + /** + * @dev Returns the value of the team number bit in a bitmap. + */ function _getTeam(bytes32 bitmap, uint8 team) private pure returns (bool) { return bitmap & _teamMask(team) > 0; } + /** + * @dev Changes the value of the team number bit in a bitmap. + */ function _setTeam(bytes32 bitmap, uint8 team, bool value) private pure returns (bytes32) { bytes32 mask = _teamMask(team); if (value) { From 833f2192c44597fdcb38619f042ee79f090de9de Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 01:56:51 -0300 Subject: [PATCH 10/78] add docs for AccessManagerAdapter --- .../access/manager/AccessManagerAdapter.sol | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/contracts/access/manager/AccessManagerAdapter.sol b/contracts/access/manager/AccessManagerAdapter.sol index 501178ba0ff..b3dbfc227b8 100644 --- a/contracts/access/manager/AccessManagerAdapter.sol +++ b/contracts/access/manager/AccessManagerAdapter.sol @@ -5,6 +5,18 @@ pragma solidity ^0.8.0; import "./AccessManager.sol"; import "../../utils/Address.sol"; +/** + * @dev This contract can be used to migrate existing {Ownable} or {AccessControl} contracts into an {AccessManager} + * system. + * + * Ownable contracts can have their ownership transferred to an instance of this adapter. AccessControl contracts can + * grant all roles to the adapter, while ideally revoking them from all other accounts. Subsequently, the permissions + * for those contracts can be managed centrally and with function granularity in the {AccessManager} instance the + * adapter is connected to. + * + * Permissioned interactions with thus migrated contracts must go through the adapter's {relay} function and will + * proceed if the function is allowed for the caller in the AccessManager instance. + */ contract AccessManagerAdapter { using Address for address; @@ -12,6 +24,19 @@ contract AccessManagerAdapter { bytes32 private _DEFAULT_ADMIN_ROLE = 0; + /** + * @dev Initializes an adapter connected to an AccessManager instance. + */ + constructor(AccessManager manager) { + _manager = manager; + } + + /** + * @dev Relays a function call to the target contract. The call will be relayed if the AccessManager allows the + * caller access to this function in the target contract, i.e. if the caller is in a team that is allowed for the + * function, or if the caller is the default admin for the AccessManager. The latter is meant to be used for + * ad hoc operations such as asset recovery. + */ function relay(address target, bytes memory data) external payable { bytes4 sig = bytes4(data); require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); From 0f277390ec618ae219a6fac330ebde2dbd5b8bb4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 01:57:30 -0300 Subject: [PATCH 11/78] add to docs site --- contracts/access/README.adoc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/contracts/access/README.adoc b/contracts/access/README.adoc index 80ca0020f4b..59d5d86a312 100644 --- a/contracts/access/README.adoc +++ b/contracts/access/README.adoc @@ -25,3 +25,13 @@ This directory provides ways to restrict who can access the functions of a contr {{AccessControlEnumerable}} {{AccessControlDefaultAdminRules}} + +== AccessManager + +{{IAuthority}} + +{{AccessManager}} + +{{AccessManaged}} + +{{AccessManagerAdapter}} From 2ac5aa111d8b466aa9a643295e806b3862afd2fd Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 02:01:25 -0300 Subject: [PATCH 12/78] docs for IAuthority --- contracts/access/manager/AccessManager.sol | 2 +- contracts/access/manager/IAuthority.sol | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 1b8be8ad0fe..9d24f74fb88 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -80,7 +80,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns true if the caller can call on the target the function identified by a function selector. + * @dev Returns true if the caller can invoke on a target the function identified by a function selector. * Entrypoint for {AccessManaged} contracts. */ function canCall(address caller, address target, bytes4 selector) public view returns (bool) { diff --git a/contracts/access/manager/IAuthority.sol b/contracts/access/manager/IAuthority.sol index 6546ee2d356..61a85d2f98e 100644 --- a/contracts/access/manager/IAuthority.sol +++ b/contracts/access/manager/IAuthority.sol @@ -2,6 +2,12 @@ pragma solidity ^0.8.0; +/** + * @dev Standard interface for permissioning originally defined in Dappsys. + */ interface IAuthority { + /** + * @dev Returns true if the caller can invoke on a target the function identified by a function selector. + */ function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); } From f26c4d3abc49ee966aab3d07f5f2485f159a58d8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 02:03:57 -0300 Subject: [PATCH 13/78] lint --- contracts/access/AccessControl.sol | 2 +- contracts/access/manager/AccessManaged.sol | 4 +-- contracts/access/manager/AccessManager.sol | 34 +++++++++++++++------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index d32c4da5314..951db485062 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -72,7 +72,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { _; } - modifier onlyDefaultAdmin { + modifier onlyDefaultAdmin() { _checkRole(DEFAULT_ADMIN_ROLE); _; } diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 553a5a56c44..535f37f6e93 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -9,7 +9,7 @@ contract AccessManaged { IAuthority private _authority; - modifier restricted { + modifier restricted() { require(_authority.canCall(msg.sender, address(this), msg.sig)); _; } @@ -18,7 +18,7 @@ contract AccessManaged { _authority = initialAuthority; } - function authority() public virtual view returns (IAuthority) { + function authority() public view virtual returns (IAuthority) { return _authority; } diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 9d24f74fb88..ee1f8f73162 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -13,16 +13,27 @@ interface IAccessManager is IAuthority { event ContractGroupUpdated(address indexed target, bytes32 indexed group); function createTeam(uint8 team, string calldata name) external; + function updateTeamName(uint8 team, string calldata name) external; + function getUserTeams(address user) external view returns (bytes32 teams); + function grantTeam(address user, uint8 team) external; + function revokeTeam(address user, uint8 team) external; + function renounceTeam(address user, uint8 team) external; + function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); + function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) external; + function getContractGroup(address target) external view returns (bytes32 group); + function setContractGroup(address target, bytes32 group) external; + function setContractOpen(address target) external; + function setContractClosed(address target) external; } @@ -30,16 +41,16 @@ interface IAccessManager is IAuthority { * @dev AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, * i.e. it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set * of roles, with a particular structure. - * + * * Users are grouped in "teams". Teams must be created before users can be assigned into them, up to a maximum of 255 * teams. A user can be assigned to multiple teams. Each team defines an AccessControl role, identified by a role id * that starts with the ASCII characters `team:`, followed by zeroes, and ending with the single byte corresponding to * the team number. - * + * * Contracts in the system are grouped as well. These are simply called "contract groups". There can be an arbitrary * number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned * to its own separate group, but groups can also be shared among similar contracts. - * + * * All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between * functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least * one of the allowed teams they can call that funcion. @@ -58,9 +69,9 @@ interface IAccessManager is IAuthority { */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdTeams; - mapping (address => bytes32) private _userTeams; - mapping (bytes32 => mapping (bytes4 => bytes32)) private _allowedTeams; - mapping (address => bytes32) private _contractGroup; + mapping(address => bytes32) private _userTeams; + mapping(bytes32 => mapping(bytes4 => bytes32)) private _allowedTeams; + mapping(address => bytes32) private _contractGroup; uint8 private constant _TEAM_ALL = 255; bytes32 private constant _GROUP_UNSET = 0; @@ -73,9 +84,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { constructor( uint48 initialDefaultAdminDelay, address initialDefaultAdmin - ) - AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) - { + ) AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) { createTeam(_TEAM_ALL, "all"); } @@ -166,7 +175,12 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Changes whether a team is allowed to call a function selector on contracts belonging to a group, according * to the `allowed` argument. The caller must be the default admin. */ - function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) public virtual onlyDefaultAdmin { + function setFunctionAllowedTeam( + bytes32 group, + bytes4 selector, + uint8 team, + bool allowed + ) public virtual onlyDefaultAdmin { require(group != 0); _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); emit TeamAllowed(group, selector, team, allowed); From 63f93936d9626d5963bf1c58e12057a4930ab3e6 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 02:12:58 -0300 Subject: [PATCH 14/78] add initial AccessManaged docs --- contracts/access/manager/AccessManaged.sol | 5 +++++ contracts/access/manager/AccessManager.sol | 3 +++ 2 files changed, 8 insertions(+) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 535f37f6e93..f71d7b45b77 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -4,6 +4,11 @@ pragma solidity ^0.8.0; import "./IAuthority.sol"; +/** + * @dev This contract module makes available a {restricted} modifier. Functions decorated with this modifier will be + * permissioned according to an "authority": a contract like {AccessManager} that follows the {IAuthority} interface, + * implementing a policy that allows certain callers access to certain functions. + */ contract AccessManaged { event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index ee1f8f73162..1933a14d03b 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -35,6 +35,9 @@ interface IAccessManager is IAuthority { function setContractOpen(address target) external; function setContractClosed(address target) external; + + // TODO: Ability to eject a contract. See AccessManaged.setAuthority + // function transferContractAuthority(address target) external; } /** From 6fecd25912c2144a8a0c94e93c10bfd33b57d8a0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 02:14:53 -0300 Subject: [PATCH 15/78] typos --- contracts/access/manager/AccessManager.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 1933a14d03b..9f2b1f210a1 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -56,13 +56,13 @@ interface IAccessManager is IAuthority { * * All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between * functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least - * one of the allowed teams they can call that funcion. + * one of the allowed teams they can call that function. * * Note that a function in a target contract may become permissioned only when: 1) said contract is {AccessManaged} and * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. * * There is a special team defined by default named "all" of which all accounts are considered members, and two special - * contract grups: 1) the "open" team, where all functions are allowed to the "all" team, and 2) the "closed" team, + * contract groups: 1) the "open" team, where all functions are allowed to the "all" team, and 2) the "closed" team, * where no function is allowed to any team. * * Permissioning schemes and team and contract group assignments can be configured by the default admin. The contract @@ -103,7 +103,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Creates a new team with a team number that can be chosen arbitarily but must be unused, and gives it a + * @dev Creates a new team with a team number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. * * Emits {TeamUpdated}. From 589b5b1bfb662d12e7210cc44bb037ed1789a209 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 16 Mar 2023 13:10:48 -0300 Subject: [PATCH 16/78] Update contracts/access/manager/AccessManager.sol Co-authored-by: Hadrien Croubois --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 9f2b1f210a1..d27b075f6cf 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -267,7 +267,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * This role id starts with the ASCII characters `team:`, followed by zeroes, and ends with the single byte * corresponding to the team number. */ - function _encodeTeamRole(uint8 team) internal virtual returns (bytes32) { + function _encodeTeamRole(uint8 team) internal virtual view returns (bytes32) { return bytes32("team:") | bytes32(uint256(team)); } From 619cc7312eca7591d215fd24e56c23c42bd82f8e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 16:45:43 -0300 Subject: [PATCH 17/78] fix docs --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index d27b075f6cf..ba31e6ec14d 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -62,7 +62,7 @@ interface IAccessManager is IAuthority { * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. * * There is a special team defined by default named "all" of which all accounts are considered members, and two special - * contract groups: 1) the "open" team, where all functions are allowed to the "all" team, and 2) the "closed" team, + * contract groups: 1) the "open" group, where all functions are allowed to the "all" team, and 2) the "closed" group, * where no function is allowed to any team. * * Permissioning schemes and team and contract group assignments can be configured by the default admin. The contract From 1b681a54cc7e1b6c16d7f841c30cae510fcb19b2 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 19:07:35 -0300 Subject: [PATCH 18/78] rename team "all" -> "public" --- contracts/access/manager/AccessManager.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index ba31e6ec14d..15213091cba 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -61,9 +61,9 @@ interface IAccessManager is IAuthority { * Note that a function in a target contract may become permissioned only when: 1) said contract is {AccessManaged} and * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. * - * There is a special team defined by default named "all" of which all accounts are considered members, and two special - * contract groups: 1) the "open" group, where all functions are allowed to the "all" team, and 2) the "closed" group, - * where no function is allowed to any team. + * There is a special team defined by default named "public" of which all accounts are automatically members, and two + * special contract groups: 1) the "open" group, where all functions are allowed to the "public" team, and 2) the + * "closed" group, where no function is allowed to any team. * * Permissioning schemes and team and contract group assignments can be configured by the default admin. The contract * includes {AccessControlDefaultAdminRules} by default to enforce security rules on this account. Additionally, it is @@ -76,7 +76,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { mapping(bytes32 => mapping(bytes4 => bytes32)) private _allowedTeams; mapping(address => bytes32) private _contractGroup; - uint8 private constant _TEAM_ALL = 255; + uint8 private constant _TEAM_PUBLIC = 255; bytes32 private constant _GROUP_UNSET = 0; bytes32 private constant _GROUP_OPEN = "group:open"; bytes32 private constant _GROUP_CLOSED = "group:closed"; @@ -88,7 +88,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint48 initialDefaultAdminDelay, address initialDefaultAdmin ) AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) { - createTeam(_TEAM_ALL, "all"); + createTeam(_TEAM_PUBLIC, "public"); } /** @@ -118,7 +118,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Updates an existing team's name. The caller must be the default admin. */ function updateTeamName(uint8 team, string calldata name) public virtual onlyDefaultAdmin { - require(team != _TEAM_ALL && _teamExists(team)); + require(team != _TEAM_PUBLIC && _teamExists(team)); emit TeamUpdated(team, name); } @@ -127,7 +127,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * counting from least significant bit. */ function getUserTeams(address user) public view virtual returns (bytes32) { - return _userTeams[user] | _teamMask(_TEAM_ALL); + return _userTeams[user] | _teamMask(_TEAM_PUBLIC); } /** @@ -166,7 +166,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { */ function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { if (group == _GROUP_OPEN) { - return _teamMask(_TEAM_ALL); + return _teamMask(_TEAM_PUBLIC); } else if (group == _GROUP_CLOSED) { return 0; } else { @@ -256,7 +256,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { (bool isTeam, uint8 team) = _decodeTeamRole(role); if (isTeam) { require(_teamExists(team)); - require(team != _TEAM_ALL); + require(team != _TEAM_PUBLIC); _userTeams[user] = _setTeam(_userTeams[user], team, false); } } From 48b31c0643c3d7a50d10c46c5e9806b830f1e4f8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 19:09:09 -0300 Subject: [PATCH 19/78] ad hoc selector batching --- contracts/access/manager/AccessManager.sol | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 15213091cba..77a48ac49fe 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -26,7 +26,7 @@ interface IAccessManager is IAuthority { function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); - function setFunctionAllowedTeam(bytes32 group, bytes4 selector, uint8 team, bool allowed) external; + function setFunctionAllowedTeam(bytes32 group, bytes4[] calldata selectors, uint8 team, bool allowed) external; function getContractGroup(address target) external view returns (bytes32 group); @@ -180,13 +180,16 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { */ function setFunctionAllowedTeam( bytes32 group, - bytes4 selector, + bytes4[] calldata selectors, uint8 team, bool allowed ) public virtual onlyDefaultAdmin { require(group != 0); - _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); - emit TeamAllowed(group, selector, team, allowed); + for (uint256 i = 0; i < selectors.length; i++) { + bytes4 selector = selectors[i]; + _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); + emit TeamAllowed(group, selector, team, allowed); + } } /** From b0781579d4dd6d5be1a096f17637aa517cd06d34 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 22:45:43 -0300 Subject: [PATCH 20/78] whitespace --- contracts/access/manager/AccessManager.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 77a48ac49fe..44aa8e53697 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -9,7 +9,9 @@ import "./IAuthority.sol"; interface IAccessManager is IAuthority { event TeamUpdated(uint8 indexed team, string name); + event TeamAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed team, bool allowed); + event ContractGroupUpdated(address indexed target, bytes32 indexed group); function createTeam(uint8 team, string calldata name) external; From 274c04d45601fa90bd3f5ea1631050ed2fa262b8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 22:45:56 -0300 Subject: [PATCH 21/78] add docs on team number choice --- contracts/access/manager/AccessManager.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 44aa8e53697..0c3fa4833ad 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -108,6 +108,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Creates a new team with a team number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. * + * Team numbers are not auto-incremented in order to avoid race conditions, but administrators can safely use + * sequential numbers. + * * Emits {TeamUpdated}. */ function createTeam(uint8 team, string memory name) public virtual onlyDefaultAdmin { From 082b05c5f0904e26c0f90398d6000068476867e5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 23:20:48 -0300 Subject: [PATCH 22/78] add missing function argument --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 0c3fa4833ad..90bbcd73609 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -39,7 +39,7 @@ interface IAccessManager is IAuthority { function setContractClosed(address target) external; // TODO: Ability to eject a contract. See AccessManaged.setAuthority - // function transferContractAuthority(address target) external; + // function transferContractAuthority(address target, address newAuthority) external; } /** From 52f27c83cfaea675b8a5d0e7c3e27a9afa93262d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 23:21:45 -0300 Subject: [PATCH 23/78] fix team role decoding --- contracts/access/manager/AccessManager.sol | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 90bbcd73609..0618f5322a0 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -283,11 +283,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Decodes a role id into a team, if it is a role id of the kind returned by {_encodeTeamRole}. */ function _decodeTeamRole(bytes32 role) internal virtual returns (bool, uint8) { - bytes32 tagMask = hex"ffffffff"; - bytes32 teamMask = hex"ff"; - bool isTeam = (role & tagMask) == bytes32("team:"); - uint8 team = uint8(uint256(role & teamMask)); - return (isTeam, team); + bytes32 tagMask = ~bytes32(uint256(0xff)); + bytes32 tag = role & tagMask; + uint8 team = uint8(role[31]); + return (tag == bytes32("team:"), team); } /** From 1285c49c7cd55c3cb54f4719afefe54a837d04ae Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 16 Mar 2023 23:21:54 -0300 Subject: [PATCH 24/78] add ungrouped interface --- contracts/access/manager/AccessManager.sol | 77 +++++++++++++++++++--- 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 0618f5322a0..189b8636237 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -26,9 +26,11 @@ interface IAccessManager is IAuthority { function renounceTeam(address user, uint8 team) external; - function getFunctionAllowedTeams(bytes32 group, bytes4 selector) external view returns (bytes32 teams); + function getFunctionAllowedTeams(address target, bytes4 selector) external view returns (bytes32 teams); + function getFunctionAllowedTeams(string calldata group, bytes4 selector) external view returns (bytes32 teams); - function setFunctionAllowedTeam(bytes32 group, bytes4[] calldata selectors, uint8 team, bool allowed) external; + function setFunctionAllowedTeam(address target, bytes4[] calldata selectors, uint8 team, bool allowed) external; + function setFunctionAllowedTeam(string calldata group, bytes4[] calldata selectors, uint8 team, bool allowed) external; function getContractGroup(address target) external view returns (bytes32 group); @@ -52,7 +54,7 @@ interface IAccessManager is IAuthority { * that starts with the ASCII characters `team:`, followed by zeroes, and ending with the single byte corresponding to * the team number. * - * Contracts in the system are grouped as well. These are simply called "contract groups". There can be an arbitrary + * Contracts in the system may be grouped as well. These are simply called "contract groups". There can be an arbitrary * number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned * to its own separate group, but groups can also be shared among similar contracts. * @@ -79,10 +81,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { mapping(address => bytes32) private _contractGroup; uint8 private constant _TEAM_PUBLIC = 255; + bytes32 private constant _GROUP_UNSET = 0; bytes32 private constant _GROUP_OPEN = "group:open"; bytes32 private constant _GROUP_CLOSED = "group:closed"; + bytes14 private constant _GROUP_ISOLATE_PREFIX = "group:isolate:"; + bytes13 private constant _GROUP_CUSTOM_PREFIX = "group:custom:"; + /** * @dev Initializes an AccessManager with initial default admin and transfer delay. */ @@ -98,12 +104,12 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Entrypoint for {AccessManaged} contracts. */ function canCall(address caller, address target, bytes4 selector) public view returns (bool) { - bytes32 group = getContractGroup(target); - bytes32 allowedTeams = getFunctionAllowedTeams(group, selector); + bytes32 allowedTeams = getFunctionAllowedTeams(target, selector); bytes32 callerTeams = getUserTeams(caller); return callerTeams & allowedTeams != 0; } + // TODO: consider sanitizing name /** * @dev Creates a new team with a team number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. @@ -165,11 +171,19 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { renounceRole(_encodeTeamRole(team), user); } + function getFunctionAllowedTeams(address target, bytes4 selector) public view virtual returns (bytes32) { + return _getFunctionAllowedTeams(getContractGroup(target), selector); + } + + function getFunctionAllowedTeams(string calldata group, bytes4 selector) public view virtual returns (bytes32) { + return _getFunctionAllowedTeams(_encodeCustomGroup(group), selector); + } + /** * @dev Returns a bitmap of the teams that are allowed to call a function selector on contracts belonging to a * group. Bit `n` is set if team `n` is allowed, counting from least significant bit. */ - function getFunctionAllowedTeams(bytes32 group, bytes4 selector) public view virtual returns (bytes32) { + function _getFunctionAllowedTeams(bytes32 group, bytes4 selector) internal view virtual returns (bytes32) { if (group == _GROUP_OPEN) { return _teamMask(_TEAM_PUBLIC); } else if (group == _GROUP_CLOSED) { @@ -179,16 +193,35 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } + function setFunctionAllowedTeam( + address target, + bytes4[] calldata selectors, + uint8 team, + bool allowed + ) public virtual { + require(_contractGroup[target] == 0); + _setFunctionAllowedTeam(_encodeIsolateGroup(target), selectors, team, allowed); + } + + function setFunctionAllowedTeam( + string calldata group, + bytes4[] calldata selectors, + uint8 team, + bool allowed + ) public virtual { + _setFunctionAllowedTeam(_encodeCustomGroup(group), selectors, team, allowed); + } + /** * @dev Changes whether a team is allowed to call a function selector on contracts belonging to a group, according * to the `allowed` argument. The caller must be the default admin. */ - function setFunctionAllowedTeam( + function _setFunctionAllowedTeam( bytes32 group, bytes4[] calldata selectors, uint8 team, bool allowed - ) public virtual onlyDefaultAdmin { + ) internal virtual onlyDefaultAdmin { require(group != 0); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; @@ -201,7 +234,12 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Returns the contract group that the target contract currently belongs to. May be 0 if not set. */ function getContractGroup(address target) public view virtual returns (bytes32) { - return _contractGroup[target]; + bytes32 group = _contractGroup[target]; + if (group == 0) { + return _encodeIsolateGroup(target); + } else { + return group; + } } /** @@ -289,11 +327,30 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return (tag == bytes32("team:"), team); } + /** + * @dev Returns the isolate group id for a target contract. + * + * The group id consists of the ASCII characters `group:isolate:` followed by the contract address bytes. + */ + function _encodeIsolateGroup(address target) internal virtual pure returns (bytes32) { + return _GROUP_ISOLATE_PREFIX | (bytes20(target) >> _GROUP_ISOLATE_PREFIX.length); + } + + /** + * @dev Returns the isolate group id for a target contract. + * + * The group id consists of the ASCII characters `group:custom:` followed by the group name. + */ + function _encodeCustomGroup(string calldata name) internal virtual pure returns (bytes32) { + require(bytes(name).length + _GROUP_CUSTOM_PREFIX.length < 32); + return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(name)) >> _GROUP_CUSTOM_PREFIX.length); + } + /** * @dev Returns true if the group is one of "open", "closed", or unset (zero). */ function _isSpecialGroup(bytes32 group) private pure returns (bool) { - return group == _GROUP_UNSET || group == _GROUP_OPEN || group == _GROUP_CLOSED; + return group == _GROUP_OPEN || group == _GROUP_CLOSED; } /** From 5c7472ae857d53f4ce680fbf0e040f461f585bfd Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 17 Mar 2023 12:02:28 -0300 Subject: [PATCH 25/78] improve docs --- contracts/access/manager/AccessManager.sol | 29 +++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 189b8636237..6f717bdd813 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -73,6 +73,10 @@ interface IAccessManager is IAuthority { * includes {AccessControlDefaultAdminRules} by default to enforce security rules on this account. Additionally, it is * expected that the account will be highly secured (e.g., a multisig or a well-configured DAO) as all the permissions * of the managed system can be modified by it. + * + * NOTE: Some of the functions in this contract, such as {getUserTeams}, return a `bytes32` bitmap to succintly + * represent a set of teams. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if + * the team with number `n` is in the set. */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdTeams; @@ -134,8 +138,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns a bitmap of the teams the user is a member of. Bit `n` is set if the user is in team `n`, - * counting from least significant bit. + * @dev Returns a bitmap of the teams the user is a member of. See note on bitmaps above. */ function getUserTeams(address user) public view virtual returns (bytes32) { return _userTeams[user] | _teamMask(_TEAM_PUBLIC); @@ -171,17 +174,23 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { renounceRole(_encodeTeamRole(team), user); } + /** + * @dev Returns a bitmap of the teams that are allowed to call a function of a target contract. If the target + * contract is in a group, the group's permissions are returned. + */ function getFunctionAllowedTeams(address target, bytes4 selector) public view virtual returns (bytes32) { return _getFunctionAllowedTeams(getContractGroup(target), selector); } + /** + * @dev Returns a bitmap of the teams that are allowed to call a function of a group of contracts. + */ function getFunctionAllowedTeams(string calldata group, bytes4 selector) public view virtual returns (bytes32) { return _getFunctionAllowedTeams(_encodeCustomGroup(group), selector); } /** - * @dev Returns a bitmap of the teams that are allowed to call a function selector on contracts belonging to a - * group. Bit `n` is set if team `n` is allowed, counting from least significant bit. + * @dev Returns a bitmap of the teams that are allowed to call a function selector of a contract group. */ function _getFunctionAllowedTeams(bytes32 group, bytes4 selector) internal view virtual returns (bytes32) { if (group == _GROUP_OPEN) { @@ -193,6 +202,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } + /** + * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * argument. The caller must be the default admin. + */ function setFunctionAllowedTeam( address target, bytes4[] calldata selectors, @@ -203,6 +216,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { _setFunctionAllowedTeam(_encodeIsolateGroup(target), selectors, team, allowed); } + /** + * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * argument. The caller must be the default admin. + */ function setFunctionAllowedTeam( string calldata group, bytes4[] calldata selectors, @@ -213,8 +230,8 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Changes whether a team is allowed to call a function selector on contracts belonging to a group, according - * to the `allowed` argument. The caller must be the default admin. + * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * argument. The caller must be the default admin. */ function _setFunctionAllowedTeam( bytes32 group, From 3cc1e73f9a5fd5133936e5d568b3c7e9d2abd73c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 17 Mar 2023 15:21:50 -0300 Subject: [PATCH 26/78] remove todo --- contracts/access/manager/AccessManager.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 6f717bdd813..5aea8d0c78f 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -113,7 +113,6 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return callerTeams & allowedTeams != 0; } - // TODO: consider sanitizing name /** * @dev Creates a new team with a team number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. From ae3482d46163fad94b662d0cdc265f647394e799 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 19 Mar 2023 18:51:15 -0300 Subject: [PATCH 27/78] add initial tests for teams --- contracts/access/manager/AccessManager.sol | 117 ++++++++++++++++----- test/access/manager/AccessManager.test.js | 64 +++++++++++ 2 files changed, 153 insertions(+), 28 deletions(-) create mode 100644 test/access/manager/AccessManager.test.js diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 5aea8d0c78f..dda031f5160 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -18,6 +18,8 @@ interface IAccessManager is IAuthority { function updateTeamName(uint8 team, string calldata name) external; + function hasTeam(uint8 team) external view returns (bool); + function getUserTeams(address user) external view returns (bytes32 teams); function grantTeam(address user, uint8 team) external; @@ -32,9 +34,9 @@ interface IAccessManager is IAuthority { function setFunctionAllowedTeam(address target, bytes4[] calldata selectors, uint8 team, bool allowed) external; function setFunctionAllowedTeam(string calldata group, bytes4[] calldata selectors, uint8 team, bool allowed) external; - function getContractGroup(address target) external view returns (bytes32 group); + function getContractGroup(address target) external view returns (bytes32); - function setContractGroup(address target, bytes32 group) external; + function setContractGroup(address target, string calldata groupName) external; function setContractOpen(address target) external; @@ -100,7 +102,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint48 initialDefaultAdminDelay, address initialDefaultAdmin ) AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) { - createTeam(_TEAM_PUBLIC, "public"); + _createTeam(_TEAM_PUBLIC, "public"); } /** @@ -123,19 +125,24 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Emits {TeamUpdated}. */ function createTeam(uint8 team, string memory name) public virtual onlyDefaultAdmin { - require(!_teamExists(team)); - _setTeam(_createdTeams, team, true); - emit TeamUpdated(team, name); + _createTeam(team, name); } /** * @dev Updates an existing team's name. The caller must be the default admin. */ - function updateTeamName(uint8 team, string calldata name) public virtual onlyDefaultAdmin { - require(team != _TEAM_PUBLIC && _teamExists(team)); + function updateTeamName(uint8 team, string memory name) public virtual onlyDefaultAdmin { + require(team != _TEAM_PUBLIC && hasTeam(team)); emit TeamUpdated(team, name); } + /** + * @dev Returns true if the team has already been created via {createTeam}. + */ + function hasTeam(uint8 team) public virtual view returns (bool) { + return _getTeam(_createdTeams, team); + } + /** * @dev Returns a bitmap of the teams the user is a member of. See note on bitmaps above. */ @@ -241,23 +248,25 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { require(group != 0); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; - _allowedTeams[group][selector] = _setTeam(_allowedTeams[group][selector], team, allowed); + _allowedTeams[group][selector] = _withUpdatedTeam(_allowedTeams[group][selector], team, allowed); emit TeamAllowed(group, selector, team, allowed); } } /** - * @dev Returns the contract group that the target contract currently belongs to. May be 0 if not set. + * @dev Returns the group of the target contract, which may be its isolate group (the default), a custom group, or + * the open or closed groups. */ function getContractGroup(address target) public view virtual returns (bytes32) { bytes32 group = _contractGroup[target]; - if (group == 0) { + if (group == _GROUP_UNSET) { return _encodeIsolateGroup(target); } else { return group; } } + // TODO: filter open/closed? /** * @dev Sets the contract group that the target belongs to. The caller must be the default admin. * @@ -265,8 +274,8 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Accounts that were previously able to call permissioned functions on the target contract may no longer be * allowed, and new sets of account may be allowed after this operation. */ - function setContractGroup(address target, bytes32 group) public virtual onlyDefaultAdmin { - require(!_isSpecialGroup(group)); + function setContractGroup(address target, string calldata groupName) public virtual onlyDefaultAdmin { + bytes32 group = _encodeCustomGroup(groupName); _contractGroup[target] = group; emit ContractGroupUpdated(target, group); } @@ -292,10 +301,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns true if the team has already been created via {createTeam}. + * @dev Creates a new team. + * + * Emits {TeamUpdated}. */ - function _teamExists(uint8 team) internal virtual returns (bool) { - return _getTeam(_createdTeams, team); + function _createTeam(uint8 team, string memory name) internal virtual { + require(!hasTeam(team)); + _createdTeams = _withUpdatedTeam(_createdTeams, team, true); + emit TeamUpdated(team, name); } /** @@ -305,8 +318,8 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { super._grantRole(role, user); (bool isTeam, uint8 team) = _decodeTeamRole(role); if (isTeam) { - require(_teamExists(team)); - _userTeams[user] = _setTeam(_userTeams[user], team, true); + require(hasTeam(team)); + _userTeams[user] = _withUpdatedTeam(_userTeams[user], team, true); } } @@ -317,9 +330,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { super._revokeRole(role, user); (bool isTeam, uint8 team) = _decodeTeamRole(role); if (isTeam) { - require(_teamExists(team)); + require(hasTeam(team)); require(team != _TEAM_PUBLIC); - _userTeams[user] = _setTeam(_userTeams[user], team, false); + _userTeams[user] = _withUpdatedTeam(_userTeams[user], team, false); } } @@ -329,14 +342,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * This role id starts with the ASCII characters `team:`, followed by zeroes, and ends with the single byte * corresponding to the team number. */ - function _encodeTeamRole(uint8 team) internal virtual view returns (bytes32) { + function _encodeTeamRole(uint8 team) internal virtual pure returns (bytes32) { return bytes32("team:") | bytes32(uint256(team)); } /** * @dev Decodes a role id into a team, if it is a role id of the kind returned by {_encodeTeamRole}. */ - function _decodeTeamRole(bytes32 role) internal virtual returns (bool, uint8) { + function _decodeTeamRole(bytes32 role) internal virtual pure returns (bool, uint8) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; uint8 team = uint8(role[31]); @@ -353,13 +366,31 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns the isolate group id for a target contract. + * @dev Returns the custom group id for a given group name. + * + * The group id consists of the ASCII characters `group:custom:` followed by the group name. + */ + function _encodeCustomGroup(string calldata groupName) internal virtual pure returns (bytes32) { + require(!_containsNullBytes(bytes32(bytes(groupName)), bytes(groupName).length)); + require(bytes(groupName).length + _GROUP_CUSTOM_PREFIX.length < 31); + return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(groupName)) >> _GROUP_CUSTOM_PREFIX.length); + } + + /** + * @dev Returns the custom group id for a given group name. * * The group id consists of the ASCII characters `group:custom:` followed by the group name. */ - function _encodeCustomGroup(string calldata name) internal virtual pure returns (bytes32) { - require(bytes(name).length + _GROUP_CUSTOM_PREFIX.length < 32); - return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(name)) >> _GROUP_CUSTOM_PREFIX.length); + function _decodeCustomGroup(bytes32 group) internal virtual pure returns (string memory) { + string memory name = new string(32); + uint256 nameLength = uint256(group) & 0xff; + bytes32 nameBytes = group << _GROUP_CUSTOM_PREFIX.length; + /// @solidity memory-safe-assembly + assembly { + mstore(name, nameLength) + mstore(add(name, 0x20), nameBytes) + } + return name; } /** @@ -384,9 +415,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Changes the value of the team number bit in a bitmap. + * @dev Returns a new team bitmap where a specific team was updated. */ - function _setTeam(bytes32 bitmap, uint8 team, bool value) private pure returns (bytes32) { + function _withUpdatedTeam(bytes32 bitmap, uint8 team, bool value) private pure returns (bytes32) { bytes32 mask = _teamMask(team); if (value) { return bitmap | mask; @@ -394,4 +425,34 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return bitmap & ~mask; } } + + /** + * @dev Returns true if the word contains any NUL bytes in the highest `scope` bytes. `scope` must be at most 32. + */ + function _containsNullBytes(bytes32 word, uint256 scope) private pure returns (bool) { + // We will operate on every byte of the word simultaneously + // We visualize the 8 bits of each byte as word[i] = 01234567 + + // Take bitwise OR of all bits in each byte + word |= word >> 4; // word[i] = 01234567 | ____0123 = ____abcd + word |= word >> 2; // word[i] = ____abcd | ______ab = ______xy + word |= word >> 1; // word[i] = ______xy | _______x = _______z + + // z is 1 iff any bit of word[i] is 1 + + // Every byte in `low` is 0x01 + bytes32 low = hex"0101010101010101010101010101010101010101010101010101010101010101"; + + // Select the lowest bit only and take its complement + word &= low; // word[i] = 0000000z + word ^= low; // word[i] = 0000000Z (Z = ~z) + + // Ignore anything past the scope + unchecked { + word >>= 32 - scope; + } + + // If any byte in scope was 0x00 it will now contain 00000001 + return word != 0; + } } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js new file mode 100644 index 00000000000..cb1d9facca7 --- /dev/null +++ b/test/access/manager/AccessManager.test.js @@ -0,0 +1,64 @@ +const { expectEvent, expectRevert, time: { duration } } = require('@openzeppelin/test-helpers'); +const helpers = require('@nomicfoundation/hardhat-network-helpers'); + +const AccessManager = artifacts.require('$AccessManager'); +const AccessManaged = artifacts.require('$AccessManaged'); + +const teamMask = team => 1n << BigInt(team); + +contract('AccessManager', function ([admin, nonAdmin, user1]) { + beforeEach('deploy', async function () { + this.delay = duration.days(1); + this.manager = await AccessManager.new(this.delay, admin); + this.managed = await AccessManaged.new(this.manager.address); + }); + + it('configures default admin rules', async function () { + expect(await this.manager.defaultAdmin()).to.equal(admin); + expect(await this.manager.defaultAdminDelay()).to.be.bignumber.equal(this.delay); + }); + + describe('public team', function () { + const publicTeam = '255'; + + it('is created automatically', async function () { + await expectEvent.inConstruction(this.manager, 'TeamUpdated', { + team: publicTeam, + name: 'public', + }); + }); + + it('includes all users automatically', async function () { + const user1Teams = await this.manager.getUserTeams(user1); + + expect(BigInt(user1Teams) & teamMask(publicTeam)).to.not.equal(0n); + }); + }); + + describe('teams', function () { + const team = '1'; + const name = 'dao'; + const otherTeam = '2'; + const otherName = 'council'; + + it('can create teams', async function () { + const created = await this.manager.createTeam(team, name, { from: admin }); + expectEvent(created, 'TeamUpdated', { team, name }); + expect(await this.manager.hasTeam(team)).to.equal(true); + expect(await this.manager.hasTeam(otherTeam)).to.equal(false); + }); + + it('only admin can create teams', async function () { + await expectRevert( + this.manager.createTeam(team, name, { from: nonAdmin }), + 'missing role', + ); + }); + + it('can update team', async function () { + await this.manager.createTeam(team, name, { from: admin }); + const updated = await this.manager.updateTeamName(team, otherName, { from: admin }); + expectEvent(updated, 'TeamUpdated', { team, name: otherName }); + }); + }); +}); From 535ea13a00944fc958390535d568507c3ebf33cb Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 19 Mar 2023 18:59:17 -0300 Subject: [PATCH 28/78] rename team -> badge --- contracts/access/manager/AccessManager.sol | 240 ++++++++++----------- test/access/manager/AccessManager.test.js | 42 ++-- 2 files changed, 141 insertions(+), 141 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index dda031f5160..66861265041 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -8,31 +8,31 @@ import "../../utils/Create2.sol"; import "./IAuthority.sol"; interface IAccessManager is IAuthority { - event TeamUpdated(uint8 indexed team, string name); + event BadgeUpdated(uint8 indexed badge, string name); - event TeamAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed team, bool allowed); + event BadgeAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed badge, bool allowed); event ContractGroupUpdated(address indexed target, bytes32 indexed group); - function createTeam(uint8 team, string calldata name) external; + function createBadge(uint8 badge, string calldata name) external; - function updateTeamName(uint8 team, string calldata name) external; + function updateBadgeName(uint8 badge, string calldata name) external; - function hasTeam(uint8 team) external view returns (bool); + function hasBadge(uint8 badge) external view returns (bool); - function getUserTeams(address user) external view returns (bytes32 teams); + function getUserBadges(address user) external view returns (bytes32 badges); - function grantTeam(address user, uint8 team) external; + function grantBadge(address user, uint8 badge) external; - function revokeTeam(address user, uint8 team) external; + function revokeBadge(address user, uint8 badge) external; - function renounceTeam(address user, uint8 team) external; + function renounceBadge(address user, uint8 badge) external; - function getFunctionAllowedTeams(address target, bytes4 selector) external view returns (bytes32 teams); - function getFunctionAllowedTeams(string calldata group, bytes4 selector) external view returns (bytes32 teams); + function getFunctionAllowedBadges(address target, bytes4 selector) external view returns (bytes32 badges); + function getFunctionAllowedBadges(string calldata group, bytes4 selector) external view returns (bytes32 badges); - function setFunctionAllowedTeam(address target, bytes4[] calldata selectors, uint8 team, bool allowed) external; - function setFunctionAllowedTeam(string calldata group, bytes4[] calldata selectors, uint8 team, bool allowed) external; + function setFunctionAllowedBadge(address target, bytes4[] calldata selectors, uint8 badge, bool allowed) external; + function setFunctionAllowedBadge(string calldata group, bytes4[] calldata selectors, uint8 badge, bool allowed) external; function getContractGroup(address target) external view returns (bytes32); @@ -51,42 +51,42 @@ interface IAccessManager is IAuthority { * i.e. it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set * of roles, with a particular structure. * - * Users are grouped in "teams". Teams must be created before users can be assigned into them, up to a maximum of 255 - * teams. A user can be assigned to multiple teams. Each team defines an AccessControl role, identified by a role id - * that starts with the ASCII characters `team:`, followed by zeroes, and ending with the single byte corresponding to - * the team number. + * Users are granted "badges". Badges must be created before they can be granted, with a maximum of 255 created badges. + * A user can be granted multiple badges. Each badge defines an AccessControl role, identified by a role id that starts + * with the ASCII characters `badge:`, followed by zeroes, and ending with the single byte corresponding to the badge + * number. * * Contracts in the system may be grouped as well. These are simply called "contract groups". There can be an arbitrary * number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned * to its own separate group, but groups can also be shared among similar contracts. * * All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between - * functions and allowed teams. Each function can be allowed to multiple teams, meaning that if a user is in at least - * one of the allowed teams they can call that function. + * functions and allowed badges. Each function can be allowed to multiple badges, meaning that if a user has at least + * one of the allowed badges they can call that function. * * Note that a function in a target contract may become permissioned only when: 1) said contract is {AccessManaged} and * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. * - * There is a special team defined by default named "public" of which all accounts are automatically members, and two - * special contract groups: 1) the "open" group, where all functions are allowed to the "public" team, and 2) the - * "closed" group, where no function is allowed to any team. + * There is a special badge defined by default named "public" which all accounts automatically have, and two special + * contract groups: 1) the "open" group, where all functions are allowed to the "public" badge, and 2) the "closed" + * group, where no function is allowed to any badge. * - * Permissioning schemes and team and contract group assignments can be configured by the default admin. The contract + * Permissioning schemes and badge and contract group assignments can be configured by the default admin. The contract * includes {AccessControlDefaultAdminRules} by default to enforce security rules on this account. Additionally, it is * expected that the account will be highly secured (e.g., a multisig or a well-configured DAO) as all the permissions * of the managed system can be modified by it. * - * NOTE: Some of the functions in this contract, such as {getUserTeams}, return a `bytes32` bitmap to succintly - * represent a set of teams. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if - * the team with number `n` is in the set. + * NOTE: Some of the functions in this contract, such as {getUserBadges}, return a `bytes32` bitmap to succintly + * represent a set of badges. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if + * the badge with number `n` is in the set. */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { - bytes32 _createdTeams; - mapping(address => bytes32) private _userTeams; - mapping(bytes32 => mapping(bytes4 => bytes32)) private _allowedTeams; + bytes32 _createdBadges; + mapping(address => bytes32) private _userBadges; + mapping(bytes32 => mapping(bytes4 => bytes32)) private _allowedBadges; mapping(address => bytes32) private _contractGroup; - uint8 private constant _TEAM_PUBLIC = 255; + uint8 private constant _BADGE_PUBLIC = 255; bytes32 private constant _GROUP_UNSET = 0; bytes32 private constant _GROUP_OPEN = "group:open"; @@ -102,7 +102,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint48 initialDefaultAdminDelay, address initialDefaultAdmin ) AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) { - _createTeam(_TEAM_PUBLIC, "public"); + _createBadge(_BADGE_PUBLIC, "public"); } /** @@ -110,146 +110,146 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Entrypoint for {AccessManaged} contracts. */ function canCall(address caller, address target, bytes4 selector) public view returns (bool) { - bytes32 allowedTeams = getFunctionAllowedTeams(target, selector); - bytes32 callerTeams = getUserTeams(caller); - return callerTeams & allowedTeams != 0; + bytes32 allowedBadges = getFunctionAllowedBadges(target, selector); + bytes32 callerBadges = getUserBadges(caller); + return callerBadges & allowedBadges != 0; } /** - * @dev Creates a new team with a team number that can be chosen arbitrarily but must be unused, and gives it a + * @dev Creates a new badge with a badge number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. * - * Team numbers are not auto-incremented in order to avoid race conditions, but administrators can safely use + * Badge numbers are not auto-incremented in order to avoid race conditions, but administrators can safely use * sequential numbers. * - * Emits {TeamUpdated}. + * Emits {BadgeUpdated}. */ - function createTeam(uint8 team, string memory name) public virtual onlyDefaultAdmin { - _createTeam(team, name); + function createBadge(uint8 badge, string memory name) public virtual onlyDefaultAdmin { + _createBadge(badge, name); } /** - * @dev Updates an existing team's name. The caller must be the default admin. + * @dev Updates an existing badge's name. The caller must be the default admin. */ - function updateTeamName(uint8 team, string memory name) public virtual onlyDefaultAdmin { - require(team != _TEAM_PUBLIC && hasTeam(team)); - emit TeamUpdated(team, name); + function updateBadgeName(uint8 badge, string memory name) public virtual onlyDefaultAdmin { + require(badge != _BADGE_PUBLIC && hasBadge(badge)); + emit BadgeUpdated(badge, name); } /** - * @dev Returns true if the team has already been created via {createTeam}. + * @dev Returns true if the badge has already been created via {createBadge}. */ - function hasTeam(uint8 team) public virtual view returns (bool) { - return _getTeam(_createdTeams, team); + function hasBadge(uint8 badge) public virtual view returns (bool) { + return _getBadge(_createdBadges, badge); } /** - * @dev Returns a bitmap of the teams the user is a member of. See note on bitmaps above. + * @dev Returns a bitmap of the badges the user has. See note on bitmaps above. */ - function getUserTeams(address user) public view virtual returns (bytes32) { - return _userTeams[user] | _teamMask(_TEAM_PUBLIC); + function getUserBadges(address user) public view virtual returns (bytes32) { + return _userBadges[user] | _badgeMask(_BADGE_PUBLIC); } /** - * @dev Adds a user to a team. + * @dev Grants a user a badge. * - * Emits {RoleGranted} with the role id of the team, if not previously a member. + * Emits {RoleGranted} with the role id of the badge, if wasn't already held by the user. */ - function grantTeam(address user, uint8 team) public virtual { + function grantBadge(address user, uint8 badge) public virtual { // grantRole checks that msg.sender is admin for the role - grantRole(_encodeTeamRole(team), user); + grantRole(_encodeBadgeRole(badge), user); } /** - * @dev Removes a user from a team. + * @dev Removes a badge from a user. * - * Emits {RoleRevoked} with the role id of the team, if previously a member. + * Emits {RoleRevoked} with the role id of the badge, if previously held by the user. */ - function revokeTeam(address user, uint8 team) public virtual { + function revokeBadge(address user, uint8 badge) public virtual { // revokeRole checks that msg.sender is admin for the role - revokeRole(_encodeTeamRole(team), user); + revokeRole(_encodeBadgeRole(badge), user); } /** - * @dev Renounces a user from a team. + * @dev Allows a user to renounce a badge. * - * Emits {RoleRevoked} with the role id of the team, if previously a member. + * Emits {RoleRevoked} with the role id of the badge, if previously held by the user. */ - function renounceTeam(address user, uint8 team) public virtual { + function renounceBadge(address user, uint8 badge) public virtual { // renounceRole checks that msg.sender is user - renounceRole(_encodeTeamRole(team), user); + renounceRole(_encodeBadgeRole(badge), user); } /** - * @dev Returns a bitmap of the teams that are allowed to call a function of a target contract. If the target + * @dev Returns a bitmap of the badges that are allowed to call a function of a target contract. If the target * contract is in a group, the group's permissions are returned. */ - function getFunctionAllowedTeams(address target, bytes4 selector) public view virtual returns (bytes32) { - return _getFunctionAllowedTeams(getContractGroup(target), selector); + function getFunctionAllowedBadges(address target, bytes4 selector) public view virtual returns (bytes32) { + return _getFunctionAllowedBadges(getContractGroup(target), selector); } /** - * @dev Returns a bitmap of the teams that are allowed to call a function of a group of contracts. + * @dev Returns a bitmap of the badges that are allowed to call a function of a group of contracts. */ - function getFunctionAllowedTeams(string calldata group, bytes4 selector) public view virtual returns (bytes32) { - return _getFunctionAllowedTeams(_encodeCustomGroup(group), selector); + function getFunctionAllowedBadges(string calldata group, bytes4 selector) public view virtual returns (bytes32) { + return _getFunctionAllowedBadges(_encodeCustomGroup(group), selector); } /** - * @dev Returns a bitmap of the teams that are allowed to call a function selector of a contract group. + * @dev Returns a bitmap of the badges that are allowed to call a function selector of a contract group. */ - function _getFunctionAllowedTeams(bytes32 group, bytes4 selector) internal view virtual returns (bytes32) { + function _getFunctionAllowedBadges(bytes32 group, bytes4 selector) internal view virtual returns (bytes32) { if (group == _GROUP_OPEN) { - return _teamMask(_TEAM_PUBLIC); + return _badgeMask(_BADGE_PUBLIC); } else if (group == _GROUP_CLOSED) { return 0; } else { - return _allowedTeams[group][selector]; + return _allowedBadges[group][selector]; } } /** - * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` * argument. The caller must be the default admin. */ - function setFunctionAllowedTeam( + function setFunctionAllowedBadge( address target, bytes4[] calldata selectors, - uint8 team, + uint8 badge, bool allowed ) public virtual { require(_contractGroup[target] == 0); - _setFunctionAllowedTeam(_encodeIsolateGroup(target), selectors, team, allowed); + _setFunctionAllowedBadge(_encodeIsolateGroup(target), selectors, badge, allowed); } /** - * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` * argument. The caller must be the default admin. */ - function setFunctionAllowedTeam( + function setFunctionAllowedBadge( string calldata group, bytes4[] calldata selectors, - uint8 team, + uint8 badge, bool allowed ) public virtual { - _setFunctionAllowedTeam(_encodeCustomGroup(group), selectors, team, allowed); + _setFunctionAllowedBadge(_encodeCustomGroup(group), selectors, badge, allowed); } /** - * @dev Changes whether a team is allowed to call a function of a contract group, according to the `allowed` + * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` * argument. The caller must be the default admin. */ - function _setFunctionAllowedTeam( + function _setFunctionAllowedBadge( bytes32 group, bytes4[] calldata selectors, - uint8 team, + uint8 badge, bool allowed ) internal virtual onlyDefaultAdmin { require(group != 0); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; - _allowedTeams[group][selector] = _withUpdatedTeam(_allowedTeams[group][selector], team, allowed); - emit TeamAllowed(group, selector, team, allowed); + _allowedBadges[group][selector] = _withUpdatedBadge(_allowedBadges[group][selector], badge, allowed); + emit BadgeAllowed(group, selector, badge, allowed); } } @@ -270,7 +270,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Sets the contract group that the target belongs to. The caller must be the default admin. * - * CAUTION: This is a batch operation that will immediately switch the mapping of functions to allowed teams. + * CAUTION: This is a batch operation that will immediately switch the mapping of functions to allowed badges. * Accounts that were previously able to call permissioned functions on the target contract may no longer be * allowed, and new sets of account may be allowed after this operation. */ @@ -301,59 +301,59 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Creates a new team. + * @dev Creates a new badge. * - * Emits {TeamUpdated}. + * Emits {BadgeUpdated}. */ - function _createTeam(uint8 team, string memory name) internal virtual { - require(!hasTeam(team)); - _createdTeams = _withUpdatedTeam(_createdTeams, team, true); - emit TeamUpdated(team, name); + function _createBadge(uint8 badge, string memory name) internal virtual { + require(!hasBadge(badge)); + _createdBadges = _withUpdatedBadge(_createdBadges, badge, true); + emit BadgeUpdated(badge, name); } /** - * @dev Augmented version of {AccessControl-_grantRole} that keeps track of user team bitmaps. + * @dev Augmented version of {AccessControl-_grantRole} that keeps track of user badge bitmaps. */ function _grantRole(bytes32 role, address user) internal virtual override { super._grantRole(role, user); - (bool isTeam, uint8 team) = _decodeTeamRole(role); - if (isTeam) { - require(hasTeam(team)); - _userTeams[user] = _withUpdatedTeam(_userTeams[user], team, true); + (bool isBadge, uint8 badge) = _decodeBadgeRole(role); + if (isBadge) { + require(hasBadge(badge)); + _userBadges[user] = _withUpdatedBadge(_userBadges[user], badge, true); } } /** - * @dev Augmented version of {AccessControl-_revokeRole} that keeps track of user team bitmaps. + * @dev Augmented version of {AccessControl-_revokeRole} that keeps track of user badge bitmaps. */ function _revokeRole(bytes32 role, address user) internal virtual override { super._revokeRole(role, user); - (bool isTeam, uint8 team) = _decodeTeamRole(role); - if (isTeam) { - require(hasTeam(team)); - require(team != _TEAM_PUBLIC); - _userTeams[user] = _withUpdatedTeam(_userTeams[user], team, false); + (bool isBadge, uint8 badge) = _decodeBadgeRole(role); + if (isBadge) { + require(hasBadge(badge)); + require(badge != _BADGE_PUBLIC); + _userBadges[user] = _withUpdatedBadge(_userBadges[user], badge, false); } } /** - * @dev Returns the {AccessControl} role id that corresponds to a team. + * @dev Returns the {AccessControl} role id that corresponds to a badge. * - * This role id starts with the ASCII characters `team:`, followed by zeroes, and ends with the single byte - * corresponding to the team number. + * This role id starts with the ASCII characters `badge:`, followed by zeroes, and ends with the single byte + * corresponding to the badge number. */ - function _encodeTeamRole(uint8 team) internal virtual pure returns (bytes32) { - return bytes32("team:") | bytes32(uint256(team)); + function _encodeBadgeRole(uint8 badge) internal virtual pure returns (bytes32) { + return bytes32("badge:") | bytes32(uint256(badge)); } /** - * @dev Decodes a role id into a team, if it is a role id of the kind returned by {_encodeTeamRole}. + * @dev Decodes a role id into a badge, if it is a role id of the kind returned by {_encodeBadgeRole}. */ - function _decodeTeamRole(bytes32 role) internal virtual pure returns (bool, uint8) { + function _decodeBadgeRole(bytes32 role) internal virtual pure returns (bool, uint8) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; - uint8 team = uint8(role[31]); - return (tag == bytes32("team:"), team); + uint8 badge = uint8(role[31]); + return (tag == bytes32("badge:"), badge); } /** @@ -401,24 +401,24 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns a bit mask where the only non-zero bit is the team number bit. + * @dev Returns a bit mask where the only non-zero bit is the badge number bit. */ - function _teamMask(uint8 team) private pure returns (bytes32) { - return bytes32(1 << team); + function _badgeMask(uint8 badge) private pure returns (bytes32) { + return bytes32(1 << badge); } /** - * @dev Returns the value of the team number bit in a bitmap. + * @dev Returns the value of the badge number bit in a bitmap. */ - function _getTeam(bytes32 bitmap, uint8 team) private pure returns (bool) { - return bitmap & _teamMask(team) > 0; + function _getBadge(bytes32 bitmap, uint8 badge) private pure returns (bool) { + return bitmap & _badgeMask(badge) > 0; } /** - * @dev Returns a new team bitmap where a specific team was updated. + * @dev Returns a new badge bitmap where a specific badge was updated. */ - function _withUpdatedTeam(bytes32 bitmap, uint8 team, bool value) private pure returns (bytes32) { - bytes32 mask = _teamMask(team); + function _withUpdatedBadge(bytes32 bitmap, uint8 badge, bool value) private pure returns (bytes32) { + bytes32 mask = _badgeMask(badge); if (value) { return bitmap | mask; } else { diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index cb1d9facca7..1708807fab7 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -4,7 +4,7 @@ const helpers = require('@nomicfoundation/hardhat-network-helpers'); const AccessManager = artifacts.require('$AccessManager'); const AccessManaged = artifacts.require('$AccessManaged'); -const teamMask = team => 1n << BigInt(team); +const badgeMask = badge => 1n << BigInt(badge); contract('AccessManager', function ([admin, nonAdmin, user1]) { beforeEach('deploy', async function () { @@ -18,47 +18,47 @@ contract('AccessManager', function ([admin, nonAdmin, user1]) { expect(await this.manager.defaultAdminDelay()).to.be.bignumber.equal(this.delay); }); - describe('public team', function () { - const publicTeam = '255'; + describe('public badge', function () { + const publicBadge = '255'; it('is created automatically', async function () { - await expectEvent.inConstruction(this.manager, 'TeamUpdated', { - team: publicTeam, + await expectEvent.inConstruction(this.manager, 'BadgeUpdated', { + badge: publicBadge, name: 'public', }); }); it('includes all users automatically', async function () { - const user1Teams = await this.manager.getUserTeams(user1); + const user1Badges = await this.manager.getUserBadges(user1); - expect(BigInt(user1Teams) & teamMask(publicTeam)).to.not.equal(0n); + expect(BigInt(user1Badges) & badgeMask(publicBadge)).to.not.equal(0n); }); }); - describe('teams', function () { - const team = '1'; + describe('badges', function () { + const badge = '1'; const name = 'dao'; - const otherTeam = '2'; + const otherBadge = '2'; const otherName = 'council'; - it('can create teams', async function () { - const created = await this.manager.createTeam(team, name, { from: admin }); - expectEvent(created, 'TeamUpdated', { team, name }); - expect(await this.manager.hasTeam(team)).to.equal(true); - expect(await this.manager.hasTeam(otherTeam)).to.equal(false); + it('can create badges', async function () { + const created = await this.manager.createBadge(badge, name, { from: admin }); + expectEvent(created, 'BadgeUpdated', { badge, name }); + expect(await this.manager.hasBadge(badge)).to.equal(true); + expect(await this.manager.hasBadge(otherBadge)).to.equal(false); }); - it('only admin can create teams', async function () { + it('only admin can create badges', async function () { await expectRevert( - this.manager.createTeam(team, name, { from: nonAdmin }), + this.manager.createBadge(badge, name, { from: nonAdmin }), 'missing role', ); }); - it('can update team', async function () { - await this.manager.createTeam(team, name, { from: admin }); - const updated = await this.manager.updateTeamName(team, otherName, { from: admin }); - expectEvent(updated, 'TeamUpdated', { team, name: otherName }); + it('can update badge', async function () { + await this.manager.createBadge(badge, name, { from: admin }); + const updated = await this.manager.updateBadgeName(badge, otherName, { from: admin }); + expectEvent(updated, 'BadgeUpdated', { badge, name: otherName }); }); }); }); From 68bb5fa32aabf79895f5a99445696bda64fe16f3 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 19 Mar 2023 20:38:38 -0300 Subject: [PATCH 29/78] improve testing --- test/access/manager/AccessManager.test.js | 161 +++++++++++++++++----- 1 file changed, 130 insertions(+), 31 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 1708807fab7..79fcb5e9bee 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -4,9 +4,17 @@ const helpers = require('@nomicfoundation/hardhat-network-helpers'); const AccessManager = artifacts.require('$AccessManager'); const AccessManaged = artifacts.require('$AccessManaged'); -const badgeMask = badge => 1n << BigInt(badge); +const badgeUtils = { + mask: badge => 1n << BigInt(badge), + decodeBitmap: hexBitmap => { + const m = BigInt(hexBitmap); + const allBadges = new Array(256).fill().map((_, i) => i.toString()); + return allBadges.filter(i => (m & badgeUtils.mask(i)) !== 0n); + }, + role: badge => web3.utils.asciiToHex('badge:').padEnd(64, '0') + badge.toString(16).padStart(2, '0'), +}; -contract('AccessManager', function ([admin, nonAdmin, user1]) { +contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { beforeEach('deploy', async function () { this.delay = duration.days(1); this.manager = await AccessManager.new(this.delay, admin); @@ -18,47 +26,138 @@ contract('AccessManager', function ([admin, nonAdmin, user1]) { expect(await this.manager.defaultAdminDelay()).to.be.bignumber.equal(this.delay); }); - describe('public badge', function () { - const publicBadge = '255'; + describe('badges', function () { + const badge = '0'; + const name = 'dao'; + const otherBadge = '1'; + const otherName = 'council'; + + describe('public badge', function () { + const publicBadge = '255'; + + it('is created automatically', async function () { + await expectEvent.inConstruction(this.manager, 'BadgeUpdated', { + badge: publicBadge, + name: 'public', + }); + }); - it('is created automatically', async function () { - await expectEvent.inConstruction(this.manager, 'BadgeUpdated', { - badge: publicBadge, - name: 'public', + it('includes all users automatically', async function () { + const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); + expect(badges).to.include(publicBadge); }); }); - it('includes all users automatically', async function () { - const user1Badges = await this.manager.getUserBadges(user1); + describe('creating', function () { + it('admin can create badges', async function () { + const created = await this.manager.createBadge(badge, name, { from: admin }); + expectEvent(created, 'BadgeUpdated', { badge, name }); + expect(await this.manager.hasBadge(badge)).to.equal(true); + expect(await this.manager.hasBadge(otherBadge)).to.equal(false); + }); - expect(BigInt(user1Badges) & badgeMask(publicBadge)).to.not.equal(0n); + it('non-admin cannot create badges', async function () { + await expectRevert( + this.manager.createBadge(badge, name, { from: nonAdmin }), + 'missing role', + ); + }); }); - }); - describe('badges', function () { - const badge = '1'; - const name = 'dao'; - const otherBadge = '2'; - const otherName = 'council'; + describe('updating', function () { + beforeEach('create badge', async function () { + await this.manager.createBadge(badge, name, { from: admin }); + }); + + it('admin can update badge', async function () { + const updated = await this.manager.updateBadgeName(badge, otherName, { from: admin }); + expectEvent(updated, 'BadgeUpdated', { badge, name: otherName }); + }); - it('can create badges', async function () { - const created = await this.manager.createBadge(badge, name, { from: admin }); - expectEvent(created, 'BadgeUpdated', { badge, name }); - expect(await this.manager.hasBadge(badge)).to.equal(true); - expect(await this.manager.hasBadge(otherBadge)).to.equal(false); + it('non-admin cannot update badge', async function () { + await expectRevert( + this.manager.updateBadgeName(badge, name, { from: nonAdmin }), + 'missing role', + ); + }); }); - it('only admin can create badges', async function () { - await expectRevert( - this.manager.createBadge(badge, name, { from: nonAdmin }), - 'missing role', - ); + describe('granting', function () { + beforeEach('create badge', async function () { + await this.manager.createBadge(badge, name, { from: admin }); + }); + + it('admin can grant badge', async function () { + const granted = await this.manager.grantBadge(user1, badge, { from: admin }); + expectEvent(granted, 'RoleGranted', { account: user1, role: badgeUtils.role(badge) }); + const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); + expect(badges).to.include(badge); + }); + + it('non-admin cannot grant badge', async function () { + await expectRevert( + this.manager.grantBadge(user1, badge, { from: nonAdmin }), + 'missing role', + ); + }); + }); + + describe('revoking & renouncing', function () { + beforeEach('create and grant badge', async function () { + await this.manager.createBadge(badge, name, { from: admin }); + await this.manager.grantBadge(user1, badge, { from: admin }); + }); + + it('admin can revoke badge', async function () { + await this.manager.revokeBadge(user1, badge, { from: admin }); + const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); + expect(badges).to.not.include(badge); + }); + + it('non-admin cannot revoke badge', async function () { + await expectRevert( + this.manager.revokeBadge(user1, badge, { from: nonAdmin }), + 'missing role', + ); + }); + + it('user can renounce badge', async function () { + await this.manager.renounceBadge(user1, badge, { from: user1 }); + const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); + expect(badges).to.not.include(badge); + }); + + it(`user cannot renounce other user's badges`, async function () { + await expectRevert( + this.manager.renounceBadge(user1, badge, { from: user2 }), + 'can only renounce roles for self', + ); + await expectRevert( + this.manager.renounceBadge(user2, badge, { from: user1 }), + 'can only renounce roles for self', + ); + }); }); - it('can update badge', async function () { - await this.manager.createBadge(badge, name, { from: admin }); - const updated = await this.manager.updateBadgeName(badge, otherName, { from: admin }); - expectEvent(updated, 'BadgeUpdated', { badge, name: otherName }); + describe('querying', function () { + it('returns expected badges', async function () { + const getBadges = () => this.manager.getUserBadges(user1); + + // only public badge initially + expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000000'); + + await this.manager.createBadge('0', '0', { from: admin }); + await this.manager.grantBadge(user1, '0', { from: admin }); + expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000001'); + + await this.manager.createBadge('1', '1', { from: admin }); + await this.manager.grantBadge(user1, '1', { from: admin }); + expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000003'); + + await this.manager.createBadge('16', '16', { from: admin }); + await this.manager.grantBadge(user1, '16', { from: admin }); + expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000010003'); + }); }); }); }); From fa58ec54fa5bb59c7ad6142547a7d9e8121c3f84 Mon Sep 17 00:00:00 2001 From: Francisco Date: Sun, 19 Mar 2023 20:45:59 -0300 Subject: [PATCH 30/78] Update contracts/access/manager/AccessManager.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 66861265041..c65127e4b7c 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.0; import "../AccessControl.sol"; import "../AccessControlDefaultAdminRules.sol"; -import "../../utils/Create2.sol"; import "./IAuthority.sol"; interface IAccessManager is IAuthority { From 6d760583c5f9b07895e4b33128ec2c9fa70dafc9 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 19 Mar 2023 21:03:01 -0300 Subject: [PATCH 31/78] use mapping labels from solidity 0.8.18 --- contracts/access/manager/AccessManager.sol | 8 ++++---- hardhat.config.js | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index c65127e4b7c..810b348539a 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; +pragma solidity ^0.8.18; import "../AccessControl.sol"; import "../AccessControlDefaultAdminRules.sol"; @@ -81,9 +81,9 @@ interface IAccessManager is IAuthority { */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdBadges; - mapping(address => bytes32) private _userBadges; - mapping(bytes32 => mapping(bytes4 => bytes32)) private _allowedBadges; - mapping(address => bytes32) private _contractGroup; + mapping(address user => bytes32 badges) private _userBadges; + mapping(bytes32 group => mapping(bytes4 selector => bytes32 badges)) private _allowedBadges; + mapping(address target => bytes32 group) private _contractGroup; uint8 private constant _BADGE_PUBLIC = 255; diff --git a/hardhat.config.js b/hardhat.config.js index 32f721b656c..0b16d04a290 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -3,7 +3,7 @@ // - COVERAGE: enable coverage report // - ENABLE_GAS_REPORT: enable gas report // - COMPILE_MODE: production modes enables optimizations (default: development) -// - COMPILE_VERSION: compiler version (default: 0.8.9) +// - COMPILE_VERSION: compiler version // - COINMARKETCAP: coinmarkercat api key for USD value in gas report const fs = require('fs'); @@ -40,7 +40,7 @@ const argv = require('yargs/yargs')() compiler: { alias: 'compileVersion', type: 'string', - default: '0.8.13', + default: '0.8.18', }, coinmarketcap: { alias: 'coinmarketcapApiKey', From 34474382df1e29644de6f1ca21670bde597c8fef Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sun, 19 Mar 2023 22:18:28 -0300 Subject: [PATCH 32/78] add AccessManaged tests --- contracts/access/manager/AccessManaged.sol | 10 ++-- contracts/mocks/AccessManagerMocks.sol | 30 ++++++++++++ test/access/manager/AccessManaged.test.js | 56 ++++++++++++++++++++++ test/access/manager/AccessManager.test.js | 2 +- 4 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 contracts/mocks/AccessManagerMocks.sol create mode 100644 test/access/manager/AccessManaged.test.js diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index f71d7b45b77..a99da33e74c 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -15,12 +15,12 @@ contract AccessManaged { IAuthority private _authority; modifier restricted() { - require(_authority.canCall(msg.sender, address(this), msg.sig)); + require(_authority.canCall(msg.sender, address(this), msg.sig), "AccessManaged: authority rejected"); _; } constructor(IAuthority initialAuthority) { - _authority = initialAuthority; + _setAuthority(initialAuthority); } function authority() public view virtual returns (IAuthority) { @@ -28,7 +28,11 @@ contract AccessManaged { } function setAuthority(IAuthority newAuthority) public virtual { - require(msg.sender == address(_authority)); + require(msg.sender == address(_authority), "AccessManaged: not current authority"); + _setAuthority(newAuthority); + } + + function _setAuthority(IAuthority newAuthority) internal virtual { IAuthority oldAuthority = _authority; _authority = newAuthority; emit AuthorityUpdated(oldAuthority, newAuthority); diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol new file mode 100644 index 00000000000..01b8183e01f --- /dev/null +++ b/contracts/mocks/AccessManagerMocks.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.18; + +import "../access/manager/IAuthority.sol"; +import "../access/manager/AccessManaged.sol"; + +contract SimpleAuthority is IAuthority { + address allowedCaller; + address allowedTarget; + bytes4 allowedSelector; + + function setAllowed(address _allowedCaller, address _allowedTarget, bytes4 _allowedSelector) public { + allowedCaller = _allowedCaller; + allowedTarget = _allowedTarget; + allowedSelector = _allowedSelector; + } + + function canCall(address caller, address target, bytes4 selector) external view override returns (bool) { + return caller == allowedCaller && target == allowedTarget && selector == allowedSelector; + } +} + +abstract contract AccessManagedMock is AccessManaged { + event RestrictedRan(); + + function restrictedFunction() public restricted { + emit RestrictedRan(); + } +} diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js new file mode 100644 index 00000000000..9938796c915 --- /dev/null +++ b/test/access/manager/AccessManaged.test.js @@ -0,0 +1,56 @@ +const { expectEvent, expectRevert, constants: { ZERO_ADDRESS } } = require('@openzeppelin/test-helpers'); + +const AccessManaged = artifacts.require('$AccessManagedMock'); +const SimpleAuthority = artifacts.require('SimpleAuthority'); + +contract('AccessManager', function ([authority, other, user]) { + it('construction', async function () { + const managed = await AccessManaged.new(authority); + expectEvent.inConstruction(managed, 'AuthorityUpdated', { + oldAuthority: ZERO_ADDRESS, + newAuthority: authority, + }); + expect(await managed.authority()).to.equal(authority); + }); + + describe('setAuthority', function () { + it(`current authority can change managed's authority`, async function () { + const managed = await AccessManaged.new(authority); + const set = await managed.setAuthority(other, { from: authority }); + expectEvent(set, 'AuthorityUpdated', { + oldAuthority: authority, + newAuthority: other, + }); + expect(await managed.authority()).to.equal(other); + }); + + it(`other account cannot change managed's authority`, async function () { + const managed = await AccessManaged.new(authority); + await expectRevert( + managed.setAuthority(other, { from: other }), + 'AccessManaged: not current authority', + ); + }); + }); + + describe('restricted', function () { + const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); + + it('allows if authority says true', async function () { + const authority = await SimpleAuthority.new(); + const managed = await AccessManaged.new(authority.address); + await authority.setAllowed(user, managed.address, selector); + const restricted = await managed.restrictedFunction({ from: user }); + expectEvent(restricted, 'RestrictedRan'); + }); + + it('reverts if authority says false', async function () { + const authority = await SimpleAuthority.new(); + const managed = await AccessManaged.new(authority.address); + await expectRevert( + managed.restrictedFunction({ from: user }), + 'AccessManaged: authority rejected', + ); + }); + }); +}); diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 79fcb5e9bee..4f07d70e86c 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -2,7 +2,7 @@ const { expectEvent, expectRevert, time: { duration } } = require('@openzeppelin const helpers = require('@nomicfoundation/hardhat-network-helpers'); const AccessManager = artifacts.require('$AccessManager'); -const AccessManaged = artifacts.require('$AccessManaged'); +const AccessManagedMock = artifacts.require('$AccessManagedMock'); const badgeUtils = { mask: badge => 1n << BigInt(badge), From 1d10639cb96a47dbe055beaede59f40198e6fe32 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 20 Mar 2023 00:16:38 -0300 Subject: [PATCH 33/78] fix group encoding --- contracts/access/manager/AccessManager.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 810b348539a..e20e8b72d8e 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -91,7 +91,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 private constant _GROUP_OPEN = "group:open"; bytes32 private constant _GROUP_CLOSED = "group:closed"; - bytes14 private constant _GROUP_ISOLATE_PREFIX = "group:isolate:"; + bytes14 private constant _GROUP_SOLO_PREFIX = "group:solo:"; bytes13 private constant _GROUP_CUSTOM_PREFIX = "group:custom:"; /** @@ -218,7 +218,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bool allowed ) public virtual { require(_contractGroup[target] == 0); - _setFunctionAllowedBadge(_encodeIsolateGroup(target), selectors, badge, allowed); + _setFunctionAllowedBadge(_encodeSoloGroup(target), selectors, badge, allowed); } /** @@ -253,13 +253,13 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns the group of the target contract, which may be its isolate group (the default), a custom group, or + * @dev Returns the group of the target contract, which may be its solo group (the default), a custom group, or * the open or closed groups. */ function getContractGroup(address target) public view virtual returns (bytes32) { bytes32 group = _contractGroup[target]; if (group == _GROUP_UNSET) { - return _encodeIsolateGroup(target); + return _encodeSoloGroup(target); } else { return group; } @@ -356,12 +356,12 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Returns the isolate group id for a target contract. + * @dev Returns the solo group id for a target contract. * - * The group id consists of the ASCII characters `group:isolate:` followed by the contract address bytes. + * The group id consists of the ASCII characters `group:solo:` followed by the contract address bytes. */ - function _encodeIsolateGroup(address target) internal virtual pure returns (bytes32) { - return _GROUP_ISOLATE_PREFIX | (bytes20(target) >> _GROUP_ISOLATE_PREFIX.length); + function _encodeSoloGroup(address target) internal virtual pure returns (bytes32) { + return _GROUP_SOLO_PREFIX | (bytes32(bytes20(target)) >> (_GROUP_SOLO_PREFIX.length << 3)); } /** @@ -372,7 +372,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { function _encodeCustomGroup(string calldata groupName) internal virtual pure returns (bytes32) { require(!_containsNullBytes(bytes32(bytes(groupName)), bytes(groupName).length)); require(bytes(groupName).length + _GROUP_CUSTOM_PREFIX.length < 31); - return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(groupName)) >> _GROUP_CUSTOM_PREFIX.length); + return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(groupName)) >> (_GROUP_CUSTOM_PREFIX.length << 3)); } /** From d875a7b58804100ff636da246d9b21adaea5a628 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 20 Mar 2023 00:17:23 -0300 Subject: [PATCH 34/78] add test for allowing --- test/access/manager/AccessManager.test.js | 28 ++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 4f07d70e86c..1db9fe4411d 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1,8 +1,8 @@ const { expectEvent, expectRevert, time: { duration } } = require('@openzeppelin/test-helpers'); const helpers = require('@nomicfoundation/hardhat-network-helpers'); -const AccessManager = artifacts.require('$AccessManager'); -const AccessManagedMock = artifacts.require('$AccessManagedMock'); +const AccessManager = artifacts.require('AccessManager'); +const AccessManaged = artifacts.require('$AccessManagedMock'); const badgeUtils = { mask: badge => 1n << BigInt(badge), @@ -18,7 +18,6 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { beforeEach('deploy', async function () { this.delay = duration.days(1); this.manager = await AccessManager.new(this.delay, admin); - this.managed = await AccessManaged.new(this.manager.address); }); it('configures default admin rules', async function () { @@ -160,4 +159,27 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); }); }); + + describe('allowing', function () { + const badge = '1'; + const badgeHolder = user1; + const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); + const otherSelector = web3.eth.abi.encodeFunctionSignature('other()'); + + beforeEach('deploying managed contract', async function () { + await this.manager.createBadge(badge, '', { from: admin }); + await this.manager.grantBadge(badgeHolder, badge, { from: admin }); + this.managed = await AccessManaged.new(this.manager.address); + }); + + it('allow', async function () { + await this.manager.methods['setFunctionAllowedBadge(address,bytes4[],uint8,bool)'](this.managed.address, [selector], badge, true, { from: admin }); + const restricted = await this.managed.restrictedFunction({ from: badgeHolder }); + expectEvent(restricted, 'RestrictedRan'); + }); + }); + + describe('groups', function () { + // TODO + }); }); From d517d27135ac43df103e61c1162cff3cee7fafbc Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 20 Mar 2023 00:20:07 -0300 Subject: [PATCH 35/78] lint --- contracts/access/manager/AccessManager.sol | 25 +++++++++------ test/access/manager/AccessManaged.test.js | 16 +++++----- test/access/manager/AccessManager.test.js | 36 ++++++++++------------ 3 files changed, 39 insertions(+), 38 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index e20e8b72d8e..26268eafa0e 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -28,10 +28,17 @@ interface IAccessManager is IAuthority { function renounceBadge(address user, uint8 badge) external; function getFunctionAllowedBadges(address target, bytes4 selector) external view returns (bytes32 badges); + function getFunctionAllowedBadges(string calldata group, bytes4 selector) external view returns (bytes32 badges); function setFunctionAllowedBadge(address target, bytes4[] calldata selectors, uint8 badge, bool allowed) external; - function setFunctionAllowedBadge(string calldata group, bytes4[] calldata selectors, uint8 badge, bool allowed) external; + + function setFunctionAllowedBadge( + string calldata group, + bytes4[] calldata selectors, + uint8 badge, + bool allowed + ) external; function getContractGroup(address target) external view returns (bytes32); @@ -138,7 +145,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Returns true if the badge has already been created via {createBadge}. */ - function hasBadge(uint8 badge) public virtual view returns (bool) { + function hasBadge(uint8 badge) public view virtual returns (bool) { return _getBadge(_createdBadges, badge); } @@ -341,17 +348,17 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * This role id starts with the ASCII characters `badge:`, followed by zeroes, and ends with the single byte * corresponding to the badge number. */ - function _encodeBadgeRole(uint8 badge) internal virtual pure returns (bytes32) { + function _encodeBadgeRole(uint8 badge) internal pure virtual returns (bytes32) { return bytes32("badge:") | bytes32(uint256(badge)); } /** * @dev Decodes a role id into a badge, if it is a role id of the kind returned by {_encodeBadgeRole}. */ - function _decodeBadgeRole(bytes32 role) internal virtual pure returns (bool, uint8) { + function _decodeBadgeRole(bytes32 role) internal pure virtual returns (bool, uint8) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; - uint8 badge = uint8(role[31]); + uint8 badge = uint8(role[31]); return (tag == bytes32("badge:"), badge); } @@ -360,7 +367,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * The group id consists of the ASCII characters `group:solo:` followed by the contract address bytes. */ - function _encodeSoloGroup(address target) internal virtual pure returns (bytes32) { + function _encodeSoloGroup(address target) internal pure virtual returns (bytes32) { return _GROUP_SOLO_PREFIX | (bytes32(bytes20(target)) >> (_GROUP_SOLO_PREFIX.length << 3)); } @@ -369,7 +376,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * The group id consists of the ASCII characters `group:custom:` followed by the group name. */ - function _encodeCustomGroup(string calldata groupName) internal virtual pure returns (bytes32) { + function _encodeCustomGroup(string calldata groupName) internal pure virtual returns (bytes32) { require(!_containsNullBytes(bytes32(bytes(groupName)), bytes(groupName).length)); require(bytes(groupName).length + _GROUP_CUSTOM_PREFIX.length < 31); return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(groupName)) >> (_GROUP_CUSTOM_PREFIX.length << 3)); @@ -380,7 +387,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * The group id consists of the ASCII characters `group:custom:` followed by the group name. */ - function _decodeCustomGroup(bytes32 group) internal virtual pure returns (string memory) { + function _decodeCustomGroup(bytes32 group) internal pure virtual returns (string memory) { string memory name = new string(32); uint256 nameLength = uint256(group) & 0xff; bytes32 nameBytes = group << _GROUP_CUSTOM_PREFIX.length; @@ -432,7 +439,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { // We will operate on every byte of the word simultaneously // We visualize the 8 bits of each byte as word[i] = 01234567 - // Take bitwise OR of all bits in each byte + // Take bitwise OR of all bits in each byte word |= word >> 4; // word[i] = 01234567 | ____0123 = ____abcd word |= word >> 2; // word[i] = ____abcd | ______ab = ______xy word |= word >> 1; // word[i] = ______xy | _______x = _______z diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index 9938796c915..9290ac17c2e 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -1,4 +1,8 @@ -const { expectEvent, expectRevert, constants: { ZERO_ADDRESS } } = require('@openzeppelin/test-helpers'); +const { + expectEvent, + expectRevert, + constants: { ZERO_ADDRESS }, +} = require('@openzeppelin/test-helpers'); const AccessManaged = artifacts.require('$AccessManagedMock'); const SimpleAuthority = artifacts.require('SimpleAuthority'); @@ -26,10 +30,7 @@ contract('AccessManager', function ([authority, other, user]) { it(`other account cannot change managed's authority`, async function () { const managed = await AccessManaged.new(authority); - await expectRevert( - managed.setAuthority(other, { from: other }), - 'AccessManaged: not current authority', - ); + await expectRevert(managed.setAuthority(other, { from: other }), 'AccessManaged: not current authority'); }); }); @@ -47,10 +48,7 @@ contract('AccessManager', function ([authority, other, user]) { it('reverts if authority says false', async function () { const authority = await SimpleAuthority.new(); const managed = await AccessManaged.new(authority.address); - await expectRevert( - managed.restrictedFunction({ from: user }), - 'AccessManaged: authority rejected', - ); + await expectRevert(managed.restrictedFunction({ from: user }), 'AccessManaged: authority rejected'); }); }); }); diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 1db9fe4411d..8bdd0703874 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -1,5 +1,8 @@ -const { expectEvent, expectRevert, time: { duration } } = require('@openzeppelin/test-helpers'); -const helpers = require('@nomicfoundation/hardhat-network-helpers'); +const { + expectEvent, + expectRevert, + time: { duration }, +} = require('@openzeppelin/test-helpers'); const AccessManager = artifacts.require('AccessManager'); const AccessManaged = artifacts.require('$AccessManagedMock'); @@ -56,10 +59,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); it('non-admin cannot create badges', async function () { - await expectRevert( - this.manager.createBadge(badge, name, { from: nonAdmin }), - 'missing role', - ); + await expectRevert(this.manager.createBadge(badge, name, { from: nonAdmin }), 'missing role'); }); }); @@ -74,10 +74,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); it('non-admin cannot update badge', async function () { - await expectRevert( - this.manager.updateBadgeName(badge, name, { from: nonAdmin }), - 'missing role', - ); + await expectRevert(this.manager.updateBadgeName(badge, name, { from: nonAdmin }), 'missing role'); }); }); @@ -94,10 +91,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); it('non-admin cannot grant badge', async function () { - await expectRevert( - this.manager.grantBadge(user1, badge, { from: nonAdmin }), - 'missing role', - ); + await expectRevert(this.manager.grantBadge(user1, badge, { from: nonAdmin }), 'missing role'); }); }); @@ -114,10 +108,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); it('non-admin cannot revoke badge', async function () { - await expectRevert( - this.manager.revokeBadge(user1, badge, { from: nonAdmin }), - 'missing role', - ); + await expectRevert(this.manager.revokeBadge(user1, badge, { from: nonAdmin }), 'missing role'); }); it('user can renounce badge', async function () { @@ -164,7 +155,6 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { const badge = '1'; const badgeHolder = user1; const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); - const otherSelector = web3.eth.abi.encodeFunctionSignature('other()'); beforeEach('deploying managed contract', async function () { await this.manager.createBadge(badge, '', { from: admin }); @@ -173,7 +163,13 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); it('allow', async function () { - await this.manager.methods['setFunctionAllowedBadge(address,bytes4[],uint8,bool)'](this.managed.address, [selector], badge, true, { from: admin }); + await this.manager.methods['setFunctionAllowedBadge(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + badge, + true, + { from: admin }, + ); const restricted = await this.managed.restrictedFunction({ from: badgeHolder }); expectEvent(restricted, 'RestrictedRan'); }); From 6d33cda34bf1bbcbf3a378eb96ce18bcd3fd2abf Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 20 Mar 2023 00:21:13 -0300 Subject: [PATCH 36/78] add changeset --- .changeset/quiet-trainers-kick.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quiet-trainers-kick.md diff --git a/.changeset/quiet-trainers-kick.md b/.changeset/quiet-trainers-kick.md new file mode 100644 index 00000000000..bb9bb57a6f5 --- /dev/null +++ b/.changeset/quiet-trainers-kick.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessManager`: Added a new contract for managing access control of complex systems in a central location. From 9ed1aef661448241f5415f5276b94acaaadcac1a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 20 Mar 2023 00:26:28 -0300 Subject: [PATCH 37/78] lint --- contracts/access/manager/AccessManagerAdapter.sol | 2 +- contracts/mocks/AccessManagerMocks.sol | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/contracts/access/manager/AccessManagerAdapter.sol b/contracts/access/manager/AccessManagerAdapter.sol index b3dbfc227b8..02ae81c7e0c 100644 --- a/contracts/access/manager/AccessManagerAdapter.sol +++ b/contracts/access/manager/AccessManagerAdapter.sol @@ -22,7 +22,7 @@ contract AccessManagerAdapter { AccessManager private _manager; - bytes32 private _DEFAULT_ADMIN_ROLE = 0; + bytes32 private constant _DEFAULT_ADMIN_ROLE = 0; /** * @dev Initializes an adapter connected to an AccessManager instance. diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol index 01b8183e01f..93807cc85a5 100644 --- a/contracts/mocks/AccessManagerMocks.sol +++ b/contracts/mocks/AccessManagerMocks.sol @@ -6,14 +6,14 @@ import "../access/manager/IAuthority.sol"; import "../access/manager/AccessManaged.sol"; contract SimpleAuthority is IAuthority { - address allowedCaller; - address allowedTarget; - bytes4 allowedSelector; + address _allowedCaller; + address _allowedTarget; + bytes4 _allowedSelector; - function setAllowed(address _allowedCaller, address _allowedTarget, bytes4 _allowedSelector) public { - allowedCaller = _allowedCaller; - allowedTarget = _allowedTarget; - allowedSelector = _allowedSelector; + function setAllowed(address allowedCaller, address allowedTarget, bytes4 allowedSelector) public { + _allowedCaller = allowedCaller; + _allowedTarget = allowedTarget; + _allowedSelector = allowedSelector; } function canCall(address caller, address target, bytes4 selector) external view override returns (bool) { From 6adc0d8517b0c397a376f64d88b3cf655bc41d8a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 01:15:14 -0300 Subject: [PATCH 38/78] fix mock --- contracts/mocks/AccessManagerMocks.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol index 93807cc85a5..8b4767c2885 100644 --- a/contracts/mocks/AccessManagerMocks.sol +++ b/contracts/mocks/AccessManagerMocks.sol @@ -17,7 +17,7 @@ contract SimpleAuthority is IAuthority { } function canCall(address caller, address target, bytes4 selector) external view override returns (bool) { - return caller == allowedCaller && target == allowedTarget && selector == allowedSelector; + return caller == _allowedCaller && target == _allowedTarget && selector == _allowedSelector; } } From 0624794581576126c866a4f54a0c5e5c1f72c875 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 01:15:43 -0300 Subject: [PATCH 39/78] remove contract groups --- contracts/access/manager/AccessManager.sol | 255 +++++---------------- test/access/manager/AccessManager.test.js | 2 +- 2 files changed, 53 insertions(+), 204 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 26268eafa0e..42b48cdca19 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -7,11 +7,17 @@ import "../AccessControlDefaultAdminRules.sol"; import "./IAuthority.sol"; interface IAccessManager is IAuthority { + enum RestrictedMode { + Custom, + Closed, + Open + } + event BadgeUpdated(uint8 indexed badge, string name); - event BadgeAllowed(bytes32 indexed group, bytes4 indexed selector, uint8 indexed badge, bool allowed); + event BadgeAllowed(address indexed target, bytes4 indexed selector, uint8 indexed badge, bool allowed); - event ContractGroupUpdated(address indexed target, bytes32 indexed group); + event RestrictedModeUpdated(address indexed target, RestrictedMode indexed mode); function createBadge(uint8 badge, string calldata name) external; @@ -29,20 +35,9 @@ interface IAccessManager is IAuthority { function getFunctionAllowedBadges(address target, bytes4 selector) external view returns (bytes32 badges); - function getFunctionAllowedBadges(string calldata group, bytes4 selector) external view returns (bytes32 badges); - function setFunctionAllowedBadge(address target, bytes4[] calldata selectors, uint8 badge, bool allowed) external; - function setFunctionAllowedBadge( - string calldata group, - bytes4[] calldata selectors, - uint8 badge, - bool allowed - ) external; - - function getContractGroup(address target) external view returns (bytes32); - - function setContractGroup(address target, string calldata groupName) external; + function getContractMode(address target) external view returns (RestrictedMode); function setContractOpen(address target) external; @@ -53,54 +48,44 @@ interface IAccessManager is IAuthority { } /** - * @dev AccessManager is a central contract that stores the permissions of a system. It is an AccessControl contract, - * i.e. it has roles and all the standard functions like `grantRole` and `revokeRole`, but it defines a particular set - * of roles, with a particular structure. + * @dev AccessManager is a central contract to store the permissions of a system. * - * Users are granted "badges". Badges must be created before they can be granted, with a maximum of 255 created badges. - * A user can be granted multiple badges. Each badge defines an AccessControl role, identified by a role id that starts - * with the ASCII characters `badge:`, followed by zeroes, and ending with the single byte corresponding to the badge - * number. + * The smart contracts under the control of an AccessManager instance will have a set of "restricted" functions, and the + * exact details of how access is restricted for each of those functions is configurable by the admins of the instance. + * These restrictions are expressed in terms of "badges". * - * Contracts in the system may be grouped as well. These are simply called "contract groups". There can be an arbitrary - * number of groups. Each contract can only be in one group at a time. In the simplest setup, each contract is assigned - * to its own separate group, but groups can also be shared among similar contracts. + * An AccessManager instance will define a set of badges. Each of them must be created before they can be granted, with + * a maximum of 255 created badges. Users can be granted any number of these badges. Each of them defines an + * AccessControl role, and may confer access to some of the restricted functions in the system, as configured by admins + * through the use of {setFunctionAllowedBadge}. * - * All contracts in a group share the same permissioning scheme. A permissioning scheme consists of a mapping between - * functions and allowed badges. Each function can be allowed to multiple badges, meaning that if a user has at least - * one of the allowed badges they can call that function. + * Note that a function in a target contract may become permissioned in this way only when: 1) said contract is + * {AccessManaged} and is connected to this contract as its manager, and 2) said function is decorated with the + * `restricted` modifier. * - * Note that a function in a target contract may become permissioned only when: 1) said contract is {AccessManaged} and - * is connected to this contract as its manager, and 2) said function is decorated with the `restricted` modifier. + * There is a special badge defined by default named "public" which all accounts automatically have. * - * There is a special badge defined by default named "public" which all accounts automatically have, and two special - * contract groups: 1) the "open" group, where all functions are allowed to the "public" badge, and 2) the "closed" - * group, where no function is allowed to any badge. + * Contracts can also be configured in two special modes: 1) the "open" mode, where all functions are allowed to the + * "public" badge, and 2) the "closed" mode, where no function is allowed to any badge. * - * Permissioning schemes and badge and contract group assignments can be configured by the default admin. The contract - * includes {AccessControlDefaultAdminRules} by default to enforce security rules on this account. Additionally, it is - * expected that the account will be highly secured (e.g., a multisig or a well-configured DAO) as all the permissions - * of the managed system can be modified by it. + * Since all the permissions of the managed system can be modified by the admins of this instance, it is expected that + * it will be highly secured (e.g., a multisig or a well-configured DAO). Additionally, {AccessControlDefaultAdminRules} + * is included to enforce security rules on this account. * * NOTE: Some of the functions in this contract, such as {getUserBadges}, return a `bytes32` bitmap to succintly * represent a set of badges. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if - * the badge with number `n` is in the set. + * the badge with number `n` is in the set. For example, the hex value `0x05` represents the set of the two badges + * numbered 0 and 2. */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdBadges; + mapping(address user => bytes32 badges) private _userBadges; - mapping(bytes32 group => mapping(bytes4 selector => bytes32 badges)) private _allowedBadges; - mapping(address target => bytes32 group) private _contractGroup; + mapping(address target => mapping(bytes4 selector => bytes32 badges)) private _allowedBadges; + mapping(address target => RestrictedMode mode) private _contractMode; uint8 private constant _BADGE_PUBLIC = 255; - bytes32 private constant _GROUP_UNSET = 0; - bytes32 private constant _GROUP_OPEN = "group:open"; - bytes32 private constant _GROUP_CLOSED = "group:closed"; - - bytes14 private constant _GROUP_SOLO_PREFIX = "group:solo:"; - bytes13 private constant _GROUP_CUSTOM_PREFIX = "group:custom:"; - /** * @dev Initializes an AccessManager with initial default admin and transfer delay. */ @@ -188,35 +173,22 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Returns a bitmap of the badges that are allowed to call a function of a target contract. If the target - * contract is in a group, the group's permissions are returned. + * contract is in open or closed mode it will be reflected in the return value. */ function getFunctionAllowedBadges(address target, bytes4 selector) public view virtual returns (bytes32) { - return _getFunctionAllowedBadges(getContractGroup(target), selector); - } - - /** - * @dev Returns a bitmap of the badges that are allowed to call a function of a group of contracts. - */ - function getFunctionAllowedBadges(string calldata group, bytes4 selector) public view virtual returns (bytes32) { - return _getFunctionAllowedBadges(_encodeCustomGroup(group), selector); - } - - /** - * @dev Returns a bitmap of the badges that are allowed to call a function selector of a contract group. - */ - function _getFunctionAllowedBadges(bytes32 group, bytes4 selector) internal view virtual returns (bytes32) { - if (group == _GROUP_OPEN) { + RestrictedMode mode = getContractMode(target); + if (mode == RestrictedMode.Open) { return _badgeMask(_BADGE_PUBLIC); - } else if (group == _GROUP_CLOSED) { + } else if (mode == RestrictedMode.Closed) { return 0; } else { - return _allowedBadges[group][selector]; + return _allowedBadges[target][selector]; } } /** - * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` - * argument. The caller must be the default admin. + * @dev Changes whether a badge is allowed to call a function of a contract, according to the `allowed` argument. + * The caller must be the default admin. */ function setFunctionAllowedBadge( address target, @@ -224,86 +196,37 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint8 badge, bool allowed ) public virtual { - require(_contractGroup[target] == 0); - _setFunctionAllowedBadge(_encodeSoloGroup(target), selectors, badge, allowed); - } - - /** - * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` - * argument. The caller must be the default admin. - */ - function setFunctionAllowedBadge( - string calldata group, - bytes4[] calldata selectors, - uint8 badge, - bool allowed - ) public virtual { - _setFunctionAllowedBadge(_encodeCustomGroup(group), selectors, badge, allowed); - } - - /** - * @dev Changes whether a badge is allowed to call a function of a contract group, according to the `allowed` - * argument. The caller must be the default admin. - */ - function _setFunctionAllowedBadge( - bytes32 group, - bytes4[] calldata selectors, - uint8 badge, - bool allowed - ) internal virtual onlyDefaultAdmin { - require(group != 0); + require(_contractMode[target] == RestrictedMode.Custom); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; - _allowedBadges[group][selector] = _withUpdatedBadge(_allowedBadges[group][selector], badge, allowed); - emit BadgeAllowed(group, selector, badge, allowed); - } - } - - /** - * @dev Returns the group of the target contract, which may be its solo group (the default), a custom group, or - * the open or closed groups. - */ - function getContractGroup(address target) public view virtual returns (bytes32) { - bytes32 group = _contractGroup[target]; - if (group == _GROUP_UNSET) { - return _encodeSoloGroup(target); - } else { - return group; + _allowedBadges[target][selector] = _withUpdatedBadge(_allowedBadges[target][selector], badge, allowed); + emit BadgeAllowed(target, selector, badge, allowed); } } - // TODO: filter open/closed? /** - * @dev Sets the contract group that the target belongs to. The caller must be the default admin. - * - * CAUTION: This is a batch operation that will immediately switch the mapping of functions to allowed badges. - * Accounts that were previously able to call permissioned functions on the target contract may no longer be - * allowed, and new sets of account may be allowed after this operation. + * @dev Returns the mode of the target contract, which may be custom (`0`), closed (`1`), or open (`2`). */ - function setContractGroup(address target, string calldata groupName) public virtual onlyDefaultAdmin { - bytes32 group = _encodeCustomGroup(groupName); - _contractGroup[target] = group; - emit ContractGroupUpdated(target, group); + function getContractMode(address target) public view virtual returns (RestrictedMode) { + return _contractMode[target]; } /** - * @dev Sets the target contract to be in the "open" group. All restricted functions in the target contract will - * become callable by anyone. The caller must be the default admin. + * @dev Sets the target contract to be in "open" mode. All restricted functions in the target contract will become + * callable by anyone. The caller must be the default admin. */ function setContractOpen(address target) public virtual onlyDefaultAdmin { - bytes32 group = _GROUP_OPEN; - _contractGroup[target] = group; - emit ContractGroupUpdated(target, group); + _contractMode[target] = RestrictedMode.Open; + emit RestrictedModeUpdated(target, RestrictedMode.Open); } /** - * @dev Sets the target contract to be in the "closed" group. All restricted functions in the target contract will - * be closed down and disallowed to all. The caller must be the default admin. + * @dev Sets the target contract to be in "closed" mode. All restricted functions in the target contract will be + * closed down and disallowed to all. The caller must be the default admin. */ function setContractClosed(address target) public virtual onlyDefaultAdmin { - bytes32 group = _GROUP_CLOSED; - _contractGroup[target] = group; - emit ContractGroupUpdated(target, group); + _contractMode[target] = RestrictedMode.Closed; + emit RestrictedModeUpdated(target, RestrictedMode.Closed); } /** @@ -362,50 +285,6 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return (tag == bytes32("badge:"), badge); } - /** - * @dev Returns the solo group id for a target contract. - * - * The group id consists of the ASCII characters `group:solo:` followed by the contract address bytes. - */ - function _encodeSoloGroup(address target) internal pure virtual returns (bytes32) { - return _GROUP_SOLO_PREFIX | (bytes32(bytes20(target)) >> (_GROUP_SOLO_PREFIX.length << 3)); - } - - /** - * @dev Returns the custom group id for a given group name. - * - * The group id consists of the ASCII characters `group:custom:` followed by the group name. - */ - function _encodeCustomGroup(string calldata groupName) internal pure virtual returns (bytes32) { - require(!_containsNullBytes(bytes32(bytes(groupName)), bytes(groupName).length)); - require(bytes(groupName).length + _GROUP_CUSTOM_PREFIX.length < 31); - return _GROUP_CUSTOM_PREFIX | (bytes32(bytes(groupName)) >> (_GROUP_CUSTOM_PREFIX.length << 3)); - } - - /** - * @dev Returns the custom group id for a given group name. - * - * The group id consists of the ASCII characters `group:custom:` followed by the group name. - */ - function _decodeCustomGroup(bytes32 group) internal pure virtual returns (string memory) { - string memory name = new string(32); - uint256 nameLength = uint256(group) & 0xff; - bytes32 nameBytes = group << _GROUP_CUSTOM_PREFIX.length; - /// @solidity memory-safe-assembly - assembly { - mstore(name, nameLength) - mstore(add(name, 0x20), nameBytes) - } - return name; - } - - /** - * @dev Returns true if the group is one of "open", "closed", or unset (zero). - */ - function _isSpecialGroup(bytes32 group) private pure returns (bool) { - return group == _GROUP_OPEN || group == _GROUP_CLOSED; - } - /** * @dev Returns a bit mask where the only non-zero bit is the badge number bit. */ @@ -431,34 +310,4 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return bitmap & ~mask; } } - - /** - * @dev Returns true if the word contains any NUL bytes in the highest `scope` bytes. `scope` must be at most 32. - */ - function _containsNullBytes(bytes32 word, uint256 scope) private pure returns (bool) { - // We will operate on every byte of the word simultaneously - // We visualize the 8 bits of each byte as word[i] = 01234567 - - // Take bitwise OR of all bits in each byte - word |= word >> 4; // word[i] = 01234567 | ____0123 = ____abcd - word |= word >> 2; // word[i] = ____abcd | ______ab = ______xy - word |= word >> 1; // word[i] = ______xy | _______x = _______z - - // z is 1 iff any bit of word[i] is 1 - - // Every byte in `low` is 0x01 - bytes32 low = hex"0101010101010101010101010101010101010101010101010101010101010101"; - - // Select the lowest bit only and take its complement - word &= low; // word[i] = 0000000z - word ^= low; // word[i] = 0000000Z (Z = ~z) - - // Ignore anything past the scope - unchecked { - word >>= 32 - scope; - } - - // If any byte in scope was 0x00 it will now contain 00000001 - return word != 0; - } } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 8bdd0703874..cf9803b4031 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -175,7 +175,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { }); }); - describe('groups', function () { + describe('modes', function () { // TODO }); }); From 17ec3b69fbcbd8eecbbc7ee14fcd634732155366 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 01:26:00 -0300 Subject: [PATCH 40/78] add transferContractAuthority --- contracts/access/manager/AccessManager.sol | 11 +++++++++-- test/access/manager/AccessManager.test.js | 20 +++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 42b48cdca19..e13500d5c3b 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.18; import "../AccessControl.sol"; import "../AccessControlDefaultAdminRules.sol"; import "./IAuthority.sol"; +import "./AccessManaged.sol"; interface IAccessManager is IAuthority { enum RestrictedMode { @@ -43,8 +44,7 @@ interface IAccessManager is IAuthority { function setContractClosed(address target) external; - // TODO: Ability to eject a contract. See AccessManaged.setAuthority - // function transferContractAuthority(address target, address newAuthority) external; + function transferContractAuthority(address target, address newAuthority) external; } /** @@ -229,6 +229,13 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { emit RestrictedModeUpdated(target, RestrictedMode.Closed); } + /** + * @dev Transfers a target contract onto a new authority. The caller must be the default admin. + */ + function transferContractAuthority(address target, address newAuthority) onlyDefaultAdmin public virtual { + AccessManaged(target).setAuthority(IAuthority(newAuthority)); + } + /** * @dev Creates a new badge. * diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index cf9803b4031..50cb4931e8f 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -17,7 +17,7 @@ const badgeUtils = { role: badge => web3.utils.asciiToHex('badge:').padEnd(64, '0') + badge.toString(16).padStart(2, '0'), }; -contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { +contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthority]) { beforeEach('deploy', async function () { this.delay = duration.days(1); this.manager = await AccessManager.new(this.delay, admin); @@ -178,4 +178,22 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2]) { describe('modes', function () { // TODO }); + + describe('transfering authority', function () { + beforeEach('deploying managed contract', async function () { + this.managed = await AccessManaged.new(this.manager.address); + }); + + it('admin can transfer authority', async function () { + await this.manager.transferContractAuthority(this.managed.address, otherAuthority, { from: admin }); + expect(await this.managed.authority()).to.equal(otherAuthority); + }); + + it('non-admin cannot transfer authority', async function () { + await expectRevert( + this.manager.transferContractAuthority(this.managed.address, otherAuthority, { from: nonAdmin }), + 'missing role', + ); + }); + }); }); From f9210c59c05d1a37b3e8df821c3aae881defa1b5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 01:26:55 -0300 Subject: [PATCH 41/78] lint --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index e13500d5c3b..e6c6db4115a 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -232,7 +232,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Transfers a target contract onto a new authority. The caller must be the default admin. */ - function transferContractAuthority(address target, address newAuthority) onlyDefaultAdmin public virtual { + function transferContractAuthority(address target, address newAuthority) public virtual onlyDefaultAdmin { AccessManaged(target).setAuthority(IAuthority(newAuthority)); } From a54cd1082aed32a6ad40e3c4d4ca3f1fd7a60acd Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 01:30:35 -0300 Subject: [PATCH 42/78] rename badge -> group --- contracts/access/manager/AccessManager.sol | 198 ++++++++++----------- test/access/manager/AccessManager.test.js | 146 +++++++-------- 2 files changed, 172 insertions(+), 172 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index e6c6db4115a..c63dcf8ac14 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -14,29 +14,29 @@ interface IAccessManager is IAuthority { Open } - event BadgeUpdated(uint8 indexed badge, string name); + event GroupUpdated(uint8 indexed group, string name); - event BadgeAllowed(address indexed target, bytes4 indexed selector, uint8 indexed badge, bool allowed); + event GroupAllowed(address indexed target, bytes4 indexed selector, uint8 indexed group, bool allowed); event RestrictedModeUpdated(address indexed target, RestrictedMode indexed mode); - function createBadge(uint8 badge, string calldata name) external; + function createGroup(uint8 group, string calldata name) external; - function updateBadgeName(uint8 badge, string calldata name) external; + function updateGroupName(uint8 group, string calldata name) external; - function hasBadge(uint8 badge) external view returns (bool); + function hasGroup(uint8 group) external view returns (bool); - function getUserBadges(address user) external view returns (bytes32 badges); + function getUserGroups(address user) external view returns (bytes32 groups); - function grantBadge(address user, uint8 badge) external; + function grantGroup(address user, uint8 group) external; - function revokeBadge(address user, uint8 badge) external; + function revokeGroup(address user, uint8 group) external; - function renounceBadge(address user, uint8 badge) external; + function renounceGroup(address user, uint8 group) external; - function getFunctionAllowedBadges(address target, bytes4 selector) external view returns (bytes32 badges); + function getFunctionAllowedGroups(address target, bytes4 selector) external view returns (bytes32 groups); - function setFunctionAllowedBadge(address target, bytes4[] calldata selectors, uint8 badge, bool allowed) external; + function setFunctionAllowedGroup(address target, bytes4[] calldata selectors, uint8 group, bool allowed) external; function getContractMode(address target) external view returns (RestrictedMode); @@ -52,39 +52,39 @@ interface IAccessManager is IAuthority { * * The smart contracts under the control of an AccessManager instance will have a set of "restricted" functions, and the * exact details of how access is restricted for each of those functions is configurable by the admins of the instance. - * These restrictions are expressed in terms of "badges". + * These restrictions are expressed in terms of "groups". * - * An AccessManager instance will define a set of badges. Each of them must be created before they can be granted, with - * a maximum of 255 created badges. Users can be granted any number of these badges. Each of them defines an + * An AccessManager instance will define a set of groups. Each of them must be created before they can be granted, with + * a maximum of 255 created groups. Users can be added into any number of these groups. Each of them defines an * AccessControl role, and may confer access to some of the restricted functions in the system, as configured by admins - * through the use of {setFunctionAllowedBadge}. + * through the use of {setFunctionAllowedGroup}. * * Note that a function in a target contract may become permissioned in this way only when: 1) said contract is * {AccessManaged} and is connected to this contract as its manager, and 2) said function is decorated with the * `restricted` modifier. * - * There is a special badge defined by default named "public" which all accounts automatically have. + * There is a special group defined by default named "public" which all accounts automatically have. * * Contracts can also be configured in two special modes: 1) the "open" mode, where all functions are allowed to the - * "public" badge, and 2) the "closed" mode, where no function is allowed to any badge. + * "public" group, and 2) the "closed" mode, where no function is allowed to any group. * * Since all the permissions of the managed system can be modified by the admins of this instance, it is expected that * it will be highly secured (e.g., a multisig or a well-configured DAO). Additionally, {AccessControlDefaultAdminRules} * is included to enforce security rules on this account. * - * NOTE: Some of the functions in this contract, such as {getUserBadges}, return a `bytes32` bitmap to succintly - * represent a set of badges. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if - * the badge with number `n` is in the set. For example, the hex value `0x05` represents the set of the two badges + * NOTE: Some of the functions in this contract, such as {getUserGroups}, return a `bytes32` bitmap to succintly + * represent a set of groups. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if + * the group with number `n` is in the set. For example, the hex value `0x05` represents the set of the two groups * numbered 0 and 2. */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { - bytes32 _createdBadges; + bytes32 _createdGroups; - mapping(address user => bytes32 badges) private _userBadges; - mapping(address target => mapping(bytes4 selector => bytes32 badges)) private _allowedBadges; + mapping(address user => bytes32 groups) private _userGroups; + mapping(address target => mapping(bytes4 selector => bytes32 groups)) private _allowedGroups; mapping(address target => RestrictedMode mode) private _contractMode; - uint8 private constant _BADGE_PUBLIC = 255; + uint8 private constant _GROUP_PUBLIC = 255; /** * @dev Initializes an AccessManager with initial default admin and transfer delay. @@ -93,7 +93,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint48 initialDefaultAdminDelay, address initialDefaultAdmin ) AccessControlDefaultAdminRules(initialDefaultAdminDelay, initialDefaultAdmin) { - _createBadge(_BADGE_PUBLIC, "public"); + _createGroup(_GROUP_PUBLIC, "public"); } /** @@ -101,106 +101,106 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Entrypoint for {AccessManaged} contracts. */ function canCall(address caller, address target, bytes4 selector) public view returns (bool) { - bytes32 allowedBadges = getFunctionAllowedBadges(target, selector); - bytes32 callerBadges = getUserBadges(caller); - return callerBadges & allowedBadges != 0; + bytes32 allowedGroups = getFunctionAllowedGroups(target, selector); + bytes32 callerGroups = getUserGroups(caller); + return callerGroups & allowedGroups != 0; } /** - * @dev Creates a new badge with a badge number that can be chosen arbitrarily but must be unused, and gives it a + * @dev Creates a new group with a group number that can be chosen arbitrarily but must be unused, and gives it a * human-readable name. The caller must be the default admin. * - * Badge numbers are not auto-incremented in order to avoid race conditions, but administrators can safely use + * Group numbers are not auto-incremented in order to avoid race conditions, but administrators can safely use * sequential numbers. * - * Emits {BadgeUpdated}. + * Emits {GroupUpdated}. */ - function createBadge(uint8 badge, string memory name) public virtual onlyDefaultAdmin { - _createBadge(badge, name); + function createGroup(uint8 group, string memory name) public virtual onlyDefaultAdmin { + _createGroup(group, name); } /** - * @dev Updates an existing badge's name. The caller must be the default admin. + * @dev Updates an existing group's name. The caller must be the default admin. */ - function updateBadgeName(uint8 badge, string memory name) public virtual onlyDefaultAdmin { - require(badge != _BADGE_PUBLIC && hasBadge(badge)); - emit BadgeUpdated(badge, name); + function updateGroupName(uint8 group, string memory name) public virtual onlyDefaultAdmin { + require(group != _GROUP_PUBLIC && hasGroup(group)); + emit GroupUpdated(group, name); } /** - * @dev Returns true if the badge has already been created via {createBadge}. + * @dev Returns true if the group has already been created via {createGroup}. */ - function hasBadge(uint8 badge) public view virtual returns (bool) { - return _getBadge(_createdBadges, badge); + function hasGroup(uint8 group) public view virtual returns (bool) { + return _getGroup(_createdGroups, group); } /** - * @dev Returns a bitmap of the badges the user has. See note on bitmaps above. + * @dev Returns a bitmap of the groups the user has. See note on bitmaps above. */ - function getUserBadges(address user) public view virtual returns (bytes32) { - return _userBadges[user] | _badgeMask(_BADGE_PUBLIC); + function getUserGroups(address user) public view virtual returns (bytes32) { + return _userGroups[user] | _groupMask(_GROUP_PUBLIC); } /** - * @dev Grants a user a badge. + * @dev Grants a user a group. * - * Emits {RoleGranted} with the role id of the badge, if wasn't already held by the user. + * Emits {RoleGranted} with the role id of the group, if wasn't already held by the user. */ - function grantBadge(address user, uint8 badge) public virtual { + function grantGroup(address user, uint8 group) public virtual { // grantRole checks that msg.sender is admin for the role - grantRole(_encodeBadgeRole(badge), user); + grantRole(_encodeGroupRole(group), user); } /** - * @dev Removes a badge from a user. + * @dev Removes a group from a user. * - * Emits {RoleRevoked} with the role id of the badge, if previously held by the user. + * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ - function revokeBadge(address user, uint8 badge) public virtual { + function revokeGroup(address user, uint8 group) public virtual { // revokeRole checks that msg.sender is admin for the role - revokeRole(_encodeBadgeRole(badge), user); + revokeRole(_encodeGroupRole(group), user); } /** - * @dev Allows a user to renounce a badge. + * @dev Allows a user to renounce a group. * - * Emits {RoleRevoked} with the role id of the badge, if previously held by the user. + * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ - function renounceBadge(address user, uint8 badge) public virtual { + function renounceGroup(address user, uint8 group) public virtual { // renounceRole checks that msg.sender is user - renounceRole(_encodeBadgeRole(badge), user); + renounceRole(_encodeGroupRole(group), user); } /** - * @dev Returns a bitmap of the badges that are allowed to call a function of a target contract. If the target + * @dev Returns a bitmap of the groups that are allowed to call a function of a target contract. If the target * contract is in open or closed mode it will be reflected in the return value. */ - function getFunctionAllowedBadges(address target, bytes4 selector) public view virtual returns (bytes32) { + function getFunctionAllowedGroups(address target, bytes4 selector) public view virtual returns (bytes32) { RestrictedMode mode = getContractMode(target); if (mode == RestrictedMode.Open) { - return _badgeMask(_BADGE_PUBLIC); + return _groupMask(_GROUP_PUBLIC); } else if (mode == RestrictedMode.Closed) { return 0; } else { - return _allowedBadges[target][selector]; + return _allowedGroups[target][selector]; } } /** - * @dev Changes whether a badge is allowed to call a function of a contract, according to the `allowed` argument. + * @dev Changes whether a group is allowed to call a function of a contract, according to the `allowed` argument. * The caller must be the default admin. */ - function setFunctionAllowedBadge( + function setFunctionAllowedGroup( address target, bytes4[] calldata selectors, - uint8 badge, + uint8 group, bool allowed ) public virtual { require(_contractMode[target] == RestrictedMode.Custom); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; - _allowedBadges[target][selector] = _withUpdatedBadge(_allowedBadges[target][selector], badge, allowed); - emit BadgeAllowed(target, selector, badge, allowed); + _allowedGroups[target][selector] = _withUpdatedGroup(_allowedGroups[target][selector], group, allowed); + emit GroupAllowed(target, selector, group, allowed); } } @@ -237,80 +237,80 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } /** - * @dev Creates a new badge. + * @dev Creates a new group. * - * Emits {BadgeUpdated}. + * Emits {GroupUpdated}. */ - function _createBadge(uint8 badge, string memory name) internal virtual { - require(!hasBadge(badge)); - _createdBadges = _withUpdatedBadge(_createdBadges, badge, true); - emit BadgeUpdated(badge, name); + function _createGroup(uint8 group, string memory name) internal virtual { + require(!hasGroup(group)); + _createdGroups = _withUpdatedGroup(_createdGroups, group, true); + emit GroupUpdated(group, name); } /** - * @dev Augmented version of {AccessControl-_grantRole} that keeps track of user badge bitmaps. + * @dev Augmented version of {AccessControl-_grantRole} that keeps track of user group bitmaps. */ function _grantRole(bytes32 role, address user) internal virtual override { super._grantRole(role, user); - (bool isBadge, uint8 badge) = _decodeBadgeRole(role); - if (isBadge) { - require(hasBadge(badge)); - _userBadges[user] = _withUpdatedBadge(_userBadges[user], badge, true); + (bool isGroup, uint8 group) = _decodeGroupRole(role); + if (isGroup) { + require(hasGroup(group)); + _userGroups[user] = _withUpdatedGroup(_userGroups[user], group, true); } } /** - * @dev Augmented version of {AccessControl-_revokeRole} that keeps track of user badge bitmaps. + * @dev Augmented version of {AccessControl-_revokeRole} that keeps track of user group bitmaps. */ function _revokeRole(bytes32 role, address user) internal virtual override { super._revokeRole(role, user); - (bool isBadge, uint8 badge) = _decodeBadgeRole(role); - if (isBadge) { - require(hasBadge(badge)); - require(badge != _BADGE_PUBLIC); - _userBadges[user] = _withUpdatedBadge(_userBadges[user], badge, false); + (bool isGroup, uint8 group) = _decodeGroupRole(role); + if (isGroup) { + require(hasGroup(group)); + require(group != _GROUP_PUBLIC); + _userGroups[user] = _withUpdatedGroup(_userGroups[user], group, false); } } /** - * @dev Returns the {AccessControl} role id that corresponds to a badge. + * @dev Returns the {AccessControl} role id that corresponds to a group. * - * This role id starts with the ASCII characters `badge:`, followed by zeroes, and ends with the single byte - * corresponding to the badge number. + * This role id starts with the ASCII characters `group:`, followed by zeroes, and ends with the single byte + * corresponding to the group number. */ - function _encodeBadgeRole(uint8 badge) internal pure virtual returns (bytes32) { - return bytes32("badge:") | bytes32(uint256(badge)); + function _encodeGroupRole(uint8 group) internal pure virtual returns (bytes32) { + return bytes32("group:") | bytes32(uint256(group)); } /** - * @dev Decodes a role id into a badge, if it is a role id of the kind returned by {_encodeBadgeRole}. + * @dev Decodes a role id into a group, if it is a role id of the kind returned by {_encodeGroupRole}. */ - function _decodeBadgeRole(bytes32 role) internal pure virtual returns (bool, uint8) { + function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool, uint8) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; - uint8 badge = uint8(role[31]); - return (tag == bytes32("badge:"), badge); + uint8 group = uint8(role[31]); + return (tag == bytes32("group:"), group); } /** - * @dev Returns a bit mask where the only non-zero bit is the badge number bit. + * @dev Returns a bit mask where the only non-zero bit is the group number bit. */ - function _badgeMask(uint8 badge) private pure returns (bytes32) { - return bytes32(1 << badge); + function _groupMask(uint8 group) private pure returns (bytes32) { + return bytes32(1 << group); } /** - * @dev Returns the value of the badge number bit in a bitmap. + * @dev Returns the value of the group number bit in a bitmap. */ - function _getBadge(bytes32 bitmap, uint8 badge) private pure returns (bool) { - return bitmap & _badgeMask(badge) > 0; + function _getGroup(bytes32 bitmap, uint8 group) private pure returns (bool) { + return bitmap & _groupMask(group) > 0; } /** - * @dev Returns a new badge bitmap where a specific badge was updated. + * @dev Returns a new group bitmap where a specific group was updated. */ - function _withUpdatedBadge(bytes32 bitmap, uint8 badge, bool value) private pure returns (bytes32) { - bytes32 mask = _badgeMask(badge); + function _withUpdatedGroup(bytes32 bitmap, uint8 group, bool value) private pure returns (bytes32) { + bytes32 mask = _groupMask(group); if (value) { return bitmap | mask; } else { diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 50cb4931e8f..5c11916ea65 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -7,14 +7,14 @@ const { const AccessManager = artifacts.require('AccessManager'); const AccessManaged = artifacts.require('$AccessManagedMock'); -const badgeUtils = { - mask: badge => 1n << BigInt(badge), +const groupUtils = { + mask: group => 1n << BigInt(group), decodeBitmap: hexBitmap => { const m = BigInt(hexBitmap); - const allBadges = new Array(256).fill().map((_, i) => i.toString()); - return allBadges.filter(i => (m & badgeUtils.mask(i)) !== 0n); + const allGroups = new Array(256).fill().map((_, i) => i.toString()); + return allGroups.filter(i => (m & groupUtils.mask(i)) !== 0n); }, - role: badge => web3.utils.asciiToHex('badge:').padEnd(64, '0') + badge.toString(16).padStart(2, '0'), + role: group => web3.utils.asciiToHex('group:').padEnd(64, '0') + group.toString(16).padStart(2, '0'), }; contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthority]) { @@ -28,149 +28,149 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori expect(await this.manager.defaultAdminDelay()).to.be.bignumber.equal(this.delay); }); - describe('badges', function () { - const badge = '0'; + describe('groups', function () { + const group = '0'; const name = 'dao'; - const otherBadge = '1'; + const otherGroup = '1'; const otherName = 'council'; - describe('public badge', function () { - const publicBadge = '255'; + describe('public group', function () { + const publicGroup = '255'; it('is created automatically', async function () { - await expectEvent.inConstruction(this.manager, 'BadgeUpdated', { - badge: publicBadge, + await expectEvent.inConstruction(this.manager, 'GroupUpdated', { + group: publicGroup, name: 'public', }); }); it('includes all users automatically', async function () { - const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); - expect(badges).to.include(publicBadge); + const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); + expect(groups).to.include(publicGroup); }); }); describe('creating', function () { - it('admin can create badges', async function () { - const created = await this.manager.createBadge(badge, name, { from: admin }); - expectEvent(created, 'BadgeUpdated', { badge, name }); - expect(await this.manager.hasBadge(badge)).to.equal(true); - expect(await this.manager.hasBadge(otherBadge)).to.equal(false); + it('admin can create groups', async function () { + const created = await this.manager.createGroup(group, name, { from: admin }); + expectEvent(created, 'GroupUpdated', { group, name }); + expect(await this.manager.hasGroup(group)).to.equal(true); + expect(await this.manager.hasGroup(otherGroup)).to.equal(false); }); - it('non-admin cannot create badges', async function () { - await expectRevert(this.manager.createBadge(badge, name, { from: nonAdmin }), 'missing role'); + it('non-admin cannot create groups', async function () { + await expectRevert(this.manager.createGroup(group, name, { from: nonAdmin }), 'missing role'); }); }); describe('updating', function () { - beforeEach('create badge', async function () { - await this.manager.createBadge(badge, name, { from: admin }); + beforeEach('create group', async function () { + await this.manager.createGroup(group, name, { from: admin }); }); - it('admin can update badge', async function () { - const updated = await this.manager.updateBadgeName(badge, otherName, { from: admin }); - expectEvent(updated, 'BadgeUpdated', { badge, name: otherName }); + it('admin can update group', async function () { + const updated = await this.manager.updateGroupName(group, otherName, { from: admin }); + expectEvent(updated, 'GroupUpdated', { group, name: otherName }); }); - it('non-admin cannot update badge', async function () { - await expectRevert(this.manager.updateBadgeName(badge, name, { from: nonAdmin }), 'missing role'); + it('non-admin cannot update group', async function () { + await expectRevert(this.manager.updateGroupName(group, name, { from: nonAdmin }), 'missing role'); }); }); describe('granting', function () { - beforeEach('create badge', async function () { - await this.manager.createBadge(badge, name, { from: admin }); + beforeEach('create group', async function () { + await this.manager.createGroup(group, name, { from: admin }); }); - it('admin can grant badge', async function () { - const granted = await this.manager.grantBadge(user1, badge, { from: admin }); - expectEvent(granted, 'RoleGranted', { account: user1, role: badgeUtils.role(badge) }); - const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); - expect(badges).to.include(badge); + it('admin can grant group', async function () { + const granted = await this.manager.grantGroup(user1, group, { from: admin }); + expectEvent(granted, 'RoleGranted', { account: user1, role: groupUtils.role(group) }); + const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); + expect(groups).to.include(group); }); - it('non-admin cannot grant badge', async function () { - await expectRevert(this.manager.grantBadge(user1, badge, { from: nonAdmin }), 'missing role'); + it('non-admin cannot grant group', async function () { + await expectRevert(this.manager.grantGroup(user1, group, { from: nonAdmin }), 'missing role'); }); }); describe('revoking & renouncing', function () { - beforeEach('create and grant badge', async function () { - await this.manager.createBadge(badge, name, { from: admin }); - await this.manager.grantBadge(user1, badge, { from: admin }); + beforeEach('create and grant group', async function () { + await this.manager.createGroup(group, name, { from: admin }); + await this.manager.grantGroup(user1, group, { from: admin }); }); - it('admin can revoke badge', async function () { - await this.manager.revokeBadge(user1, badge, { from: admin }); - const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); - expect(badges).to.not.include(badge); + it('admin can revoke group', async function () { + await this.manager.revokeGroup(user1, group, { from: admin }); + const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); + expect(groups).to.not.include(group); }); - it('non-admin cannot revoke badge', async function () { - await expectRevert(this.manager.revokeBadge(user1, badge, { from: nonAdmin }), 'missing role'); + it('non-admin cannot revoke group', async function () { + await expectRevert(this.manager.revokeGroup(user1, group, { from: nonAdmin }), 'missing role'); }); - it('user can renounce badge', async function () { - await this.manager.renounceBadge(user1, badge, { from: user1 }); - const badges = badgeUtils.decodeBitmap(await this.manager.getUserBadges(user1)); - expect(badges).to.not.include(badge); + it('user can renounce group', async function () { + await this.manager.renounceGroup(user1, group, { from: user1 }); + const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); + expect(groups).to.not.include(group); }); - it(`user cannot renounce other user's badges`, async function () { + it(`user cannot renounce other user's groups`, async function () { await expectRevert( - this.manager.renounceBadge(user1, badge, { from: user2 }), + this.manager.renounceGroup(user1, group, { from: user2 }), 'can only renounce roles for self', ); await expectRevert( - this.manager.renounceBadge(user2, badge, { from: user1 }), + this.manager.renounceGroup(user2, group, { from: user1 }), 'can only renounce roles for self', ); }); }); describe('querying', function () { - it('returns expected badges', async function () { - const getBadges = () => this.manager.getUserBadges(user1); + it('returns expected groups', async function () { + const getGroups = () => this.manager.getUserGroups(user1); - // only public badge initially - expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000000'); + // only public group initially + expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000000'); - await this.manager.createBadge('0', '0', { from: admin }); - await this.manager.grantBadge(user1, '0', { from: admin }); - expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000001'); + await this.manager.createGroup('0', '0', { from: admin }); + await this.manager.grantGroup(user1, '0', { from: admin }); + expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000001'); - await this.manager.createBadge('1', '1', { from: admin }); - await this.manager.grantBadge(user1, '1', { from: admin }); - expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000003'); + await this.manager.createGroup('1', '1', { from: admin }); + await this.manager.grantGroup(user1, '1', { from: admin }); + expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000003'); - await this.manager.createBadge('16', '16', { from: admin }); - await this.manager.grantBadge(user1, '16', { from: admin }); - expect(await getBadges()).to.equal('0x8000000000000000000000000000000000000000000000000000000000010003'); + await this.manager.createGroup('16', '16', { from: admin }); + await this.manager.grantGroup(user1, '16', { from: admin }); + expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000010003'); }); }); }); describe('allowing', function () { - const badge = '1'; - const badgeHolder = user1; + const group = '1'; + const groupMember = user1; const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); beforeEach('deploying managed contract', async function () { - await this.manager.createBadge(badge, '', { from: admin }); - await this.manager.grantBadge(badgeHolder, badge, { from: admin }); + await this.manager.createGroup(group, '', { from: admin }); + await this.manager.grantGroup(groupMember, group, { from: admin }); this.managed = await AccessManaged.new(this.manager.address); }); it('allow', async function () { - await this.manager.methods['setFunctionAllowedBadge(address,bytes4[],uint8,bool)']( + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector], - badge, + group, true, { from: admin }, ); - const restricted = await this.managed.restrictedFunction({ from: badgeHolder }); + const restricted = await this.managed.restrictedFunction({ from: groupMember }); expectEvent(restricted, 'RestrictedRan'); }); }); From 8af9ca49bc4f25bbe6e9e9c3bb39e10d1f396a2b Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 11:59:36 -0300 Subject: [PATCH 43/78] change test name --- test/access/manager/AccessManaged.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index 9290ac17c2e..520b10c49b4 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -7,7 +7,7 @@ const { const AccessManaged = artifacts.require('$AccessManagedMock'); const SimpleAuthority = artifacts.require('SimpleAuthority'); -contract('AccessManager', function ([authority, other, user]) { +contract('AccessManaged', function ([authority, other, user]) { it('construction', async function () { const managed = await AccessManaged.new(authority); expectEvent.inConstruction(managed, 'AuthorityUpdated', { From 2a05bccb47e8ec9fe576b98e6b7c902c699cf411 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 12:24:22 -0300 Subject: [PATCH 44/78] add setContractModeCustom and tests --- contracts/access/manager/AccessManager.sol | 19 ++++-- test/access/manager/AccessManager.test.js | 70 ++++++++++++++++++++-- test/helpers/enums.js | 1 + 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index c63dcf8ac14..019ba561c26 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -40,9 +40,11 @@ interface IAccessManager is IAuthority { function getContractMode(address target) external view returns (RestrictedMode); - function setContractOpen(address target) external; + function setContractModeCustom(address target) external; - function setContractClosed(address target) external; + function setContractModeOpen(address target) external; + + function setContractModeClosed(address target) external; function transferContractAuthority(address target, address newAuthority) external; } @@ -211,11 +213,20 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { return _contractMode[target]; } + /** + * @dev Sets the target contract to be in custom restricted mode. All restricted functions in the target contract + * will follow the group-based restrictions defined by the AccessManager. The caller must be the default admin. + */ + function setContractModeCustom(address target) public virtual onlyDefaultAdmin { + _contractMode[target] = RestrictedMode.Custom; + emit RestrictedModeUpdated(target, RestrictedMode.Custom); + } + /** * @dev Sets the target contract to be in "open" mode. All restricted functions in the target contract will become * callable by anyone. The caller must be the default admin. */ - function setContractOpen(address target) public virtual onlyDefaultAdmin { + function setContractModeOpen(address target) public virtual onlyDefaultAdmin { _contractMode[target] = RestrictedMode.Open; emit RestrictedModeUpdated(target, RestrictedMode.Open); } @@ -224,7 +235,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Sets the target contract to be in "closed" mode. All restricted functions in the target contract will be * closed down and disallowed to all. The caller must be the default admin. */ - function setContractClosed(address target) public virtual onlyDefaultAdmin { + function setContractModeClosed(address target) public virtual onlyDefaultAdmin { _contractMode[target] = RestrictedMode.Closed; emit RestrictedModeUpdated(target, RestrictedMode.Closed); } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 5c11916ea65..b68f799faf6 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -3,6 +3,7 @@ const { expectRevert, time: { duration }, } = require('@openzeppelin/test-helpers'); +const { RestrictedMode } = require('../../helpers/enums'); const AccessManager = artifacts.require('AccessManager'); const AccessManaged = artifacts.require('$AccessManagedMock'); @@ -17,6 +18,8 @@ const groupUtils = { role: group => web3.utils.asciiToHex('group:').padEnd(64, '0') + group.toString(16).padStart(2, '0'), }; +const PUBLIC_GROUP = '255'; + contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthority]) { beforeEach('deploy', async function () { this.delay = duration.days(1); @@ -35,18 +38,16 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori const otherName = 'council'; describe('public group', function () { - const publicGroup = '255'; - it('is created automatically', async function () { await expectEvent.inConstruction(this.manager, 'GroupUpdated', { - group: publicGroup, + group: PUBLIC_GROUP, name: 'public', }); }); it('includes all users automatically', async function () { const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); - expect(groups).to.include(publicGroup); + expect(groups).to.include(PUBLIC_GROUP); }); }); @@ -176,7 +177,66 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori }); describe('modes', function () { - // TODO + const group = '1'; + const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); + + beforeEach('deploying managed contract', async function () { + this.managed = await AccessManaged.new(this.manager.address); + await this.manager.createGroup('1', 'a group', { from: admin }); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, true, { from: admin }); + }); + + it('custom mode is default', async function () { + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Custom); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); + }); + + it('open mode', async function () { + const receipt = await this.manager.setContractModeOpen(this.managed.address, { from: admin }); + expectEvent(receipt, 'RestrictedModeUpdated', { + target: this.managed.address, + mode: RestrictedMode.Open, + }); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Open); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([PUBLIC_GROUP]); + }); + + it('closed mode', async function () { + const receipt = await this.manager.setContractModeClosed(this.managed.address, { from: admin }); + expectEvent(receipt, 'RestrictedModeUpdated', { + target: this.managed.address, + mode: RestrictedMode.Closed, + }); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Closed); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); + }); + + it('mode cycle', async function () { + await this.manager.setContractModeOpen(this.managed.address, { from: admin }); + await this.manager.setContractModeClosed(this.managed.address, { from: admin }); + await this.manager.setContractModeCustom(this.managed.address, { from: admin }); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Custom); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); + }); + + it('non-admin cannot change mode', async function () { + await expectRevert( + this.manager.setContractModeCustom(this.managed.address), + 'missing role', + ); + await expectRevert( + this.manager.setContractModeOpen(this.managed.address), + 'missing role', + ); + await expectRevert( + this.manager.setContractModeClosed(this.managed.address), + 'missing role', + ); + }); }); describe('transfering authority', function () { diff --git a/test/helpers/enums.js b/test/helpers/enums.js index ced6c3858dc..cacc3192725 100644 --- a/test/helpers/enums.js +++ b/test/helpers/enums.js @@ -9,4 +9,5 @@ module.exports = { ProposalState: Enum('Pending', 'Active', 'Canceled', 'Defeated', 'Succeeded', 'Queued', 'Expired', 'Executed'), VoteType: Enum('Against', 'For', 'Abstain'), Rounding: Enum('Down', 'Up', 'Zero'), + RestrictedMode: Enum('Custom', 'Closed', 'Open'), }; From 5f22f2d923085f4aa4fced98537d5af09ad98b55 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 17:29:13 -0300 Subject: [PATCH 45/78] remove use of mapping labels --- contracts/access/manager/AccessManager.sol | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 019ba561c26..56cff66bb5d 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -82,9 +82,14 @@ interface IAccessManager is IAuthority { contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdGroups; - mapping(address user => bytes32 groups) private _userGroups; - mapping(address target => mapping(bytes4 selector => bytes32 groups)) private _allowedGroups; - mapping(address target => RestrictedMode mode) private _contractMode; + // user -> groups + mapping(address => bytes32) private _userGroups; + + // target -> selector -> groups + mapping(address => mapping(bytes4 => bytes32)) private _allowedGroups; + + // target -> mode + mapping(address => RestrictedMode) private _contractMode; uint8 private constant _GROUP_PUBLIC = 255; From 058019805d21d8823f510a21eac246a68998db17 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 21:53:42 -0300 Subject: [PATCH 46/78] tweak comments --- contracts/access/manager/AccessManager.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 56cff66bb5d..c7b375aec1f 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -154,8 +154,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Emits {RoleGranted} with the role id of the group, if wasn't already held by the user. */ function grantGroup(address user, uint8 group) public virtual { - // grantRole checks that msg.sender is admin for the role - grantRole(_encodeGroupRole(group), user); + grantRole(_encodeGroupRole(group), user); // will check msg.sender } /** @@ -164,8 +163,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ function revokeGroup(address user, uint8 group) public virtual { - // revokeRole checks that msg.sender is admin for the role - revokeRole(_encodeGroupRole(group), user); + revokeRole(_encodeGroupRole(group), user); // will check msg.sender } /** @@ -174,8 +172,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ function renounceGroup(address user, uint8 group) public virtual { - // renounceRole checks that msg.sender is user - renounceRole(_encodeGroupRole(group), user); + renounceRole(_encodeGroupRole(group), user); // will check msg.sender } /** From e01e362d9080b89984148bdd7760f64044082273 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 21:53:51 -0300 Subject: [PATCH 47/78] add tests for adapter --- .../access/manager/AccessManagerAdapter.sol | 17 +++--- test/access/manager/AccessManager.test.js | 52 +++++++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/contracts/access/manager/AccessManagerAdapter.sol b/contracts/access/manager/AccessManagerAdapter.sol index 02ae81c7e0c..f1fecebd797 100644 --- a/contracts/access/manager/AccessManagerAdapter.sol +++ b/contracts/access/manager/AccessManagerAdapter.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; import "./AccessManager.sol"; +import "./AccessManaged.sol"; import "../../utils/Address.sol"; /** @@ -17,19 +18,13 @@ import "../../utils/Address.sol"; * Permissioned interactions with thus migrated contracts must go through the adapter's {relay} function and will * proceed if the function is allowed for the caller in the AccessManager instance. */ -contract AccessManagerAdapter { - using Address for address; - - AccessManager private _manager; - +contract AccessManagerAdapter is AccessManaged { bytes32 private constant _DEFAULT_ADMIN_ROLE = 0; /** * @dev Initializes an adapter connected to an AccessManager instance. */ - constructor(AccessManager manager) { - _manager = manager; - } + constructor(AccessManager manager) AccessManaged(manager) {} /** * @dev Relays a function call to the target contract. The call will be relayed if the AccessManager allows the @@ -39,7 +34,11 @@ contract AccessManagerAdapter { */ function relay(address target, bytes memory data) external payable { bytes4 sig = bytes4(data); - require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); + AccessManager manager = AccessManager(address(authority())); + require( + manager.canCall(msg.sender, target, sig) || manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender), + "AccessManagerAdapter: caller not allowed" + ); (bool ok, bytes memory result) = target.call{value: msg.value}(data); assembly { let result_pointer := add(32, result) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index b68f799faf6..3db98bb21d0 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -6,8 +6,12 @@ const { const { RestrictedMode } = require('../../helpers/enums'); const AccessManager = artifacts.require('AccessManager'); +const AccessManagerAdapter = artifacts.require('AccessManagerAdapter'); const AccessManaged = artifacts.require('$AccessManagedMock'); +const Ownable = artifacts.require('$Ownable'); +const AccessControl = artifacts.require('$AccessControl'); + const groupUtils = { mask: group => 1n << BigInt(group), decodeBitmap: hexBitmap => { @@ -256,4 +260,52 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori ); }); }); + + describe('adapter', function () { + const group = '0'; + + beforeEach('deploying adapter', async function () { + await this.manager.createGroup(group, 'a group', { from: admin }); + await this.manager.grantGroup(user1, group, { from: admin }); + this.adapter = await AccessManagerAdapter.new(this.manager.address); + }); + + it('with ownable', async function () { + const target = await Ownable.new(); + await target.transferOwnership(this.adapter.address); + + const { data } = await target.$_checkOwner.request(); + const selector = data.slice(0, 10); + + await expectRevert( + this.adapter.relay(target.address, data, { from: user1 }), + 'AccessManagerAdapter: caller not allowed', + ); + + await this.manager.setFunctionAllowedGroup(target.address, [selector], group, true, { from: admin }); + await this.adapter.relay(target.address, data, { from: user1 }); + }); + + it('with access control', async function () { + const ROLE = web3.utils.soliditySha3('ROLE'); + const target = await AccessControl.new(); + await target.$_grantRole(ROLE, this.adapter.address); + + const { data } = await target.$_checkRole.request(ROLE); + const selector = data.slice(0, 10); + + await expectRevert( + this.adapter.relay(target.address, data, { from: user1 }), + 'AccessManagerAdapter: caller not allowed', + ); + + await this.manager.setFunctionAllowedGroup(target.address, [selector], group, true, { from: admin }); + await this.adapter.relay(target.address, data, { from: user1 }); + }); + + it('transfer authority', async function () { + await this.manager.transferContractAuthority(this.adapter.address, otherAuthority, { from: admin }); + expect(await this.adapter.authority()).to.equal(otherAuthority); + }); + }); }); From cb026bca75a1d24c1ceda928e2406e5437d54db4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 21:56:34 -0300 Subject: [PATCH 48/78] lint --- test/access/manager/AccessManager.test.js | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 3db98bb21d0..05210e3e66c 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -228,18 +228,9 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori }); it('non-admin cannot change mode', async function () { - await expectRevert( - this.manager.setContractModeCustom(this.managed.address), - 'missing role', - ); - await expectRevert( - this.manager.setContractModeOpen(this.managed.address), - 'missing role', - ); - await expectRevert( - this.manager.setContractModeClosed(this.managed.address), - 'missing role', - ); + await expectRevert(this.manager.setContractModeCustom(this.managed.address), 'missing role'); + await expectRevert(this.manager.setContractModeOpen(this.managed.address), 'missing role'); + await expectRevert(this.manager.setContractModeClosed(this.managed.address), 'missing role'); }); }); From 76d35dad5659912f72b5790d3a1fd42771941ebe Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 22:14:33 -0300 Subject: [PATCH 49/78] add revert reasons and group tests --- contracts/access/manager/AccessManager.sol | 11 +++--- test/access/manager/AccessManager.test.js | 41 ++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index c7b375aec1f..ad1c205cef4 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -130,7 +130,8 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Updates an existing group's name. The caller must be the default admin. */ function updateGroupName(uint8 group, string memory name) public virtual onlyDefaultAdmin { - require(group != _GROUP_PUBLIC && hasGroup(group)); + require(group != _GROUP_PUBLIC, "AccessManager: built-in group"); + require(hasGroup(group), "AccessManager: unknown group"); emit GroupUpdated(group, name); } @@ -255,7 +256,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * Emits {GroupUpdated}. */ function _createGroup(uint8 group, string memory name) internal virtual { - require(!hasGroup(group)); + require(!hasGroup(group), "AccessManager: existing group"); _createdGroups = _withUpdatedGroup(_createdGroups, group, true); emit GroupUpdated(group, name); } @@ -267,7 +268,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { super._grantRole(role, user); (bool isGroup, uint8 group) = _decodeGroupRole(role); if (isGroup) { - require(hasGroup(group)); + require(hasGroup(group), "AccessManager: unknown group"); _userGroups[user] = _withUpdatedGroup(_userGroups[user], group, true); } } @@ -279,8 +280,8 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { super._revokeRole(role, user); (bool isGroup, uint8 group) = _decodeGroupRole(role); if (isGroup) { - require(hasGroup(group)); - require(group != _GROUP_PUBLIC); + require(hasGroup(group), "AccessManager: unknown group"); + require(group != _GROUP_PUBLIC, "AccessManager: irrevocable group"); _userGroups[user] = _withUpdatedGroup(_userGroups[user], group, false); } } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 05210e3e66c..d82d7798f98 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -66,6 +66,11 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori it('non-admin cannot create groups', async function () { await expectRevert(this.manager.createGroup(group, name, { from: nonAdmin }), 'missing role'); }); + + it('cannot recreate a group', async function () { + await this.manager.createGroup(group, name, { from: admin }); + await expectRevert(this.manager.createGroup(group, name, { from: admin }), 'AccessManager: existing group'); + }); }); describe('updating', function () { @@ -81,6 +86,20 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori it('non-admin cannot update group', async function () { await expectRevert(this.manager.updateGroupName(group, name, { from: nonAdmin }), 'missing role'); }); + + it('cannot update built in group', async function () { + await expectRevert( + this.manager.updateGroupName(PUBLIC_GROUP, name, { from: admin }), + 'AccessManager: built-in group', + ); + }); + + it('cannot update nonexistent group', async function () { + await expectRevert( + this.manager.updateGroupName(otherGroup, name, { from: admin }), + 'AccessManager: unknown group', + ); + }); }); describe('granting', function () { @@ -98,6 +117,10 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori it('non-admin cannot grant group', async function () { await expectRevert(this.manager.grantGroup(user1, group, { from: nonAdmin }), 'missing role'); }); + + it('cannot grant nonexistent group', async function () { + await expectRevert(this.manager.grantGroup(user1, otherGroup, { from: admin }), 'AccessManager: unknown group'); + }); }); describe('revoking & renouncing', function () { @@ -132,6 +155,24 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori 'can only renounce roles for self', ); }); + + it('cannot revoke public group', async function () { + await expectRevert( + this.manager.revokeGroup(user1, PUBLIC_GROUP, { from: admin }), + 'AccessManager: irrevocable group', + ); + }); + + it('cannot revoke nonexistent group', async function () { + await expectRevert( + this.manager.revokeGroup(user1, otherGroup, { from: admin }), + 'AccessManager: unknown group', + ); + await expectRevert( + this.manager.renounceGroup(user1, otherGroup, { from: user1 }), + 'AccessManager: unknown group', + ); + }); }); describe('querying', function () { From 4069781b0f095b79c26daf604cb899928cbfceac Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 22:34:24 -0300 Subject: [PATCH 50/78] add tests for allowing and disallowing roles --- contracts/access/manager/AccessManager.sol | 2 +- contracts/mocks/AccessManagerMocks.sol | 4 + test/access/manager/AccessManager.test.js | 152 ++++++++++++++++++++- 3 files changed, 156 insertions(+), 2 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index ad1c205cef4..c3664f300c8 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -201,7 +201,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint8 group, bool allowed ) public virtual { - require(_contractMode[target] == RestrictedMode.Custom); + require(_contractMode[target] == RestrictedMode.Custom, "AccessManager: target in special mode"); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; _allowedGroups[target][selector] = _withUpdatedGroup(_allowedGroups[target][selector], group, allowed); diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol index 8b4767c2885..1d437506d73 100644 --- a/contracts/mocks/AccessManagerMocks.sol +++ b/contracts/mocks/AccessManagerMocks.sol @@ -27,4 +27,8 @@ abstract contract AccessManagedMock is AccessManaged { function restrictedFunction() public restricted { emit RestrictedRan(); } + + function otherRestrictedFunction() public restricted { + emit RestrictedRan(); + } } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index d82d7798f98..0768847ef7f 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -201,6 +201,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori const group = '1'; const groupMember = user1; const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); + const otherSelector = web3.eth.abi.encodeFunctionSignature('otherRestrictedFunction()'); beforeEach('deploying managed contract', async function () { await this.manager.createGroup(group, '', { from: admin }); @@ -208,7 +209,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori this.managed = await AccessManaged.new(this.manager.address); }); - it('allow', async function () { + it('single selector', async function () { await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector], @@ -216,9 +217,158 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori true, { from: admin }, ); + + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); + + const otherAllowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, otherSelector); + expect(groupUtils.decodeBitmap(otherAllowedGroups)).to.deep.equal([]); + + const restricted = await this.managed.restrictedFunction({ from: groupMember }); + expectEvent(restricted, 'RestrictedRan'); + + await expectRevert(this.managed.otherRestrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); + }); + + it('multiple selectors', async function () { + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector, otherSelector], + group, + true, + { from: admin }, + ); + + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); + + const otherAllowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, otherSelector); + expect(groupUtils.decodeBitmap(otherAllowedGroups)).to.deep.equal([group]); + const restricted = await this.managed.restrictedFunction({ from: groupMember }); expectEvent(restricted, 'RestrictedRan'); + + const otherRestricted = await this.managed.otherRestrictedFunction({ from: groupMember }); + expectEvent(restricted, 'RestrictedRan'); + }); + + it('reject open target', async function () { + await this.manager.setContractModeOpen(this.managed.address, { from: admin }); + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: admin }, + ), + 'AccessManager: target in special mode', + ); + }); + + it('reject closed target', async function () { + await this.manager.setContractModeClosed(this.managed.address, { from: admin }); + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: admin }, + ), + 'AccessManager: target in special mode', + ); + }); + + }); + + describe('disallowing', function () { + const group = '1'; + const groupMember = user1; + const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); + const otherSelector = web3.eth.abi.encodeFunctionSignature('otherRestrictedFunction()'); + + beforeEach('deploying managed contract', async function () { + await this.manager.createGroup(group, '', { from: admin }); + await this.manager.grantGroup(groupMember, group, { from: admin }); + this.managed = await AccessManaged.new(this.manager.address); + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector, otherSelector], + group, + true, + { from: admin }, + ); + }); + + it('single selector', async function () { + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: admin }, + ); + + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); + + const otherAllowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, otherSelector); + expect(groupUtils.decodeBitmap(otherAllowedGroups)).to.deep.equal([group]); + + await expectRevert(this.managed.restrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); + + const otherRestricted = await this.managed.otherRestrictedFunction({ from: groupMember }); + expectEvent(otherRestricted, 'RestrictedRan'); + }); + + it('multiple selectors', async function () { + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector, otherSelector], + group, + false, + { from: admin }, + ); + + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); + expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); + + const otherAllowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, otherSelector); + expect(groupUtils.decodeBitmap(otherAllowedGroups)).to.deep.equal([]); + + await expectRevert(this.managed.restrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); + await expectRevert(this.managed.otherRestrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); }); + + it('reject open target', async function () { + await this.manager.setContractModeOpen(this.managed.address, { from: admin }); + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: admin }, + ), + 'AccessManager: target in special mode', + ); + }); + + it('reject closed target', async function () { + await this.manager.setContractModeClosed(this.managed.address, { from: admin }); + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: admin }, + ), + 'AccessManager: target in special mode', + ); + }); + }); describe('modes', function () { From 80ffd2a8d2d676b480a4fa808019e4b3022b7f5e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 22:35:19 -0300 Subject: [PATCH 51/78] lint --- test/access/manager/AccessManager.test.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 0768847ef7f..c6e2581d6d8 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -227,7 +227,10 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori const restricted = await this.managed.restrictedFunction({ from: groupMember }); expectEvent(restricted, 'RestrictedRan'); - await expectRevert(this.managed.otherRestrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); + await expectRevert( + this.managed.otherRestrictedFunction({ from: groupMember }), + 'AccessManaged: authority rejected', + ); }); it('multiple selectors', async function () { @@ -248,7 +251,7 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori const restricted = await this.managed.restrictedFunction({ from: groupMember }); expectEvent(restricted, 'RestrictedRan'); - const otherRestricted = await this.managed.otherRestrictedFunction({ from: groupMember }); + await this.managed.otherRestrictedFunction({ from: groupMember }); expectEvent(restricted, 'RestrictedRan'); }); @@ -279,7 +282,6 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori 'AccessManager: target in special mode', ); }); - }); describe('disallowing', function () { @@ -338,7 +340,10 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori expect(groupUtils.decodeBitmap(otherAllowedGroups)).to.deep.equal([]); await expectRevert(this.managed.restrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); - await expectRevert(this.managed.otherRestrictedFunction({ from: groupMember }), 'AccessManaged: authority rejected'); + await expectRevert( + this.managed.otherRestrictedFunction({ from: groupMember }), + 'AccessManaged: authority rejected', + ); }); it('reject open target', async function () { @@ -368,7 +373,6 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori 'AccessManager: target in special mode', ); }); - }); describe('modes', function () { From 3094d0c31405669cf1f78f91b7d162ad47671564 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 23:15:36 -0300 Subject: [PATCH 52/78] add docs for AccessManaged --- contracts/access/manager/AccessManaged.sol | 37 +++++++++++++++++++++- contracts/mocks/AccessManagerMocks.sol | 4 +-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index a99da33e74c..8403977e125 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -8,33 +8,68 @@ import "./IAuthority.sol"; * @dev This contract module makes available a {restricted} modifier. Functions decorated with this modifier will be * permissioned according to an "authority": a contract like {AccessManager} that follows the {IAuthority} interface, * implementing a policy that allows certain callers access to certain functions. + * + * IMPORTANT: The `restricted` modifier should never be used on `internal` functions, judiciously used in `public` + * functions, and ideally only used in `external` functions. See {restricted}. */ contract AccessManaged { event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); IAuthority private _authority; + /** + * @dev Restricts access to a function as defined by the connected Authority for this contract and the + * caller and selector of the function that entered the contract. + * + * [IMPORTANT] + * ==== + * In general, this modifier should only be used on `external` functions. It is okay to use it on `public` functions + * that are used as external entry points and are not called internally. Unless you know what you're oding, it + * should never be used on `internal` functions. Failure to follow these rules can have critical security + * implications! This is because the permissions are determined by the function that entered the contract, i.e. the + * function at the bottom of the call stack, and not the function where the modifier is visible in the source code. + * ==== + */ modifier restricted() { - require(_authority.canCall(msg.sender, address(this), msg.sig), "AccessManaged: authority rejected"); + _checkCanCall(msg.sender, msg.sig); _; } + /** + * @dev Initializes the contract connected to an initial authority. + */ constructor(IAuthority initialAuthority) { _setAuthority(initialAuthority); } + /** + * @dev Returns the current authority. + */ function authority() public view virtual returns (IAuthority) { return _authority; } + /** + * @dev Transfers control to a new authority. The caller must be the current authority. + */ function setAuthority(IAuthority newAuthority) public virtual { require(msg.sender == address(_authority), "AccessManaged: not current authority"); _setAuthority(newAuthority); } + /** + * @dev Transfers control to a new authority. Internal function with no access restriction. + */ function _setAuthority(IAuthority newAuthority) internal virtual { IAuthority oldAuthority = _authority; _authority = newAuthority; emit AuthorityUpdated(oldAuthority, newAuthority); } + + /** + * @dev Reverts if the caller is not allowed to call the function identified by a selector. + */ + function _checkCanCall(address caller, bytes4 selector) internal view virtual { + require(_authority.canCall(caller, address(this), selector), "AccessManaged: authority rejected"); + } } diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol index 1d437506d73..beb4788021f 100644 --- a/contracts/mocks/AccessManagerMocks.sol +++ b/contracts/mocks/AccessManagerMocks.sol @@ -24,11 +24,11 @@ contract SimpleAuthority is IAuthority { abstract contract AccessManagedMock is AccessManaged { event RestrictedRan(); - function restrictedFunction() public restricted { + function restrictedFunction() external restricted { emit RestrictedRan(); } - function otherRestrictedFunction() public restricted { + function otherRestrictedFunction() external restricted { emit RestrictedRan(); } } From b3a8b1ba0d1f24401e4fe4c1a4846a5a1b143b5e Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Mar 2023 23:19:10 -0300 Subject: [PATCH 53/78] Update contracts/access/manager/AccessManager.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index c3664f300c8..8761bbdc95d 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -200,7 +200,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes4[] calldata selectors, uint8 group, bool allowed - ) public virtual { + ) public virtual onlyDefaultAdmin { require(_contractMode[target] == RestrictedMode.Custom, "AccessManager: target in special mode"); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; From 8a683225afad61a19ac950db9f009cbcdcb57123 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Mar 2023 23:21:01 -0300 Subject: [PATCH 54/78] Update contracts/access/manager/AccessManager.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 8761bbdc95d..507d82abc15 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -77,7 +77,7 @@ interface IAccessManager is IAuthority { * NOTE: Some of the functions in this contract, such as {getUserGroups}, return a `bytes32` bitmap to succintly * represent a set of groups. In a bitmap, bit `n` (counting from the least significant bit) will be 1 if and only if * the group with number `n` is in the set. For example, the hex value `0x05` represents the set of the two groups - * numbered 0 and 2. + * numbered 0 and 2 from its binary equivalence `0b101` */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes32 _createdGroups; From eeab8cf36d7784faaf2a5d577de4a7c1fceaf1d3 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Mar 2023 23:21:56 -0300 Subject: [PATCH 55/78] Update contracts/access/manager/AccessManager.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 507d82abc15..fd48ac17d08 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -91,7 +91,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { // target -> mode mapping(address => RestrictedMode) private _contractMode; - uint8 private constant _GROUP_PUBLIC = 255; + uint8 private constant _GROUP_PUBLIC = type(uint8).max; /** * @dev Initializes an AccessManager with initial default admin and transfer delay. From ca61e38bfee9146e9da6520d8a18842385b055cf Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 23:22:04 -0300 Subject: [PATCH 56/78] add tests for onlyDefaultAdmin --- test/access/manager/AccessManager.test.js | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index c6e2581d6d8..898ae3708de 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -209,6 +209,19 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori this.managed = await AccessManaged.new(this.manager.address); }); + it('non-admin cannot change allowed groups', async function () { + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: nonAdmin }, + ), + 'missing role', + ); + }); + it('single selector', async function () { await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, @@ -303,6 +316,19 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori ); }); + it('non-admin cannot change disallowed groups', async function () { + await expectRevert( + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: nonAdmin }, + ), + 'missing role', + ); + }); + it('single selector', async function () { await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, From 8824c319ed46a5670df6bf2fd5a800a30110c346 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 23:27:01 -0300 Subject: [PATCH 57/78] lint --- test/access/manager/AccessManager.test.js | 28 +++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 898ae3708de..1ad05049ed8 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -211,13 +211,13 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori it('non-admin cannot change allowed groups', async function () { await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - true, - { from: nonAdmin }, - ), + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + true, + { from: nonAdmin }, + ), 'missing role', ); }); @@ -318,13 +318,13 @@ contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthori it('non-admin cannot change disallowed groups', async function () { await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - false, - { from: nonAdmin }, - ), + this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: nonAdmin }, + ), 'missing role', ); }); From 67e33b632b731efb4388458aeb007536c844d613 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Mar 2023 23:28:40 -0300 Subject: [PATCH 58/78] add internal _setContractMode --- contracts/access/manager/AccessManager.sol | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index fd48ac17d08..b45188789ec 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -221,8 +221,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * will follow the group-based restrictions defined by the AccessManager. The caller must be the default admin. */ function setContractModeCustom(address target) public virtual onlyDefaultAdmin { - _contractMode[target] = RestrictedMode.Custom; - emit RestrictedModeUpdated(target, RestrictedMode.Custom); + _setContractMode(target, RestrictedMode.Custom); } /** @@ -230,8 +229,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * callable by anyone. The caller must be the default admin. */ function setContractModeOpen(address target) public virtual onlyDefaultAdmin { - _contractMode[target] = RestrictedMode.Open; - emit RestrictedModeUpdated(target, RestrictedMode.Open); + _setContractMode(target, RestrictedMode.Open); } /** @@ -239,8 +237,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * closed down and disallowed to all. The caller must be the default admin. */ function setContractModeClosed(address target) public virtual onlyDefaultAdmin { - _contractMode[target] = RestrictedMode.Closed; - emit RestrictedModeUpdated(target, RestrictedMode.Closed); + _setContractMode(target, RestrictedMode.Closed); } /** @@ -286,6 +283,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { } } + /** + * @dev Sets the restricted mode of a target contract. + */ + function _setContractMode(address target, RestrictedMode mode) internal virtual { + _contractMode[target] = mode; + emit RestrictedModeUpdated(target, mode); + } + /** * @dev Returns the {AccessControl} role id that corresponds to a group. * From 5f270c76c78558e70a2fcfdbb2ba6b157446212f Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Mar 2023 23:28:55 -0300 Subject: [PATCH 59/78] Update .changeset/quiet-trainers-kick.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- .changeset/quiet-trainers-kick.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/quiet-trainers-kick.md b/.changeset/quiet-trainers-kick.md index bb9bb57a6f5..5de96467de8 100644 --- a/.changeset/quiet-trainers-kick.md +++ b/.changeset/quiet-trainers-kick.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`AccessManager`: Added a new contract for managing access control of complex systems in a central location. +`AccessManager`: Added a new contract for managing access control of complex systems in a consolidated location. From 6f7ac962af1482bade818e87529db14a2c9a818a Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Mar 2023 23:29:32 -0300 Subject: [PATCH 60/78] Update contracts/access/manager/AccessManager.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index b45188789ec..31e5730217b 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -304,7 +304,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Decodes a role id into a group, if it is a role id of the kind returned by {_encodeGroupRole}. */ - function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool, uint8) { + function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool isGroup, uint8 group) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; uint8 group = uint8(role[31]); From 7ec5311402256fa060802b8afaf9fa8923ef37d1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 22 Mar 2023 11:03:52 -0300 Subject: [PATCH 61/78] Update contracts/access/manager/AccessManager.sol Co-authored-by: Hadrien Croubois --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 31e5730217b..b45188789ec 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -304,7 +304,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Decodes a role id into a group, if it is a role id of the kind returned by {_encodeGroupRole}. */ - function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool isGroup, uint8 group) { + function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool, uint8) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; uint8 group = uint8(role[31]); From 82402d242e6c167e754a6bcbaff45fa107857bba Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 22 Mar 2023 13:24:17 -0300 Subject: [PATCH 62/78] typo --- contracts/access/manager/AccessManaged.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 8403977e125..f7a1a452f14 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -24,7 +24,7 @@ contract AccessManaged { * [IMPORTANT] * ==== * In general, this modifier should only be used on `external` functions. It is okay to use it on `public` functions - * that are used as external entry points and are not called internally. Unless you know what you're oding, it + * that are used as external entry points and are not called internally. Unless you know what you're doing, it * should never be used on `internal` functions. Failure to follow these rules can have critical security * implications! This is because the permissions are determined by the function that entered the contract, i.e. the * function at the bottom of the call stack, and not the function where the modifier is visible in the source code. From 27807e690f0a3038dffec6c019841f4ce92f1ed0 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 22 Mar 2023 13:47:04 -0300 Subject: [PATCH 63/78] Apply suggestions from code review Co-authored-by: Hadrien Croubois --- test/access/manager/AccessManaged.test.js | 3 ++- test/access/manager/AccessManager.test.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index 520b10c49b4..b99aa03b525 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -7,7 +7,8 @@ const { const AccessManaged = artifacts.require('$AccessManagedMock'); const SimpleAuthority = artifacts.require('SimpleAuthority'); -contract('AccessManaged', function ([authority, other, user]) { +contract('AccessManaged', function (accounts) { + const [authority, other, user] = accounts; it('construction', async function () { const managed = await AccessManaged.new(authority); expectEvent.inConstruction(managed, 'AuthorityUpdated', { diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 1ad05049ed8..73d3a016d82 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -24,7 +24,8 @@ const groupUtils = { const PUBLIC_GROUP = '255'; -contract('AccessManager', function ([admin, nonAdmin, user1, user2, otherAuthority]) { +contract('AccessManager', function (accounts) { + const [admin, nonAdmin, user1, user2, otherAuthority] = accounts; beforeEach('deploy', async function () { this.delay = duration.days(1); this.manager = await AccessManager.new(this.delay, admin); From 80bc88b9dfc179cfec1bfdee74ffe0290103711e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 13:59:31 -0300 Subject: [PATCH 64/78] roll back to 0.8.13 --- contracts/access/manager/AccessManager.sol | 2 +- contracts/mocks/AccessManagerMocks.sol | 2 +- hardhat.config.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index b45188789ec..6fbba7f760c 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.18; +pragma solidity ^0.8.13; import "../AccessControl.sol"; import "../AccessControlDefaultAdminRules.sol"; diff --git a/contracts/mocks/AccessManagerMocks.sol b/contracts/mocks/AccessManagerMocks.sol index beb4788021f..caf81366e63 100644 --- a/contracts/mocks/AccessManagerMocks.sol +++ b/contracts/mocks/AccessManagerMocks.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.18; +pragma solidity ^0.8.13; import "../access/manager/IAuthority.sol"; import "../access/manager/AccessManaged.sol"; diff --git a/hardhat.config.js b/hardhat.config.js index 0b16d04a290..8edddd2f6e7 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -40,7 +40,7 @@ const argv = require('yargs/yargs')() compiler: { alias: 'compileVersion', type: 'string', - default: '0.8.18', + default: '0.8.13', }, coinmarketcap: { alias: 'coinmarketcapApiKey', From cf4df2288b4374192b6c987d2e75f2552c1073c4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:11:46 -0300 Subject: [PATCH 65/78] add test for setFunctionAllowedGroup events --- test/access/manager/AccessManager.test.js | 50 +++++++++++++++++++++-- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 73d3a016d82..014a89c807b 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -224,7 +224,7 @@ contract('AccessManager', function (accounts) { }); it('single selector', async function () { - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector], group, @@ -232,6 +232,13 @@ contract('AccessManager', function (accounts) { { from: admin }, ); + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: selector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4 + group, + allowed: true, + }); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); @@ -248,7 +255,7 @@ contract('AccessManager', function (accounts) { }); it('multiple selectors', async function () { - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector, otherSelector], group, @@ -256,6 +263,20 @@ contract('AccessManager', function (accounts) { { from: admin }, ); + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: selector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4 + group, + allowed: true, + }); + + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: otherSelector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4 + group, + allowed: true, + }); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); @@ -331,7 +352,7 @@ contract('AccessManager', function (accounts) { }); it('single selector', async function () { - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector], group, @@ -339,6 +360,13 @@ contract('AccessManager', function (accounts) { { from: admin }, ); + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: selector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4, + group, + allowed: false, + }); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); @@ -352,7 +380,7 @@ contract('AccessManager', function (accounts) { }); it('multiple selectors', async function () { - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( this.managed.address, [selector, otherSelector], group, @@ -360,6 +388,20 @@ contract('AccessManager', function (accounts) { { from: admin }, ); + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: selector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4 + group, + allowed: false, + }); + + expectEvent(receipt, 'GroupAllowed', { + target: this.managed.address, + selector: otherSelector.padEnd(66, '0'), // there seems to be a bug in decoding the indexed bytes4 + group, + allowed: false, + }); + const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); From bab4d3417f0abaf49cbac7948a380163920a339e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:19:01 -0300 Subject: [PATCH 66/78] remove mode restriction on setFunctionAllowedGroup --- contracts/access/manager/AccessManager.sol | 1 - test/access/manager/AccessManager.test.js | 68 +++++++++------------- 2 files changed, 28 insertions(+), 41 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 6fbba7f760c..640bccc46b9 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -201,7 +201,6 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint8 group, bool allowed ) public virtual onlyDefaultAdmin { - require(_contractMode[target] == RestrictedMode.Custom, "AccessManager: target in special mode"); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; _allowedGroups[target][selector] = _withUpdatedGroup(_allowedGroups[target][selector], group, allowed); diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 014a89c807b..41ee1eb95ae 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -290,31 +290,25 @@ contract('AccessManager', function (accounts) { expectEvent(restricted, 'RestrictedRan'); }); - it('reject open target', async function () { + it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - true, - { from: admin }, - ), - 'AccessManager: target in special mode', + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: admin }, ); }); - it('reject closed target', async function () { + it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - true, - { from: admin }, - ), - 'AccessManager: target in special mode', + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: admin }, ); }); }); @@ -415,31 +409,25 @@ contract('AccessManager', function (accounts) { ); }); - it('reject open target', async function () { + it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - true, - { from: admin }, - ), - 'AccessManager: target in special mode', + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: admin }, ); }); - it('reject closed target', async function () { + it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( - this.managed.address, - [selector], - group, - true, - { from: admin }, - ), - 'AccessManager: target in special mode', + await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.managed.address, + [selector], + group, + false, + { from: admin }, ); }); }); From 642e279faae0c8d6728655bef11e6e63a48e43fb Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:21:04 -0300 Subject: [PATCH 67/78] simplify use of setFunctionAllowedGroup --- test/access/manager/AccessManager.test.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 41ee1eb95ae..8050e7032a3 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -212,7 +212,7 @@ contract('AccessManager', function (accounts) { it('non-admin cannot change allowed groups', async function () { await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -224,7 +224,7 @@ contract('AccessManager', function (accounts) { }); it('single selector', async function () { - const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -255,7 +255,7 @@ contract('AccessManager', function (accounts) { }); it('multiple selectors', async function () { - const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.setFunctionAllowedGroup( this.managed.address, [selector, otherSelector], group, @@ -292,7 +292,7 @@ contract('AccessManager', function (accounts) { it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -303,7 +303,7 @@ contract('AccessManager', function (accounts) { it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -323,7 +323,7 @@ contract('AccessManager', function (accounts) { await this.manager.createGroup(group, '', { from: admin }); await this.manager.grantGroup(groupMember, group, { from: admin }); this.managed = await AccessManaged.new(this.manager.address); - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + await this.manager.setFunctionAllowedGroup( this.managed.address, [selector, otherSelector], group, @@ -334,7 +334,7 @@ contract('AccessManager', function (accounts) { it('non-admin cannot change disallowed groups', async function () { await expectRevert( - this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -346,7 +346,7 @@ contract('AccessManager', function (accounts) { }); it('single selector', async function () { - const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -374,7 +374,7 @@ contract('AccessManager', function (accounts) { }); it('multiple selectors', async function () { - const receipt = await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + const receipt = await this.manager.setFunctionAllowedGroup( this.managed.address, [selector, otherSelector], group, @@ -411,7 +411,7 @@ contract('AccessManager', function (accounts) { it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, @@ -422,7 +422,7 @@ contract('AccessManager', function (accounts) { it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await this.manager.methods['setFunctionAllowedGroup(address,bytes4[],uint8,bool)']( + await this.manager.setFunctionAllowedGroup( this.managed.address, [selector], group, From 51aff23c3c267274c3a51104a6923cec24a9f3a1 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:21:48 -0300 Subject: [PATCH 68/78] lint --- test/access/manager/AccessManaged.test.js | 2 +- test/access/manager/AccessManager.test.js | 80 +++++------------------ 2 files changed, 17 insertions(+), 65 deletions(-) diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index b99aa03b525..27234f0d566 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -8,7 +8,7 @@ const AccessManaged = artifacts.require('$AccessManagedMock'); const SimpleAuthority = artifacts.require('SimpleAuthority'); contract('AccessManaged', function (accounts) { - const [authority, other, user] = accounts; + const [authority, other, user] = accounts; it('construction', async function () { const managed = await AccessManaged.new(authority); expectEvent.inConstruction(managed, 'AuthorityUpdated', { diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index 8050e7032a3..c124e7c5cde 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -25,7 +25,7 @@ const groupUtils = { const PUBLIC_GROUP = '255'; contract('AccessManager', function (accounts) { - const [admin, nonAdmin, user1, user2, otherAuthority] = accounts; + const [admin, nonAdmin, user1, user2, otherAuthority] = accounts; beforeEach('deploy', async function () { this.delay = duration.days(1); this.manager = await AccessManager.new(this.delay, admin); @@ -212,25 +212,15 @@ contract('AccessManager', function (accounts) { it('non-admin cannot change allowed groups', async function () { await expectRevert( - this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - true, - { from: nonAdmin }, - ), + this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, true, { from: nonAdmin }), 'missing role', ); }); it('single selector', async function () { - const receipt = await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - true, - { from: admin }, - ); + const receipt = await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, true, { + from: admin, + }); expectEvent(receipt, 'GroupAllowed', { target: this.managed.address, @@ -292,24 +282,12 @@ contract('AccessManager', function (accounts) { it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: admin }, - ); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: admin }); }); it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: admin }, - ); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: admin }); }); }); @@ -323,36 +301,22 @@ contract('AccessManager', function (accounts) { await this.manager.createGroup(group, '', { from: admin }); await this.manager.grantGroup(groupMember, group, { from: admin }); this.managed = await AccessManaged.new(this.manager.address); - await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector, otherSelector], - group, - true, - { from: admin }, - ); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector, otherSelector], group, true, { + from: admin, + }); }); it('non-admin cannot change disallowed groups', async function () { await expectRevert( - this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: nonAdmin }, - ), + this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: nonAdmin }), 'missing role', ); }); it('single selector', async function () { - const receipt = await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: admin }, - ); + const receipt = await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { + from: admin, + }); expectEvent(receipt, 'GroupAllowed', { target: this.managed.address, @@ -411,24 +375,12 @@ contract('AccessManager', function (accounts) { it('works on open target', async function () { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: admin }, - ); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: admin }); }); it('works on closed target', async function () { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - await this.manager.setFunctionAllowedGroup( - this.managed.address, - [selector], - group, - false, - { from: admin }, - ); + await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: admin }); }); }); From 6e3da6621d7f8e5ff893b73b90b41ba2a049d42b Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:22:51 -0300 Subject: [PATCH 69/78] remove unused import --- contracts/access/manager/AccessManagerAdapter.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/access/manager/AccessManagerAdapter.sol b/contracts/access/manager/AccessManagerAdapter.sol index f1fecebd797..afa92264a89 100644 --- a/contracts/access/manager/AccessManagerAdapter.sol +++ b/contracts/access/manager/AccessManagerAdapter.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.0; import "./AccessManager.sol"; import "./AccessManaged.sol"; -import "../../utils/Address.sol"; /** * @dev This contract can be used to migrate existing {Ownable} or {AccessControl} contracts into an {AccessManager} From 6b56c8f4cc0a9dd75aedbfd8c9abc31e43fb5872 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:30:24 -0300 Subject: [PATCH 70/78] remove onlyDefaultAdmin modifier --- contracts/access/AccessControl.sol | 5 ----- contracts/access/manager/AccessManager.sol | 14 +++++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 951db485062..3a73de78b55 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -72,11 +72,6 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { _; } - modifier onlyDefaultAdmin() { - _checkRole(DEFAULT_ADMIN_ROLE); - _; - } - /** * @dev See {IERC165-supportsInterface}. */ diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 640bccc46b9..2263cbbd9ab 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -122,14 +122,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * Emits {GroupUpdated}. */ - function createGroup(uint8 group, string memory name) public virtual onlyDefaultAdmin { + function createGroup(uint8 group, string memory name) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _createGroup(group, name); } /** * @dev Updates an existing group's name. The caller must be the default admin. */ - function updateGroupName(uint8 group, string memory name) public virtual onlyDefaultAdmin { + function updateGroupName(uint8 group, string memory name) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { require(group != _GROUP_PUBLIC, "AccessManager: built-in group"); require(hasGroup(group), "AccessManager: unknown group"); emit GroupUpdated(group, name); @@ -200,7 +200,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { bytes4[] calldata selectors, uint8 group, bool allowed - ) public virtual onlyDefaultAdmin { + ) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; _allowedGroups[target][selector] = _withUpdatedGroup(_allowedGroups[target][selector], group, allowed); @@ -219,7 +219,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Sets the target contract to be in custom restricted mode. All restricted functions in the target contract * will follow the group-based restrictions defined by the AccessManager. The caller must be the default admin. */ - function setContractModeCustom(address target) public virtual onlyDefaultAdmin { + function setContractModeCustom(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _setContractMode(target, RestrictedMode.Custom); } @@ -227,7 +227,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Sets the target contract to be in "open" mode. All restricted functions in the target contract will become * callable by anyone. The caller must be the default admin. */ - function setContractModeOpen(address target) public virtual onlyDefaultAdmin { + function setContractModeOpen(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _setContractMode(target, RestrictedMode.Open); } @@ -235,14 +235,14 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Sets the target contract to be in "closed" mode. All restricted functions in the target contract will be * closed down and disallowed to all. The caller must be the default admin. */ - function setContractModeClosed(address target) public virtual onlyDefaultAdmin { + function setContractModeClosed(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { _setContractMode(target, RestrictedMode.Closed); } /** * @dev Transfers a target contract onto a new authority. The caller must be the default admin. */ - function transferContractAuthority(address target, address newAuthority) public virtual onlyDefaultAdmin { + function transferContractAuthority(address target, address newAuthority) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { AccessManaged(target).setAuthority(IAuthority(newAuthority)); } From b7e3b3fc173867454cd37491960352666f17977c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:30:45 -0300 Subject: [PATCH 71/78] lint --- contracts/access/manager/AccessManager.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 2263cbbd9ab..060e3b093b1 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -242,7 +242,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Transfers a target contract onto a new authority. The caller must be the default admin. */ - function transferContractAuthority(address target, address newAuthority) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + function transferContractAuthority( + address target, + address newAuthority + ) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { AccessManaged(target).setAuthority(IAuthority(newAuthority)); } From 82009865071601a8714dcbfda0808b76d9a571d8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:33:32 -0300 Subject: [PATCH 72/78] use return value names for _decodeGroupRole --- contracts/access/manager/AccessManager.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 060e3b093b1..54616fd1687 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -306,11 +306,11 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Decodes a role id into a group, if it is a role id of the kind returned by {_encodeGroupRole}. */ - function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool, uint8) { + function _decodeGroupRole(bytes32 role) internal pure virtual returns (bool isGroup, uint8 group) { bytes32 tagMask = ~bytes32(uint256(0xff)); bytes32 tag = role & tagMask; - uint8 group = uint8(role[31]); - return (tag == bytes32("group:"), group); + isGroup = tag == bytes32("group:"); + group = uint8(role[31]); } /** From 18e53e2c199bac08ef92f1a0106d8f2f571a9551 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:48:23 -0300 Subject: [PATCH 73/78] rename RestrictedMode -> AccessMode --- contracts/access/manager/AccessManager.sol | 26 +++++++++++----------- test/access/manager/AccessManager.test.js | 18 +++++++-------- test/helpers/enums.js | 2 +- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 54616fd1687..8ced9a3961d 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -8,7 +8,7 @@ import "./IAuthority.sol"; import "./AccessManaged.sol"; interface IAccessManager is IAuthority { - enum RestrictedMode { + enum AccessMode { Custom, Closed, Open @@ -18,7 +18,7 @@ interface IAccessManager is IAuthority { event GroupAllowed(address indexed target, bytes4 indexed selector, uint8 indexed group, bool allowed); - event RestrictedModeUpdated(address indexed target, RestrictedMode indexed mode); + event AccessModeUpdated(address indexed target, AccessMode indexed mode); function createGroup(uint8 group, string calldata name) external; @@ -38,7 +38,7 @@ interface IAccessManager is IAuthority { function setFunctionAllowedGroup(address target, bytes4[] calldata selectors, uint8 group, bool allowed) external; - function getContractMode(address target) external view returns (RestrictedMode); + function getContractMode(address target) external view returns (AccessMode); function setContractModeCustom(address target) external; @@ -89,7 +89,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { mapping(address => mapping(bytes4 => bytes32)) private _allowedGroups; // target -> mode - mapping(address => RestrictedMode) private _contractMode; + mapping(address => AccessMode) private _contractMode; uint8 private constant _GROUP_PUBLIC = type(uint8).max; @@ -181,10 +181,10 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * contract is in open or closed mode it will be reflected in the return value. */ function getFunctionAllowedGroups(address target, bytes4 selector) public view virtual returns (bytes32) { - RestrictedMode mode = getContractMode(target); - if (mode == RestrictedMode.Open) { + AccessMode mode = getContractMode(target); + if (mode == AccessMode.Open) { return _groupMask(_GROUP_PUBLIC); - } else if (mode == RestrictedMode.Closed) { + } else if (mode == AccessMode.Closed) { return 0; } else { return _allowedGroups[target][selector]; @@ -211,7 +211,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Returns the mode of the target contract, which may be custom (`0`), closed (`1`), or open (`2`). */ - function getContractMode(address target) public view virtual returns (RestrictedMode) { + function getContractMode(address target) public view virtual returns (AccessMode) { return _contractMode[target]; } @@ -220,7 +220,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * will follow the group-based restrictions defined by the AccessManager. The caller must be the default admin. */ function setContractModeCustom(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _setContractMode(target, RestrictedMode.Custom); + _setContractMode(target, AccessMode.Custom); } /** @@ -228,7 +228,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * callable by anyone. The caller must be the default admin. */ function setContractModeOpen(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _setContractMode(target, RestrictedMode.Open); + _setContractMode(target, AccessMode.Open); } /** @@ -236,7 +236,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * closed down and disallowed to all. The caller must be the default admin. */ function setContractModeClosed(address target) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { - _setContractMode(target, RestrictedMode.Closed); + _setContractMode(target, AccessMode.Closed); } /** @@ -288,9 +288,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { /** * @dev Sets the restricted mode of a target contract. */ - function _setContractMode(address target, RestrictedMode mode) internal virtual { + function _setContractMode(address target, AccessMode mode) internal virtual { _contractMode[target] = mode; - emit RestrictedModeUpdated(target, mode); + emit AccessModeUpdated(target, mode); } /** diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index c124e7c5cde..ad39ddc32b3 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -3,7 +3,7 @@ const { expectRevert, time: { duration }, } = require('@openzeppelin/test-helpers'); -const { RestrictedMode } = require('../../helpers/enums'); +const { AccessMode } = require('../../helpers/enums'); const AccessManager = artifacts.require('AccessManager'); const AccessManagerAdapter = artifacts.require('AccessManagerAdapter'); @@ -395,29 +395,29 @@ contract('AccessManager', function (accounts) { }); it('custom mode is default', async function () { - expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Custom); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(AccessMode.Custom); const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); }); it('open mode', async function () { const receipt = await this.manager.setContractModeOpen(this.managed.address, { from: admin }); - expectEvent(receipt, 'RestrictedModeUpdated', { + expectEvent(receipt, 'AccessModeUpdated', { target: this.managed.address, - mode: RestrictedMode.Open, + mode: AccessMode.Open, }); - expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Open); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(AccessMode.Open); const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([PUBLIC_GROUP]); }); it('closed mode', async function () { const receipt = await this.manager.setContractModeClosed(this.managed.address, { from: admin }); - expectEvent(receipt, 'RestrictedModeUpdated', { + expectEvent(receipt, 'AccessModeUpdated', { target: this.managed.address, - mode: RestrictedMode.Closed, + mode: AccessMode.Closed, }); - expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Closed); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(AccessMode.Closed); const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([]); }); @@ -426,7 +426,7 @@ contract('AccessManager', function (accounts) { await this.manager.setContractModeOpen(this.managed.address, { from: admin }); await this.manager.setContractModeClosed(this.managed.address, { from: admin }); await this.manager.setContractModeCustom(this.managed.address, { from: admin }); - expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(RestrictedMode.Custom); + expect(await this.manager.getContractMode(this.managed.address)).to.bignumber.equal(AccessMode.Custom); const allowedGroups = await this.manager.getFunctionAllowedGroups(this.managed.address, selector); expect(groupUtils.decodeBitmap(allowedGroups)).to.deep.equal([group]); }); diff --git a/test/helpers/enums.js b/test/helpers/enums.js index cacc3192725..874d8375f9d 100644 --- a/test/helpers/enums.js +++ b/test/helpers/enums.js @@ -9,5 +9,5 @@ module.exports = { ProposalState: Enum('Pending', 'Active', 'Canceled', 'Defeated', 'Succeeded', 'Queued', 'Expired', 'Executed'), VoteType: Enum('Against', 'For', 'Abstain'), Rounding: Enum('Down', 'Up', 'Zero'), - RestrictedMode: Enum('Custom', 'Closed', 'Open'), + AccessMode: Enum('Custom', 'Closed', 'Open'), }; From f94a8813526f8da4f8d8b076713e64923e628cd0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:51:50 -0300 Subject: [PATCH 74/78] use Context._msgSender --- contracts/access/manager/AccessManaged.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index f7a1a452f14..2dcdeb7aae6 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.0; +import "../../utils/Context.sol"; import "./IAuthority.sol"; /** @@ -12,8 +13,8 @@ import "./IAuthority.sol"; * IMPORTANT: The `restricted` modifier should never be used on `internal` functions, judiciously used in `public` * functions, and ideally only used in `external` functions. See {restricted}. */ -contract AccessManaged { - event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); +contract AccessManaged is Context { + event AuthorityUpdated(address indexed sender, IAuthority indexed newAuthority); IAuthority private _authority; @@ -31,7 +32,7 @@ contract AccessManaged { * ==== */ modifier restricted() { - _checkCanCall(msg.sender, msg.sig); + _checkCanCall(_msgSender(), msg.sig); _; } @@ -53,7 +54,7 @@ contract AccessManaged { * @dev Transfers control to a new authority. The caller must be the current authority. */ function setAuthority(IAuthority newAuthority) public virtual { - require(msg.sender == address(_authority), "AccessManaged: not current authority"); + require(_msgSender() == address(_authority), "AccessManaged: not current authority"); _setAuthority(newAuthority); } @@ -61,9 +62,8 @@ contract AccessManaged { * @dev Transfers control to a new authority. Internal function with no access restriction. */ function _setAuthority(IAuthority newAuthority) internal virtual { - IAuthority oldAuthority = _authority; _authority = newAuthority; - emit AuthorityUpdated(oldAuthority, newAuthority); + emit AuthorityUpdated(_msgSender(), newAuthority); } /** From cd8babdda7fa5feff722c038038cb4f0643a72d8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 14:54:23 -0300 Subject: [PATCH 75/78] reorder arguments to grant/revoke/renounceGroup --- contracts/access/manager/AccessManager.sol | 12 ++++---- test/access/manager/AccessManager.test.js | 36 +++++++++++----------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 8ced9a3961d..65e9bb941e2 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -28,11 +28,11 @@ interface IAccessManager is IAuthority { function getUserGroups(address user) external view returns (bytes32 groups); - function grantGroup(address user, uint8 group) external; + function grantGroup(uint8 group, address user) external; - function revokeGroup(address user, uint8 group) external; + function revokeGroup(uint8 group, address user) external; - function renounceGroup(address user, uint8 group) external; + function renounceGroup(uint8 group, address user) external; function getFunctionAllowedGroups(address target, bytes4 selector) external view returns (bytes32 groups); @@ -154,7 +154,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * Emits {RoleGranted} with the role id of the group, if wasn't already held by the user. */ - function grantGroup(address user, uint8 group) public virtual { + function grantGroup(uint8 group, address user) public virtual { grantRole(_encodeGroupRole(group), user); // will check msg.sender } @@ -163,7 +163,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ - function revokeGroup(address user, uint8 group) public virtual { + function revokeGroup(uint8 group, address user) public virtual { revokeRole(_encodeGroupRole(group), user); // will check msg.sender } @@ -172,7 +172,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * * Emits {RoleRevoked} with the role id of the group, if previously held by the user. */ - function renounceGroup(address user, uint8 group) public virtual { + function renounceGroup(uint8 group, address user) public virtual { renounceRole(_encodeGroupRole(group), user); // will check msg.sender } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index ad39ddc32b3..bf194d3885a 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -109,68 +109,68 @@ contract('AccessManager', function (accounts) { }); it('admin can grant group', async function () { - const granted = await this.manager.grantGroup(user1, group, { from: admin }); + const granted = await this.manager.grantGroup(group, user1, { from: admin }); expectEvent(granted, 'RoleGranted', { account: user1, role: groupUtils.role(group) }); const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); expect(groups).to.include(group); }); it('non-admin cannot grant group', async function () { - await expectRevert(this.manager.grantGroup(user1, group, { from: nonAdmin }), 'missing role'); + await expectRevert(this.manager.grantGroup(group, user1, { from: nonAdmin }), 'missing role'); }); it('cannot grant nonexistent group', async function () { - await expectRevert(this.manager.grantGroup(user1, otherGroup, { from: admin }), 'AccessManager: unknown group'); + await expectRevert(this.manager.grantGroup(otherGroup, user1, { from: admin }), 'AccessManager: unknown group'); }); }); describe('revoking & renouncing', function () { beforeEach('create and grant group', async function () { await this.manager.createGroup(group, name, { from: admin }); - await this.manager.grantGroup(user1, group, { from: admin }); + await this.manager.grantGroup(group, user1, { from: admin }); }); it('admin can revoke group', async function () { - await this.manager.revokeGroup(user1, group, { from: admin }); + await this.manager.revokeGroup(group, user1, { from: admin }); const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); expect(groups).to.not.include(group); }); it('non-admin cannot revoke group', async function () { - await expectRevert(this.manager.revokeGroup(user1, group, { from: nonAdmin }), 'missing role'); + await expectRevert(this.manager.revokeGroup(group, user1, { from: nonAdmin }), 'missing role'); }); it('user can renounce group', async function () { - await this.manager.renounceGroup(user1, group, { from: user1 }); + await this.manager.renounceGroup(group, user1, { from: user1 }); const groups = groupUtils.decodeBitmap(await this.manager.getUserGroups(user1)); expect(groups).to.not.include(group); }); it(`user cannot renounce other user's groups`, async function () { await expectRevert( - this.manager.renounceGroup(user1, group, { from: user2 }), + this.manager.renounceGroup(group, user1, { from: user2 }), 'can only renounce roles for self', ); await expectRevert( - this.manager.renounceGroup(user2, group, { from: user1 }), + this.manager.renounceGroup(group, user2, { from: user1 }), 'can only renounce roles for self', ); }); it('cannot revoke public group', async function () { await expectRevert( - this.manager.revokeGroup(user1, PUBLIC_GROUP, { from: admin }), + this.manager.revokeGroup(PUBLIC_GROUP, user1, { from: admin }), 'AccessManager: irrevocable group', ); }); it('cannot revoke nonexistent group', async function () { await expectRevert( - this.manager.revokeGroup(user1, otherGroup, { from: admin }), + this.manager.revokeGroup(otherGroup, user1, { from: admin }), 'AccessManager: unknown group', ); await expectRevert( - this.manager.renounceGroup(user1, otherGroup, { from: user1 }), + this.manager.renounceGroup(otherGroup, user1, { from: user1 }), 'AccessManager: unknown group', ); }); @@ -184,15 +184,15 @@ contract('AccessManager', function (accounts) { expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000000'); await this.manager.createGroup('0', '0', { from: admin }); - await this.manager.grantGroup(user1, '0', { from: admin }); + await this.manager.grantGroup('0', user1, { from: admin }); expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000001'); await this.manager.createGroup('1', '1', { from: admin }); - await this.manager.grantGroup(user1, '1', { from: admin }); + await this.manager.grantGroup('1', user1, { from: admin }); expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000000003'); await this.manager.createGroup('16', '16', { from: admin }); - await this.manager.grantGroup(user1, '16', { from: admin }); + await this.manager.grantGroup('16', user1, { from: admin }); expect(await getGroups()).to.equal('0x8000000000000000000000000000000000000000000000000000000000010003'); }); }); @@ -206,7 +206,7 @@ contract('AccessManager', function (accounts) { beforeEach('deploying managed contract', async function () { await this.manager.createGroup(group, '', { from: admin }); - await this.manager.grantGroup(groupMember, group, { from: admin }); + await this.manager.grantGroup(group, groupMember, { from: admin }); this.managed = await AccessManaged.new(this.manager.address); }); @@ -299,7 +299,7 @@ contract('AccessManager', function (accounts) { beforeEach('deploying managed contract', async function () { await this.manager.createGroup(group, '', { from: admin }); - await this.manager.grantGroup(groupMember, group, { from: admin }); + await this.manager.grantGroup(group, groupMember, { from: admin }); this.managed = await AccessManaged.new(this.manager.address); await this.manager.setFunctionAllowedGroup(this.managed.address, [selector, otherSelector], group, true, { from: admin, @@ -461,7 +461,7 @@ contract('AccessManager', function (accounts) { beforeEach('deploying adapter', async function () { await this.manager.createGroup(group, 'a group', { from: admin }); - await this.manager.grantGroup(user1, group, { from: admin }); + await this.manager.grantGroup(group, user1, { from: admin }); this.adapter = await AccessManagerAdapter.new(this.manager.address); }); From 597edc037b57d384e99de82cb5e9378ca30ae246 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 22 Mar 2023 15:04:03 -0300 Subject: [PATCH 76/78] add IAccessControlDefaultAdminRules --- contracts/access/manager/AccessManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 65e9bb941e2..c1abf3ddd0c 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -7,7 +7,7 @@ import "../AccessControlDefaultAdminRules.sol"; import "./IAuthority.sol"; import "./AccessManaged.sol"; -interface IAccessManager is IAuthority { +interface IAccessManager is IAuthority, IAccessControlDefaultAdminRules { enum AccessMode { Custom, Closed, From ee560d0266e3d25ce716aa583b204f0f815ac017 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 22 Mar 2023 16:55:19 -0300 Subject: [PATCH 77/78] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/manager/AccessManager.sol | 2 +- test/access/manager/AccessManaged.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index c1abf3ddd0c..8820ef4a06c 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -107,7 +107,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Returns true if the caller can invoke on a target the function identified by a function selector. * Entrypoint for {AccessManaged} contracts. */ - function canCall(address caller, address target, bytes4 selector) public view returns (bool) { + function canCall(address caller, address target, bytes4 selector) public view virtual returns (bool) { bytes32 allowedGroups = getFunctionAllowedGroups(target, selector); bytes32 callerGroups = getUserGroups(caller); return callerGroups & allowedGroups != 0; diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index 27234f0d566..fd09a3296df 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -38,7 +38,7 @@ contract('AccessManaged', function (accounts) { describe('restricted', function () { const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); - it('allows if authority says true', async function () { + it('allows if authority returns true', async function () { const authority = await SimpleAuthority.new(); const managed = await AccessManaged.new(authority.address); await authority.setAllowed(user, managed.address, selector); @@ -46,7 +46,7 @@ contract('AccessManaged', function (accounts) { expectEvent(restricted, 'RestrictedRan'); }); - it('reverts if authority says false', async function () { + it('reverts if authority returns false', async function () { const authority = await SimpleAuthority.new(); const managed = await AccessManaged.new(authority.address); await expectRevert(managed.restrictedFunction({ from: user }), 'AccessManaged: authority rejected'); From 33f5acedc18d008c7408ff5fe188f3d6307cea59 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 22 Mar 2023 16:29:49 -0600 Subject: [PATCH 78/78] Fix expected event parameter name in tests --- test/access/manager/AccessManaged.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/manager/AccessManaged.test.js b/test/access/manager/AccessManaged.test.js index fd09a3296df..caf1eea7e5c 100644 --- a/test/access/manager/AccessManaged.test.js +++ b/test/access/manager/AccessManaged.test.js @@ -23,7 +23,7 @@ contract('AccessManaged', function (accounts) { const managed = await AccessManaged.new(authority); const set = await managed.setAuthority(other, { from: authority }); expectEvent(set, 'AuthorityUpdated', { - oldAuthority: authority, + sender: authority, newAuthority: other, }); expect(await managed.authority()).to.equal(other);