Skip to content

Commit

Permalink
Add a canceller role to the TimelockController (#3165)
Browse files Browse the repository at this point in the history 
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
  • Loading branch information
@Amxx @frangio
Amxx and frangio authored Mar 8, 2022
1 parent 62eb456 commit 8b162e3
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 22 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
20 changes: 16 additions & 4 deletions contracts/governance/TimelockController.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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];

Expand Down
50 changes: 37 additions & 13 deletions test/governance/TimelockController.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -53,16 +58,35 @@ 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 });
});

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 () {
Expand Down Expand Up @@ -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}`,
);
});

Expand Down Expand Up @@ -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}`,
);
});
});
Expand Down Expand Up @@ -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}`,
);
});

Expand Down Expand Up @@ -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}`,
);
});

Expand Down Expand Up @@ -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}`,
);
});
});
Expand Down
24 changes: 19 additions & 5 deletions test/governance/extensions/GovernorTimelockControl.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 });
});
Expand Down

0 comments on commit 8b162e3

Please sign in to comment.