Prevent proposals with duplicate actions in GovernorTimelockCompound

[gabrielrondon]

Closes #6431

Introduced changes

Override _propose in GovernorTimelockCompound to reject proposals containing duplicate actions (same target, value, and calldata) at creation time.

The Compound Timelock identifies queued transactions by a hash of (target, value, signature, calldata, eta). If two actions share the same target, value, and calldata, they produce the same hash — causing the second one to fail during queueing. This check prevents such proposals from being created in the first place, rather than letting them fail after voting ends.

• New error: GovernorDuplicateProposalAction(uint256 index) — reports the index of the first duplicate found
• O(n²) pairwise comparison of (target, value, keccak256(calldata)) tuples — negligible cost since action arrays are typically single digits
• Override _propose (not propose) since NatSpec marks it as the intended extension point — also covers proposeBySig

Checklist

• Linked relevant issue
• Updated relevant documentation
• Added relevant tests
• Performed self-review of the code
• Added changes to CHANGELOG.md

[changeset-bot]

🦋 Changeset detected

Latest commit: 2049859

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

The changes introduce duplicate action detection to the proposal submission process in GovernorTimelockCompound. A new custom error GovernorDuplicateProposalAction(uint256 index) was added, along with an _propose override that scans proposal action arrays for duplicates—matching targets, values, and calldata hashes. Duplicate proposals are rejected at submission time with the custom error, preventing unexecutable proposals from advancing. The mock contract was updated to forward the override correctly, and test cases were revised to verify the rejection occurs during proposal creation rather than at later stages.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Prevent proposals with duplicate actions in GovernorTimelockCompound' clearly and concisely summarizes the main change: adding duplicate action prevention to the Governor component.
Description check	✅ Passed	The description is directly related to the changeset, explaining the override of _propose to reject proposals with duplicate actions, the rationale, and implementation details.
Linked Issues check	✅ Passed	The PR fully addresses issue #6431 by implementing duplicate action detection in _propose and rejecting proposals with matching target, value, and calldata at creation time.
Out of Scope Changes check	✅ Passed	All changes are scoped to the stated objectives: adding duplicate action detection, new error type, updated tests, and mock implementation to support the new override.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

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

80-90: Consider precomputing calldata hashes for gas efficiency (optional).

The current implementation computes keccak256(calldatas[i]) and keccak256(calldatas[j]) inside the inner loop, resulting in ~n² hash computations. For typical small action arrays this is negligible, but precomputing hashes would reduce this to n computations.

🔧 Potential optimization

 function _propose(
     address[] memory targets,
     uint256[] memory values,
     bytes[] memory calldatas,
     string memory description,
     address proposer
 ) internal virtual override returns (uint256) {
+    bytes32[] memory calldataHashes = new bytes32[](calldatas.length);
+    for (uint256 k = 0; k < calldatas.length; ++k) {
+        calldataHashes[k] = keccak256(calldatas[k]);
+    }
     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])
+                calldataHashes[i] == calldataHashes[j]
             ) {
                 revert GovernorDuplicateProposalAction(i);
             }
         }
     }
     return super._propose(targets, values, calldatas, description, proposer);
 }

This trades memory allocation (n × 32 bytes) for reduced hash computations. Given that proposal creation is infrequent and action counts are typically small, the current implementation is acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/governance/extensions/GovernorTimelockCompound.sol` around lines 80
- 90, The duplicate-action check in the loop computes keccak256(calldatas[i])
and keccak256(calldatas[j]) repeatedly; to optimize, precompute an array of
calldata hashes once before the nested loops (e.g., create bytes32[]
calldataHashes and fill it by hashing each calldatas[k]), then use
calldataHashes[i] and calldataHashes[j] inside the inner loop when comparing
alongside targets and values and keep the existing revert
GovernorDuplicateProposalAction(i) behavior.

test/governance/extensions/GovernorTimelockCompound.test.js (1)

128-128: Consider reorganizing duplicate action tests under a more appropriate describe block.

These tests now verify behavior at proposal creation time (propose()), not during queueing. Keeping them under describe('on queue', ...) may confuse future readers.

Suggested reorganization

Consider moving the duplicate action tests to their own describe block, e.g.:

describe('on propose', function () {
  it('if proposal contains duplicate calls', async function () { ... });
  it('if proposal contains non-adjacent duplicate calls', async function () { ... });
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/governance/extensions/GovernorTimelockCompound.test.js` at line 128, The
tests that check duplicate-action behavior run at proposal creation (propose())
but are currently nested under describe('on queue', ...) which is misleading;
move those test cases into a new describe block named something like
describe('on propose', ...) and place the two it(...) tests that assert "if
proposal contains duplicate calls" and "if proposal contains non-adjacent
duplicate calls" inside it so they clearly reflect they exercise propose()
instead of the queue path, updating any setup/describe-level hooks if needed to
keep test context consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@contracts/governance/extensions/GovernorTimelockCompound.sol`:
- Around line 80-90: The duplicate-action check in the loop computes
keccak256(calldatas[i]) and keccak256(calldatas[j]) repeatedly; to optimize,
precompute an array of calldata hashes once before the nested loops (e.g.,
create bytes32[] calldataHashes and fill it by hashing each calldatas[k]), then
use calldataHashes[i] and calldataHashes[j] inside the inner loop when comparing
alongside targets and values and keep the existing revert
GovernorDuplicateProposalAction(i) behavior.

In `@test/governance/extensions/GovernorTimelockCompound.test.js`:
- Line 128: The tests that check duplicate-action behavior run at proposal
creation (propose()) but are currently nested under describe('on queue', ...)
which is misleading; move those test cases into a new describe block named
something like describe('on propose', ...) and place the two it(...) tests that
assert "if proposal contains duplicate calls" and "if proposal contains
non-adjacent duplicate calls" inside it so they clearly reflect they exercise
propose() instead of the queue path, updating any setup/describe-level hooks if
needed to keep test context consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 561c70a2-c88f-4b66-a895-bcb7574656dc

📥 Commits

Reviewing files that changed from the base of the PR and between 9cfdccd and e8301cb.

📒 Files selected for processing (3)

• contracts/governance/extensions/GovernorTimelockCompound.sol
• contracts/mocks/governance/GovernorTimelockCompoundMock.sol
• test/governance/extensions/GovernorTimelockCompound.test.js