diff --git a/.changeset/prevent-duplicate-proposal-actions.md b/.changeset/prevent-duplicate-proposal-actions.md new file mode 100644 index 00000000000..4e007de8994 --- /dev/null +++ b/.changeset/prevent-duplicate-proposal-actions.md @@ -0,0 +1,5 @@ +--- +"openzeppelin-solidity": patch +--- + +Prevent proposals with duplicate actions from being submitted in `GovernorTimelockCompound` diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index c3225f19865..86a2f6a6d7d 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -21,6 +21,12 @@ import {SafeCast} from "../../utils/math/SafeCast.sol"; abstract contract GovernorTimelockCompound is Governor { ICompoundTimelock private _timelock; + /** + * @dev The proposal contains duplicate actions (same target, value, and calldata), which would cause queueing to + * fail in the Compound Timelock. + */ + error GovernorDuplicateProposalAction(uint256 index); + /** * @dev Emitted when the timelock controller used for proposal execution is modified. */ @@ -58,6 +64,33 @@ abstract contract GovernorTimelockCompound is Governor { return true; } + /** + * @dev Override of {Governor-_propose} that rejects proposals containing duplicate actions. The Compound Timelock + * identifies queued transactions by a hash of (target, value, signature, calldata, eta). If two actions in a + * proposal share the same target, value, and calldata, they would produce the same hash, causing the second one + * to fail during queueing. This check prevents such proposals from being created in the first place. + */ + function _propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description, + address proposer + ) internal virtual override returns (uint256) { + for (uint256 i = 1; 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); + } + } + } + return super._propose(targets, values, calldatas, description, proposer); + } + /** * @dev Function to queue a proposal to the timelock. */ diff --git a/contracts/mocks/governance/GovernorTimelockCompoundMock.sol b/contracts/mocks/governance/GovernorTimelockCompoundMock.sol index 71508cd5ac0..8cbdc9f3da7 100644 --- a/contracts/mocks/governance/GovernorTimelockCompoundMock.sol +++ b/contracts/mocks/governance/GovernorTimelockCompoundMock.sol @@ -34,6 +34,16 @@ abstract contract GovernorTimelockCompoundMock is return super.proposalNeedsQueuing(proposalId); } + 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); + } + function _queueOperations( uint256 proposalId, address[] memory targets, diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index fc8e1e561b2..44b93e50d39 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -146,18 +146,27 @@ describe('GovernorTimelockCompound', function () { target: this.token.target, data: this.token.interface.encodeFunctionData('approve', [this.receiver.target, ethers.MaxUint256]), }; - const { id } = this.helper.setProposal([action, action], ''); + this.helper.setProposal([action, action], ''); - 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])); + await expect(this.helper.propose()) + .to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction') + .withArgs(1n); + }); + + it('if proposal contains non-adjacent duplicate calls', async function () { + const action1 = { + target: this.token.target, + data: this.token.interface.encodeFunctionData('approve', [this.receiver.target, ethers.MaxUint256]), + }; + const action2 = { + target: this.receiver.target, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + }; + this.helper.setProposal([action1, action2, action1], ''); + + await expect(this.helper.propose()) + .to.be.revertedWithCustomError(this.mock, 'GovernorDuplicateProposalAction') + .withArgs(2n); }); });