diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index f91e3b3..9626b17 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -1,6 +1,6 @@ name: coverage -on: [push, pull_request] +on: [push] jobs: coverage: diff --git a/.github/workflows/forge-test.yml b/.github/workflows/forge-test.yml index 880d113..a06a48a 100644 --- a/.github/workflows/forge-test.yml +++ b/.github/workflows/forge-test.yml @@ -1,7 +1,7 @@ -on: [push] - name: Build and Test Contracts +on: [push] + jobs: check: name: Foundry project diff --git a/integration/basic.test.ts b/integration/basic.test.ts deleted file mode 100644 index d0e1469..0000000 --- a/integration/basic.test.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { expect } from "chai"; -import { ethers } from "hardhat"; -import { Contract, Signer } from "ethers"; - -describe("Deploy", function () { - it("the protocol should deploy", async function () { - const [admin] = await ethers.getSigners(); - const wethFactory = await ethers.getContractFactory("WETH"); - const weth = await wethFactory.deploy(); - const HookProtocol = await ethers.getContractFactory("HookProtocol"); - - const protocol = await HookProtocol.deploy(admin.address, weth.address); - }); -}); diff --git a/integration/integration.test.ts b/integration/integration.test.ts index 4fc5ee1..01161e0 100644 --- a/integration/integration.test.ts +++ b/integration/integration.test.ts @@ -1,15 +1,142 @@ import { ethers } from "hardhat"; import { expect, use } from "chai"; -import { BigNumber, Contract, Signer } from "ethers"; +import { BigNumber, Contract } from "ethers"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { solidity } from "ethereum-waffle"; import { signEntitlement } from "./helpers"; import { getAddress } from "ethers/lib/utils"; -import { writeHeapSnapshot } from "v8"; -import { create } from "domain"; use(solidity); + +describe("Protocol", function () { + let protocol: Contract; + let admin: SignerWithAddress, one: SignerWithAddress, two: SignerWithAddress; + beforeEach(async () => { + [admin, one, two] = await ethers.getSigners(); + const protocolFactory = await ethers.getContractFactory("HookProtocol"); + protocol = await protocolFactory.deploy(admin.address, admin.address); + }); + + it("can be paused", async () => { + await protocol.connect(admin).pause(); + await expect(protocol.throwWhenPaused()).to.be.reverted; + }); + + it("can be unpaused", async () => { + await protocol.connect(admin).pause(); + await expect(protocol.throwWhenPaused()).to.be.reverted; + await protocol.connect(admin).unpause(); + await expect(protocol.throwWhenPaused()).not.to.be.reverted; + }); + + it("vault factory can be set", async () => { + await protocol.connect(admin).pause(); + await expect(protocol.throwWhenPaused()).to.be.reverted; + await protocol.connect(admin).unpause(); + await expect(protocol.throwWhenPaused()).not.to.be.reverted; + }); + + it("admin role can be revoked", async () => { + /// give role to new user + await protocol + .connect(admin) + .grantRole(ethers.utils.id("VAULT_UPGRADER"), one.address); + // drop role + await protocol + .connect(admin) + .renounceRole(ethers.utils.id("VAULT_UPGRADER"), admin.address); + // new user gives role to a third user + await protocol + .connect(one) + .grantRole(ethers.utils.id("VAULT_UPGRADER"), two.address); + expect( + await protocol.hasRole(ethers.utils.id("VAULT_UPGRADER"), one.address) + ).to.be.true; + expect( + await protocol.hasRole(ethers.utils.id("VAULT_UPGRADER"), two.address) + ).to.be.true; + expect( + await protocol.hasRole(ethers.utils.id("VAULT_UPGRADER"), admin.address) + ).to.be.false; + }); + + it("collection configs can be set", async () => { + await protocol + .connect(admin) + .setCollectionConfig(two.address, ethers.utils.id("config"), true); + expect( + await protocol.getCollectionConfig(two.address, ethers.utils.id("config")) + ).to.be.true; + }); +}); + +describe("UpgradeableBeacon", function () { + let beacon: Contract, impl1: Contract, impl2: Contract, protocol: Contract; + let admin: SignerWithAddress; + beforeEach(async () => { + [admin] = await ethers.getSigners(); + const protocolFactory = await ethers.getContractFactory("HookProtocol"); + protocol = await protocolFactory.deploy(admin.address, admin.address); + + const vaultImplFactory = await ethers.getContractFactory( + "HookERC721VaultImplV1" + ); + + const vaultBeaconFactory = await ethers.getContractFactory( + "HookUpgradeableBeacon" + ); + + impl1 = await vaultImplFactory.deploy(); + impl2 = await vaultImplFactory.deploy(); + + beacon = await vaultBeaconFactory.deploy( + impl1.address, + protocol.address, + ethers.utils.id("VAULT_UPGRADER") + ); + }); + + it("the beacon should show an implementation", async function () { + expect(await beacon.implementation()).to.eq(impl1.address); + }); + + it("the beacon should be upgradeable by the correct role", async () => { + await beacon.connect(admin).upgradeTo(impl2.address); + expect(await beacon.implementation()).to.eq(impl2.address); + }); + + it("the beacon should not be upgradeable by a random address", async () => { + const [, actor] = await ethers.getSigners(); + expect( + await protocol.hasRole(ethers.utils.id("VAULT_UPGRADER"), actor.address) + ).to.be.false; + await expect( + beacon.connect(actor).upgradeTo(impl2.address) + ).to.be.revertedWith("w"); + expect(await beacon.implementation()).to.eq(impl1.address); + }); + + it("the beacon should not be upgradeable by a granted address", async () => { + const [, actor] = await ethers.getSigners(); + expect( + await protocol.hasRole(ethers.utils.id("VAULT_UPGRADER"), actor.address) + ).to.be.false; + + await protocol + .connect(admin) + .grantRole(ethers.utils.id("VAULT_UPGRADER"), actor.address); + await beacon.connect(actor).upgradeTo(impl2.address); + expect(await beacon.implementation()).to.eq(impl2.address); + }); + it("the beacon should not be upgradeable not a non-contract", async () => { + const [, actor] = await ethers.getSigners(); + await expect( + beacon.connect(admin).upgradeTo(actor.address) + ).to.be.revertedWith("UpgradeableBeacon: implementation is not a contract"); + }); +}); + describe("Vault", function () { let vaultFactory: Contract, protocol: Contract, @@ -1949,6 +2076,8 @@ describe("Call Instrument Tests", function () { getAddress("0x0000000000000000000000000000000000000000") ); + protocol.setCoveredCallFactory(callFactory.address); + // Create another call instrument contract instance await callFactory.makeCallInstrument(token.address); const callInstrumentAddress = await callFactory.getCallInstrument( diff --git a/src/HookBeaconProxy.sol b/src/HookBeaconProxy.sol index 3fae7fa..a80c1f3 100644 --- a/src/HookBeaconProxy.sol +++ b/src/HookBeaconProxy.sol @@ -39,13 +39,6 @@ contract HookBeaconProxy is Proxy, ERC1967Upgrade { _upgradeBeaconToAndCall(beacon, data, false); } - /** - * @dev Returns the current beacon address. - */ - function _beacon() internal view virtual returns (address) { - return _getBeacon(); - } - /** * @dev Returns the current implementation address of the associated beacon. */ diff --git a/src/HookProtocol.sol b/src/HookProtocol.sol index 06f2864..f7aa4a9 100644 --- a/src/HookProtocol.sol +++ b/src/HookProtocol.sol @@ -8,6 +8,8 @@ import "./interfaces/IHookProtocol.sol"; import "./mixin/PermissionConstants.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; + /// @dev Other contracts in the protocol refer to this one to get configuration and pausing issues. /// to reduce attack surface area, this contract cannot be upgraded; however, additional roles can be /// added. @@ -38,12 +40,12 @@ contract HookProtocol is _setupRole(COLLECTION_CONF, admin); // allow the admin to add and remove other roles - _setRoleAdmin(ALLOWLISTER_ROLE, ADMIN_ROLE); - _setRoleAdmin(PAUSER_ROLE, ADMIN_ROLE); - _setRoleAdmin(VAULT_UPGRADER, ADMIN_ROLE); - _setRoleAdmin(CALL_UPGRADER, ADMIN_ROLE); - _setRoleAdmin(MARKET_CONF, ADMIN_ROLE); - _setRoleAdmin(COLLECTION_CONF, ADMIN_ROLE); + _setRoleAdmin(ALLOWLISTER_ROLE, ALLOWLISTER_ROLE); + _setRoleAdmin(PAUSER_ROLE, PAUSER_ROLE); + _setRoleAdmin(VAULT_UPGRADER, VAULT_UPGRADER); + _setRoleAdmin(CALL_UPGRADER, CALL_UPGRADER); + _setRoleAdmin(MARKET_CONF, MARKET_CONF); + _setRoleAdmin(COLLECTION_CONF, COLLECTION_CONF); // set weth getWETHAddress = weth; } @@ -96,6 +98,10 @@ contract HookProtocol is external adminOnly { + require( + Address.isContract(coveredCallFactoryContract), + "setCoveredCallFactory: implementation is not a contract" + ); coveredCallContract = coveredCallFactoryContract; } @@ -104,6 +110,10 @@ contract HookProtocol is /// vault factory. /// @param vaultFactoryContract the deployed vault factory function setVaultFactory(address vaultFactoryContract) external adminOnly { + require( + Address.isContract(vaultFactoryContract), + "setVaultFactory: implementation is not a contract" + ); vaultContract = vaultFactoryContract; } } diff --git a/src/lib/Signatures.sol b/src/lib/Signatures.sol index f5feac5..1f75101 100644 --- a/src/lib/Signatures.sol +++ b/src/lib/Signatures.sol @@ -2,10 +2,6 @@ pragma solidity ^0.8.10; /// @dev A library for validating signatures from ZeroEx library Signatures { - // '\x19Ethereum Signed Message:\n32\x00\x00\x00\x00'. - uint256 private constant ETH_SIGN_HASH_PREFIX = - 0x19457468657265756d205369676e6564204d6573736167653a0a333200000000; - /// @dev Allowed signature types. enum SignatureType { EIP712,