Add a canceller role to the TimelockController (#3165)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

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
 

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];
 

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();
+    expect(await this.timelock.hasRole(CANCELLER_ROLE, proposer)).to.be.equal(true);
-    this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE();
+    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 () {
+      it('canceller can cancel', async function () {
-        const receipt = await this.timelock.cancel(this.operation.id, { from: proposer });
+        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}`,
         );
       });
     });

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);
+    this.TIMELOCK_ADMIN_ROLE = await this.timelock.TIMELOCK_ADMIN_ROLE();
-    await this.timelock.grantRole(await this.timelock.PROPOSER_ROLE(), admin);
+    this.PROPOSER_ROLE = await this.timelock.PROPOSER_ROLE();
-    await this.timelock.grantRole(await this.timelock.EXECUTOR_ROLE(), constants.ZERO_ADDRESS);
+    this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE();
-    await this.timelock.revokeRole(await this.timelock.TIMELOCK_ADMIN_ROLE(), deployer);
+    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 });
   });