Governance: reject duplicate actions before proposal creation

[timeless-hayoka]

Summary

Reject duplicate actions in GovernorTimelockCompound at proposal creation time instead of letting them fail later during queueing. This keeps the failure closer to the source and avoids creating proposals that are guaranteed to fail queue-time validation.

Details

• Adds an internal duplicate-action check in GovernorTimelockCompound
• Reuses GovernorAlreadyQueuedProposal for the revert path
• Updates the governance test to assert the revert happens during propose()
• Adds a changeset for release notes

Verification

• npm exec -- hardhat test test/governance/extensions/GovernorTimelockCompound.test.js
• forge fmt --check contracts/governance/extensions/GovernorTimelockCompound.sol test/governance/extensions/GovernorTimelockCompound.test.js

[changeset-bot]

🦋 Changeset detected

Latest commit: 24eea91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coderabbitai]

Walkthrough

GovernorTimelockCompound gains a new internal helper _hasDuplicateActions that performs an O(n²) scan over the targets, values, and calldatas arrays, flagging duplicates by matching address, uint value, calldata length, and keccak256 of calldata. A new _propose override calls this helper and, when duplicates are found, computes the proposalId via getProposalId and reverts with GovernorAlreadyQueuedProposal(proposalId); otherwise it delegates to super._propose. GovernorTimelockCompoundMock adds a matching _propose override to resolve multi-inheritance. The corresponding test is updated to assert the revert occurs at propose() time rather than at queue() time. A changeset entry documents the patch release.

Possibly related issues

• Prevent proposals with duplicate actions from being submitted in GovernorTimelockCompound #6431: This PR directly implements the change described in that issue — rejecting proposals with duplicate actions in _propose before they reach the queue, matching the stated objective.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting duplicate governance actions before proposal creation.
Description check	✅ Passed	The description matches the implemented change, tests, and release-note update in the pull request.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

contracts/governance/extensions/GovernorTimelockCompound.sol (1)

71-82: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Hoist/precompute calldata hashes to avoid redundant keccak256 in the inner loop.

keccak256(calldatas[i]) is recomputed on every j iteration. Precomputing each calldata hash once reduces hashing from O(n²) to O(n) while preserving the cheap length short-circuit.

♻️ Proposed optimization

     ) internal pure returns (bool) {
-        for (uint256 i = 0; i < targets.length; ++i) {
-            for (uint256 j = i + 1; j < targets.length; ++j) {
+        bytes32[] memory calldataHashes = new bytes32[](calldatas.length);
+        for (uint256 i = 0; i < calldatas.length; ++i) {
+            calldataHashes[i] = keccak256(calldatas[i]);
+        }
+        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])
+                    calldataHashes[i] == calldataHashes[j]
                 ) {
                     return true;
                 }
             }
         }

Note: since calldata-hash equality already implies equal length, the calldatas[i].length == calldatas[j].length check becomes redundant once hashes are compared, and could be dropped.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/governance/extensions/GovernorTimelockCompound.sol` around lines 71
- 82, In GovernorTimelockCompound’s duplicate-check loop,
`keccak256(calldatas[i])` is being recomputed for every inner-loop comparison,
causing unnecessary O(n²) hashing. Precompute and cache each calldata hash once
in the surrounding logic, then compare the cached hashes inside the nested
loops; you can keep the cheap equality checks on `targets` and `values`, and
drop the `calldatas[i].length == calldatas[j].length` check because hash
equality already covers it.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@contracts/governance/extensions/GovernorTimelockCompound.sol`:
- Around line 71-82: In GovernorTimelockCompound’s duplicate-check loop,
`keccak256(calldatas[i])` is being recomputed for every inner-loop comparison,
causing unnecessary O(n²) hashing. Precompute and cache each calldata hash once
in the surrounding logic, then compare the cached hashes inside the nested
loops; you can keep the cheap equality checks on `targets` and `values`, and
drop the `calldatas[i].length == calldatas[j].length` check because hash
equality already covers it.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6048d330-f5e6-4606-a4f2-1dccb6eb7103

📥 Commits

Reviewing files that changed from the base of the PR and between 5c70e56 and dff5b63.

📒 Files selected for processing (4)

• .changeset/green-teams-taste.md
• contracts/governance/extensions/GovernorTimelockCompound.sol
• contracts/mocks/governance/GovernorTimelockCompoundMock.sol
• test/governance/extensions/GovernorTimelockCompound.test.js

[timeless-hayoka]

happy to help