diff --git a/.github/workflows/npm.yaml b/.github/workflows/npm.yaml index fa4f33e..2c6a038 100644 --- a/.github/workflows/npm.yaml +++ b/.github/workflows/npm.yaml @@ -27,11 +27,11 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4 - uses: actions/setup-node@v3 with: - node-version: "20" + node-version: "22" cache: "npm" - run: npm ci diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index feb4c64..909593c 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -15,6 +15,7 @@ jobs: node-version: "22" cache: "npm" - run: npm ci + - run: node scripts/generateAMPSkips.js 1 5 10 24 - run: npx hardhat compile - run: npx hardhat size-contracts - run: npm run solhint diff --git a/.gitignore b/.gitignore index 5e688fd..276cee7 100644 --- a/.gitignore +++ b/.gitignore @@ -16,5 +16,7 @@ node_modules # Hardhat Ignition default folder for deployments against a local node ignition/deployments/chain-31337 -/verifiable-binaries -/contracts-exposed +soljson-latest.js + +# AMP generated files +contracts/amps/*.sol diff --git a/.prettierrc.json b/.prettierrc.json index 96a2fe0..786d11b 100644 --- a/.prettierrc.json +++ b/.prettierrc.json @@ -4,7 +4,9 @@ "semi": true, "singleQuote": false, "tabWidth": 2, - "plugins": ["prettier-plugin-solidity"], + "plugins": [ + "prettier-plugin-solidity" + ], "overrides": [ { "files": "*.sol", diff --git a/.solhint.json b/.solhint.json index b543e1a..46b3449 100644 --- a/.solhint.json +++ b/.solhint.json @@ -6,6 +6,7 @@ "prettier/prettier": "error", "payable-fallback": "off", "avoid-throw": "off", + "use-natspec": "off", "avoid-suicide": "error", "avoid-sha3": "warn", "compiler-version": [ diff --git a/.solhintignore b/.solhintignore index a3f25d1..b9a5a96 100644 --- a/.solhintignore +++ b/.solhintignore @@ -1,2 +1,3 @@ contracts/dependencies/* contracts/mock/* +contracts/amps/* diff --git a/contracts/AccessManagedProxy.sol b/contracts/AccessManagedProxy.sol index 94449b4..b95d4a4 100644 --- a/contracts/AccessManagedProxy.sol +++ b/contracts/AccessManagedProxy.sol @@ -62,8 +62,24 @@ contract AccessManagedProxy is ERC1967Proxy { * 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 { - (bool immediate, ) = ACCESS_MANAGER.canCall(msg.sender, address(this), bytes4(msg.data[0:4])); - if (!immediate) revert AccessManagedUnauthorized(msg.sender); + 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/mock/DummyImplementation.sol b/contracts/mock/DummyImplementation.sol index 096084d..15f72f6 100644 --- a/contracts/mock/DummyImplementation.sol +++ b/contracts/mock/DummyImplementation.sol @@ -3,18 +3,13 @@ pragma solidity ^0.8.16; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; -interface IDummy { - function method1() external; - function method2() external; -} - /** * @title Dummy implementation contract that supports upgrade and logs methods called * * @custom:security-contact security@ensuro.co * @author Ensuro */ -contract DummyImplementation is UUPSUpgradeable, IDummy { +contract DummyImplementation is UUPSUpgradeable { event MethodCalled(bytes4 selector); /// @custom:oz-upgrades-unsafe-allow constructor @@ -27,11 +22,32 @@ contract DummyImplementation is UUPSUpgradeable, IDummy { /// @inheritdoc UUPSUpgradeable function _authorizeUpgrade(address newImplementation) internal override {} - function method1() external override { - emit MethodCalled(IDummy.method1.selector); + // For making gas usage comparisons easier, I'm going to use different methods for each variant + function callThruAMP() external { + 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 { + emit MethodCalled(this.callThruAMPNonSkippedMethod.selector); + } + + function viewMethod() external view returns (address) { + return msg.sender; } - function method2() external override { - emit MethodCalled(IDummy.method2.selector); + function pureMethod() external pure returns (uint256) { + return 123456; } } diff --git a/hardhat.config.js b/hardhat.config.js index 632d48d..408ff9b 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -1,6 +1,7 @@ require("@openzeppelin/hardhat-upgrades"); require("hardhat-dependency-compiler"); require("hardhat-contract-sizer"); +require("hardhat-ignore-warnings"); require("@nomicfoundation/hardhat-toolbox"); /** @type import('hardhat/config').HardhatUserConfig */ @@ -21,8 +22,16 @@ module.exports = { disambiguatePaths: false, }, dependencyCompiler: { - paths: [ - "@openzeppelin/contracts/access/manager/AccessManager.sol", - ], + paths: ["@openzeppelin/contracts/access/manager/AccessManager.sol"], + }, + warnings: { + "contracts/AccessManagedProxy.sol": { + "missing-receive": "off", + "unused-param": "off", + }, + "contracts/amps/AccessManagedProxyS*.sol": { + "missing-receive": "off", + "unused-param": "off", + }, }, }; diff --git a/js/deployProxy.js b/js/deployProxy.js new file mode 100644 index 0000000..dfdd640 --- /dev/null +++ b/js/deployProxy.js @@ -0,0 +1,57 @@ +const hre = require("hardhat"); +const { ethers } = hre; +const { deploy: ozUpgradesDeploy } = require("@openzeppelin/hardhat-upgrades/dist/utils"); + +/** + * Deploys a contract using an AccessManagedProxy. Similar to hre.upgrades.deployProxy, but using AccessManagedProxy + * + * @param {contractFactory} The contract factory of the implementation contract + * @param {initializeArgs} Arguments for `initialize` + * @param {opts} Options for hre.upgrades.deployProxy with some AccessManagedProxy additions: + * - skipViewsAndPure: if true, deploys a proxy that will skip the access control for all the view and + * pure methods + * - skipMethods: list of method names that will skip the access control. Added to views and pure, if + * skipViewsAndPure is true. + * - acMgr: mandatory argument that will be used for the AMP + * @returns {contract} Promise + */ +async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { + const { acMgr, skipViewsAndPure, skipMethods } = opts; + if (acMgr === undefined) throw new Error("Missing required `acMgr` in opts"); + let skipSelectors = []; + if (skipViewsAndPure) { + skipSelectors = contractFactory.interface.fragments + .filter( + (fragment) => + fragment.type === "function" && (fragment.stateMutability === "pure" || fragment.stateMutability === "view") + ) + .map((fragment) => fragment.selector); + } + if (skipMethods !== undefined && skipMethods.length > 0) { + skipSelectors.push( + ...skipMethods.map((method) => + method.startsWith("0x") ? method : contractFactory.interface.getFunction(method).selector + ) + ); + } + let proxyFactory, deployFunction; + if (skipSelectors.length > 0) { + proxyFactory = await ethers.getContractFactory(`AccessManagedProxyS${skipSelectors.length}`); + deployFunction = async (hre_, opts, factory, ...args) => + ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr, skipSelectors); + } else { + proxyFactory = await ethers.getContractFactory("AccessManagedProxy"); + deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr); + } + + return hre.upgrades.deployProxy(contractFactory, [], { + ...opts, + kind: "uups", + proxyFactory, + deployFunction, + }); +} + +module.exports = { + deployAMPProxy, +}; diff --git a/package-lock.json b/package-lock.json index bebed5f..0bb5326 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,13 +12,16 @@ "@openzeppelin/contracts-upgradeable": "^5.4.0" }, "devDependencies": { + "@ensuro/utils": "^0.2.9", "@nomicfoundation/hardhat-toolbox": "^6.1.0", "@openzeppelin/hardhat-upgrades": "^3.9.1", "eslint": "^9.32.0", "eslint-config-prettier": "^10.1.8", + "handlebars": "^4.7.8", "hardhat": "^2.26.2", "hardhat-contract-sizer": "^2.10.0", "hardhat-dependency-compiler": "^1.2.1", + "hardhat-ignore-warnings": "^0.2.12", "prettier": "^3.6.2", "solhint": "^6.0.0", "solhint-plugin-prettier": "^0.1.0" @@ -775,6 +778,13 @@ "node": ">=12" } }, + "node_modules/@ensuro/utils": { + "version": "0.2.9", + "resolved": "https://registry.npmjs.org/@ensuro/utils/-/utils-0.2.9.tgz", + "integrity": "sha512-lfHt+cPZRMBC6tR9kCgMrJCmGsYq+6f9VFbHkGtLtyUjvEZezqbFGPtiDSHDQ79vqFBAev3BMQc6LF5guP2Dfw==", + "dev": true, + "license": "Apache-2.0" + }, "node_modules/@eslint-community/eslint-utils": { "version": "4.7.0", "resolved": "https://registry.npmjs.org/@eslint-community/eslint-utils/-/eslint-utils-4.7.0.tgz", @@ -6394,7 +6404,6 @@ "integrity": "sha512-vafaFqs8MZkRrSX7sFVUdo3ap/eNiLnb4IakshzvP56X5Nr1iGKAIqdX6tMlm6HcNRIkr6AxO5jFEoJzzpT8aQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "minimist": "^1.2.5", "neo-async": "^2.6.2", @@ -6417,7 +6426,6 @@ "integrity": "sha512-UjgapumWlbMhkBgzT7Ykc5YXUT46F0iKu8SGXq0bcwP5dz/h0Plj6enJqjz1Zbq2l5WaqYnrVbwWOWMyF3F47g==", "dev": true, "license": "BSD-3-Clause", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -6625,6 +6633,31 @@ "@scure/bip39": "1.3.0" } }, + "node_modules/hardhat-ignore-warnings": { + "version": "0.2.12", + "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.12.tgz", + "integrity": "sha512-SaxCLKzYBMk3Rd1275TnanUmmxwgU+bu4Ekf2MKcqXxxt6xTGcPTtTaM+USrLgmejZHC4Itg/PaWITlOp4RL3g==", + "dev": true, + "license": "MIT", + "dependencies": { + "minimatch": "^5.1.0", + "node-interval-tree": "^2.0.1", + "solidity-comments": "^0.0.2" + } + }, + "node_modules/hardhat-ignore-warnings/node_modules/minimatch": { + "version": "5.1.6", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-5.1.6.tgz", + "integrity": "sha512-lKwV/1brpG6mBUFHtb7NUmtABCb2WZZmm2wNiOA5hAb8VdCS4B3dtMWyvcoViccwAW/COERjXLt0zP1zXUN26g==", + "dev": true, + "license": "ISC", + "dependencies": { + "brace-expansion": "^2.0.1" + }, + "engines": { + "node": ">=10" + } + }, "node_modules/hardhat/node_modules/@noble/hashes": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-1.2.0.tgz", @@ -8040,8 +8073,7 @@ "resolved": "https://registry.npmjs.org/neo-async/-/neo-async-2.6.2.tgz", "integrity": "sha512-Yd3UES5mWCSqR+qNT93S3UoYUkqAZ9lLg8a7g9rimsWmYGK8cVToA4/sF3RrshdyV3sAGMXVUmpMYOw+dLpOuw==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/node-addon-api": { "version": "2.0.2", @@ -8094,6 +8126,19 @@ "node-gyp-build-test": "build-test.js" } }, + "node_modules/node-interval-tree": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/node-interval-tree/-/node-interval-tree-2.1.2.tgz", + "integrity": "sha512-bJ9zMDuNGzVQg1xv0bCPzyEDxHgbrx7/xGj6CDokvizZZmastPsOh0JJLuY8wA5q2SfX1TLNMk7XNV8WxbGxzA==", + "dev": true, + "license": "MIT", + "dependencies": { + "shallowequal": "^1.1.0" + }, + "engines": { + "node": ">= 14.0.0" + } + }, "node_modules/nofilter": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/nofilter/-/nofilter-3.1.0.tgz", @@ -9401,6 +9446,13 @@ "node": "*" } }, + "node_modules/shallowequal": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/shallowequal/-/shallowequal-1.1.0.tgz", + "integrity": "sha512-y0m1JoUZSlPAjXVtPPW70aZWfIL/dSP7AFkRnniLCrK/8MDKog3TySTBmckD+RObVxH0v4Tox67+F14PdED2oQ==", + "dev": true, + "license": "MIT" + }, "node_modules/shebang-command": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", @@ -9722,6 +9774,198 @@ "dev": true, "license": "MIT" }, + "node_modules/solidity-comments": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments/-/solidity-comments-0.0.2.tgz", + "integrity": "sha512-G+aK6qtyUfkn1guS8uzqUeua1dURwPlcOjoTYW/TwmXAcE7z/1+oGCfZUdMSe4ZMKklNbVZNiG5ibnF8gkkFfw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">= 12" + }, + "optionalDependencies": { + "solidity-comments-darwin-arm64": "0.0.2", + "solidity-comments-darwin-x64": "0.0.2", + "solidity-comments-freebsd-x64": "0.0.2", + "solidity-comments-linux-arm64-gnu": "0.0.2", + "solidity-comments-linux-arm64-musl": "0.0.2", + "solidity-comments-linux-x64-gnu": "0.0.2", + "solidity-comments-linux-x64-musl": "0.0.2", + "solidity-comments-win32-arm64-msvc": "0.0.2", + "solidity-comments-win32-ia32-msvc": "0.0.2", + "solidity-comments-win32-x64-msvc": "0.0.2" + } + }, + "node_modules/solidity-comments-darwin-arm64": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-darwin-arm64/-/solidity-comments-darwin-arm64-0.0.2.tgz", + "integrity": "sha512-HidWkVLSh7v+Vu0CA7oI21GWP/ZY7ro8g8OmIxE8oTqyMwgMbE8F1yc58Sj682Hj199HCZsjmtn1BE4PCbLiGA==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-darwin-x64": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-darwin-x64/-/solidity-comments-darwin-x64-0.0.2.tgz", + "integrity": "sha512-Zjs0Ruz6faBTPT6fBecUt6qh4CdloT8Bwoc0+qxRoTn9UhYscmbPQkUgQEbS0FQPysYqVzzxJB4h1Ofbf4wwtA==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-freebsd-x64": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-freebsd-x64/-/solidity-comments-freebsd-x64-0.0.2.tgz", + "integrity": "sha512-8Qe4mpjuAxFSwZJVk7B8gAoLCdbtS412bQzBwk63L8dmlHogvE39iT70aAk3RHUddAppT5RMBunlPUCFYJ3ZTw==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-linux-arm64-gnu": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-linux-arm64-gnu/-/solidity-comments-linux-arm64-gnu-0.0.2.tgz", + "integrity": "sha512-spkb0MZZnmrP+Wtq4UxP+nyPAVRe82idOjqndolcNR0S9Xvu4ebwq+LvF4HiUgjTDmeiqYiFZQ8T9KGdLSIoIg==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-linux-arm64-musl": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-linux-arm64-musl/-/solidity-comments-linux-arm64-musl-0.0.2.tgz", + "integrity": "sha512-guCDbHArcjE+JDXYkxx5RZzY1YF6OnAKCo+sTC5fstyW/KGKaQJNPyBNWuwYsQiaEHpvhW1ha537IvlGek8GqA==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-linux-x64-gnu": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-linux-x64-gnu/-/solidity-comments-linux-x64-gnu-0.0.2.tgz", + "integrity": "sha512-zIqLehBK/g7tvrFmQljrfZXfkEeLt2v6wbe+uFu6kH/qAHZa7ybt8Vc0wYcmjo2U0PeBm15d79ee3AkwbIjFdQ==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-linux-x64-musl": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-linux-x64-musl/-/solidity-comments-linux-x64-musl-0.0.2.tgz", + "integrity": "sha512-R9FeDloVlFGTaVkOlELDVC7+1Tjx5WBPI5L8r0AGOPHK3+jOcRh6sKYpI+VskSPDc3vOO46INkpDgUXrKydlIw==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-win32-arm64-msvc": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-win32-arm64-msvc/-/solidity-comments-win32-arm64-msvc-0.0.2.tgz", + "integrity": "sha512-QnWJoCQcJj+rnutULOihN9bixOtYWDdF5Rfz9fpHejL1BtNjdLW1om55XNVHGAHPqBxV4aeQQ6OirKnp9zKsug==", + "cpu": [ + "arm64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-win32-ia32-msvc": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-win32-ia32-msvc/-/solidity-comments-win32-ia32-msvc-0.0.2.tgz", + "integrity": "sha512-vUg4nADtm/NcOtlIymG23NWJUSuMsvX15nU7ynhGBsdKtt8xhdP3C/zA6vjDk8Jg+FXGQL6IHVQ++g/7rSQi0w==", + "cpu": [ + "ia32" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/solidity-comments-win32-x64-msvc": { + "version": "0.0.2", + "resolved": "https://registry.npmjs.org/solidity-comments-win32-x64-msvc/-/solidity-comments-win32-x64-msvc-0.0.2.tgz", + "integrity": "sha512-36j+KUF4V/y0t3qatHm/LF5sCUCBx2UndxE1kq5bOzh/s+nQgatuyB+Pd5BfuPQHdWu2KaExYe20FlAa6NL7+Q==", + "cpu": [ + "x64" + ], + "dev": true, + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, "node_modules/solidity-coverage": { "version": "0.8.16", "resolved": "https://registry.npmjs.org/solidity-coverage/-/solidity-coverage-0.8.16.tgz", @@ -10648,7 +10892,6 @@ "dev": true, "license": "BSD-2-Clause", "optional": true, - "peer": true, "bin": { "uglifyjs": "bin/uglifyjs" }, @@ -11055,8 +11298,7 @@ "resolved": "https://registry.npmjs.org/wordwrap/-/wordwrap-1.0.0.tgz", "integrity": "sha512-gvVzJFlPycKc5dZN4yPkP8w7Dc37BtP1yczEneOb4uq34pXZcvrtRTmWV8W+Ume+XCxKgbjM+nevkyFPMybd4Q==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/wordwrapjs": { "version": "4.0.1", diff --git a/package.json b/package.json index b62695a..5e5593b 100644 --- a/package.json +++ b/package.json @@ -6,13 +6,16 @@ "solhint": "solhint -w0 'contracts/**/*.sol'" }, "devDependencies": { + "@ensuro/utils": "^0.2.9", "@nomicfoundation/hardhat-toolbox": "^6.1.0", "@openzeppelin/hardhat-upgrades": "^3.9.1", "eslint": "^9.32.0", "eslint-config-prettier": "^10.1.8", + "handlebars": "^4.7.8", "hardhat": "^2.26.2", "hardhat-contract-sizer": "^2.10.0", "hardhat-dependency-compiler": "^1.2.1", + "hardhat-ignore-warnings": "^0.2.12", "prettier": "^3.6.2", "solhint": "^6.0.0", "solhint-plugin-prettier": "^0.1.0" diff --git a/scripts/generateAMPSkips.js b/scripts/generateAMPSkips.js new file mode 100644 index 0000000..a078de2 --- /dev/null +++ b/scripts/generateAMPSkips.js @@ -0,0 +1,26 @@ +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 f3ff092..49dba74 100755 --- a/scripts/make-npm-package.sh +++ b/scripts/make-npm-package.sh @@ -6,23 +6,27 @@ VERSION=$1 shift if [ -z $VERSION ]; then - echo "Usage: $0 []" >&2 - exit 13 + echo "Usage: $0 []" >&2 + exit 13 fi TARGET_DIR=$1 if [ -z $TARGET_DIR ]; then - TARGET_DIR=./build/npm-package/ + TARGET_DIR=./build/npm-package/ fi rm -fr $TARGET_DIR 2>/dev/null mkdir -p $TARGET_DIR +# Generate 1 to 24 skip method contracts +node scripts/generateAMPSkips.js $(seq -s" " 1 24) + npx hardhat clean env COMPILE_MODE=production npx hardhat compile git archive --format tar HEAD README.md contracts/ | tar xv -C $TARGET_DIR +cp -r contracts/amps $TARGET_DIR/contracts # rm -fR $TARGET_DIR/contracts/mocks/ mkdir $TARGET_DIR/build @@ -31,7 +35,7 @@ cp -r artifacts/contracts $TARGET_DIR/build cp artifacts/build-info/*.json $TARGET_DIR/build/build-info.json find $TARGET_DIR -name "*.dbg.json" -delete -sed "s/%%VERSION%%/$VERSION/" npm-package/package.json > "$TARGET_DIR/package.json" +sed "s/%%VERSION%%/$VERSION/" npm-package/package.json >"$TARGET_DIR/package.json" find $TARGET_DIR echo " diff --git a/templates/AccessManagedProxySXTemplate.sol.handlebars b/templates/AccessManagedProxySXTemplate.sol.handlebars new file mode 100644 index 0000000..33a923b --- /dev/null +++ b/templates/AccessManagedProxySXTemplate.sol.handlebars @@ -0,0 +1,71 @@ +// 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 21b8fed..8434538 100644 --- a/test/test-access-managed-proxy.js +++ b/test/test-access-managed-proxy.js @@ -2,64 +2,218 @@ const { expect } = require("chai"); const hre = require("hardhat"); const { ethers } = hre; -const { ZeroAddress } = ethers; +const { tagitVariant, setupAMRole } = require("@ensuro/utils/js/utils"); const helpers = require("@nomicfoundation/hardhat-network-helpers"); +const { deployAMPProxy } = require("../js/deployProxy"); const { deploy: ozUpgradesDeploy } = require("@openzeppelin/hardhat-upgrades/dist/utils"); -async function setUp() { +async function setUpCommon() { const [admin, anon] = await ethers.getSigners(); const DummyImplementation = await ethers.getContractFactory("DummyImplementation"); const AccessManager = await ethers.getContractFactory("AccessManager"); + + return { + admin, + anon, + AccessManager, + DummyImplementation, + }; +} + +async function setUpAMP() { + const ret = await setUpCommon(); const AccessManagedProxy = await ethers.getContractFactory("AccessManagedProxy"); + const { admin, AccessManager, DummyImplementation } = ret; + const acMgr = await AccessManager.deploy(admin); + + async function deployProxy() { + return deployAMPProxy(DummyImplementation, [], { acMgr }); + } + + return { + acMgr, + AccessManagedProxy, + deployProxy, + ...ret, + }; +} + +function randomSelector() { + return ( + "0x" + + Math.floor(Math.random() * 2 ** 32) + .toString(16) + .padStart(8, "0") + ); +} + +async function setUpAMPSkip(nMethods, skipViewsAndPure = false) { + const ret = await setUpCommon(); + const { admin, AccessManager, DummyImplementation } = ret; + const AccessManagedProxySkip = await ethers.getContractFactory(`AccessManagedProxyS${nMethods}`); const acMgr = await AccessManager.deploy(admin); + 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 }); + } + + return { + skipSelectors: selectors, + acMgr, + AccessManagedProxy: AccessManagedProxySkip, + deployProxy, + ...ret, + }; +} - async function deployProxy(implContract = DummyImplementation) { +async function setUpDirect() { + const ret = await setUpCommon(); + + async function deployProxy(implContract = ret.DummyImplementation) { + return implContract.deploy(); + } + + return { + acMgr: null, + deployProxy, + ...ret, + }; +} + +async function setUpERC1967() { + const ret = await setUpCommon(); + + async function deployProxy(implContract = ret.DummyImplementation) { return hre.upgrades.deployProxy(implContract, [], { kind: "uups", - unsafeAllow: [ - "delegatecall", - "missing-initializer-call", // This is to fix an error because it says we are not calling - // parent initializer - ], - proxyFactory: AccessManagedProxy, - deployFunction: async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr), }); } return { - admin, - anon, - AccessManager, - AccessManagedProxy, - acMgr, - DummyImplementation, + acMgr: null, deployProxy, + ...ret, }; } -describe("AccessManagedProxy tests", function () { - it("Checks it can deploy the proxy", async () => { - const { acMgr, deployProxy, AccessManagedProxy } = await helpers.loadFixture(setUp); - const dummy = await deployProxy(); - const dummyAsAMP = AccessManagedProxy.attach(dummy); - expect(await dummyAsAMP.ACCESS_MANAGER()).to.equal(acMgr); - }); +// 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: "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: "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, + }, +]; + +variants.forEach((variant) => { + // eslint-disable-next-line func-style + const it = (testDescription, test) => tagitVariant(variant, false, testDescription, test); + it.only = (testDescription, test) => tagitVariant(variant, true, testDescription, test); + + describe(`AccessManagedProxy tests - variant: ${variant.name}`, function () { + it("Checks it can deploy the proxy [?hasAC]", async () => { + const { acMgr, deployProxy, AccessManagedProxy } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const dummyAsAMP = AccessManagedProxy.attach(dummy); + expect(await dummyAsAMP.ACCESS_MANAGER()).to.equal(acMgr); + }); + + it("Checks methods can be called with the right permissions [?hasAC]", async () => { + const { acMgr, deployProxy, AccessManagedProxy, anon, admin, DummyImplementation } = await helpers.loadFixture( + variant.fixture + ); + const dummy = await deployProxy(); + const dummyAsAMP = AccessManagedProxy.attach(dummy); + await expect(dummy.connect(anon)[variant.method]()) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(anon); + const methodSelector = DummyImplementation.interface.getFunction(variant.method).selector; + // Call from admin works fine + await expect(dummy.connect(admin)[variant.method]()).to.emit(dummy, "MethodCalled").withArgs(methodSelector); + + // Grant the permission and now anon can call + await setupAMRole(acMgr, dummy, { DUMMY: 123 }, "DUMMY", [variant.method]); + await acMgr.grantRole(123, anon, 0); + await expect(dummy.connect(anon)[variant.method]()).to.emit(dummy, "MethodCalled").withArgs(methodSelector); + }); - it("Checks methods can be called with the right permissions", async () => { - const { acMgr, deployProxy, AccessManagedProxy, anon, admin, DummyImplementation } = - await helpers.loadFixture(setUp); - const dummy = await deployProxy(); - const dummyAsAMP = AccessManagedProxy.attach(dummy); - await expect(dummy.connect(anon).method1()) - .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") - .withArgs(anon); - const method1Selector = DummyImplementation.interface.getFunction("method1").selector; - // Call from admin works fine - await expect(dummy.connect(admin).method1()).to.emit(dummy, "MethodCalled").withArgs(method1Selector); - - // Grant the permission and now anon can call - await acMgr.setTargetFunctionRole(dummy, [method1Selector], 123); - await acMgr.grantRole(123, anon, 0); - await expect(dummy.connect(anon).method1()).to.emit(dummy, "MethodCalled").withArgs(method1Selector); + it("Checks calling the method emits the log", async () => { + const { deployProxy, admin, DummyImplementation } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const methodSelector = DummyImplementation.interface.getFunction(variant.method).selector; + await expect(dummy.connect(admin)[variant.method]()).to.emit(dummy, "MethodCalled").withArgs(methodSelector); + }); + + it("Checks calling the skipped method with anon works [?hasSkippedMethod]", async () => { + const { deployProxy, anon, DummyImplementation } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + const methodSelector = DummyImplementation.interface.getFunction("callThruAMPSkippedMethod").selector; + await expect(dummy.connect(anon)["callThruAMPSkippedMethod"]()) + .to.emit(dummy, "MethodCalled") + .withArgs(methodSelector); + }); + + it("Checks skipped methods are observable calling PASS_THRU_METHODS [?hasSkippedMethod]", async () => { + const { deployProxy, skipSelectors, AccessManagedProxy, DummyImplementation } = await helpers.loadFixture( + variant.fixture + ); + const dummy = await deployProxy(); + const dummyAsAMP = AccessManagedProxy.attach(dummy); + if (variant.hasViews) { + const expectedSelectors = ["UPGRADE_INTERFACE_VERSION", "proxiableUUID", "pureMethod", "viewMethod"] + .map((methodName) => DummyImplementation.interface.getFunction(methodName).selector) + .concat(skipSelectors); + expect(await dummyAsAMP.PASS_THRU_METHODS()).to.deep.equal(expectedSelectors); + } else { + expect(await dummyAsAMP.PASS_THRU_METHODS()).to.deep.equal(skipSelectors); + } + }); + + it("Checks access to views [?hasAC]", async () => { + const { deployProxy, anon, AccessManagedProxy } = await helpers.loadFixture(variant.fixture); + const dummy = await deployProxy(); + if (variant.hasViews) { + expect(await dummy.connect(anon).viewMethod()).to.equal(anon); + expect(await dummy.connect(anon).pureMethod()).to.equal(123456n); + } else { + const dummyAsAMP = AccessManagedProxy.attach(dummy); + await expect(dummy.connect(anon).viewMethod()) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(anon); + await expect(dummy.connect(anon).pureMethod()) + .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") + .withArgs(anon); + } + }); }); });