Upgradable Authenticator (gnosis/gp-v2-contracts#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: OpenZeppelin/openzeppelin-upgrades#273
- Manual Safety Override: OpenZeppelin/openzeppelin-upgrades#280

### Test Plan

Old + new unit and e2e tests.

Author: LunaTechLab56

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);
     }
 

src/contracts/ownership/CustomInitiallyOwnable.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;
+    }
+}

src/contracts/test/GPv2AllowListAuthenticationV2.sol

@@ -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],
   });
 };
 

src/deploy/001_authenticator.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[];

src/ts/deploy.ts

@@ -25,4 +25,5 @@ export * from "./settlement";
 export * from "./reader";
 export * from "./deploy";
 export * from "./sign";
+export * from "./proxy";
 export * from "./types/ethers";

src/ts/index.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 <https://eips.ethereum.org/EIPS/eip-1967>.
+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<string> {
+  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<string> {
+  const [owner] = await ethers.utils.defaultAbiCoder.decode(
+    ["address"],
+    await ethers.provider.getStorageAt(proxy, OWNER_STORAGE_SLOT),
+  );
+  return owner;
+}

src/ts/proxy.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);
   });
 

test/AllowListStorageReader.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");
     });
   });
 

test/GPv2AllowListAuthenticator.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",