Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GovernorTimelockControl address to TimelockController salt #4432

Merged
5 changes: 5 additions & 0 deletions .changeset/purple-cats-cheer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
---

`GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController
17 changes: 14 additions & 3 deletions contracts/governance/extensions/GovernorTimelockControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,9 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor {
}

uint256 delay = _timelock.getMinDelay();
_timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, descriptionHash);
_timelock.scheduleBatch(targets, values, calldatas, 0, descriptionHash, delay);
bytes32 salt = _timelockSalt(descriptionHash);
_timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, salt);
_timelock.scheduleBatch(targets, values, calldatas, 0, salt, delay);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The decision to not have schedule and scheduleBatch return the id that they compute really was a bad one.


emit ProposalQueued(proposalId, block.timestamp + delay);

Expand All @@ -125,7 +126,7 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor {
bytes32 descriptionHash
) internal virtual override {
// execute
_timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, descriptionHash);
_timelock.executeBatch{value: msg.value}(targets, values, calldatas, 0, _timelockSalt(descriptionHash));
// cleanup for refund
delete _timelockIds[proposalId];
}
Expand Down Expand Up @@ -177,4 +178,14 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor {
emit TimelockChange(address(_timelock), address(newTimelock));
_timelock = newTimelock;
}

/**
* @dev Computes the {TimelockController} operation salt.
*
* It is computed with the governor address itself to avoid collisions across governor instances using the
* same timelock.
*/
function _timelockSalt(bytes32 descriptionHash) private view returns (bytes32) {
return bytes20(address(this)) ^ descriptionHash;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to use an ^ instead of a keccak256 because it's cheaper and it doesn't matter if the preimage is known since the objective is to avoid unintentional collisions between governors.

If a governor instance wants to purposely block another, it'll require to mine the most significant 20 bytes of the descriptionHash which is infeasible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting proposal. I'd have done a full hash, just because it would have been clearer to everyone what is going on ... but IMO that is also a valid option.

}
}
8 changes: 6 additions & 2 deletions test/governance/extensions/GovernorTimelockControl.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ contract('GovernorTimelockControl', function (accounts) {

for (const { mode, Token } of TOKENS) {
describe(`using ${Token._json.contractName}`, function () {
const timelockSalt = (address, descriptionHash) =>
web3.utils.numberToHex(web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)));

beforeEach(async function () {
const [deployer] = await web3.eth.getAccounts();

Expand Down Expand Up @@ -86,10 +89,11 @@ contract('GovernorTimelockControl', function (accounts) {
],
'<proposal description>',
);

this.proposal.timelockid = await this.timelock.hashOperationBatch(
...this.proposal.shortProposal.slice(0, 3),
'0x0',
this.proposal.shortProposal[3],
timelockSalt(this.mock.address, this.proposal.shortProposal[3]),
);
});

Expand Down Expand Up @@ -204,7 +208,7 @@ contract('GovernorTimelockControl', function (accounts) {
await this.timelock.executeBatch(
...this.proposal.shortProposal.slice(0, 3),
'0x0',
this.proposal.shortProposal[3],
timelockSalt(this.mock.address, this.proposal.shortProposal[3]),
);

await expectRevertCustomError(this.helper.execute(), 'GovernorUnexpectedProposalState', [
Expand Down