Governance: reject duplicate actions before proposal creation

.changeset/green-teams-taste.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`GovernorTimelockCompound`: reject proposals with duplicate actions before queueing to avoid creating proposals that can only fail later at queue time.

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -58,6 +58,54 @@ abstract contract GovernorTimelockCompound is Governor {
         return true;
     }
 
+    /**
+     * @dev Check whether a proposal contains duplicate actions.
+     *
+     * Duplicate actions are identified by an exact match on target, value, and calldata.
+     */
+    function _hasDuplicateActions(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas
+    ) internal pure returns (bool) {
+        for (uint256 i = 0; i < targets.length; ++i) {
+            for (uint256 j = i + 1; j < targets.length; ++j) {
+                if (
+                    targets[i] == targets[j] &&
+                    values[i] == values[j] &&
+                    calldatas[i].length == calldatas[j].length &&
+                    keccak256(calldatas[i]) == keccak256(calldatas[j])
+                ) {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * @inheritdoc Governor
+     */
+    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 &&
+            _hasDuplicateActions(targets, values, calldatas)
+        ) {
+            uint256 proposalId = getProposalId(targets, values, calldatas, keccak256(bytes(description)));
+            revert GovernorAlreadyQueuedProposal(proposalId);
+        }
+
+        return super._propose(targets, values, calldatas, description, proposer);
+    }
+
     /**
      * @dev Function to queue a proposal to the timelock.
      */
@@ -148,7 +196,7 @@ abstract contract GovernorTimelockCompound is Governor {
      * Note that if the timelock admin has been handed over in a previous operation, we refuse updates made through the
      * timelock if admin of the timelock has already been accepted and the operation is executed outside the scope of
      * governance.
-
+     *
      * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
      */
     function updateTimelock(ICompoundTimelock newTimelock) public virtual onlyGovernance {

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

@@ -148,16 +148,9 @@ describe('GovernorTimelockCompound', function () {
             };
             const { id } = 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())
+            await expect(this.helper.propose())
               .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]));
           });
         });