From 4a9a6aac94cc6b00b2cde344c11c9659e65779a1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 6 Jul 2023 13:12:42 -0600 Subject: [PATCH 01/11] Add GovernorTimelockControl address to TimelockController salt --- .../extensions/GovernorTimelockControl.sol | 17 ++++++++++++++--- .../extensions/GovernorTimelockControl.test.js | 8 ++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 11156940843..e6dea036f4f 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -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); emit ProposalQueued(proposalId, block.timestamp + delay); @@ -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]; } @@ -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; + } } diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 5954d4117f7..a502120c446 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -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(); @@ -86,10 +89,11 @@ contract('GovernorTimelockControl', function (accounts) { ], '', ); + 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]), ); }); @@ -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', [ From 00ff5e138a37833e20edc2287d1918f23f97f005 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 6 Jul 2023 13:12:50 -0600 Subject: [PATCH 02/11] Add changeset --- .changeset/purple-cats-cheer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-cats-cheer.md diff --git a/.changeset/purple-cats-cheer.md b/.changeset/purple-cats-cheer.md new file mode 100644 index 00000000000..b4114f05023 --- /dev/null +++ b/.changeset/purple-cats-cheer.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController From fd0ef64303ef0a7c642f55edb020a1ef05428009 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 13:39:48 -0300 Subject: [PATCH 03/11] add logging --- test/governance/extensions/GovernorTimelockControl.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index a502120c446..4567e199399 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -153,7 +153,7 @@ contract('GovernorTimelockControl', function (accounts) { }); describe('on execute', function () { - it('if not queued', async function () { + it.only('if not queued', async function () { await this.helper.propose(); await this.helper.waitForSnapshot(); await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); @@ -161,6 +161,8 @@ contract('GovernorTimelockControl', function (accounts) { expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Succeeded); + console.log(this.mock.address, this.proposal.shortProposal[3]); + await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [ this.proposal.timelockid, proposalStatesToBitMap(Enums.OperationState.Ready), From 59038617e78a7609c7896a733fbec0b0cff2a30e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 13:43:42 -0300 Subject: [PATCH 04/11] remove only --- test/governance/extensions/GovernorTimelockControl.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 4567e199399..27dbf5b50b9 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -153,7 +153,7 @@ contract('GovernorTimelockControl', function (accounts) { }); describe('on execute', function () { - it.only('if not queued', async function () { + it('if not queued', async function () { await this.helper.propose(); await this.helper.waitForSnapshot(); await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); From ce910625c627b43a0ec3eab05b42e7b90cf8bb9b Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 13:44:06 -0300 Subject: [PATCH 05/11] add only --- test/governance/extensions/GovernorTimelockControl.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 27dbf5b50b9..34cc7da94a5 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -18,7 +18,7 @@ const TOKENS = [ { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, ]; -contract('GovernorTimelockControl', function (accounts) { +contract.only('GovernorTimelockControl', function (accounts) { const [owner, voter1, voter2, voter3, voter4, other] = accounts; const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; From 29a1b1da90540e3474937833fcc6caf2893ebea4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 13:48:03 -0300 Subject: [PATCH 06/11] fix only --- test/governance/extensions/GovernorTimelockControl.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 34cc7da94a5..0a804517554 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -18,7 +18,8 @@ const TOKENS = [ { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, ]; -contract.only('GovernorTimelockControl', function (accounts) { +describe.only('only', function () { +contract('GovernorTimelockControl', function (accounts) { const [owner, voter1, voter2, voter3, voter4, other] = accounts; const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; @@ -512,3 +513,4 @@ contract.only('GovernorTimelockControl', function (accounts) { }); } }); +}) From 4d21d328f8df4ea02ed6b556411491881acacbeb Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 13:56:19 -0300 Subject: [PATCH 07/11] remove only --- test/governance/extensions/GovernorTimelockControl.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 0a804517554..27dbf5b50b9 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -18,7 +18,6 @@ const TOKENS = [ { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, ]; -describe.only('only', function () { contract('GovernorTimelockControl', function (accounts) { const [owner, voter1, voter2, voter3, voter4, other] = accounts; @@ -513,4 +512,3 @@ contract('GovernorTimelockControl', function (accounts) { }); } }); -}) From 225c6ec94e9b94e5205a42aecfe9faec7b8d2996 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 16:51:19 -0300 Subject: [PATCH 08/11] fix timelockSalt --- test/governance/extensions/GovernorTimelockControl.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 27dbf5b50b9..27628a8d363 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -38,7 +38,7 @@ 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))); + '0x' + web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)).toString(16, 64); beforeEach(async function () { const [deployer] = await web3.eth.getAccounts(); From 3563815ddc0e2f9bc1ea18b63dcf16d94cdc985c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 12 Jul 2023 16:57:04 -0300 Subject: [PATCH 09/11] remove console.log --- test/governance/extensions/GovernorTimelockControl.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 27628a8d363..8dde8acb97b 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -161,8 +161,6 @@ contract('GovernorTimelockControl', function (accounts) { expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Succeeded); - console.log(this.mock.address, this.proposal.shortProposal[3]); - await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [ this.proposal.timelockid, proposalStatesToBitMap(Enums.OperationState.Ready), From e775b46556c5b2f67950197a4b1e5fd1abc2549f Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 12 Jul 2023 16:58:16 -0300 Subject: [PATCH 10/11] add period --- .changeset/purple-cats-cheer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/purple-cats-cheer.md b/.changeset/purple-cats-cheer.md index b4114f05023..3d033ec941d 100644 --- a/.changeset/purple-cats-cheer.md +++ b/.changeset/purple-cats-cheer.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController +`GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController. From 9dd7f3915ad8ab8a8f959c1ded5d55221a4b3126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 12 Jul 2023 15:10:56 -0600 Subject: [PATCH 11/11] Update .changeset/purple-cats-cheer.md Co-authored-by: Francisco --- .changeset/purple-cats-cheer.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/purple-cats-cheer.md b/.changeset/purple-cats-cheer.md index 3d033ec941d..7e9dc1c4ddd 100644 --- a/.changeset/purple-cats-cheer.md +++ b/.changeset/purple-cats-cheer.md @@ -1,5 +1,5 @@ --- -'openzeppelin-solidity': minor +'openzeppelin-solidity': major --- `GovernorTimelockControl`: Add the Governor instance address as part of the TimelockController operation `salt` to avoid operation id collisions between governors using the same TimelockController.