Timelock Setup (volt-protocol#80)

* add scripts to construct calldata * proposal calldata constructing * Construct call and execution data * Bump OZ contracts version, fix failing tests, set up timelock integration testing framework * remove dummy vip vip_2 * cleanup * update comment on cancelling proposals in OA contract * update comment in OA, upgrade to async 2.6.4 * add integration test for OA timelock * forge oa timelock integration tests
LaTribuWeb3 · Jun 7, 2022 · 271b580 · 271b580
1 parent 1a46091
commit 271b580
Show file tree

Hide file tree

Showing 24 changed files with 1,006 additions and 13 deletions.
diff --git a/.solhint.json b/.solhint.json
@@ -2,7 +2,7 @@
   "extends": "solhint:recommended",
   "plugins": ["prettier"],
     "rules": {
-      "compiler-version" : ["warn", "^0.6.0"],
+      "compiler-version" : ["warn", "^0.8.4"],
       "not-rely-on-time" : "off",
       "reason-string" : "off",
       "no-empty-blocks" : "off",

diff --git a/contracts/dao/OptimisticTimelock.sol b/contracts/dao/OptimisticTimelock.sol
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity ^0.8.4;
+
+import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";
+import {CoreRef} from "../refs/CoreRef.sol";
+
+// Timelock with proposer, executor, canceller and admin roles
+contract OptimisticTimelock is TimelockController, CoreRef {
+    constructor(
+        address core_,
+        uint256 minDelay,
+        address[] memory proposers,
+        address[] memory executors
+    ) TimelockController(minDelay, proposers, executors) CoreRef(core_) {
+        // Only guardians and governors are timelock admins
+        revokeRole(TIMELOCK_ADMIN_ROLE, msg.sender);
+    }
+
+    /**
+        @notice allow guardian or governor to assume timelock admin roles
+        This more elegantly achieves optimistic timelock as follows:
+        - veto: all proposers are granted the CANCELLER_ROLE in the constructor, 
+          CANCELLER_ROLE can cancel any in-flight proposal
+        - pause proposals: revoke PROPOSER_ROLE from target
+        - pause execution: revoke EXECUTOR_ROLE from target
+        - set new proposer: revoke old proposer and add new one
+
+        In addition it allows for much more granular and flexible access for multisig operators
+    */
+    function becomeAdmin() public onlyGuardianOrGovernor {
+        this.grantRole(TIMELOCK_ADMIN_ROLE, msg.sender);
+    }
+}
diff --git a/contracts/test/integration/IntegrationTestOATimelock.t.sol b/contracts/test/integration/IntegrationTestOATimelock.t.sol
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity ^0.8.4;
+
+import {Vm} from "../unit/utils/Vm.sol";
+import {ICore} from "../../core/ICore.sol";
+import {DSTest} from "../unit/utils/DSTest.sol";
+import {PSMRouter} from "./../../peg/PSMRouter.sol";
+import {OptimisticTimelock} from "./../../dao/OptimisticTimelock.sol";
+import {INonCustodialPSM} from "./../../peg/NonCustodialPSM.sol";
+import {IVolt, Volt} from "../../volt/Volt.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {getCore, getAddresses, getVoltAddresses, FeiTestAddresses, VoltAddresses} from "../unit/utils/Fixtures.sol";
+
+contract IntegrationTestOATimelock is DSTest {
+    Vm public constant vm = Vm(HEVM_ADDRESS);
+    ICore public core = ICore(0xEC7AD284f7Ad256b64c6E69b84Eb0F48f42e8196);
+    OptimisticTimelock public oaTimelock;
+    FeiTestAddresses public addresses = getAddresses();
+    VoltAddresses public voltAddresses = getVoltAddresses();
+    address public proposer1 = voltAddresses.pcvGuardAddress1;
+    address public proposer2 = voltAddresses.pcvGuardAddress2;
+    address public executorAddress = voltAddresses.executorAddress;
+
+    function setUp() public {
+        address[] memory proposerCancellerAddresses = new address[](3);
+        proposerCancellerAddresses[0] = proposer1;
+        proposerCancellerAddresses[1] = proposer2;
+        proposerCancellerAddresses[2] = executorAddress;
+
+        address[] memory executorAddresses = new address[](1);
+        executorAddresses[0] = voltAddresses.executorAddress;
+
+        oaTimelock = new OptimisticTimelock(
+            address(core),
+            600,
+            proposerCancellerAddresses,
+            executorAddresses
+        );
+    }
+
+    function testSetup() public {
+        assertTrue(oaTimelock.hasRole(oaTimelock.CANCELLER_ROLE(), proposer1));
+        assertTrue(oaTimelock.hasRole(oaTimelock.PROPOSER_ROLE(), proposer1));
+
+        assertTrue(oaTimelock.hasRole(oaTimelock.CANCELLER_ROLE(), proposer2));
+        assertTrue(oaTimelock.hasRole(oaTimelock.PROPOSER_ROLE(), proposer2));
+
+        /// ensure multisig has PROPOSER, CANCELLER and EXECUTOR roles
+        assertTrue(
+            oaTimelock.hasRole(oaTimelock.CANCELLER_ROLE(), executorAddress)
+        );
+        assertTrue(
+            oaTimelock.hasRole(oaTimelock.PROPOSER_ROLE(), executorAddress)
+        );
+        assertTrue(
+            oaTimelock.hasRole(oaTimelock.EXECUTOR_ROLE(), executorAddress)
+        );
+    }
+
+    function testTimelockEthReceive() public {
+        assertEq(address(oaTimelock).balance, 0); /// starts with 0 balance
+
+        uint256 ethSendAmount = 100 ether;
+        vm.deal(proposer1, ethSendAmount);
+
+        vm.prank(proposer1);
+        (bool success, ) = address(oaTimelock).call{value: ethSendAmount}("");
+
+        assertTrue(success);
+        assertEq(address(oaTimelock).balance, ethSendAmount);
+    }
+
+    function testTimelockSendEth() public {
+        uint256 ethSendAmount = 100 ether;
+        vm.deal(address(oaTimelock), ethSendAmount);
+
+        assertEq(address(oaTimelock).balance, ethSendAmount); /// starts with 0 balance
+
+        bytes memory data = "";
+        bytes32 predecessor = bytes32(0);
+        bytes32 salt = bytes32(0);
+        vm.prank(proposer1);
+        oaTimelock.schedule(
+            proposer1,
+            ethSendAmount,
+            data,
+            predecessor,
+            salt,
+            600
+        );
+        bytes32 id = oaTimelock.hashOperation(
+            proposer1,
+            ethSendAmount,
+            data,
+            predecessor,
+            salt
+        );
+
+        uint256 startingProposerEthBalance = proposer1.balance;
+
+        assertTrue(!oaTimelock.isOperationDone(id)); /// operation is not done
+        assertTrue(!oaTimelock.isOperationReady(id)); /// operation is not ready
+
+        vm.warp(block.timestamp + 600);
+        assertTrue(oaTimelock.isOperationReady(id)); /// operation is ready
+
+        vm.prank(executorAddress);
+        oaTimelock.execute(proposer1, ethSendAmount, data, predecessor, salt);
+
+        assertTrue(oaTimelock.isOperationDone(id)); /// operation is done
+
+        assertEq(address(oaTimelock).balance, 0);
+        assertEq(proposer1.balance, ethSendAmount + startingProposerEthBalance); /// assert proposer received their eth
+    }
+}
diff --git a/contracts/test/unit/psm/NonCustodialPSM.t.sol b/contracts/test/unit/psm/NonCustodialPSM.t.sol
@@ -400,14 +400,14 @@ contract NonCustodialPSMTest is DSTest {
 
     /// @notice redeem fails without approval
     function testSwapFeiForUnderlyingFailsWithoutApproval() public {
-        vm.expectRevert(bytes("ERC20: transfer amount exceeds allowance"));
+        vm.expectRevert(bytes("ERC20: insufficient allowance"));
 
         psm.redeem(address(this), mintAmount, mintAmount);
     }
 
     /// @notice mint fails without approval
     function testSwapUnderlyingForFeiFailsWithoutApproval() public {
-        vm.expectRevert(bytes("ERC20: transfer amount exceeds allowance"));
+        vm.expectRevert(bytes("ERC20: insufficient allowance"));
 
         psm.mint(address(this), mintAmount, mintAmount);
     }

diff --git a/contracts/test/unit/utils/Fixtures.sol b/contracts/test/unit/utils/Fixtures.sol
@@ -21,6 +21,12 @@ struct FeiTestAddresses {
     address voltDeployerAddress;
 }
 
+struct VoltAddresses {
+    address pcvGuardAddress1; // address(0xf8D0387538E8e03F3B4394dA89f221D7565a28Ee),
+    address pcvGuardAddress2; // address(0xd90E9181B20D8D1B5034d9f5737804Da182039F6),
+    address executorAddress; // address(0xcBB83206698E8788F85EFbEeeCAd17e53366EBDf) // msig is executor
+}
+
 /// @dev Get a list of addresses
 function getAddresses() pure returns (FeiTestAddresses memory) {
     FeiTestAddresses memory addresses = FeiTestAddresses({
@@ -42,6 +48,14 @@ function getAddresses() pure returns (FeiTestAddresses memory) {
     return addresses;
 }
 
+function getVoltAddresses() pure returns (VoltAddresses memory addresses) {
+    addresses = VoltAddresses({
+        pcvGuardAddress1: 0xf8D0387538E8e03F3B4394dA89f221D7565a28Ee,
+        pcvGuardAddress2: 0xd90E9181B20D8D1B5034d9f5737804Da182039F6,
+        executorAddress: 0xcBB83206698E8788F85EFbEeeCAd17e53366EBDf
+    });
+}
+
 /// @dev Get a list of addresses
 function getMainnetAddresses() pure returns (FeiTestAddresses memory) {
     FeiTestAddresses memory addresses = FeiTestAddresses({

diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -57,7 +57,7 @@ export default {
       forking: enableMainnetForking
         ? {
             url: `https://eth-mainnet.alchemyapi.io/v2/${mainnetAlchemyApiKey}`,
-            blockNumber: 14787549
+            blockNumber: 14899146
           }
         : undefined
     },
@@ -81,7 +81,7 @@ export default {
     mainnet: {
       url: `https://eth-mainnet.alchemyapi.io/v2/${mainnetAlchemyApiKey}`,
       accounts: privateKey ? [privateKey] : [],
-      gasPrice: 80000000000 // gas price that is paid for mainnet transactions. currently 80 gigawei
+      gasPrice: 40000000000 // gas price that is paid for mainnet transactions. currently 40 gigawei
     }
   },
 

diff --git a/package.json b/package.json
@@ -34,7 +34,7 @@
     "deploy:timelock": "DEPLOY_FILE=optimisticTimelockDeploy npx hardhat run --network mainnet scripts/deploy/migrations.ts",
     "deploy:stw": "DEPLOY_FILE=deployStakedTokenWrapper npx hardhat run --network mainnet scripts/deploy/migrations.ts",
     "abis": "node scripts/utils/abis.js",
-    "calldata": "npx hardhat run scripts/utils/getProposalCalldata.ts",
+    "calldata": "npx hardhat run scripts/simulation/getProposalCalldata.ts",
     "check-proposal": "ENABLE_MAINNET_FORKING=true npx hardhat run scripts/utils/checkProposal.ts",
     "prettier:sol": "prettier --config ./contracts/.prettierrc --write 'contracts/**/*.sol'",
     "prettier:ts": "prettier --config .prettierrc './**/*.ts' --write",
@@ -48,12 +48,13 @@
     "@chainlink/contracts": "^0.3.1",
     "@ethersproject/providers": "^5.5.3",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
-    "@openzeppelin/contracts": "^4.4.2",
+    "@openzeppelin/contracts": "4.6.0",
     "@openzeppelin/test-environment": "^0.1.7",
     "@openzeppelin/test-helpers": "^0.5.15",
     "@uniswap/lib": "^1.1.2",
     "@uniswap/v2-core": "^1.0.1",
     "@uniswap/v2-periphery": "^1.1.0-beta.0",
+    "async": "2.6.4",
     "chai": "^4.3.4",
     "dotenv": "^14.1.0",
     "hardhat": "^2.8.2",

diff --git a/proposals/dao/vip_1.ts b/proposals/dao/vip_1.ts
@@ -0,0 +1,59 @@
+import { expect } from 'chai';
+import {
+  DeployUpgradeFunc,
+  NamedAddresses,
+  SetupUpgradeFunc,
+  TeardownUpgradeFunc,
+  ValidateUpgradeFunc
+} from '@custom-types/types';
+import { getImpersonatedSigner } from '@test/helpers';
+
+/*
+
+Timelock Proposal #1
+
+Description:
+
+Steps:
+  1 - First grant the timelock the governor role by the multisig
+  2 - Grant Timelock PCV Controller
+  3 - Validate state changes
+
+*/
+
+const vipNumber = '1';
+
+// Do any deployments
+// This should exclusively include new contract deployments
+const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: NamedAddresses, logging: boolean) => {
+  console.log(`No deploy actions for vip${vipNumber}`);
+  return {
+    // put returned contract objects here
+  };
+};
+
+// Do any setup necessary for running the test.
+// This could include setting up Hardhat to impersonate accounts,
+// ensuring contracts have a specific state, etc.
+const setup: SetupUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  const signer = await getImpersonatedSigner(addresses.protocolMultisig);
+  await contracts.core.connect(signer).grantGovernor(addresses.optimisticTimelock);
+  console.log('setup function run');
+};
+
+// Tears down any changes made in setup() that need to be
+// cleaned up before doing any validation checks.
+const teardown: TeardownUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  console.log(`No actions to complete in teardown for vip${vipNumber}`);
+};
+
+// Run any validations required on the vip using mocha or console logging
+// IE check balances, check state of contracts, etc.
+const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  const { core, optimisticTimelock } = contracts;
+
+  expect(await core.isGovernor(optimisticTimelock.address)).to.be.true;
+  expect(await core.isPCVController(optimisticTimelock.address)).to.be.true;
+};
+
+export { deploy, setup, teardown, validate };
diff --git a/proposals/dao/vip_x.ts b/proposals/dao/vip_x.ts
@@ -0,0 +1,54 @@
+import hre, { ethers, artifacts } from 'hardhat';
+import { expect } from 'chai';
+import {
+  DeployUpgradeFunc,
+  NamedAddresses,
+  SetupUpgradeFunc,
+  TeardownUpgradeFunc,
+  ValidateUpgradeFunc
+} from '@custom-types/types';
+
+/*
+
+Timelock Proposal #9001
+
+Description:
+
+Steps:
+  1 - 
+  2 -
+  3 - 
+
+*/
+
+const vipNumber = '1'; // Change me!
+
+// Do any deployments
+// This should exclusively include new contract deployments
+const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: NamedAddresses, logging: boolean) => {
+  console.log(`No deploy actions for vip${vipNumber}`);
+  return {
+    // put returned contract objects here
+  };
+};
+
+// Do any setup necessary for running the test.
+// This could include setting up Hardhat to impersonate accounts,
+// ensuring contracts have a specific state, etc.
+const setup: SetupUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  console.log(`No actions to complete in setup for vip${vipNumber}`);
+};
+
+// Tears down any changes made in setup() that need to be
+// cleaned up before doing any validation checks.
+const teardown: TeardownUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  console.log(`No actions to complete in teardown for vip${vipNumber}`);
+};
+
+// Run any validations required on the vip using mocha or console logging
+// IE check balances, check state of contracts, etc.
+const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => {
+  console.log(`No actions to complete in validate for vip${vipNumber}`);
+};
+
+export { deploy, setup, teardown, validate };
diff --git a/proposals/vip_1.ts b/proposals/vip_1.ts
@@ -0,0 +1,17 @@
+import { ProposalDescription } from '@custom-types/types';
+
+const vip_1: ProposalDescription = {
+  title: 'VIP-1: Grant Timelock PCV Controller',
+  commands: [
+    {
+      target: 'core',
+      values: '0',
+      method: 'grantPCVController(address)',
+      arguments: ['{optimisticTimelock}'],
+      description: 'Grant timelock PCV Controller role'
+    }
+  ],
+  description: `Grant the Timelock PCV Controller Role`
+};
+
+export default vip_1;
diff --git a/proposals/vip_x.ts b/proposals/vip_x.ts
@@ -0,0 +1,17 @@
+import { ProposalDescription } from '@custom-types/types';
+
+const vip_x: ProposalDescription = {
+  title: 'VIP-X: Title',
+  commands: [
+    {
+      target: '',
+      values: '',
+      method: '',
+      arguments: [],
+      description: ''
+    }
+  ],
+  description: 'vip_x will change the game!'
+};
+
+export default vip_x;