From 8b162e39b598924ef611dba90ff9d0aa0b676ecc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 8 Mar 2022 22:28:20 +0100 Subject: [PATCH] Add a canceller role to the TimelockController (#3165) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 1 + contracts/governance/TimelockController.sol | 20 ++++++-- test/governance/TimelockController.test.js | 50 ++++++++++++++----- .../GovernorTimelockControl.test.js | 24 +++++++-- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31e5a191222..69c036e41e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * `Governor`: improved security of `onlyGovernance` modifier when using an external executor contract (e.g. a timelock) that can operate without necessarily going through the governance protocol. ([#3147](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3147)) * `ERC20FlashMint`: support infinite allowance when paying back a flash loan. ([#3226](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3226)) * `Governor`: Add a way to parameterize votes. This can be used to implement voting systems such as fractionalized voting, ERC721 based voting, or any number of other systems. The `params` argument added to `_countVote` method, and included in the newly added `_getVotes` method, can be used by counting and voting modules respectively for such purposes. + * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165)) ### Breaking changes diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index c375f074456..ba3d54fe1f4 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -24,6 +24,7 @@ contract TimelockController is AccessControl { bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE"); bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE"); bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); + bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE"); uint256 internal constant _DONE_TIMESTAMP = uint256(1); mapping(bytes32 => uint256) private _timestamps; @@ -58,7 +59,16 @@ contract TimelockController is AccessControl { event MinDelayChange(uint256 oldDuration, uint256 newDuration); /** - * @dev Initializes the contract with a given `minDelay`. + * @dev Initializes the contract with a given `minDelay`, and a list of + * initial proposers and executors. The proposers receive both the + * proposer and the canceller role (for backward compatibility). The + * executors receive the executor role. + * + * NOTE: At construction, both the deployer and the timelock itself are + * administrators. This helps further configuration of the timelock by the + * deployer. After configuration is done, it is recommended that the + * deployer renounces its admin position and relies on timelocked + * operations to perform future maintenance. */ constructor( uint256 minDelay, @@ -68,14 +78,16 @@ contract TimelockController is AccessControl { _setRoleAdmin(TIMELOCK_ADMIN_ROLE, TIMELOCK_ADMIN_ROLE); _setRoleAdmin(PROPOSER_ROLE, TIMELOCK_ADMIN_ROLE); _setRoleAdmin(EXECUTOR_ROLE, TIMELOCK_ADMIN_ROLE); + _setRoleAdmin(CANCELLER_ROLE, TIMELOCK_ADMIN_ROLE); // deployer + self administration _setupRole(TIMELOCK_ADMIN_ROLE, _msgSender()); _setupRole(TIMELOCK_ADMIN_ROLE, address(this)); - // register proposers + // register proposers and cancellers for (uint256 i = 0; i < proposers.length; ++i) { _setupRole(PROPOSER_ROLE, proposers[i]); + _setupRole(CANCELLER_ROLE, proposers[i]); } // register executors @@ -243,9 +255,9 @@ contract TimelockController is AccessControl { * * Requirements: * - * - the caller must have the 'proposer' role. + * - the caller must have the 'canceller' role. */ - function cancel(bytes32 id) public virtual onlyRole(PROPOSER_ROLE) { + function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { require(isOperationPending(id), "TimelockController: operation cannot be cancelled"); delete _timestamps[id]; diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index f39b963481d..84cec6c5794 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -43,7 +43,12 @@ function genOperationBatch (targets, values, datas, predecessor, salt) { } contract('TimelockController', function (accounts) { - const [ admin, proposer, executor, other ] = accounts; + const [ admin, proposer, canceller, executor, other ] = accounts; + + const TIMELOCK_ADMIN_ROLE = web3.utils.soliditySha3('TIMELOCK_ADMIN_ROLE'); + const PROPOSER_ROLE = web3.utils.soliditySha3('PROPOSER_ROLE'); + const EXECUTOR_ROLE = web3.utils.soliditySha3('EXECUTOR_ROLE'); + const CANCELLER_ROLE = web3.utils.soliditySha3('CANCELLER_ROLE'); beforeEach(async function () { // Deploy new timelock @@ -53,9 +58,11 @@ contract('TimelockController', function (accounts) { [ executor ], { from: admin }, ); - this.TIMELOCK_ADMIN_ROLE = await this.timelock.TIMELOCK_ADMIN_ROLE(); - this.PROPOSER_ROLE = await this.timelock.PROPOSER_ROLE(); - this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE(); + + expect(await this.timelock.hasRole(CANCELLER_ROLE, proposer)).to.be.equal(true); + await this.timelock.revokeRole(CANCELLER_ROLE, proposer, { from: admin }); + await this.timelock.grantRole(CANCELLER_ROLE, canceller, { from: admin }); + // Mocks this.callreceivermock = await CallReceiverMock.new({ from: admin }); this.implementation2 = await Implementation2.new({ from: admin }); @@ -63,6 +70,23 @@ contract('TimelockController', function (accounts) { it('initial state', async function () { expect(await this.timelock.getMinDelay()).to.be.bignumber.equal(MINDELAY); + + expect(await this.timelock.TIMELOCK_ADMIN_ROLE()).to.be.equal(TIMELOCK_ADMIN_ROLE); + expect(await this.timelock.PROPOSER_ROLE()).to.be.equal(PROPOSER_ROLE); + expect(await this.timelock.EXECUTOR_ROLE()).to.be.equal(EXECUTOR_ROLE); + expect(await this.timelock.CANCELLER_ROLE()).to.be.equal(CANCELLER_ROLE); + + expect(await Promise.all([ PROPOSER_ROLE, CANCELLER_ROLE, EXECUTOR_ROLE ].map(role => + this.timelock.hasRole(role, proposer), + ))).to.be.deep.equal([ true, false, false ]); + + expect(await Promise.all([ PROPOSER_ROLE, CANCELLER_ROLE, EXECUTOR_ROLE ].map(role => + this.timelock.hasRole(role, canceller), + ))).to.be.deep.equal([ false, true, false ]); + + expect(await Promise.all([ PROPOSER_ROLE, CANCELLER_ROLE, EXECUTOR_ROLE ].map(role => + this.timelock.hasRole(role, executor), + ))).to.be.deep.equal([ false, false, true ]); }); describe('methods', function () { @@ -175,7 +199,7 @@ contract('TimelockController', function (accounts) { MINDELAY, { from: other }, ), - `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`, + `AccessControl: account ${other.toLowerCase()} is missing role ${PROPOSER_ROLE}`, ); }); @@ -298,7 +322,7 @@ contract('TimelockController', function (accounts) { this.operation.salt, { from: other }, ), - `AccessControl: account ${other.toLowerCase()} is missing role ${this.EXECUTOR_ROLE}`, + `AccessControl: account ${other.toLowerCase()} is missing role ${EXECUTOR_ROLE}`, ); }); }); @@ -412,7 +436,7 @@ contract('TimelockController', function (accounts) { MINDELAY, { from: other }, ), - `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`, + `AccessControl: account ${other.toLowerCase()} is missing role ${PROPOSER_ROLE}`, ); }); @@ -537,7 +561,7 @@ contract('TimelockController', function (accounts) { this.operation.salt, { from: other }, ), - `AccessControl: account ${other.toLowerCase()} is missing role ${this.EXECUTOR_ROLE}`, + `AccessControl: account ${other.toLowerCase()} is missing role ${EXECUTOR_ROLE}`, ); }); @@ -651,22 +675,22 @@ contract('TimelockController', function (accounts) { )); }); - it('proposer can cancel', async function () { - const receipt = await this.timelock.cancel(this.operation.id, { from: proposer }); + it('canceller can cancel', async function () { + const receipt = await this.timelock.cancel(this.operation.id, { from: canceller }); expectEvent(receipt, 'Cancelled', { id: this.operation.id }); }); it('cannot cancel invalid operation', async function () { await expectRevert( - this.timelock.cancel(constants.ZERO_BYTES32, { from: proposer }), + this.timelock.cancel(constants.ZERO_BYTES32, { from: canceller }), 'TimelockController: operation cannot be cancelled', ); }); - it('prevent non-proposer from canceling', async function () { + it('prevent non-canceller from canceling', async function () { await expectRevert( this.timelock.cancel(this.operation.id, { from: other }), - `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`, + `AccessControl: account ${other.toLowerCase()} is missing role ${CANCELLER_ROLE}`, ); }); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index ade1c545a7c..85dd0ea7503 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -18,6 +18,11 @@ const CallReceiver = artifacts.require('CallReceiverMock'); contract('GovernorTimelockControl', function (accounts) { const [ admin, voter, other ] = accounts; + const TIMELOCK_ADMIN_ROLE = web3.utils.soliditySha3('TIMELOCK_ADMIN_ROLE'); + const PROPOSER_ROLE = web3.utils.soliditySha3('PROPOSER_ROLE'); + const EXECUTOR_ROLE = web3.utils.soliditySha3('EXECUTOR_ROLE'); + const CANCELLER_ROLE = web3.utils.soliditySha3('CANCELLER_ROLE'); + const name = 'OZ-Governor'; // const version = '1'; const tokenName = 'MockToken'; @@ -31,11 +36,20 @@ contract('GovernorTimelockControl', function (accounts) { this.timelock = await Timelock.new(3600, [], []); this.mock = await Governor.new(name, this.token.address, 4, 16, this.timelock.address, 0); this.receiver = await CallReceiver.new(); - // normal setup: governor and admin are proposers, everyone is executor, timelock is its own admin - await this.timelock.grantRole(await this.timelock.PROPOSER_ROLE(), this.mock.address); - await this.timelock.grantRole(await this.timelock.PROPOSER_ROLE(), admin); - await this.timelock.grantRole(await this.timelock.EXECUTOR_ROLE(), constants.ZERO_ADDRESS); - await this.timelock.revokeRole(await this.timelock.TIMELOCK_ADMIN_ROLE(), deployer); + + this.TIMELOCK_ADMIN_ROLE = await this.timelock.TIMELOCK_ADMIN_ROLE(); + this.PROPOSER_ROLE = await this.timelock.PROPOSER_ROLE(); + this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE(); + this.CANCELLER_ROLE = await this.timelock.CANCELLER_ROLE(); + + // normal setup: governor is proposer, everyone is executor, timelock is its own admin + await this.timelock.grantRole(PROPOSER_ROLE, this.mock.address); + await this.timelock.grantRole(PROPOSER_ROLE, admin); + await this.timelock.grantRole(CANCELLER_ROLE, this.mock.address); + await this.timelock.grantRole(CANCELLER_ROLE, admin); + await this.timelock.grantRole(EXECUTOR_ROLE, constants.ZERO_ADDRESS); + await this.timelock.revokeRole(TIMELOCK_ADMIN_ROLE, deployer); + await this.token.mint(voter, tokenSupply); await this.token.delegate(voter, { from: voter }); });