Skip to content

fix: reject proposals with duplicate actions in GovernorTimelockCompound #6547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aglichandrap wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: reject proposals with duplicate actions in GovernorTimelockCompound #6547

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions contracts/governance/extensions/GovernorTimelockCompound.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,38 @@ abstract contract GovernorTimelockCompound is Governor {
_updateTimelock(timelockAddress);
}

/**
* @dev Reject proposals that contain duplicate actions. Duplicate (target, value, calldata)
* actions would generate identical timelock transaction hashes, causing queueing to fail
* silently in {_queueOperations}. Detecting duplicates at proposal creation time provides
* a clear error instead.
*/
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 GovernorDuplicateAction(targets[i], values[i], calldatas[i]);
}
}
}
return super._propose(targets, values, calldatas, description, proposer);
}

/**
* @dev Emitted when a proposal contains duplicate actions.
*/
error GovernorDuplicateAction(address target, uint256 value, bytes calldata);

@coderabbitai coderabbitai Bot May 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 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 -20

Repository: 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 || true

Repository: 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:

🏁 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 -30

Repository: 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.

Suggested change
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.


/**
* @dev Overridden version of the {Governor-state} function with added support for the `Expired` state.
*/
Expand Down
Loading