Prevent proposals with duplicate actions in GovernorTimelockCompound

.changeset/prevent-duplicate-proposal-actions.md

@@ -0,0 +1,5 @@
+---
+"openzeppelin-solidity": patch
+---
+
+Prevent proposals with duplicate actions from being submitted in `GovernorTimelockCompound`

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -21,6 +21,12 @@ import {SafeCast} from "../../utils/math/SafeCast.sol";
 abstract contract GovernorTimelockCompound is Governor {
     ICompoundTimelock private _timelock;
 
+    /**
+     * @dev The proposal contains duplicate actions (same target, value, and calldata), which would cause queueing to
+     * fail in the Compound Timelock.
+     */
+    error GovernorDuplicateProposalAction(uint256 index);
+
     /**
      * @dev Emitted when the timelock controller used for proposal execution is modified.
      */
@@ -58,6 +64,33 @@ abstract contract GovernorTimelockCompound is Governor {
         return true;
     }
 
+    /**
+     * @dev Override of {Governor-_propose} that rejects proposals containing duplicate actions. The Compound Timelock
+     * identifies queued transactions by a hash of (target, value, signature, calldata, eta). If two actions in a
+     * proposal share the same target, value, and calldata, they would produce the same hash, causing the second one
+     * to fail during queueing. This check prevents such proposals from being created in the first place.
+     */
+    function _propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description,
+        address proposer
+    ) internal virtual override returns (uint256) {
+        for (uint256 i = 1; i < targets.length; ++i) {
+            for (uint256 j = 0; j < i; ++j) {
+                if (
+                    targets[i] == targets[j] &&
+                    values[i] == values[j] &&
+                    keccak256(calldatas[i]) == keccak256(calldatas[j])
+                ) {
+                    revert GovernorDuplicateProposalAction(i);
+                }
+            }
+        }
+        return super._propose(targets, values, calldatas, description, proposer);
+    }
+
     /**
      * @dev Function to queue a proposal to the timelock.
      */

contracts/mocks/governance/GovernorTimelockCompoundMock.sol

@@ -34,6 +34,16 @@ abstract contract GovernorTimelockCompoundMock is
         return super.proposalNeedsQueuing(proposalId);
     }
 
+    function _propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description,
+        address proposer
+    ) internal override(Governor, GovernorTimelockCompound) returns (uint256) {
+        return super._propose(targets, values, calldatas, description, proposer);
+    }
+
     function _queueOperations(
         uint256 proposalId,
         address[] memory targets,

test/governance/extensions/GovernorTimelockCompound.test.js

@@ -146,18 +146,27 @@ describe('GovernorTimelockCompound', function () {
               target: this.token.target,
               data: this.token.interface.encodeFunctionData('approve', [this.receiver.target, ethers.MaxUint256]),
             };
-            const { id } = this.helper.setProposal([action, action], '<proposal description>');
+            this.helper.setProposal([action, action], '<proposal description>');
 
-            await this.helper.propose();
-            await this.helper.waitForSnapshot();
-            await this.helper.connect(this.voter1).vote({ support: VoteType.For });
-            await this.helper.waitForDeadline();
-            await expect(this.helper.queue())
-              .to.be.revertedWithCustomError(this.mock, 'GovernorAlreadyQueuedProposal')
-              .withArgs(id);
-            await expect(this.helper.execute())
-              .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
-              .withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]));
+            await expect(this.helper.propose())
+              .to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
+              .withArgs(1n);
+          });
+
+          it('if proposal contains non-adjacent duplicate calls', async function () {
+            const action1 = {
+              target: this.token.target,
+              data: this.token.interface.encodeFunctionData('approve', [this.receiver.target, ethers.MaxUint256]),
+            };
+            const action2 = {
+              target: this.receiver.target,
+              data: this.receiver.interface.encodeFunctionData('mockFunction'),
+            };
+            this.helper.setProposal([action1, action2, action1], '<proposal description>');
+
+            await expect(this.helper.propose())
+              .to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
+              .withArgs(2n);
           });
         });