diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 2dcdeb7aa..08da07c58 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -8,7 +8,7 @@ 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. + * implementing a policy that allows certain callers to access 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}. @@ -30,6 +30,18 @@ contract AccessManaged is Context { * 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. * ==== + * + * [NOTE] + * ==== + * Selector collisions are mitigated by scoping permissions per contract, but some edge cases must be considered: + * + * * If the https://docs.soliditylang.org/en/latest/contracts.html#receive-ether-function[`receive()`] function is restricted, + * any other function with a `0x00000000` selector will share permissions with `receive()`. + * * Similarly, if there's no `receive()` function but a `fallback()` instead, the fallback might be called with empty `calldata`, + * sharing the `0x00000000` selector permissions as well. + * * For any other selector, if the restricted function is set on an upgradeable contract, an upgrade may remove the restricted + * function and replace it with a new method whose selector replaces the last one, keeping the previous permissions. + * ==== */ modifier restricted() { _checkCanCall(_msgSender(), msg.sig); diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 8820ef4a0..2efa667f2 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.13; -import "../AccessControl.sol"; import "../AccessControlDefaultAdminRules.sol"; import "./IAuthority.sol"; import "./AccessManaged.sol"; @@ -18,7 +17,7 @@ interface IAccessManager is IAuthority, IAccessControlDefaultAdminRules { event GroupAllowed(address indexed target, bytes4 indexed selector, uint8 indexed group, bool allowed); - event AccessModeUpdated(address indexed target, AccessMode indexed mode); + event AccessModeUpdated(address indexed target, AccessMode previousMode, AccessMode indexed mode); function createGroup(uint8 group, string calldata name) external; @@ -74,13 +73,13 @@ interface IAccessManager is IAuthority, IAccessControlDefaultAdminRules { * 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 {getUserGroups}, return a `bytes32` bitmap to succintly + * NOTE: Some of the functions in this contract, such as {getUserGroups}, return a `bytes32` bitmap to succinctly * 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 from its binary equivalence `0b101` */ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { - bytes32 _createdGroups; + bytes32 private _createdGroups; // user -> groups mapping(address => bytes32) private _userGroups; @@ -201,6 +200,7 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { uint8 group, bool allowed ) public virtual onlyRole(DEFAULT_ADMIN_ROLE) { + require(hasGroup(group), "AccessManager: unknown group"); for (uint256 i = 0; i < selectors.length; i++) { bytes4 selector = selectors[i]; _allowedGroups[target][selector] = _withUpdatedGroup(_allowedGroups[target][selector], group, allowed); @@ -289,8 +289,9 @@ contract AccessManager is IAccessManager, AccessControlDefaultAdminRules { * @dev Sets the restricted mode of a target contract. */ function _setContractMode(address target, AccessMode mode) internal virtual { + AccessMode previousMode = _contractMode[target]; _contractMode[target] = mode; - emit AccessModeUpdated(target, mode); + emit AccessModeUpdated(target, previousMode, mode); } /** diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index bf194d388..13c14e6e0 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -200,6 +200,7 @@ contract('AccessManager', function (accounts) { describe('allowing', function () { const group = '1'; + const otherGroup = '2'; const groupMember = user1; const selector = web3.eth.abi.encodeFunctionSignature('restrictedFunction()'); const otherSelector = web3.eth.abi.encodeFunctionSignature('otherRestrictedFunction()'); @@ -289,6 +290,13 @@ contract('AccessManager', function (accounts) { await this.manager.setContractModeClosed(this.managed.address, { from: admin }); await this.manager.setFunctionAllowedGroup(this.managed.address, [selector], group, false, { from: admin }); }); + + it('cannot allow nonexistent group', async function () { + await expectRevert( + this.manager.setFunctionAllowedGroup(this.managed.address, [selector], otherGroup, true, { from: admin }), + 'AccessManager: unknown group', + ); + }); }); describe('disallowing', function () {