diff --git a/contracts/AccessManagedProxy.sol b/contracts/AccessManagedProxy.sol index b95d4a4..c772fb6 100644 --- a/contracts/AccessManagedProxy.sol +++ b/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 security@ensuro.co * @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; } } diff --git a/contracts/AccessManagedProxyBase.sol b/contracts/AccessManagedProxyBase.sol new file mode 100644 index 0000000..7f71808 --- /dev/null +++ b/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 security@ensuro.co + * @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; + } +} diff --git a/contracts/AccessManagedProxyM.sol b/contracts/AccessManagedProxyM.sol new file mode 100644 index 0000000..27a43b3 --- /dev/null +++ b/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 security@ensuro.co + * @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); + } +} diff --git a/contracts/AccessManagedProxyMS.sol b/contracts/AccessManagedProxyMS.sol new file mode 100644 index 0000000..18789e2 --- /dev/null +++ b/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 security@ensuro.co + * @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; + } +} diff --git a/contracts/mock/DummyAccessManaged.sol b/contracts/mock/DummyAccessManaged.sol new file mode 100644 index 0000000..ee4e955 --- /dev/null +++ b/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 security@ensuro.co + * @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; + } +} diff --git a/contracts/mock/DummyImplementationAMPM.sol b/contracts/mock/DummyImplementationAMPM.sol new file mode 100644 index 0000000..e304488 --- /dev/null +++ b/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 security@ensuro.co + * @author Ensuro + */ +contract DummyImplementationAMPM is DummyImplementation { + function callThruAMPM() external { + emit MethodCalled(this.callThruAMPM.selector); + } +} diff --git a/js/deployProxy.js b/js/deployProxy.js index e931ef2..040e708 100644 --- a/js/deployProxy.js +++ b/js/deployProxy.js @@ -36,11 +36,11 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { } let proxyFactory, deployFunction; if (skipSelectors.length > 0) { - proxyFactory = await ethers.getContractFactory(`AccessManagedProxyS${skipSelectors.length}`); + proxyFactory = await ethers.getContractFactory(opts.proxyClass || `AccessManagedProxyS${skipSelectors.length}`); deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr, skipSelectors); } else { - proxyFactory = await ethers.getContractFactory("AccessManagedProxy"); + proxyFactory = await ethers.getContractFactory(opts.proxyClass || "AccessManagedProxy"); deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr); } @@ -57,7 +57,7 @@ async function attachAsAMP(contract, ampContractFactory = undefined) { return ampContractFactory.attach(contract); } -async function getAccessManager(contract, ampContractFactory = undefined, accessManagerFactory="AccessManager") { +async function getAccessManager(contract, ampContractFactory = undefined, accessManagerFactory = "AccessManager") { const contractAsAMP = await attachAsAMP(contract, ampContractFactory); return ethers.getContractAt(accessManagerFactory, await contractAsAMP.ACCESS_MANAGER()); } @@ -66,7 +66,6 @@ function makeSelector(role) { return ethers.keccak256(ethers.toUtf8Bytes(role)).slice(0, 10); } - module.exports = { deployAMPProxy, attachAsAMP, diff --git a/test/test-access-managed-proxy.js b/test/test-access-managed-proxy.js index 7b5da56..d43e341 100644 --- a/test/test-access-managed-proxy.js +++ b/test/test-access-managed-proxy.js @@ -9,12 +9,14 @@ const { deployAMPProxy, attachAsAMP } = require("../js/deployProxy"); async function setUpCommon() { const [admin, anon] = await ethers.getSigners(); const DummyImplementation = await ethers.getContractFactory("DummyImplementation"); + const DummyAccessManaged = await ethers.getContractFactory("DummyAccessManaged"); const AccessManager = await ethers.getContractFactory("AccessManager"); return { admin, anon, AccessManager, + DummyAccessManaged, DummyImplementation, }; } @@ -37,6 +39,26 @@ async function setUpAMP() { }; } +async function setUpAMPM() { + const ret = await setUpCommon(); + const DummyImplementation = await ethers.getContractFactory("DummyImplementationAMPM"); + const AccessManagedProxy = await ethers.getContractFactory("AccessManagedProxyM"); + const { admin, AccessManager } = ret; + const acMgr = await AccessManager.deploy(admin); + + async function deployProxy() { + return deployAMPProxy(DummyImplementation, [], { acMgr, proxyClass: "AccessManagedProxyM" }); + } + + return { + ...ret, + acMgr, + AccessManagedProxy, + deployProxy, + DummyImplementation, + }; +} + function randomSelector() { return ( "0x" + @@ -68,6 +90,35 @@ async function setUpAMPSkip(nMethods, skipViewsAndPure = false) { }; } +async function setUpAMPMSkip(nMethods, skipViewsAndPure = false) { + const ret = await setUpCommon(); + const { admin, AccessManager } = ret; + const AccessManagedProxySkip = await ethers.getContractFactory(`AccessManagedProxyMS`); + const acMgr = await AccessManager.deploy(admin); + const DummyImplementation = await ethers.getContractFactory("DummyImplementationAMPM"); + const selector = DummyImplementation.interface.getFunction("callThruAMPSkippedMethod").selector; + const selectors = [...Array(nMethods - 1).keys()].map(randomSelector); + selectors.push(selector); + + async function deployProxy() { + return deployAMPProxy(DummyImplementation, [], { + acMgr, + skipMethods: selectors, + skipViewsAndPure, + proxyClass: "AccessManagedProxyMS", + }); + } + + return { + ...ret, + DummyImplementation, + skipSelectors: selectors, + acMgr, + AccessManagedProxy: AccessManagedProxySkip, + deployProxy, + }; +} + async function setUpDirect() { const ret = await setUpCommon(); @@ -98,10 +149,35 @@ async function setUpERC1967() { }; } +async function setUpDummyAccessManaged() { + const ret = await setUpCommon(); + const { admin, AccessManager } = ret; + const acMgr = await AccessManager.deploy(admin); + + async function deployProxy(implContract = ret.DummyAccessManaged) { + const contract = await hre.upgrades.deployProxy(implContract, [await ethers.resolveAddress(acMgr)], { + kind: "uups", + }); + // Make the method public + // const PUBLIC_ROLE = await acMgr.PUBLIC_ROLE(); + // const selector = ret.DummyAccessManaged.interface.getFunction("callThruAMP").selector; + // await acMgr.setTargetFunctionRole(contract, [selector], PUBLIC_ROLE); + return contract; + } + + return { + acMgr, + deployProxy, + ...ret, + }; +} + // const variants = [{ name: "NoProxy" }, { name: "ERC1967Proxy" }, { name: "AccessManagedProxy" }]; const variants = [ { name: "NoProxy", fixture: setUpDirect, method: "callDirect", hasAC: false }, { name: "AccessManagedProxy", fixture: setUpAMP, method: "callThruAMP", hasAC: true }, + { name: "AccessManagedProxy - Mutable", fixture: setUpAMPM, method: "callThruAMPM", hasAC: true }, + { name: "DummyAccessManaged", fixture: setUpDummyAccessManaged, method: "callThruAMP", hasAC: false }, { name: "ERC1967Proxy", fixture: setUpERC1967, method: "callThru1967", hasAC: false }, { name: "AccessManagedProxyS10", @@ -139,6 +215,21 @@ const variants = [ hasSkippedMethod: true, hasViews: true, }, + { + name: "AccessManagedProxyMS40", + fixture: async () => setUpAMPMSkip(40), + method: "callThruAMPNonSkippedMethod", + hasAC: true, + hasSkippedMethod: true, + }, + { + name: "AccessManagedProxyMS1-skipViews", + fixture: async () => setUpAMPMSkip(1, true), + method: "callThruAMPNonSkippedMethod", + hasAC: true, + hasSkippedMethod: true, + hasViews: true, + }, ]; variants.forEach((variant) => {