AMP Variants where ACCESS_MANAGER and PASS_THRU_METHODS aren't immutable

contracts/AccessManagedProxy.sol

@@ -1,12 +1,12 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
-import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
+import {AccessManagedProxyBase} from "./AccessManagedProxyBase.sol";
 
 /**
  * @title AccessManagedProxy
- * @notice Proxy contract using IAccessManager to manage access control before delegating calls.
+ * @notice Proxy contract using IAccessManager to manage access control before delegating calls (immutable version)
  * @dev It's a variant of ERC1967Proxy.
  *
  *      Currently the check is executed on any call received by the proxy contract even calls to view methods
@@ -22,14 +22,11 @@ import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessMana
  * @custom:security-contact [email protected]
  * @author Ensuro
  */
-contract AccessManagedProxy is ERC1967Proxy {
+contract AccessManagedProxy is AccessManagedProxyBase {
   /**
    * @notice AccessManager contract that handles the permissions to access the implementation methods
    */
-  IAccessManager public immutable ACCESS_MANAGER;
-
-  // Error copied from IAccessManaged
-  error AccessManagedUnauthorized(address caller);
+  IAccessManager internal immutable _accessManager;
 
   /**
    * @notice Constructor of the proxy, defining the implementation and the access manager
@@ -41,45 +38,18 @@ contract AccessManagedProxy is ERC1967Proxy {
    *              encoded function call, and allows initializing the storage of the proxy like a Solidity constructor.
    * @param manager The access manager that will handle access control
    *
-   * Requirements:
-   *
-   * - If `data` is empty, `msg.value` must be zero.
+   * @custom:pre If `data` is empty, `msg.value` must be zero.
    */
   constructor(
     address implementation,
     bytes memory _data,
     IAccessManager manager
-  ) payable ERC1967Proxy(implementation, _data) {
-    ACCESS_MANAGER = manager;
+  ) payable AccessManagedProxyBase(implementation, _data) {
+    _accessManager = manager;
   }
 
-  /**
-   * @notice Intercepts the super._delegate call to implement access control
-   * @dev Checks with the ACCESS_MANAGER if msg.sender is authorized to call the current call's function,
-   * and if so, delegates the current call to `implementation`.
-   * @param implementation The implementation contract
-   *
-   * This function does not return to its internal call site, it will return directly to the external caller.
-   */
-  function _delegate(address implementation) internal virtual override {
-    bytes4 selector = bytes4(msg.data[0:4]);
-    bool immediate = _skipAC(selector); // reuse immediate variable both for skipped methods and canCall result
-    if (!immediate) {
-      (immediate, ) = ACCESS_MANAGER.canCall(msg.sender, address(this), selector);
-      if (!immediate) revert AccessManagedUnauthorized(msg.sender);
-    }
-    super._delegate(implementation);
-  }
-
-  /**
-   * @notice Returns whether to skip the access control validation or not
-   * @dev Hook called before ACCESS_MANAGER.canCall to enable skipping the call to the access manager for performance
-   *      reasons (for example on views) or to remove access control for other specific cases
-   * @param selector The selector of the method called
-   * @return Whether the access control using ACCESS_MANAGER should be skipped or not
-   */
-  // solhint-disable-next-line no-unused-vars
-  function _skipAC(bytes4 selector) internal view virtual returns (bool) {
-    return false;
+  // solhint-disable-next-line func-name-mixedcase
+  function ACCESS_MANAGER() public view override returns (IAccessManager) {
+    return _accessManager;
   }
 }

contracts/AccessManagedProxyBase.sol

@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
+
+/**
+ * @title AccessManagedProxyBase
+ * @notice Proxy contract using IAccessManager to manage access control before delegating calls.
+ * @dev It's a variant of ERC1967Proxy.
+ *
+ *      Currently the check is executed on any call received by the proxy contract even calls to view methods
+ *      (staticcall). In the setup of the ACCESS_MANAGER permissions you would want to make all the views and pure
+ *      functions enabled for the PUBLIC_ROLE.
+ *
+ *      This base contract delegates on descendent contracts the storage of the ACCESS_MANAGER.
+ *
+ *      Check https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 for a discussion on the
+ *      advantages and disadvantages of using it.
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
+abstract contract AccessManagedProxyBase is ERC1967Proxy {
+  // Error copied from IAccessManaged
+  error AccessManagedUnauthorized(address caller);
+
+  /**
+   * @notice Constructor of the proxy, defining the implementation and the access manager
+   * @dev Initializes the upgradeable proxy with an initial implementation specified by `implementation` and
+   *      with `manager` as the ACCESS_MANAGER that will handle access control.
+   *
+   * @param implementation The initial implementation contract.
+   * @param _data If nonempty, it's used as data in a delegate call to `implementation`. This will typically be an
+   *              encoded function call, and allows initializing the storage of the proxy like a Solidity constructor.
+   *
+   * Requirements:
+   *
+   * - If `data` is empty, `msg.value` must be zero.
+   */
+  constructor(address implementation, bytes memory _data) payable ERC1967Proxy(implementation, _data) {}
+
+  /**
+   * @notice AccessManager contract that handles the permissions to access the implementation methods
+   */
+  // solhint-disable-next-line func-name-mixedcase
+  function ACCESS_MANAGER() public view virtual returns (IAccessManager);
+
+  /**
+   * @notice Intercepts the super._delegate call to implement access control
+   * @dev Checks with the ACCESS_MANAGER if msg.sender is authorized to call the current call's function,
+   * and if so, delegates the current call to `implementation`.
+   * @param implementation The implementation contract
+   *
+   * This function does not return to its internal call site, it will return directly to the external caller.
+   */
+  function _delegate(address implementation) internal virtual override {
+    bytes4 selector = bytes4(msg.data[0:4]);
+    bool immediate = _skipAC(selector); // reuse immediate variable both for skipped methods and canCall result
+    if (!immediate) {
+      (immediate, ) = ACCESS_MANAGER().canCall(msg.sender, address(this), selector);
+      if (!immediate) revert AccessManagedUnauthorized(msg.sender);
+    }
+    super._delegate(implementation);
+  }
+
+  /**
+   * @notice Returns whether to skip the access control validation or not
+   * @dev Hook called before ACCESS_MANAGER.canCall to enable skipping the call to the access manager for performance
+   *      reasons (for example on views) or to remove access control for other specific cases
+   * @param selector The selector of the method called
+   * @return Whether the access control using ACCESS_MANAGER should be skipped or not
+   */
+  // solhint-disable-next-line no-unused-vars
+  function _skipAC(bytes4 selector) internal view virtual returns (bool) {
+    return false;
+  }
+}

contracts/AccessManagedProxyM.sol

@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
+import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol";
+import {AccessManagedProxyBase} from "./AccessManagedProxyBase.sol";
+
+/**
+ * @title AccessManagedProxyM
+ * @notice Proxy contract using IAccessManager to manage access control before delegating calls (mutable version)
+ * @dev It's a variant of ERC1967Proxy.
+ *
+ *      Currently the check is executed on any call received by the proxy contract even calls to view methods
+ *      (staticcall). In the setup of the ACCESS_MANAGER permissions you would want to make all the views and pure
+ *      functions enabled for the PUBLIC_ROLE.
+ *
+ *      The access manager can be changed by calling setAuthority
+ *
+ *      Check https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 for a discussion on the
+ *      advantages and disadvantages of using it.
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
+contract AccessManagedProxyM is AccessManagedProxyBase {
+  /**
+   * @notice Storage slot with the address of the current access mananger.
+   * @dev Computed as: `keccak256(
+   *    abi.encode(uint256(keccak256("ensuro.storage.AccessManagedProxyM.ACCESS_MANAGER")) - 1)
+   * ) & ~bytes32(uint256(0xff))
+   */
+  // solhint-disable-next-line const-name-snakecase
+  bytes32 internal constant ACCESS_MANAGER_SLOT = 0x2518994648e4a29af35d5d1b4b15a541173b8fabed9d3f7e10411447417eb800;
+
+  /**
+   * @notice Constructor of the proxy, defining the implementation and the access manager
+   * @dev Initializes the upgradeable proxy with an initial implementation specified by `implementation` and
+   *      with `manager` as the ACCESS_MANAGER that will handle access control.
+   *
+   * @param implementation The initial implementation contract.
+   * @param _data If nonempty, it's used as data in a delegate call to `implementation`. This will typically be an
+   *              encoded function call, and allows initializing the storage of the proxy like a Solidity constructor.
+   * @param manager The access manager that will handle access control
+   *
+   * @custom:pre If `data` is empty, `msg.value` must be zero.
+   */
+  constructor(
+    address implementation,
+    bytes memory _data,
+    IAccessManager manager
+  ) payable AccessManagedProxyBase(implementation, _data) {
+    StorageSlot.getAddressSlot(ACCESS_MANAGER_SLOT).value = address(manager);
+  }
+
+  // solhint-disable-next-line func-name-mixedcase
+  function ACCESS_MANAGER() public view override returns (IAccessManager) {
+    return IAccessManager(StorageSlot.getAddressSlot(ACCESS_MANAGER_SLOT).value);
+  }
+}

