diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 3a74e4b..feb4c64 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -15,16 +15,10 @@ jobs: node-version: "22" cache: "npm" - run: npm ci - - run: node scripts/generateAMPSkips.js 1 5 10 24 40 - run: npx hardhat compile - run: npx hardhat size-contracts - run: npm run solhint - run: npx hardhat test env: REPORT_GAS: "1" - - name: Clean to avoid sol-coverage error with AccessManagedProxyS40 - run: | - npx hardhat clean - rm contracts/amps/*.sol - node scripts/generateAMPSkips.js 1 5 10 24 - run: npx hardhat coverage diff --git a/.gitignore b/.gitignore index 276cee7..00e0e8b 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,3 @@ node_modules ignition/deployments/chain-31337 soljson-latest.js - -# AMP generated files -contracts/amps/*.sol diff --git a/README.md b/README.md index bd89512..4e7f187 100644 --- a/README.md +++ b/README.md @@ -34,18 +34,25 @@ configuration for that method. More details on the motivation of this idea here: https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 +Also check https://www.youtube.com/watch?v=DKdwJ9Ap9vM for a presentation on this approach. + ## 📝 Details -The package includes the AccessManagedProxy contract, and AccessManagedProxyS1 to AccessManagedProxyS24 that are -versions that accept from 1 to 24 methods (selectors) that will be skipped of the access manager check, to reduce gas -usage or to access immutability on some methods. +The AccessManagedProxy contract stores the configuration in the storage (uses namespaced storage layout, see EIP-7201), +but it doesn't include in the proxy functions to modify it. The implementation contracts should add the functions to +change the access manager (`setAuthority(...)`, following IAccessManaged interface of OZ 5.x) or the passThruMethods. + +The AMPUtils library includes several functions to modify the custom storage and other operations like making custom +access control checks. + +Also, an AccessManagedProxyBase abstract contract is provided in case you prefer to use immutable storage or other +variants. ## Development Try running some of the following tasks: ```shell -npx hardhat help -npx hardhat test REPORT_GAS=true npx hardhat test +npx hardhat coverage ``` diff --git a/contracts/AMPUtils.sol b/contracts/AMPUtils.sol index 59e1450..b608ac4 100644 --- a/contracts/AMPUtils.sol +++ b/contracts/AMPUtils.sol @@ -1,7 +1,8 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; -import {AccessManagedProxy} from "./AccessManagedProxy.sol"; +import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; +import {IAccessManagedProxy} from "./interfaces/IAccessManagedProxy.sol"; /** * @title AMPUtils @@ -13,6 +14,56 @@ library AMPUtils { // Error copied from IAccessManaged error AccessManagedUnauthorized(address caller); + struct AccessManagedProxyStorage { + IAccessManager accessManager; + bytes4[] passThruMethods; // This is used for observability + mapping(bytes4 => bool) skipAc; // This is the used for actual lookup + } + + /** + * @notice Storage slot with the address of the current access mananger. + * @dev Computed as: `keccak256( + * abi.encode(uint256(keccak256("ensuro.storage.AccessManagedProxy")) - 1) + * ) & ~bytes32(uint256(0xff)) + */ + // solhint-disable-next-line const-name-snakecase + bytes32 internal constant AccessManagedProxyStorageLocation = + 0x787c9d7ac910d64252bcea05acd5b7af6d59644e0451a8bb5674587555049c00; + + function getAccessManagedProxyStorage() internal pure returns (AccessManagedProxyStorage storage $) { + // solhint-disable-next-line no-inline-assembly + assembly { + $.slot := AccessManagedProxyStorageLocation + } + } + + function setAccessManager(IAccessManager accessManager) internal { + if (address(accessManager).code.length == 0) { + revert IAccessManagedProxy.AccessManagedInvalidAuthority(address(accessManager)); + } + getAccessManagedProxyStorage().accessManager = accessManager; + emit IAccessManagedProxy.AuthorityUpdated(address(accessManager)); + } + + function setPassThruMethods(bytes4[] memory passThruMethods) internal { + AccessManagedProxyStorage storage $ = AMPUtils.getAccessManagedProxyStorage(); + $.passThruMethods = new bytes4[](passThruMethods.length); + for (uint256 i; i < passThruMethods.length; ++i) { + $.passThruMethods[i] = passThruMethods[i]; + $.skipAc[passThruMethods[i]] = true; + } + emit IAccessManagedProxy.PassThruMethodsChanged(passThruMethods); + } + + function replacePassThruMethods(bytes4[] memory newPassThruMethods) internal { + AccessManagedProxyStorage storage $ = AMPUtils.getAccessManagedProxyStorage(); + bytes4[] memory oldPassThruMethods = $.passThruMethods; + for (uint256 i; i < oldPassThruMethods.length; ++i) { + $.skipAc[oldPassThruMethods[i]] = false; + } + setPassThruMethods(newPassThruMethods); + } + /** * @dev Checks if the user can call a particular selector, assuming the calling contract was deployed as an AMP. * @@ -20,7 +71,7 @@ library AMPUtils { * @param selector The selector of the method called (or a fake selector generated with makeSelector or another way) */ function checkCanCall(address user, bytes4 selector) internal view { - (bool immediate, ) = AccessManagedProxy(payable(address(this))).ACCESS_MANAGER().canCall( + (bool immediate, ) = IAccessManagedProxy(payable(address(this))).ACCESS_MANAGER().canCall( user, address(this), selector diff --git a/contracts/AccessManagedProxy.sol b/contracts/AccessManagedProxy.sol index c772fb6..ee56c4d 100644 --- a/contracts/AccessManagedProxy.sol +++ b/contracts/AccessManagedProxy.sol @@ -1,55 +1,73 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; +import {IAccessManagedProxy} from "./interfaces/IAccessManagedProxy.sol"; import {AccessManagedProxyBase} from "./AccessManagedProxyBase.sol"; +import {AMPUtils} from "./AMPUtils.sol"; /** * @title AccessManagedProxy - * @notice Proxy contract using IAccessManager to manage access control before delegating calls (immutable version) + * @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. + * Currently the check is executed on any call received by the proxy contract (even calls to view methods, i.e. + * staticcall). For gas efficiency, you can also have `passThruMethods`, and for those methods it will skip + * the call to the AccessManager, calling directly to the implementation contract. * - * For gas efficiency, the ACCESS_MANAGER is immutable, so take care you don't lose control of it, otherwise - * it will make your contract inaccesible or other bad things will happen. + * The accessManager and the passThruMethods are in the storage, but this contract doesn't include any method + * to modify them. They should be modified by the implementation contract (check AMPUtils for functions that + * encapsulate these operations). * * Check https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 for a discussion on the - * advantages and disadvantages of using it. + * advantages and disadvantages of using it. Also https://www.youtube.com/watch?v=DKdwJ9Ap9vM for a presentation + * on this approach. * * @custom:security-contact security@ensuro.co * @author Ensuro */ contract AccessManagedProxy is AccessManagedProxyBase { - /** - * @notice AccessManager contract that handles the permissions to access the implementation methods - */ - IAccessManager internal immutable _accessManager; + /// @custom:storage-location erc7201:ensuro.storage.AccessManagedProxy + /// For struct AMPUtils.AccessManagedProxyStorage /** * @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. + * with `accessManager` 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 accessManager 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. * - * @custom:pre 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 + IAccessManager accessManager, + bytes4[] memory passThruMethods ) payable AccessManagedProxyBase(implementation, _data) { - _accessManager = manager; + AMPUtils.setAccessManager(accessManager); + AMPUtils.setPassThruMethods(passThruMethods); + } + + /// @inheritdoc AccessManagedProxyBase + function _skipAC(bytes4 selector) internal view override returns (bool) { + return AMPUtils.getAccessManagedProxyStorage().skipAc[selector]; + } + + /// @inheritdoc IAccessManagedProxy + // solhint-disable-next-line func-name-mixedcase + function PASS_THRU_METHODS() external view override returns (bytes4[] memory methods) { + return AMPUtils.getAccessManagedProxyStorage().passThruMethods; } + /// @inheritdoc IAccessManagedProxy // solhint-disable-next-line func-name-mixedcase function ACCESS_MANAGER() public view override returns (IAccessManager) { - return _accessManager; + return AMPUtils.getAccessManagedProxyStorage().accessManager; } } diff --git a/contracts/AccessManagedProxyBase.sol b/contracts/AccessManagedProxyBase.sol index 7f71808..eb6b0fc 100644 --- a/contracts/AccessManagedProxyBase.sol +++ b/contracts/AccessManagedProxyBase.sol @@ -1,8 +1,9 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; +import {IAccessManagedProxy} from "./interfaces/IAccessManagedProxy.sol"; /** * @title AccessManagedProxyBase @@ -21,10 +22,7 @@ import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessMana * @custom:security-contact security@ensuro.co * @author Ensuro */ -abstract contract AccessManagedProxyBase is ERC1967Proxy { - // Error copied from IAccessManaged - error AccessManagedUnauthorized(address caller); - +abstract contract AccessManagedProxyBase is ERC1967Proxy, IAccessManagedProxy { /** * @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 @@ -34,18 +32,19 @@ abstract contract AccessManagedProxyBase is ERC1967Proxy { * @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. + * @custom:pre 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 - */ + /// @inheritdoc IAccessManagedProxy // solhint-disable-next-line func-name-mixedcase function ACCESS_MANAGER() public view virtual returns (IAccessManager); + /// @inheritdoc IAccessManagedProxy + function authority() external view virtual returns (address) { + return address(ACCESS_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, @@ -71,8 +70,5 @@ abstract contract AccessManagedProxyBase is ERC1967Proxy { * @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; - } + function _skipAC(bytes4 selector) internal view virtual returns (bool); } diff --git a/contracts/AccessManagedProxyM.sol b/contracts/AccessManagedProxyM.sol deleted file mode 100644 index 27a43b3..0000000 --- a/contracts/AccessManagedProxyM.sol +++ /dev/null @@ -1,59 +0,0 @@ -// 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 deleted file mode 100644 index 18789e2..0000000 --- a/contracts/AccessManagedProxyMS.sol +++ /dev/null @@ -1,87 +0,0 @@ -// 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/interfaces/IAccessManagedProxy.sol b/contracts/interfaces/IAccessManagedProxy.sol new file mode 100644 index 0000000..971f0a1 --- /dev/null +++ b/contracts/interfaces/IAccessManagedProxy.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.28; + +import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; + +/** + * @title IAccessManagedProxy - Interface of AccessManagedProxy contracts + * @notice This interface gives observability of the access control setup + * + * @dev The `ACCESS_MANAGER()` is the AccessManager contract that stores the access roles for most of the methods, + * except those listed in `PASS_THRU_METHODS()` that are forwarded directly to the proxy and don't have access control + * (at least not by the AccessManager contract). + * + * @author Ensuro + */ +interface IAccessManagedProxy { + /** + * @notice The ACCESS_MANAGER that manages the access controls was updated + * @dev Authority that manages this contract was updated. Uses same interface as OZ's IAccessManaged + */ + // solhint-disable-next-line gas-indexed-events + event AuthorityUpdated(address authority); + + /** + * @dev Emitted when the passThruMethods has changed. + */ + event PassThruMethodsChanged(bytes4[] newPassThruMethods); + + // Errors copied from OZ's IAccessManaged + error AccessManagedUnauthorized(address caller); + error AccessManagedInvalidAuthority(address authority); + + /** + * @notice Returns the current authority. + * @dev Returns the current authority. Same as ACCESS_MANAGER(), added for compatibility with OZ's IAccessManaged + */ + function authority() external view returns (address); + + /** + * @notice AccessManager contract that handles the permissions to access the implementation methods + */ + // solhint-disable-next-line func-name-mixedcase + function ACCESS_MANAGER() external view returns (IAccessManager); + + /** + * @notice Gives observability to the methods that are skipped from access control + * @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); +} diff --git a/contracts/mock/DummyImplementation.sol b/contracts/mock/DummyImplementation.sol index 2ab1c6b..3a7180d 100644 --- a/contracts/mock/DummyImplementation.sol +++ b/contracts/mock/DummyImplementation.sol @@ -1,8 +1,10 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.16; +import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; +import {AMPUtils} from "../AMPUtils.sol"; /** * @title Dummy implementation contract that supports upgrade and logs methods called @@ -51,4 +53,18 @@ contract DummyImplementation is UUPSUpgradeable, Initializable { function pureMethod() external pure returns (uint256) { return 123456; } + + function setAuthority(address newAuthority) external { + AMPUtils.setAccessManager(IAccessManager(newAuthority)); + } + + function setPassThruMethods(bytes4[] memory newPTMethods) external { + AMPUtils.replacePassThruMethods(newPTMethods); + } + + function checkCanCall(bytes memory something) external returns (bytes4 selector) { + selector = AMPUtils.makeSelector(something); + AMPUtils.checkCanCall(msg.sender, selector); + emit MethodCalled(selector); + } } diff --git a/contracts/mock/DummyImplementationAMPM.sol b/contracts/mock/DummyImplementationAMPM.sol deleted file mode 100644 index e304488..0000000 --- a/contracts/mock/DummyImplementationAMPM.sol +++ /dev/null @@ -1,17 +0,0 @@ -// 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/hardhat.config.js b/hardhat.config.js index 408ff9b..b640a41 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -7,7 +7,7 @@ require("@nomicfoundation/hardhat-toolbox"); /** @type import('hardhat/config').HardhatUserConfig */ module.exports = { solidity: { - version: "0.8.28", + version: "0.8.30", settings: { optimizer: { enabled: true, @@ -25,13 +25,11 @@ module.exports = { paths: ["@openzeppelin/contracts/access/manager/AccessManager.sol"], }, warnings: { - "contracts/AccessManagedProxy.sol": { + "contracts/AccessManagedProxyBase.sol": { "missing-receive": "off", - "unused-param": "off", }, - "contracts/amps/AccessManagedProxyS*.sol": { + "contracts/AccessManagedProxy.sol": { "missing-receive": "off", - "unused-param": "off", }, }, }; diff --git a/js/deployProxy.js b/js/deployProxy.js index 040e708..d58bace 100644 --- a/js/deployProxy.js +++ b/js/deployProxy.js @@ -35,14 +35,9 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { ); } let proxyFactory, deployFunction; - if (skipSelectors.length > 0) { - 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(opts.proxyClass || "AccessManagedProxy"); - deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr); - } + proxyFactory = await ethers.getContractFactory(opts.proxyClass || "AccessManagedProxy"); + deployFunction = async (hre_, opts, factory, ...args) => + ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr, skipSelectors); return hre.upgrades.deployProxy(contractFactory, initializeArgs, { ...opts, @@ -53,7 +48,7 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { } async function attachAsAMP(contract, ampContractFactory = undefined) { - ampContractFactory = ampContractFactory || (await ethers.getContractFactory("AccessManagedProxyS1")); + ampContractFactory = ampContractFactory || (await ethers.getContractFactory("AccessManagedProxy")); return ampContractFactory.attach(contract); } diff --git a/scripts/generateAMPSkips.js b/scripts/generateAMPSkips.js deleted file mode 100644 index a078de2..0000000 --- a/scripts/generateAMPSkips.js +++ /dev/null @@ -1,26 +0,0 @@ -const Handlebars = require("handlebars"); -const fs = require("fs"); - -if (process.argv.length < 3) { - console.error(`Usage: ${process.argv[0]} ${process.argv[1]} `); - console.error(`Example: ${process.argv[0]} ${process.argv[1]} 1 10 24`); - return; -} - -const templateFile = "templates/AccessManagedProxySXTemplate.sol.handlebars"; -const targetDir = "./contracts/amps/"; -const ampPath = "../"; - -if (!fs.existsSync(targetDir)) { - fs.mkdirSync(targetDir); -} - -const template = Handlebars.compile(fs.readFileSync(templateFile).toString()); - -for (const nMethodArg of process.argv.slice(2)) { - const nMethods = parseInt(nMethodArg); - const generated = template({ numOfSkipMethods: nMethods, zeroToN: [...Array(nMethods)].map((_, i) => i), ampPath }); - const outputFile = `${targetDir}AccessManagedProxyS${nMethods}.sol`; - console.log("Generating %s", outputFile); - fs.writeFileSync(outputFile, generated); -} diff --git a/scripts/make-npm-package.sh b/scripts/make-npm-package.sh index 835a96e..ff1de2b 100755 --- a/scripts/make-npm-package.sh +++ b/scripts/make-npm-package.sh @@ -19,9 +19,6 @@ rm -fr $TARGET_DIR 2>/dev/null mkdir -p $TARGET_DIR -# Generate 1 to 40 skip method contracts -node scripts/generateAMPSkips.js $(seq -s" " 1 40) - npx hardhat clean env COMPILE_MODE=production npx hardhat compile diff --git a/templates/AccessManagedProxySXTemplate.sol.handlebars b/templates/AccessManagedProxySXTemplate.sol.handlebars deleted file mode 100644 index 33a923b..0000000 --- a/templates/AccessManagedProxySXTemplate.sol.handlebars +++ /dev/null @@ -1,71 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; - -import {AccessManagedProxy} from "{{ampPath}}AccessManagedProxy.sol"; -import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol"; - -/** - * @title AccessManagedProxyS{{ numOfSkipMethods }} - * @notice Specialization of AccessManagedProxy with pass thru (skips AM) for some messages for gas optimization - * - * @custom:security-contact security@ensuro.co - * @author Ensuro - */ -contract AccessManagedProxyS{{ numOfSkipMethods }} is AccessManagedProxy { - {{#each zeroToN}} - bytes4 internal immutable PASS_THRU_METHODS_{{this}}; - {{/each}} - - /** - * @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[{{numOfSkipMethods}}] memory passThruMethods - ) payable AccessManagedProxy(implementation, _data, manager) { - {{#each zeroToN}} - PASS_THRU_METHODS_{{this}} = passThruMethods[{{this}}]; - {{/each}} - } - - /* - * @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 - {{#each zeroToN}} - selector == PASS_THRU_METHODS_{{this}}{{#if @last}};{{else}} ||{{/if}} - {{/each}} - } - - /** - * @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) { - methods = new bytes4[]({{ numOfSkipMethods}}); - {{#each zeroToN}} - methods[{{this}}] = PASS_THRU_METHODS_{{this}}; - {{/each}} - } -} diff --git a/test/test-access-managed-proxy.js b/test/test-access-managed-proxy.js index d43e341..d2e448b 100644 --- a/test/test-access-managed-proxy.js +++ b/test/test-access-managed-proxy.js @@ -2,9 +2,10 @@ const { expect } = require("chai"); const hre = require("hardhat"); const { ethers } = hre; -const { tagitVariant, setupAMRole } = require("@ensuro/utils/js/utils"); +const { tagitVariant, setupAMRole, captureAny } = require("@ensuro/utils/js/utils"); const helpers = require("@nomicfoundation/hardhat-network-helpers"); const { deployAMPProxy, attachAsAMP } = require("../js/deployProxy"); +const { ZeroAddress } = ethers; async function setUpCommon() { const [admin, anon] = await ethers.getSigners(); @@ -39,26 +40,6 @@ 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" + @@ -71,7 +52,7 @@ function randomSelector() { async function setUpAMPSkip(nMethods, skipViewsAndPure = false) { const ret = await setUpCommon(); const { admin, AccessManager, DummyImplementation } = ret; - const AccessManagedProxySkip = await ethers.getContractFactory(`AccessManagedProxyS${nMethods}`); + const AccessManagedProxy = await ethers.getContractFactory("AccessManagedProxy"); const acMgr = await AccessManager.deploy(admin); const selector = DummyImplementation.interface.getFunction("callThruAMPSkippedMethod").selector; const selectors = [...Array(nMethods - 1).keys()].map(randomSelector); @@ -84,41 +65,12 @@ async function setUpAMPSkip(nMethods, skipViewsAndPure = false) { return { skipSelectors: selectors, acMgr, - AccessManagedProxy: AccessManagedProxySkip, + AccessManagedProxy, deployProxy, ...ret, }; } -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(); @@ -172,59 +124,21 @@ async function setUpDummyAccessManaged() { }; } -// 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", - fixture: async () => setUpAMPSkip(10), - method: "callThruAMPNonSkippedMethod", - hasAC: true, - hasSkippedMethod: true, - }, - { - name: "AccessManagedProxyS24", - fixture: async () => setUpAMPSkip(24), - method: "callThruAMPNonSkippedMethod", - hasAC: true, - hasSkippedMethod: true, - }, - { - name: "AccessManagedProxyS40", - fixture: async () => setUpAMPSkip(40), - method: "callThruAMPNonSkippedMethod", - hasAC: true, - hasSkippedMethod: true, - }, - { - name: "AccessManagedProxyS1", - fixture: async () => setUpAMPSkip(1), - method: "callThruAMPNonSkippedMethod", - hasAC: true, - hasSkippedMethod: true, - }, - { - name: "AccessManagedProxyS1-skipViews", - fixture: async () => setUpAMPSkip(1, true), - method: "callThruAMPNonSkippedMethod", - hasAC: true, - hasSkippedMethod: true, - hasViews: true, - }, + { name: "DummyAccessManaged", fixture: setUpDummyAccessManaged, method: "callThruAMP", hasAC: false }, + { name: "AccessManagedProxy", fixture: setUpAMP, method: "callThruAMP", hasAC: true }, { name: "AccessManagedProxyMS40", - fixture: async () => setUpAMPMSkip(40), + fixture: async () => setUpAMPSkip(40), method: "callThruAMPNonSkippedMethod", hasAC: true, hasSkippedMethod: true, }, { name: "AccessManagedProxyMS1-skipViews", - fixture: async () => setUpAMPMSkip(1, true), + fixture: async () => setUpAMPSkip(1, true), method: "callThruAMPNonSkippedMethod", hasAC: true, hasSkippedMethod: true, @@ -245,6 +159,79 @@ variants.forEach((variant) => { expect(await dummyAsAMP.ACCESS_MANAGER()).to.equal(acMgr); }); + it("Checks events are emmited on proxy deployment [?hasAC]", async () => { + const { acMgr, deployProxy } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const dummyAsAMP = await ethers.getContractAt("AccessManagedProxy", await ethers.resolveAddress(dummy)); + const ptMethods = await dummyAsAMP.PASS_THRU_METHODS(); + await expect(dummy.deploymentTransaction()) + .to.emit(dummyAsAMP, "AuthorityUpdated") + .withArgs(acMgr) + .to.emit(dummyAsAMP, "PassThruMethodsChanged") + .withArgs(ptMethods); + }); + + it("Checks it can change the AccessManager [?hasAC]", async () => { + const { acMgr, admin, anon, deployProxy, AccessManager } = await helpers.loadFixture(variant.fixture); + const newAcMgr = await AccessManager.deploy(anon); + const dummy = await deployProxy(); + const dummyAsAMP = await ethers.getContractAt("AccessManagedProxy", await ethers.resolveAddress(dummy)); + + expect(await dummyAsAMP.authority()).to.equal(acMgr); + expect(await dummyAsAMP.ACCESS_MANAGER()).to.equal(acMgr); + + // It doesn't accept address(0) + await expect(dummy.setAuthority(ZeroAddress)) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedInvalidAuthority") + .withArgs(ZeroAddress); + + // It doesn't accept an EOA + await expect(dummy.setAuthority(anon)) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedInvalidAuthority") + .withArgs(anon); + + await expect(dummy.setAuthority(newAcMgr)).to.emit(dummyAsAMP, "AuthorityUpdated").withArgs(newAcMgr); + expect(await dummyAsAMP.connect(admin).authority()).to.equal(newAcMgr); + expect(await dummyAsAMP.connect(admin).ACCESS_MANAGER()).to.equal(newAcMgr); + + // Now the old admin can't access permissioned methods + await expect(dummy.connect(admin).setAuthority(newAcMgr)).to.be.revertedWithCustomError( + dummyAsAMP, + "AccessManagedUnauthorized" + ); + + // But the new admin (anon) can + await expect(dummy.connect(anon).setAuthority(newAcMgr)).not.to.be.reverted; + }); + + it("Checks it can change the passThruMethods [?hasAC]", async () => { + const { admin, anon, deployProxy, DummyImplementation } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const dummyAsAMP = await ethers.getContractAt("AccessManagedProxy", await ethers.resolveAddress(dummy)); + + const newSkipMethods = [DummyImplementation.interface.getFunction("setPassThruMethods").selector]; + await expect(dummy.connect(anon).setPassThruMethods(newSkipMethods)).to.be.revertedWithCustomError( + dummyAsAMP, + "AccessManagedUnauthorized" + ); + + await expect(dummy.connect(admin).setPassThruMethods(newSkipMethods)) + .to.emit(dummyAsAMP, "PassThruMethodsChanged") + .withArgs(newSkipMethods); + + expect(await dummyAsAMP.connect(admin).PASS_THRU_METHODS()).to.deep.equal(newSkipMethods); + + // Now the anon can setPassThruMethods because is skipped + await expect(dummy.connect(anon).setPassThruMethods([])) + .to.emit(dummyAsAMP, "PassThruMethodsChanged") + .withArgs([]); + // Now it can't anymore + await expect(dummy.connect(anon).setPassThruMethods(newSkipMethods)).to.be.revertedWithCustomError( + dummyAsAMP, + "AccessManagedUnauthorized" + ); + }); + it("Checks methods can be called with the right permissions [?hasAC]", async () => { const { acMgr, deployProxy, AccessManagedProxy, anon, admin, DummyImplementation } = await helpers.loadFixture( variant.fixture @@ -297,7 +284,7 @@ variants.forEach((variant) => { }); it("Checks access to views [?hasAC]", async () => { - const { deployProxy, anon, AccessManagedProxy } = await helpers.loadFixture(variant.fixture); + const { deployProxy, anon } = await helpers.loadFixture(variant.fixture); const dummy = await deployProxy(); if (variant.hasViews) { expect(await dummy.connect(anon).viewMethod()).to.equal(anon); @@ -312,5 +299,48 @@ variants.forEach((variant) => { .withArgs(anon); } }); + + it("Checks custom selectors can be checked from implementation class [?hasAC]", async () => { + const { deployProxy, anon, admin, DummyImplementation, acMgr } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const dummyAsAMP = await attachAsAMP(dummy); + + // Anon can't call because it doesn't have permission to call `checkCanCall` + await expect(dummy.connect(anon).checkCanCall(ethers.toUtf8Bytes("foobar"))) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(anon); + + // Make "checkCanCall" PUBLIC_ROLE + await acMgr + .connect(admin) + .setTargetFunctionRole( + dummy, + [DummyImplementation.interface.getFunction("checkCanCall").selector], + await acMgr.PUBLIC_ROLE() + ); + + // Anon still fails because it doesn't have the custom permission + await expect(dummy.connect(anon).checkCanCall(ethers.toUtf8Bytes("foobar"))) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(anon); + + await expect(dummy.connect(admin).checkCanCall(ethers.toUtf8Bytes("foobar"))) + .to.emit(dummy, "MethodCalled") + .withArgs(captureAny.value); + const selector = captureAny.lastValue; + + expect(selector).to.equal(ethers.keccak256(ethers.toUtf8Bytes("foobar")).slice(0, 10)); + + // Make custom "foobar" selector enabled for anon + await acMgr.grantRole(234, anon, 0); + await acMgr.connect(admin).setTargetFunctionRole(dummy, [selector], 234); + + // Now anon works fine + await expect(dummy.connect(anon).checkCanCall(ethers.toUtf8Bytes("foobar"))).not.to.be.reverted; + // Now admin fails + await expect(dummy.connect(admin).checkCanCall(ethers.toUtf8Bytes("foobar"))) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(admin); + }); }); });