From 9b5796c8284e620a665fbfcca3dcd2f41d2aab1c Mon Sep 17 00:00:00 2001 From: Benjamin Smith Date: Wed, 3 Feb 2021 15:30:01 +0100 Subject: [PATCH] Upgradable Authenticator (#351) Closes #167 This PR introduces minor changes to the `AllowListAuthenticator` making it upgradable and includes tests verifying upgradability. In particular: - removing `customInitiallyOwnable` as it is no longer used: Not yet sure how this will affect the `deployments` directory which still includes it. - Updating authenticator deployment script to deploy as proxy. - rename authenticator `owner` to `manager` because of name collision with proxy owner - introduce proxy.ts with helper methods to fetch implementation and owner address. - Include two unit tests: one verifying that upgrade is possible and the other to ensure preservation of storage. Note that the unit tests involving Authenticator's non-upgrade related functionality do not use the proxy deployment as specified in the migration scripts, so that manager must be set immediately after deployment. We had originally planned/hoped to use the `hardhat-upgrades` plugin and opened the following two PRs to `openzeppelin-upgrades`, but this plugin turned out to be unnecessary. - Custom Deployments: https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/273 - Manual Safety Override: https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/280 ### Test Plan Old + new unit and e2e tests. --- src/contracts/GPv2AllowListAuthentication.sol | 24 ++++--- .../ownership/CustomInitiallyOwnable.sol | 21 ------ .../test/GPv2AllowListAuthenticationV2.sol | 10 +++ src/deploy/001_authenticator.ts | 7 +- src/ts/deploy.ts | 2 +- src/ts/index.ts | 1 + src/ts/proxy.ts | 45 ++++++++++++ test/AllowListStorageReader.test.ts | 3 +- test/GPv2AllowListAuthenticator.test.ts | 27 +++++-- test/GPv2Settlement.test.ts | 3 +- test/e2e/deployment.test.ts | 11 +-- test/e2e/upgradeAuthenticator.test.ts | 72 +++++++++++++++++++ 12 files changed, 178 insertions(+), 48 deletions(-) delete mode 100644 src/contracts/ownership/CustomInitiallyOwnable.sol create mode 100644 src/contracts/test/GPv2AllowListAuthenticationV2.sol create mode 100644 src/ts/proxy.ts create mode 100644 test/e2e/upgradeAuthenticator.test.ts diff --git a/src/contracts/GPv2AllowListAuthentication.sol b/src/contracts/GPv2AllowListAuthentication.sol index 783063a7..4e88244d 100644 --- a/src/contracts/GPv2AllowListAuthentication.sol +++ b/src/contracts/GPv2AllowListAuthentication.sol @@ -4,27 +4,31 @@ pragma solidity ^0.7.6; import "@gnosis.pm/util-contracts/contracts/StorageAccessible.sol"; import "@openzeppelin/contracts/utils/EnumerableSet.sol"; import "./interfaces/GPv2Authentication.sol"; -import "./ownership/CustomInitiallyOwnable.sol"; /// @title Gnosis Protocol v2 Access Control Contract /// @author Gnosis Developers -contract GPv2AllowListAuthentication is - CustomInitiallyOwnable, - GPv2Authentication, - StorageAccessible -{ +contract GPv2AllowListAuthentication is GPv2Authentication, StorageAccessible { using EnumerableSet for EnumerableSet.AddressSet; + address public manager; + EnumerableSet.AddressSet private solvers; - // solhint-disable-next-line no-empty-blocks - constructor(address initialOwner) CustomInitiallyOwnable(initialOwner) {} + function initializeManager(address manager_) external { + require(manager == address(0), "GPv2: already initialized"); + manager = manager_; + } + + modifier onlyManager() { + require(manager == msg.sender, "GPv2: caller not manager"); + _; + } - function addSolver(address solver) external onlyOwner { + function addSolver(address solver) external onlyManager { solvers.add(solver); } - function removeSolver(address solver) external onlyOwner { + function removeSolver(address solver) external onlyManager { solvers.remove(solver); } diff --git a/src/contracts/ownership/CustomInitiallyOwnable.sol b/src/contracts/ownership/CustomInitiallyOwnable.sol deleted file mode 100644 index 751a9a4c..00000000 --- a/src/contracts/ownership/CustomInitiallyOwnable.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-or-later -pragma solidity ^0.7.6; - -import "@openzeppelin/contracts/access/Ownable.sol"; - -/** - * @title Ownable contract with custom intial owner - * @author Gnosis Developers - * @dev A contract extending Openzeppelin's Ownable contract that allows to - * specify the initial owner in the contructor instead of using the message - * sender. - */ -abstract contract CustomInitiallyOwnable is Ownable { - /** - * @dev Initializes the contract setting the input address as the initial - * owner. - */ - constructor(address initialOwner) { - transferOwnership(initialOwner); - } -} diff --git a/src/contracts/test/GPv2AllowListAuthenticationV2.sol b/src/contracts/test/GPv2AllowListAuthenticationV2.sol new file mode 100644 index 00000000..aad8921a --- /dev/null +++ b/src/contracts/test/GPv2AllowListAuthenticationV2.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.7.6; + +import "../GPv2AllowListAuthentication.sol"; + +contract GPv2AllowListAuthenticationV2 is GPv2AllowListAuthentication { + function newMethod() external pure returns (uint256) { + return 1337; + } +} diff --git a/src/deploy/001_authenticator.ts b/src/deploy/001_authenticator.ts index bb330a2f..7b0a299d 100644 --- a/src/deploy/001_authenticator.ts +++ b/src/deploy/001_authenticator.ts @@ -11,13 +11,16 @@ const deployAuthenticator: DeployFunction = async function ({ const { deploy } = deployments; const { authenticator } = CONTRACT_NAMES; - await deploy(authenticator, { from: deployer, gasLimit: 2000000, - args: [owner], deterministicDeployment: SALT, log: true, + proxy: { + owner, + methodName: "initializeManager", + }, + args: [owner], }); }; diff --git a/src/ts/deploy.ts b/src/ts/deploy.ts index f715f7d8..9afd436a 100644 --- a/src/ts/deploy.ts +++ b/src/ts/deploy.ts @@ -34,7 +34,7 @@ export type ContractName = typeof CONTRACT_NAMES[keyof typeof CONTRACT_NAMES]; export type DeploymentArguments< T extends ContractName > = T extends typeof CONTRACT_NAMES.authenticator - ? [string] + ? never : T extends typeof CONTRACT_NAMES.settlement ? [string] : unknown[]; diff --git a/src/ts/index.ts b/src/ts/index.ts index 48d8a6b5..5f4cbd86 100644 --- a/src/ts/index.ts +++ b/src/ts/index.ts @@ -25,4 +25,5 @@ export * from "./settlement"; export * from "./reader"; export * from "./deploy"; export * from "./sign"; +export * from "./proxy"; export * from "./types/ethers"; diff --git a/src/ts/proxy.ts b/src/ts/proxy.ts new file mode 100644 index 00000000..f2545776 --- /dev/null +++ b/src/ts/proxy.ts @@ -0,0 +1,45 @@ +// defined in https://eips.ethereum.org/EIPS/eip-1967 + +import { BigNumber } from "ethers"; +import { ethers } from "hardhat"; + +// The proxy contract used by hardhat-deploy implements EIP-1967 (Standard Proxy +// Storage Slot). See . +function slot(string: string) { + return ethers.utils.defaultAbiCoder.encode( + ["bytes32"], + [BigNumber.from(ethers.utils.id(string)).sub(1)], + ); +} +const IMPLEMENTATION_STORAGE_SLOT = slot("eip1967.proxy.implementation"); +const OWNER_STORAGE_SLOT = slot("eip1967.proxy.admin"); + +/** + * Returns the address of the implementation of an EIP-1967-compatible proxy + * from its address. + * + * @param proxy Address of the proxy contract. + * @returns The address of the contract storing the proxy implementation. + */ +export async function implementationAddress(proxy: string): Promise { + const [implementation] = await ethers.utils.defaultAbiCoder.decode( + ["address"], + await ethers.provider.getStorageAt(proxy, IMPLEMENTATION_STORAGE_SLOT), + ); + return implementation; +} + +/** + * Returns the address of the implementation of an EIP-1967-compatible proxy + * from its address. + * + * @param proxy Address of the proxy contract. + * @returns The address of the administrator of the proxy. + */ +export async function ownerAddress(proxy: string): Promise { + const [owner] = await ethers.utils.defaultAbiCoder.decode( + ["address"], + await ethers.provider.getStorageAt(proxy, OWNER_STORAGE_SLOT), + ); + return owner; +} diff --git a/test/AllowListStorageReader.test.ts b/test/AllowListStorageReader.test.ts index 61d02e10..a30db89d 100644 --- a/test/AllowListStorageReader.test.ts +++ b/test/AllowListStorageReader.test.ts @@ -21,7 +21,8 @@ describe("AllowListStorageReader", () => { ); reader = await AllowListStorageReader.deploy(); - authenticator = await GPv2AllowListAuthentication.deploy(owner.address); + authenticator = await GPv2AllowListAuthentication.deploy(); + await authenticator.initializeManager(owner.address); allowListReader = new AllowListReader(authenticator, reader); }); diff --git a/test/GPv2AllowListAuthenticator.test.ts b/test/GPv2AllowListAuthenticator.test.ts index 910243be..84e8af96 100644 --- a/test/GPv2AllowListAuthenticator.test.ts +++ b/test/GPv2AllowListAuthenticator.test.ts @@ -12,15 +12,28 @@ describe("GPv2AllowListAuthentication", () => { deployer, ); - authenticator = await GPv2AllowListAuthentication.deploy(owner.address); + authenticator = await GPv2AllowListAuthentication.deploy(); + await authenticator.initializeManager(owner.address); }); describe("constructor", () => { - it("should set the owner", async () => { - expect(await authenticator.owner()).to.equal(owner.address); + it("should initialize the manager", async () => { + expect(await authenticator.manager()).to.equal(owner.address); }); - it("deployer is not the owner", async () => { - expect(await authenticator.owner()).not.to.be.equal(deployer.address); + + it("ensures initializeManager is idempotent", async () => { + await expect( + authenticator.initializeManager(nonOwner.address), + ).to.revertedWith("GPv2: already initialized"); + + // Also reverts when called by owner. + await expect( + authenticator.connect(owner).initializeManager(nonOwner.address), + ).to.revertedWith("GPv2: already initialized"); + }); + + it("deployer is not the manager", async () => { + expect(await authenticator.manager()).not.to.be.equal(deployer.address); }); }); @@ -33,7 +46,7 @@ describe("GPv2AllowListAuthentication", () => { it("should not allow non-owner to add solver", async () => { await expect( authenticator.connect(nonOwner).addSolver(solver.address), - ).to.be.revertedWith("caller is not the owner"); + ).to.be.revertedWith("GPv2: caller not manager"); }); }); @@ -46,7 +59,7 @@ describe("GPv2AllowListAuthentication", () => { it("should not allow non-owner to remove solver", async () => { await expect( authenticator.connect(nonOwner).removeSolver(solver.address), - ).to.be.revertedWith("caller is not the owner"); + ).to.be.revertedWith("GPv2: caller not manager"); }); }); diff --git a/test/GPv2Settlement.test.ts b/test/GPv2Settlement.test.ts index e9f0bbf9..bef86c11 100644 --- a/test/GPv2Settlement.test.ts +++ b/test/GPv2Settlement.test.ts @@ -40,7 +40,8 @@ describe("GPv2Settlement", () => { "GPv2AllowListAuthentication", deployer, ); - authenticator = await GPv2AllowListAuthentication.deploy(owner.address); + authenticator = await GPv2AllowListAuthentication.deploy(); + await authenticator.initializeManager(owner.address); const GPv2Settlement = await ethers.getContractFactory( "GPv2SettlementTestInterface", diff --git a/test/e2e/deployment.test.ts b/test/e2e/deployment.test.ts index 3d5f3b77..0d0cb481 100644 --- a/test/e2e/deployment.test.ts +++ b/test/e2e/deployment.test.ts @@ -6,6 +6,7 @@ import { ContractName, DeploymentArguments, deterministicDeploymentAddress, + implementationAddress, } from "../../src/ts"; import { builtAndDeployedMetadataCoincide } from "../bytecode"; @@ -45,7 +46,7 @@ describe("E2E: Deployment", () => { it("authenticator", async () => { expect( await builtAndDeployedMetadataCoincide( - authenticator.address, + await implementationAddress(authenticator.address), "GPv2AllowListAuthentication", ), ).to.be.true; @@ -72,9 +73,9 @@ describe("E2E: Deployment", () => { describe("deterministic addresses", () => { it("authenticator", async () => { - expect( - await contractAddress("GPv2AllowListAuthentication", owner.address), - ).to.equal(authenticator.address); + expect(await contractAddress("GPv2AllowListAuthentication")).to.equal( + await implementationAddress(authenticator.address), + ); }); it("settlement", async () => { @@ -86,7 +87,7 @@ describe("E2E: Deployment", () => { describe("ownership", () => { it("authenticator has dedicated owner", async () => { - expect(await authenticator.owner()).to.equal(owner.address); + expect(await authenticator.manager()).to.equal(owner.address); }); }); }); diff --git a/test/e2e/upgradeAuthenticator.test.ts b/test/e2e/upgradeAuthenticator.test.ts new file mode 100644 index 00000000..e8f2a323 --- /dev/null +++ b/test/e2e/upgradeAuthenticator.test.ts @@ -0,0 +1,72 @@ +import { expect } from "chai"; +import { Contract, Wallet } from "ethers"; +import { deployments, ethers } from "hardhat"; + +import { deployTestContracts } from "./fixture"; + +describe("Upgrade Authenticator", () => { + let authenticator: Contract; + let deployer: Wallet; + let owner: Wallet; + let solver: Wallet; + + beforeEach(async () => { + ({ + authenticator, + deployer, + owner, + wallets: [solver], + } = await deployTestContracts()); + }); + + it("should upgrade authenticator", async () => { + const GPv2AllowListAuthenticationV2 = await ethers.getContractFactory( + "GPv2AllowListAuthenticationV2", + deployer, + ); + // Note that, before the upgrade this is actually the old instance + const authenticatorV2 = GPv2AllowListAuthenticationV2.attach( + authenticator.address, + ); + // This method doesn't exist before upgrade + await expect(authenticatorV2.newMethod()).to.be.reverted; + + await upgrade( + "GPv2AllowListAuthentication", + "GPv2AllowListAuthenticationV2", + ); + // This method should exist on after upgrade + expect(await authenticatorV2.newMethod()).to.equal(1337); + }); + + it("should preserve storage", async () => { + await authenticator.connect(owner).addSolver(solver.address); + + // Upgrade after storage is set. + await upgrade( + "GPv2AllowListAuthentication", + "GPv2AllowListAuthenticationV2", + ); + + const GPv2AllowListAuthenticationV2 = await ethers.getContractFactory( + "GPv2AllowListAuthenticationV2", + deployer, + ); + const authenticatorV2 = GPv2AllowListAuthenticationV2.attach( + authenticator.address, + ); + // Both, the listed solvers and original manager are still set + expect(await authenticatorV2.isSolver(solver.address)).to.equal(true); + expect(await authenticatorV2.manager()).to.equal(owner.address); + }); + + async function upgrade(contractName: string, newContractName: string) { + // Note that deterministic deployment and gasLimit are not needed/used here as deployment args. + await deployments.deploy(contractName, { + contract: newContractName, + // From differs from initial deployment here since the proxy owner is the Authenticator manager. + from: owner.address, + proxy: true, + }); + } +});