Add `GovernorTimelockControl` address to `TimelockController` salt (#4432)

Co-authored-by: Francisco Giordano <fg@frang.io>

.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.

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;
+    }
 }

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) {
           ],
           '<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]),
         );
       });
 
@@ -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', [