Add GovernorTimelockControl address to TimelockController salt (O…

…penZeppelin#4432) Co-authored-by: Francisco Giordano <fg@frang.io>
ernestognw · Jul 12, 2023 · b6c5abb · b6c5abb
1 parent 0abf18f
commit b6c5abb
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 5 deletions.
diff --git a/.changeset/purple-cats-cheer.md 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
@@ -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
@@ -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', [