fei-protocol · Joeysantoro · Sep 23, 2021 · Sep 22, 2021 · Sep 23, 2021
@@ -1,33 +1,36 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity ^0.8.4;
 
-import "./Timelock.sol";
+import "@openzeppelin/contracts/governance/TimelockController.sol";
 import "../refs/CoreRef.sol";
 
 // Timelock with veto admin roles
-contract OptimisticTimelock is Timelock, CoreRef {
+contract OptimisticTimelock is TimelockController, CoreRef {
 
-    constructor(address core_, address admin_, uint delay_, uint minDelay_) 
-        Timelock(admin_, delay_, minDelay_)
+    constructor(
+        address core_, 
+        uint256 minDelay, 
+        address[] memory proposers,
+        address[] memory executors
+    ) 
+        TimelockController(minDelay, proposers, executors)
         CoreRef(core_)
-    {}
-
-    function queueTransaction(address target, uint value, string memory signature, bytes memory data, uint eta) public override whenNotPaused returns (bytes32) {
-        return super.queueTransaction(target, value, signature, data, eta);
-    }
-
-    function vetoTransactions(address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory datas, uint[] memory etas) public onlyGuardianOrGovernor {
-        for (uint i = 0; i < targets.length; i++) {
-            _cancelTransaction(targets[i], values[i], signatures[i], datas[i], etas[i]);
-        }
+    {
+        // Only guardians and governors are timelock admins
+        revokeRole(TIMELOCK_ADMIN_ROLE, msg.sender);
     }
 
-    function executeTransaction(address target, uint value, string memory signature, bytes memory data, uint eta) public override whenNotPaused payable returns (bytes memory) {
-        return super.executeTransaction(target, value, signature, data, eta);
-    }
+    /**
+        @notice allow guardian or governor to assume timelock admin roles
+        This more elegantly achieves optimistic timelock as follows:
+        - veto: grant self PROPOSER_ROLE and cancel
+        - pause proposals: revoke PROPOSER_ROLE from target
+        - pause execution: revoke EXECUTOR_ROLE from target
+        - set new proposer: revoke old proposer and add new one
 
-    function governorSetPendingAdmin(address newAdmin) public onlyGovernor {
-        pendingAdmin = newAdmin;
-        emit NewPendingAdmin(newAdmin);
+        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/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -36,7 +36,7 @@
     "@nomiclabs/hardhat-truffle5": "^2.0.0",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
     "@nomiclabs/hardhat-web3": "^2.0.0",
-    "@openzeppelin/contracts": "^4.1.0",
+    "@openzeppelin/contracts": "^4.3.2",
     "@openzeppelin/test-environment": "^0.1.7",
     "@openzeppelin/test-helpers": "^0.5.4",
     "@typechain/web3-v1": "^3.0.0",

@@ -0,0 +1,57 @@
+import { ethers } from "hardhat";
+import { expect } from "chai";
+
+async function setup(addresses, oldContracts, contracts, logging) {}
+
+/*
+ 1. Revoke TribalChief role from old timelock
+ 2. Grant TribalChief admin to new timelock
+*/
+async function run(addresses, oldContracts, contracts, logging = false) {
+  const {
+    core,
+    tribalChief,
+    optimisticTimelock
+  } = contracts;
+
+  const {
+    tribalChiefOptimisticTimelockAddress
+  } = addresses;
+
+  const role = await tribalChief.CONTRACT_ADMIN_ROLE();
+
+  // 1.
+  await core.revokeRole(role, tribalChiefOptimisticTimelockAddress);
+
+  // 2. 
+  await core.grantRole(role, optimisticTimelock.address);
+}
+
+async function teardown(addresses, oldContracts, contracts, logging) {}
+
+async function validate(addresses, oldContracts, contracts) {
+    const {
+        core,
+        tribalChief,
+        optimisticTimelock
+      } = contracts;
+
+      const {
+        tribalChiefOptimisticTimelockAddress,
+        tribalChiefOptimisticMultisigAddress
+      } = addresses;
+
+  const proposerRole = await optimisticTimelock.PROPOSER_ROLE();
+  const executorRole = await optimisticTimelock.EXECUTOR_ROLE();
+  const role = await tribalChief.CONTRACT_ADMIN_ROLE();
+
+  expect(await optimisticTimelock.hasRole(proposerRole, tribalChiefOptimisticMultisigAddress)).to.be.true;
+  expect(await optimisticTimelock.hasRole(executorRole, tribalChiefOptimisticMultisigAddress)).to.be.true;
+
+  expect(await core.hasRole(role, optimisticTimelock.address)).to.be.true;
+  expect(await core.hasRole(role, tribalChiefOptimisticTimelockAddress)).to.be.false;
+}
+
+module.exports = {
+  setup, run, teardown, validate
+};
diff --git a/scripts/deploy/optimisticTimelock.js b/scripts/deploy/optimisticTimelock.js
@@ -0,0 +1,29 @@
+const OptimisticTimelock = artifacts.require('OptimisticTimelock');
+
+const fourDays = 4 * 24 * 60 * 60;
+
+async function deploy(deployAddress, addresses, logging = false) {
+  const {
+    tribalChiefOptimisticMultisigAddress,
+    coreAddress
+  } = addresses;
+
+  if (
+    !tribalChiefOptimisticMultisigAddress || !coreAddress
+  ) {
+    throw new Error('An environment variable contract address is not set');
+  }
+
+  const optimisticTimelock = await OptimisticTimelock.new(
+    coreAddress,
+    fourDays,
+    [tribalChiefOptimisticMultisigAddress],
+    [tribalChiefOptimisticMultisigAddress]
+  );
+
+  return {
+    optimisticTimelock
+  };
+}
+
+module.exports = { deploy };
diff --git a/test/integration/e2e.spec.ts b/test/integration/e2e.spec.ts
@@ -556,7 +556,6 @@ describe('e2e', function () {
   describe('Optimistic Approval', async () => {
     beforeEach(async function () {
       const { tribalChiefOptimisticMultisigAddress, timelockAddress } = contractAddresses;
-      const { tribalChiefOptimisticTimelock } = contracts;
 
       await hre.network.provider.request({
         method: 'hardhat_impersonateAccount',
@@ -570,35 +569,55 @@ describe('e2e', function () {
 
       await web3.eth.sendTransaction({from: deployAddress, to: tribalChiefOptimisticMultisigAddress, value: '40000000000000000'});
 
-    });
-    it('governor can cancel a proposal', async () => {
-      const { tribalChiefOptimisticMultisigAddress, timelockAddress } = contractAddresses;
-      const { tribalChiefOptimisticTimelock } = contracts;
+      await web3.eth.sendTransaction({from: timelockAddress, to: tribalChiefOptimisticMultisigAddress, value: '40000000000000000'});
 
-      await tribalChiefOptimisticTimelock.queueTransaction(deployAddress, 0, 'sig()', '0x', '10000000000000000', {from: tribalChiefOptimisticMultisigAddress});
-      const hash = await tribalChiefOptimisticTimelock.getTxHash(deployAddress, 0, 'sig()', '0x', '10000000000000000');
-      expect(await tribalChiefOptimisticTimelock.queuedTransactions(hash)).to.be.true;
+    });
+    it('governor can assume timelock admin', async () => {
+      const { timelockAddress } = contractAddresses;
+      const { optimisticTimelock } = contracts;
 
-      await tribalChiefOptimisticTimelock.vetoTransactions([deployAddress], [0], ['sig()'], ['0x'], ['10000000000000000'], {from: timelockAddress});
+      await optimisticTimelock.becomeAdmin({from: timelockAddress});
 
-      expect(await tribalChiefOptimisticTimelock.queuedTransactions(hash)).to.be.false;
+      const admin = await optimisticTimelock.TIMELOCK_ADMIN_ROLE();
+      expect(await optimisticTimelock.hasRole(admin, timelockAddress)).to.be.true;
     });
 
     it('proposal can execute on tribalChief', async () => {
       const { tribalChiefOptimisticMultisigAddress } = contractAddresses;
-      const { tribalChiefOptimisticTimelock, tribalChief } = contracts;
+      const { optimisticTimelock, tribalChief } = contracts;
 
       const oldBlockReward = await tribalChief.tribePerBlock();
+      await optimisticTimelock.schedule(
+        tribalChief.address, 
+        0, 
+        '0xf580ffcb0000000000000000000000000000000000000000000000000000000000000001', 
+        '0x0000000000000000000000000000000000000000000000000000000000000000', 
+        '0x0000000000000000000000000000000000000000000000000000000000000001', 
+        '500000', 
+        {from: tribalChiefOptimisticMultisigAddress}
+      );
 
-      await tribalChiefOptimisticTimelock.queueTransaction(tribalChief.address, 0, 'updateBlockReward(uint256)', '0x0000000000000000000000000000000000000000000000000000000000000001', '100000000000', {from: tribalChiefOptimisticMultisigAddress});
-      const hash = await tribalChiefOptimisticTimelock.getTxHash(tribalChief.address, 0, 'updateBlockReward(uint256)', '0x0000000000000000000000000000000000000000000000000000000000000001', '100000000000');
-      expect(await tribalChiefOptimisticTimelock.queuedTransactions(hash)).to.be.true;
+      const hash = await optimisticTimelock.hashOperation(
+        tribalChief.address, 
+        0, 
+        '0xf580ffcb0000000000000000000000000000000000000000000000000000000000000001', 
+        '0x0000000000000000000000000000000000000000000000000000000000000000', 
+        '0x0000000000000000000000000000000000000000000000000000000000000001', 
+      );
+      expect(await optimisticTimelock.isOperationPending(hash)).to.be.true;
 
-      await time.increaseTo('100000000000');
-      await tribalChiefOptimisticTimelock.executeTransaction(tribalChief.address, 0, 'updateBlockReward(uint256)', '0x0000000000000000000000000000000000000000000000000000000000000001', '100000000000', {from: tribalChiefOptimisticMultisigAddress});
+      await time.increase('500000');
+      await optimisticTimelock.execute(        
+        tribalChief.address, 
+        0, 
+        '0xf580ffcb0000000000000000000000000000000000000000000000000000000000000001', 
+        '0x0000000000000000000000000000000000000000000000000000000000000000', 
+        '0x0000000000000000000000000000000000000000000000000000000000000001', 
+        {from: tribalChiefOptimisticMultisigAddress}
+      );
 
       expect(await tribalChief.tribePerBlock()).to.be.bignumber.equal('1');
-      expect(await tribalChiefOptimisticTimelock.queuedTransactions(hash)).to.be.false;
+      expect(await optimisticTimelock.isOperationDone(hash)).to.be.true;
 
       await tribalChief.updateBlockReward(oldBlockReward);
     });

diff --git a/test/integration/proposals_config.json b/test/integration/proposals_config.json
@@ -5,6 +5,11 @@
         "proposerAddress" : "0xe0ac4559739bD36f0913FB0A3f5bFC19BCBaCD52",
         "voterAddress" : "0xB8f482539F2d3Ae2C9ea6076894df36D1f632775"
     },
+    "optimisticTimelock" : {
+        "deploy" : true,
+        "exec" : false,
+        "proposerAddress" : "0xe0ac4559739bD36f0913FB0A3f5bFC19BCBaCD52"
+    },
     "fip_22": {
         "deploy" : false,
         "exec" : true,

diff --git a/test/integration/setup/types.ts b/test/integration/setup/types.ts
@@ -76,6 +76,7 @@ export type MainnetContracts = {
   feiTribeLBPSwapper: typeof Contract,
   aaveLendingPool: typeof Contract,
   aaveTribeIncentivesController: typeof Contract,
+  optimisticTimelock: typeof Contract,
 }
 
 export type MainnetContractAddresses = {

diff --git a/test/unit/dao/OptimisticTimelock.test.js b/test/unit/dao/OptimisticTimelock.test.js
@@ -3,71 +3,35 @@ const {
   expectRevert,
   getAddresses,
   getCore,
-  time,
   expect,
 } = require('../../helpers');
 
 const OptimisticTimelock = artifacts.require('OptimisticTimelock');
 
 describe('TimelockedDelegator', function () {
   let userAddress;
-  let guardianAddress;
   let governorAddress;
 
   beforeEach(async function () {
     ({
       userAddress,
-      guardianAddress,
       governorAddress,
     } = await getAddresses());
     this.core = await getCore();
 
     this.delay = new BN(1000);
-    this.timelock = await OptimisticTimelock.new(this.core.address, userAddress, this.delay, this.delay);
+    this.timelock = await OptimisticTimelock.new(this.core.address, this.delay, [], []);
   });
 
-  describe('Pausable', function () {
-    beforeEach(async function () {
-      await this.timelock.pause({from: governorAddress});
-    });
-
-    it('queue reverts', async function() {
-      const eta = (await time.latest()).add(this.delay);
-      await expectRevert(this.timelock.queueTransaction(userAddress, 100, '', '0x0', eta, {from: userAddress}), 'Pausable: paused');
-    });
-
-    it('execute reverts', async function() {
-      const eta = (await time.latest()).add(this.delay);
-      await expectRevert(this.timelock.executeTransaction(userAddress, 100, '', '0x0', eta, {from: userAddress}), 'Pausable: paused');
-    });
-  });
-
-  describe('Veto', function () {
-    it('non-governor or guardian reverts', async function() {
-      const eta = (await time.latest()).add(this.delay);
-      await expectRevert(this.timelock.vetoTransactions([userAddress], [100], [''], ['0x0'], [eta], {from: userAddress}), 'CoreRef: Caller is not a guardian or governor');
-    });
-
-    it('guardian succeeds', async function() {
-      const eta = (await time.latest()).add(this.delay).add(this.delay);
-      await this.timelock.queueTransaction(userAddress, 100, '', '0x0', eta, {from: userAddress});
-
-      const txHash = await this.timelock.getTxHash(userAddress, 100, '', '0x0', eta);
-      expect(await this.timelock.queuedTransactions(txHash)).to.be.equal(true);
-
-      await this.timelock.vetoTransactions([userAddress], [100], [''], ['0x0'], [eta], {from: guardianAddress});
-      expect(await this.timelock.queuedTransactions(txHash)).to.be.equal(false);
+  describe('Become Admin', function () {
+    it('user reverts', async function() {
+      await expectRevert(this.timelock.becomeAdmin({from: userAddress}), 'CoreRef: Caller is not a guardian or governor');
     });
 
     it('governor succeeds', async function() {
-      const eta = (await time.latest()).add(this.delay).add(this.delay);
-      await this.timelock.queueTransaction(userAddress, 100, '', '0x0', eta, {from: userAddress});
-
-      const txHash = await this.timelock.getTxHash(userAddress, 100, '', '0x0', eta);
-      expect(await this.timelock.queuedTransactions(txHash)).to.be.equal(true);
-
-      await this.timelock.vetoTransactions([userAddress], [100], [''], ['0x0'], [eta], {from: governorAddress});
-      expect(await this.timelock.queuedTransactions(txHash)).to.be.equal(false);
+      const adminRole = await this.timelock.TIMELOCK_ADMIN_ROLE();
+      await this.timelock.becomeAdmin({from: governorAddress});
+      expect(await this.timelock.hasRole(adminRole, governorAddress)).to.be.true;
     });
   });
 });