Reject proposals with duplicate actions in GovernorTimelockCompound

.changeset/governor-timelock-compound-duplicate-actions.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`GovernorTimelockCompound`: Reject proposals containing duplicate actions (same target, value, and calldata) at proposal creation time with a new `GovernorDuplicateProposalAction` error, rather than failing later during queueing.

contracts/governance/IGovernor.sol

@@ -102,6 +102,12 @@ interface IGovernor is IERC165, IERC6372 {
      */
     error GovernorInvalidSignature(address voter);
 
+    /**
+     * @dev The proposal contains duplicate actions (same target, value, and calldata). This is not supported
+     * by the Compound timelock, which identifies queued transactions by their hash.
+     */
+    error GovernorDuplicateProposalAction(uint256 index);
+
     /**
      * @dev The given `account` is unable to cancel the proposal with given `proposalId`.
      */

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -58,6 +58,36 @@ abstract contract GovernorTimelockCompound is Governor {
         return true;
     }
 
+    /**
+     * @dev Override of {Governor-_propose} that rejects proposals containing duplicate actions (same target, value,
+     * and calldata). The Compound timelock identifies queued transactions by their hash, so duplicate actions within
+     * a single proposal would cause the second `queueTransaction` call to fail. This check prevents such proposals
+     * from being created in the first place, providing a clearer error message.
+     */
+    function _propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description,
+        address proposer
+    ) internal virtual override returns (uint256) {
+        if (targets.length != values.length || targets.length != calldatas.length || targets.length == 0) {
+            revert GovernorInvalidProposalLength(targets.length, calldatas.length, values.length);
+        }
+        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

@@ -63,6 +63,16 @@ abstract contract GovernorTimelockCompoundMock is
         return super._cancel(targets, values, calldatas, descriptionHash);
     }
 
+    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 _executor() internal view override(Governor, GovernorTimelockCompound) returns (address) {
         return super._executor();
     }

test/governance/extensions/GovernorTimelockCompound.test.js

@@ -146,18 +146,47 @@ 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 is empty', async function () {
+            this.helper.setProposal([], '<proposal description>');
+
+            await expect(this.helper.propose())
+              .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidProposalLength')
+              .withArgs(0n, 0n, 0n);
+          });
+
+          it('if targets/values lengths mismatch', async function () {
+            this.helper.setProposal(
+              {
+                targets: [this.receiver.target],
+                values: [],
+                data: [this.receiver.interface.encodeFunctionData('mockFunction')],
+              },
+              '<proposal description>',
+            );
+            await expect(this.helper.propose())
+              .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidProposalLength')
+              .withArgs(1n, 1n, 0n);
+          });
+
+          it('if targets/calldatas lengths mismatch', async function () {
+            this.helper.setProposal(
+              {
+                targets: [this.receiver.target],
+                values: [0n],
+                data: [],
+              },
+              '<proposal description>',
+            );
+            await expect(this.helper.propose())
+              .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidProposalLength')
+              .withArgs(1n, 0n, 1n);
           });
         });