diff --git a/.changeset/purple-cats-cheer.md b/.changeset/purple-cats-cheer.md new file mode 100644 index 00000000000..7e9dc1c4ddd --- /dev/null +++ b/.changeset/purple-cats-cheer.md @@ -0,0 +1,5 @@ +--- +'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. 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..8dde8acb97b 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) => + '0x' + web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)).toString(16, 64); + 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', [