Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

Commit

Permalink
Upgradable Authenticator (#351)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bh2smith authored Feb 3, 2021
1 parent 2598df9 commit 9b5796c
Show file tree
Hide file tree
Showing 12 changed files with 178 additions and 48 deletions.
24 changes: 14 additions & 10 deletions src/contracts/GPv2AllowListAuthentication.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
21 changes: 0 additions & 21 deletions src/contracts/ownership/CustomInitiallyOwnable.sol

This file was deleted.

10 changes: 10 additions & 0 deletions src/contracts/test/GPv2AllowListAuthenticationV2.sol
Original file line number Diff line number Diff line change
@@ -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;
}
}
7 changes: 5 additions & 2 deletions src/deploy/001_authenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
});
};

Expand Down
2 changes: 1 addition & 1 deletion src/ts/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down
1 change: 1 addition & 0 deletions src/ts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ export * from "./settlement";
export * from "./reader";
export * from "./deploy";
export * from "./sign";
export * from "./proxy";
export * from "./types/ethers";
45 changes: 45 additions & 0 deletions src/ts/proxy.ts
Original file line number Diff line number Diff line change
@@ -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;
}
3 changes: 2 additions & 1 deletion test/AllowListStorageReader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
27 changes: 20 additions & 7 deletions test/GPv2AllowListAuthenticator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand All @@ -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");
});
});

Expand All @@ -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");
});
});

Expand Down
3 changes: 2 additions & 1 deletion test/GPv2Settlement.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 6 additions & 5 deletions test/e2e/deployment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
ContractName,
DeploymentArguments,
deterministicDeploymentAddress,
implementationAddress,
} from "../../src/ts";
import { builtAndDeployedMetadataCoincide } from "../bytecode";

Expand Down Expand Up @@ -45,7 +46,7 @@ describe("E2E: Deployment", () => {
it("authenticator", async () => {
expect(
await builtAndDeployedMetadataCoincide(
authenticator.address,
await implementationAddress(authenticator.address),
"GPv2AllowListAuthentication",
),
).to.be.true;
Expand All @@ -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 () => {
Expand All @@ -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);
});
});
});
72 changes: 72 additions & 0 deletions test/e2e/upgradeAuthenticator.test.ts
Original file line number Diff line number Diff line change
@@ -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,
});
}
});

0 comments on commit 9b5796c

Please sign in to comment.