contracts/AccessManagedProxyMS.sol

@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
+import {AccessManagedProxyM} from "./AccessManagedProxyM.sol";
+
+/**
+ * @title AccessManagedProxyMS
+ * @notice Specialization of AccessManagedProxyM with pass thru (skips AM) for some messages for gas optimization
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
+contract AccessManagedProxyMS is AccessManagedProxyM {
+  /// @custom:storage-location erc7201:ensuro.storage.AccessManagedProxyMS
+  struct AccessManagedProxyMSStorage {
+    bytes4[] passThruMethods;
+    mapping(bytes4 => bool) skipAc;
+  }
+
+  /**
+   * @notice Storage slot with the address of the current access mananger.
+   * @dev Computed as: `keccak256(
+   *    abi.encode(uint256(keccak256("ensuro.storage.AccessManagedProxyMS")) - 1)
+   * ) & ~bytes32(uint256(0xff))
+   */
+  // solhint-disable-next-line const-name-snakecase
+  bytes32 internal constant AccessManagedProxyMSStorageLocation =
+    0x2c1649c08e6705e35d0e3e89871b1c15613bd00d2a5b7ac8b68b4ee805002700;
+
+  /**
+   * @notice Constructor of the proxy, defining the implementation and the access manager
+   * @dev Initializes the upgradeable proxy with an initial implementation specified by `implementation` and
+   *      with `manager` as the ACCESS_MANAGER that will handle access control.
+   *
+   * @param implementation The initial implementation contract.
+   * @param _data If nonempty, it's used as data in a delegate call to `implementation`. This will typically be an
+   *              encoded function call, and allows initializing the storage of the proxy like a Solidity constructor.
+   * @param manager The access manager that will handle access control
+   * @param passThruMethods The selector of methods that will skip the access control validation, typically used for
+   *                        views and other methods for gas optimization.
+   *
+   * Requirements:
+   *
+   * - If `data` is empty, `msg.value` must be zero.
+   */
+  constructor(
+    address implementation,
+    bytes memory _data,
+    IAccessManager manager,
+    bytes4[] memory passThruMethods
+  ) payable AccessManagedProxyM(implementation, _data, manager) {
+    AccessManagedProxyMSStorage storage $ = _getAccessManagedProxyMSStorage();
+    $.passThruMethods = new bytes4[](passThruMethods.length);
+    for (uint256 i; i < passThruMethods.length; ++i) {
+      $.passThruMethods[i] = passThruMethods[i];
+      $.skipAc[passThruMethods[i]] = true;
+    }
+  }
+
+  function _getAccessManagedProxyMSStorage() internal pure returns (AccessManagedProxyMSStorage storage $) {
+    // solhint-disable-next-line no-inline-assembly
+    assembly {
+      $.slot := AccessManagedProxyMSStorageLocation
+    }
+  }
+
+  /*
+   * @notice Skips the access control if the method called is one of the passThruMethods
+   * @dev See {PASS_THRU_METHODS()}
+   * @param selector The selector of the method called
+   * @return Whether the access control using ACCESS_MANAGER should be skipped or not
+   */
+  function _skipAC(bytes4 selector) internal view override returns (bool) {
+    return _getAccessManagedProxyMSStorage().skipAc[selector];
+  }
+
+  /**
+   * @notice Gives observability to the methods that are skipped from access control
+   * @dev This list is fixed and defined on contract construction
+   * @return methods The list of method selectors that skip ACCESS_MANAGER access control
+   */
+  // solhint-disable-next-line func-name-mixedcase
+  function PASS_THRU_METHODS() external view returns (bytes4[] memory methods) {
+    return _getAccessManagedProxyMSStorage().passThruMethods;
+  }
+}

