fix: reject proposals with duplicate actions in GovernorTimelockCompound#6547
fix: reject proposals with duplicate actions in GovernorTimelockCompound#6547aglichandrap wants to merge 1 commit into
Conversation
Proposals containing duplicate (target, value, calldata) actions would fail silently during queueing because the Compound timelock generates identical transaction hashes. Override _propose to detect duplicates early and revert with a clear GovernorDuplicateAction error. Fixes OpenZeppelin#6431
|
WalkthroughThis PR adds duplicate action validation to 🚥 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/governance/extensions/GovernorTimelockCompound.sol`:
- Line 66: The custom error declaration GovernorDuplicateAction currently uses
an unsupported data-location specifier (`bytes calldata`); update the error
signature to use `bytes` instead (remove `calldata`) so the compiler accepts it,
e.g., change the error declaration for GovernorDuplicateAction(address target,
uint256 value, bytes calldata) to GovernorDuplicateAction(address target,
uint256 value, bytes) and leave all revert calls that pass calldatas[i]
unchanged.
🪄 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: 9e059572-bcf1-4761-a148-83c0dd13697f
📒 Files selected for processing (1)
contracts/governance/extensions/GovernorTimelockCompound.sol
| /** | ||
| * @dev Emitted when a proposal contains duplicate actions. | ||
| */ | ||
| error GovernorDuplicateAction(address target, uint256 value, bytes calldata); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Attempt to check Solidity syntax for custom error definitions with data location specifiers
# Check if similar error definitions exist in the codebase that might indicate the pattern used
rg -n "error.*\(.*bytes (memory|calldata|storage)" --type sol
# Search for other custom error definitions to verify the standard pattern
rg -n "^\s*error\s+\w+\(" --type sol -A 0 | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate and print the surrounding lines for the reported error
FILE="contracts/governance/extensions/GovernorTimelockCompound.sol"
if [ -f "$FILE" ]; then
echo "== Found $FILE =="
nl -ba "$FILE" | sed -n '50,85p'
else
echo "== File not found: $FILE =="
# Try to locate similar file names
fd -a "GovernorTimelockCompound.sol" . || true
fi
# 2) Search for custom errors that use bytes with data locations
# (data location specifiers in error parameters are the concern: memory/calldata/storage)
echo
echo "== Search: errors with 'bytes calldata' =="
rg -n "error\s+\w+\s*\([^)]*bytes\s+(calldata|memory|storage)\b" -g '*.sol' || true
echo
echo "== Search: errors with 'bytes' and any data location words =="
rg -n "error\s+\w+\s*\([^)]*bytes\s+(calldata|memory|storage)\b" -g '*.sol' || true
# 3) Search for any 'error <Name>(' occurrences (quick context)
echo
echo "== Search: custom error declarations (first 50 matches) =="
rg -n "^\s*error\s+\w+\s*\(" -g '*.sol' | head -50 || trueRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 213
🌐 Web query:
Can Solidity custom errors include data location specifiers like bytes calldataorbytes memory in the error parameter list?
💡 Result:
No. Solidity custom error parameter lists do not allow data location specifiers like bytes calldata or bytes memory. Reason (from Solidity language definition): the grammar for an error definition uses error-parameter, which (per the Solidity grammar) is just a type parameter entry and does not provide a syntax production that includes a data-location annotation in the error parameter list; data-location exists as a separate grammar element but is used for other declarations (e.g., function parameters/return types) rather than custom error parameters [1]. Additionally, Solidity’s type system defines data locations as “memory”, “storage”, and “calldata” for reference types, but it describes these in the context of where data is stored for variables/parameters; it does not indicate that error parameter declarations support specifying such locations inline [2]. So you must write custom error parameters without calldata/memory annotations, e.g.: - error Invalid(bytes value); (no bytes calldata / bytes memory) If you need calldata vs memory optimization, that’s about function parameter locations, not custom error type declarations.
Citations:
- 1: https://docs.soliditylang.org/en/latest/grammar.html
- 2: https://docs.soliditylang.org/en/breaking/types.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/governance/extensions/GovernorTimelockCompound.sol"
echo "== Lines 55-75 from $FILE =="
awk 'NR>=55 && NR<=75 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search: errors that include bytes with a data location keyword =="
rg -n "error\s+\w+\s*\([^)]*bytes\s+(calldata|memory|storage)\b" -g '*.sol' .
echo
echo "== Search: custom error declarations (first 30 matches) =="
rg -n "^\s*error\s+\w+\s*\(" -g '*.sol' . | head -30Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 4324
Fix custom error parameter type to use bytes (remove calldata)
GovernorDuplicateAction declares bytes calldata in the custom error parameter list, but Solidity custom error parameters don’t support data-location specifiers; this should be bytes so the file compiles. The revert already passes calldatas[i] as the bytes payload, so no location is needed.
🔧 Proposed fix
- error GovernorDuplicateAction(address target, uint256 value, bytes calldata);
+ error GovernorDuplicateAction(address target, uint256 value, bytes data);📝 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.
| error GovernorDuplicateAction(address target, uint256 value, bytes calldata); | |
| error GovernorDuplicateAction(address target, uint256 value, bytes data); |
🤖 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` at line 66, The
custom error declaration GovernorDuplicateAction currently uses an unsupported
data-location specifier (`bytes calldata`); update the error signature to use
`bytes` instead (remove `calldata`) so the compiler accepts it, e.g., change the
error declaration for GovernorDuplicateAction(address target, uint256 value,
bytes calldata) to GovernorDuplicateAction(address target, uint256 value, bytes)
and leave all revert calls that pass calldatas[i] unchanged.
1 participant
Problem
Proposals containing 2+ actions with identical
(target, value, calldata)will fail during queueing in_queueOperations. The Compound timelock generates transaction hashes fromkeccak256(abi.encode(targets[i], values[i], "", calldatas[i], etaSeconds)), so duplicate actions produce identical hashes. The first action queues successfully, but the second triggersGovernorAlreadyQueuedProposalbecause the hash is already marked as queued.This can be circumvented by appending extra bytes to calldata, but users may not realize their proposal is unexecutable until after voting ends.
Solution
Override
_proposeinGovernorTimelockCompoundto detect duplicate actions at proposal creation time, providing a clear error before any gas is wasted on voting.Complexity
O(n²) pairwise comparison where n = number of actions. For typical proposals (n < 20), this is negligible gas cost.
Tests
TODO: Add test cases for:
GovernorDuplicateActionFixes #6431