diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 909593c..3a74e4b 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -15,11 +15,16 @@ jobs: node-version: "22" cache: "npm" - run: npm ci - - run: node scripts/generateAMPSkips.js 1 5 10 24 + - 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/.solcover.js b/.solcover.js index ef4435f..f965fa7 100644 --- a/.solcover.js +++ b/.solcover.js @@ -4,5 +4,9 @@ module.exports = { // instrumentedDir: path.join(process.cwd(), "instrumented"), // client: client, skipFiles: ["dependencies/", "mock/"], + mocha: { + grep: "AccessManagedProxyS40", // Find everything with this tag + invert: true, // Run the grep's inverse set. + }, // logger: console, }; diff --git a/README.md b/README.md index ed13369..bd89512 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,44 @@ This repository contains the smart contract AccessManagedProxy, a proxy with built-in access control, delegating the permissions to an OpenZeppelin's 5.x AccessManager. -See https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917/3 for a discussion on the advantages of -using it. +## 🧐 Motivation + +OZ 5.x introduces the AccessManager contract and AccessManaged base contract to move the access control decisions +from code to configuration. This is a step forward compared to the 4.x AccessControl authorization framework, where +you had to make decisions in the contract mapping methods to role names. + +But the AccessManaged approach falls short, because you still have to make the decision (at coding time) of which +methods to decorate with the restricted modifier. + +Implementing the access control with a modifier in the methods affects the observability of the access control +configuration and its formal verification. + +It also increases the test effort and makes it harder to achieve 100% branch coverage in the contracts. Finally, +this has an effect on the contract size of the implementation contracts. + +The widespread use of proxy contracts (mainly for upgradeable contracts) gives us the opportunity to move the +implementation of the access control delegation logic to the AccessManager contract (that is, in the end, what the +restricted modifier does) to the proxy contract. + +In this way, BEFORE doing the delegatecall to the implementation contract, we will check if the call is enabled by +calling ACCESS_MANAGER.canCall(msg.sender, address(this), selector). + +For gas-optimization or other reasons, we can define a list of methods (probably the views) that will be excluded +from calling the AccessManager, reducing the overhead for non-restricted method calls to the minimum. + +Another advantage of this approach is the run-time observability of the access control configuration, by checking if +a given method is included in those that skip the access control, or otherwise, we will check the access manager +configuration for that method. + +More details on the motivation of this idea here: https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 + +## 📝 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. + +## Development Try running some of the following tasks: diff --git a/contracts/AMPUtils.sol b/contracts/AMPUtils.sol new file mode 100644 index 0000000..59e1450 --- /dev/null +++ b/contracts/AMPUtils.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {AccessManagedProxy} from "./AccessManagedProxy.sol"; + +/** + * @title AMPUtils + * @dev Utility functions for doing custom access control rules, for contracts deployed + * with AccessManagedProxy + * @author Ensuro + */ +library AMPUtils { + // Error copied from IAccessManaged + error AccessManagedUnauthorized(address caller); + + /** + * @dev Checks if the user can call a particular selector, assuming the calling contract was deployed as an AMP. + * + * @param user The user for which you want to check the access, typically msg.sender + * @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( + user, + address(this), + selector + ); + require(immediate, AccessManagedUnauthorized(user)); + } + + /** + * @dev Standard way of creating "fake selectors" (not necessarily tied to a method call) for specific permissions + */ + function makeSelector(bytes memory something) internal pure returns (bytes4) { + return bytes4(keccak256(something)); + } +} diff --git a/js/deployProxy.js b/js/deployProxy.js index dfdd640..e931ef2 100644 --- a/js/deployProxy.js +++ b/js/deployProxy.js @@ -44,7 +44,7 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr); } - return hre.upgrades.deployProxy(contractFactory, [], { + return hre.upgrades.deployProxy(contractFactory, initializeArgs, { ...opts, kind: "uups", proxyFactory, @@ -52,6 +52,24 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) { }); } +async function attachAsAMP(contract, ampContractFactory = undefined) { + ampContractFactory = ampContractFactory || (await ethers.getContractFactory("AccessManagedProxyS1")); + return ampContractFactory.attach(contract); +} + +async function getAccessManager(contract, ampContractFactory = undefined, accessManagerFactory="AccessManager") { + const contractAsAMP = await attachAsAMP(contract, ampContractFactory); + return ethers.getContractAt(accessManagerFactory, await contractAsAMP.ACCESS_MANAGER()); +} + +function makeSelector(role) { + return ethers.keccak256(ethers.toUtf8Bytes(role)).slice(0, 10); +} + + module.exports = { deployAMPProxy, + attachAsAMP, + getAccessManager, + makeSelector, }; diff --git a/scripts/make-npm-package.sh b/scripts/make-npm-package.sh index 49dba74..835a96e 100755 --- a/scripts/make-npm-package.sh +++ b/scripts/make-npm-package.sh @@ -19,13 +19,13 @@ 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) +# 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 -git archive --format tar HEAD README.md contracts/ | tar xv -C $TARGET_DIR +git archive --format tar HEAD README.md contracts/ js/ | tar xv -C $TARGET_DIR cp -r contracts/amps $TARGET_DIR/contracts # rm -fR $TARGET_DIR/contracts/mocks/ diff --git a/test/test-access-managed-proxy.js b/test/test-access-managed-proxy.js index 8434538..7b5da56 100644 --- a/test/test-access-managed-proxy.js +++ b/test/test-access-managed-proxy.js @@ -4,8 +4,7 @@ const hre = require("hardhat"); const { ethers } = hre; 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"); +const { deployAMPProxy, attachAsAMP } = require("../js/deployProxy"); async function setUpCommon() { const [admin, anon] = await ethers.getSigners(); @@ -118,6 +117,13 @@ const variants = [ hasAC: true, hasSkippedMethod: true, }, + { + name: "AccessManagedProxyS40", + fixture: async () => setUpAMPSkip(40), + method: "callThruAMPNonSkippedMethod", + hasAC: true, + hasSkippedMethod: true, + }, { name: "AccessManagedProxyS1", fixture: async () => setUpAMPSkip(1), @@ -206,7 +212,7 @@ variants.forEach((variant) => { expect(await dummy.connect(anon).viewMethod()).to.equal(anon); expect(await dummy.connect(anon).pureMethod()).to.equal(123456n); } else { - const dummyAsAMP = AccessManagedProxy.attach(dummy); + const dummyAsAMP = await attachAsAMP(dummy); await expect(dummy.connect(anon).viewMethod()) .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized") .withArgs(anon);