Implement suggestions from audit of AccessManager (#4178)

Co-authored-by: Francisco Giordano <fg@frang.io>

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);

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);
     }
 
     /**

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 () {