contracts/mock/DummyAccessManaged.sol

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.16;
+
+import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import {AccessManagedUpgradeable} from "@openzeppelin/contracts-upgradeable/access/manager/AccessManagedUpgradeable.sol";
+
+/**
+ * @title Dummy implementation contract that supports upgrade and logs methods called
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
+contract DummyAccessManaged is AccessManagedUpgradeable, UUPSUpgradeable {
+  event MethodCalled(bytes4 selector);
+
+  /// @custom:oz-upgrades-unsafe-allow constructor
+  constructor() {
+    _disableInitializers();
+  }
+
+  function initialize(address initialAuthority) public virtual initializer {
+    __AccessManaged_init(initialAuthority);
+  }
+
+  /// @inheritdoc UUPSUpgradeable
+  function _authorizeUpgrade(address newImplementation) internal override {}
+
+  // For making gas usage comparisons easier, I'm going to use different methods for each variant
+  function callThruAMP() external restricted {
+    emit MethodCalled(this.callThruAMP.selector);
+  }
+
+  function callThru1967() external {
+    emit MethodCalled(this.callThru1967.selector);
+  }
+
+  function callDirect() external {
+    emit MethodCalled(this.callDirect.selector);
+  }
+
+  function callThruAMPSkippedMethod() external {
+    emit MethodCalled(this.callThruAMPSkippedMethod.selector);
+  }
+
+  function callThruAMPNonSkippedMethod() external restricted {
+    emit MethodCalled(this.callThruAMPNonSkippedMethod.selector);
+  }
+
+  function viewMethod() external view returns (address) {
+    return msg.sender;
+  }
+
+  function pureMethod() external pure returns (uint256) {
+    return 123456;
+  }
+}

contracts/mock/DummyImplementationAMPM.sol

@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.16;
+
+import {DummyImplementation} from "./DummyImplementation.sol";
+
+/**
+ * @title Dummy implementation contract that supports upgrade and logs methods called
+ * @dev Variant used to measure gas usage of the AccessManagedProxyM variants (using mutable ACCESS_MANAGER)
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
+contract DummyImplementationAMPM is DummyImplementation {
+  function callThruAMPM() external {
+    emit MethodCalled(this.callThruAMPM.selector);
+  }
+}