Prevent duplicate actions in GovernorTimelockCompound proposals#6489
Prevent duplicate actions in GovernorTimelockCompound proposals#6489JunaidCD wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: 56a83e1 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 pull request adds duplicate-action detection to the governance proposal creation workflow in GovernorTimelockCompound. The implementation introduces validation in 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 69-89: The duplicate-action nested loop in _propose reads
values[i] and calldatas[i] without first validating array lengths, risking
out-of-bounds panics; add an explicit guard that checks targets.length ==
values.length && targets.length == calldatas.length (and revert with
GovernorInvalidProposalLength) before running the duplicate scan loop in
function _propose so the child check fails fast and the call can safely
reference values[i] and calldatas[i], then call super._propose(...) as before.
🪄 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: bc664c8b-8a0d-45bc-92c6-029798bc8543
📒 Files selected for processing (3)
contracts/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 = 0; j < i; ++j) { | ||
| if ( | ||
| targets[i] == targets[j] && | ||
| values[i] == values[j] && | ||
| keccak256(calldatas[i]) == keccak256(calldatas[j]) | ||
| ) { | ||
| revert GovernorDuplicateProposalAction(i, j); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return super._propose(targets, values, calldatas, description, proposer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find contracts -name "GovernorTimelockCompound.sol" -type fRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 140
🏁 Script executed:
# Also search for the error type mentioned
rg "GovernorInvalidProposalLength" --type solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 107
🏁 Script executed:
# Search for the parent class to understand its _propose implementation
rg "class Governor" --type sol -A 5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 107
🏁 Script executed:
cat -n contracts/governance/extensions/GovernorTimelockCompound.sol | head -100Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 4516
🏁 Script executed:
rg "GovernorInvalidProposalLength" --max-count 10Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 820
🏁 Script executed:
rg "function _propose" --max-count 5 -B 2 -A 15Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 10455
🏁 Script executed:
cat -n contracts/governance/Governor.sol | grep -A 20 "function _propose"Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1134
Add length guard before duplicate scan
The duplicate action check at lines 76-86 iterates targets.length and immediately dereferences values[i] and calldatas[i] without validating array lengths first. If callers pass mismatched array lengths (e.g., targets.length=2 but values.length=1), an out-of-bounds access triggers a Solidity panic rather than reverting gracefully with GovernorInvalidProposalLength. The parent Governor._propose() validates these lengths, but only after the child override executes, so it cannot prevent the panic.
Add the length validation check before the duplicate scan loop:
Suggested patch
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 = 0; 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, j);
}
}
}
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 69
- 89, The duplicate-action nested loop in _propose reads values[i] and
calldatas[i] without first validating array lengths, risking out-of-bounds
panics; add an explicit guard that checks targets.length == values.length &&
targets.length == calldatas.length (and revert with
GovernorInvalidProposalLength) before running the duplicate scan loop in
function _propose so the child check fails fast and the call can safely
reference values[i] and calldatas[i], then call super._propose(...) as before.
1 participant
Fixes #6431
Summary
Prevent proposals with duplicate actions from being created in
GovernorTimelockCompound.Problem
Proposals containing duplicate actions (same
target,value, andcalldata) are accepted duringpropose(), but fail duringqueue()due to identical transaction hashes.Solution
Add validation in
_propose()to detect duplicates early and revert.Tests
PR Checklist