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.

Sign up for GitHub

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
merged 13 commits into from
Jul 12, 2023
Merged

Add GovernorTimelockControl address to TimelockController salt #4432

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4a9a6aa
Add GovernorTimelockControl address to TimelockController salt
ernestognw Jul 6, 2023
00ff5e1
Add changeset
ernestognw Jul 6, 2023
b63d484
Merge branch 'master' into lib-834-use-governor-address-in-id-hash
ernestognw Jul 9, 2023
bcb0b84
Merge branch 'master' into lib-834-use-governor-address-in-id-hash
ernestognw Jul 12, 2023
fd0ef64
add logging
frangio Jul 12, 2023
5903861
remove only
frangio Jul 12, 2023
ce91062
add only
frangio Jul 12, 2023
29a1b1d
fix only
frangio Jul 12, 2023
4d21d32
remove only
frangio Jul 12, 2023
225c6ec
fix timelockSalt
frangio Jul 12, 2023
3563815
remove console.log
frangio Jul 12, 2023
e775b46
add period
frangio Jul 12, 2023
9dd7f39
Update .changeset/purple-cats-cheer.md
ernestognw Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/purple-cats-cheer.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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