Prevent proposals with duplicate actions in GovernorTimelockCompound#6469
Conversation
🦋 Changeset detectedLatest commit: 97b6534 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughThis change adds validation to the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/governance/extensions/GovernorTimelockCompound.sol`:
- Around line 75-93: In _propose, validate array lengths before performing the
duplicate-action nested loop: check that targets.length == values.length and
targets.length == calldatas.length and revert with
GovernorInvalidProposalLength() if not; this ensures the function does not read
out-of-bounds (e.g., when accessing values[i] or calldatas[i]) and preserves the
original error semantics before calling super._propose(targets, values,
calldatas, description, proposer) and keeping the duplicate-action check that
reverts with GovernorTimelockCompoundDuplicateProposalAction(i).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 205db154-88b8-400d-9798-8ac8ea3f28e9
📒 Files selected for processing (4)
.changeset/governor-timelock-duplicate-actions.mdcontracts/governance/extensions/GovernorTimelockCompound.solcontracts/mocks/governance/GovernorTimelockCompoundMock.soltest/governance/extensions/GovernorTimelockCompound.test.js
| function _propose( | ||
| address[] memory targets, | ||
| uint256[] memory values, | ||
| bytes[] memory calldatas, | ||
| string memory description, | ||
| address proposer | ||
| ) internal virtual override returns (uint256) { | ||
| 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] && | ||
| keccak256(calldatas[i]) == keccak256(calldatas[j]) | ||
| ) { | ||
| revert GovernorTimelockCompoundDuplicateProposalAction(i); | ||
| } | ||
| } | ||
| } | ||
| return super._propose(targets, values, calldatas, description, proposer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Changed override:"
rg -n -C6 'function _propose\(|GovernorTimelockCompoundDuplicateProposalAction|keccak256\(calldatas\[i\]\)' contracts/governance/extensions/GovernorTimelockCompound.sol
echo
echo "Base Governor length validation:"
rg -n -C8 'function _propose\(|GovernorInvalidProposalLength' contracts/governance/Governor.sol
echo
echo "Existing tests/assertions that mention GovernorInvalidProposalLength:"
rg -n -C2 'GovernorInvalidProposalLength' testRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 4770
Add array-length validation before the duplicate-action scan.
The nested loop starting at line 82 accesses values[i] and calldatas[i] before super._propose() runs its length check. If targets.length exceeds values.length or calldatas.length, the override now panics on an out-of-bounds read instead of reverting with the expected GovernorInvalidProposalLength error, breaking the contract's error semantics and failing existing tests.
Minimal fix
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) {
+ return super._propose(targets, values, calldatas, description, proposer);
+ }
+
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] &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _propose( | |
| address[] memory targets, | |
| uint256[] memory values, | |
| bytes[] memory calldatas, | |
| string memory description, | |
| address proposer | |
| ) internal virtual override returns (uint256) { | |
| 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] && | |
| keccak256(calldatas[i]) == keccak256(calldatas[j]) | |
| ) { | |
| revert GovernorTimelockCompoundDuplicateProposalAction(i); | |
| } | |
| } | |
| } | |
| return super._propose(targets, values, calldatas, description, proposer); | |
| 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) { | |
| return super._propose(targets, values, calldatas, description, proposer); | |
| } | |
| 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] && | |
| keccak256(calldatas[i]) == keccak256(calldatas[j]) | |
| ) { | |
| revert GovernorTimelockCompoundDuplicateProposalAction(i); | |
| } | |
| } | |
| } | |
| return super._propose(targets, values, calldatas, description, proposer); |
🤖 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 75
- 93, In _propose, validate array lengths before performing the duplicate-action
nested loop: check that targets.length == values.length and targets.length ==
calldatas.length and revert with GovernorInvalidProposalLength() if not; this
ensures the function does not read out-of-bounds (e.g., when accessing values[i]
or calldatas[i]) and preserves the original error semantics before calling
super._propose(targets, values, calldatas, description, proposer) and keeping
the duplicate-action check that reverts with
GovernorTimelockCompoundDuplicateProposalAction(i).
Sanenelisiwe1975
force-pushed
the
fix/governor-timelock-duplicate-actions
branch
from
e677fbb to
c0e275c
Compare
The Compound Timelock queues each proposal action as a separate transaction identified by keccak256(target, value, calldata, eta). Two identical actions in the same proposal produce the same hash and the second queue call reverts, leaving the proposal stuck. Override _propose to detect duplicate (target, value, calldata) tuples and revert with GovernorTimelockCompoundDuplicateProposalAction before the proposal is stored, rather than failing silently at queue time. If the arrays are mismatched in length, the duplicate check is skipped and super._propose handles the validation with GovernorInvalidProposalLength. Fixes OpenZeppelin#6431
Sanenelisiwe1975
force-pushed
the
fix/governor-timelock-duplicate-actions
branch
from
c0e275c to
97b6534
Compare
2 participants
The Compound Timelock queues each proposal action as a separate transaction identified by keccak256(target, value, calldata, eta). Two identical actions in the same proposal produce the same hash and the second queue call reverts, leaving the proposal stuck.
Override _propose to detect duplicate (target, value, calldata) tuples and revert with GovernorTimelockCompoundDuplicateProposalAction before the proposal is stored, rather than failing silently at queue time.
Fixes #6431
Fixes #????
PR Checklist
npx changeset add)