Skip to content

Prevent duplicate actions in GovernorTimelockCompound proposals #6489

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
JunaidCD wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prevent duplicate actions in GovernorTimelockCompound proposals #6489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0e2852
Prevent duplicate actions in GovernorTimelockCompound proposals
JunaidCD May 1, 2026
299e062
Add changeset
JunaidCD May 1, 2026
56a83e1
Fix lint
JunaidCD May 1, 2026
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
5 changes: 5 additions & 0 deletions .changeset/eighty-parts-fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---

Prevent duplicate actions in GovernorTimelockCompound proposals
30 changes: 30 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 @@ -21,6 +21,11 @@ import {SafeCast} from "../../utils/math/SafeCast.sol";
abstract contract GovernorTimelockCompound is Governor {
ICompoundTimelock private _timelock;

/**
* @dev Thrown when a proposal contains duplicate actions.
*/
error GovernorDuplicateProposalAction(uint256 index1, uint256 index2);

/**
* @dev Emitted when the timelock controller used for proposal execution is modified.
*/
Expand Down Expand Up @@ -58,6 +63,31 @@ abstract contract GovernorTimelockCompound is Governor {
return true;
}

/**
* @dev Overridden version of the {Governor-_propose} function to check for duplicate actions.
*/
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);
}
Comment on lines +69 to +89
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find contracts -name "GovernorTimelockCompound.sol" -type f

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 140

🏁 Script executed:

# Also search for the error type mentioned
rg "GovernorInvalidProposalLength" --type sol

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

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 107

🏁 Script executed:

cat -n contracts/governance/extensions/GovernorTimelockCompound.sol | head -100

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 4516

🏁 Script executed:

rg "GovernorInvalidProposalLength" --max-count 10

Repository: OpenZeppelin/openzeppelin-contracts

Length of output: 820

🏁 Script executed:

rg "function _propose" --max-count 5 -B 2 -A 15

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


/**
* @dev Function to queue a proposal to the timelock.
*/
Expand Down
10 changes: 10 additions & 0 deletions contracts/mocks/governance/GovernorTimelockCompoundMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,14 @@ abstract contract GovernorTimelockCompoundMock is
function _executor() internal view override(Governor, GovernorTimelockCompound) returns (address) {
return super._executor();
}

function _propose(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
string memory description,
address proposer
) internal override(Governor, GovernorTimelockCompound) returns (uint256) {
return super._propose(targets, values, calldatas, description, proposer);
}
}
123 changes: 104 additions & 19 deletions test/governance/extensions/GovernorTimelockCompound.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,110 @@ describe('GovernorTimelockCompound', function () {
.to.emit(this.receiver, 'MockFunctionCalled');
});

describe('duplicate action validation', function () {
it('should allow a proposal with a single action', async function () {
await expect(
this.mock.propose(
[this.receiver.target],
[0],
[this.receiver.interface.encodeFunctionData('mockFunction')],
'description',
),
).to.emit(this.mock, 'ProposalCreated');
});

it('should allow a proposal with multiple distinct actions', async function () {
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target],
[0, 0],
[
this.receiver.interface.encodeFunctionData('mockFunction'),
this.receiver.interface.encodeFunctionData('mockFunctionNonPayable'),
],
'description',
),
).to.emit(this.mock, 'ProposalCreated');
});

it('should revert when two consecutive actions are identical', async function () {
const calldata = this.receiver.interface.encodeFunctionData('mockFunction');
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target],
[0, 0],
[calldata, calldata],
'description',
),
)
.to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
.withArgs(1, 0);
});

it('should revert when non-consecutive duplicates exist', async function () {
const calldataA = this.receiver.interface.encodeFunctionData('mockFunction');
const calldataB = this.receiver.interface.encodeFunctionData('mockFunctionNonPayable');
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target, this.receiver.target],
[0, 0, 0],
[calldataA, calldataB, calldataA],
'description',
),
)
.to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
.withArgs(2, 0);
});

it('should revert when multiple pairs of duplicates exist', async function () {
const calldataA = this.receiver.interface.encodeFunctionData('mockFunction');
const calldataB = this.receiver.interface.encodeFunctionData('mockFunctionNonPayable');
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target, this.receiver.target, this.receiver.target],
[0, 0, 0, 0],
[calldataA, calldataB, calldataA, calldataB],
'description',
),
)
.to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
.withArgs(2, 0);
});

it('should allow proposals with the same target and calldata but different value', async function () {
const calldata = this.receiver.interface.encodeFunctionData('mockFunction');
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target],
[0, 1],
[calldata, calldata],
'description',
),
).to.emit(this.mock, 'ProposalCreated');
});

it('should allow proposals with the same target and value but different calldata', async function () {
const calldataA = this.receiver.interface.encodeFunctionData('mockFunction');
const calldataB = this.receiver.interface.encodeFunctionData('mockFunctionNonPayable');
await expect(
this.mock.propose(
[this.receiver.target, this.receiver.target],
[0, 0],
[calldataA, calldataB],
'description',
),
).to.emit(this.mock, 'ProposalCreated');
});

it('should revert when duplicate empty calldata actions exist', async function () {
await expect(
this.mock.propose([this.receiver.target, this.receiver.target], [1, 1], ['0x', '0x'], 'description'),
)
.to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction')
.withArgs(1, 0);
});
});

describe('should revert', function () {
describe('on queue', function () {
it('if already queued', async function () {
Expand All @@ -140,25 +244,6 @@ describe('GovernorTimelockCompound', function () {
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
);
});

it('if proposal contains duplicate calls', async function () {
const action = {
target: this.token.target,
data: this.token.interface.encodeFunctionData('approve', [this.receiver.target, ethers.MaxUint256]),
};
const { id } = this.helper.setProposal([action, action], '<proposal description>');

await this.helper.propose();
await this.helper.waitForSnapshot();
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
await this.helper.waitForDeadline();
await expect(this.helper.queue())
.to.be.revertedWithCustomError(this.mock, 'GovernorAlreadyQueuedProposal')
.withArgs(id);
await expect(this.helper.execute())
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
.withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]));
});
});

describe('on execute', function () {
Expand Down