diff --git a/.changeset/green-teams-taste.md b/.changeset/green-teams-taste.md new file mode 100644 index 00000000000..acab8403641 --- /dev/null +++ b/.changeset/green-teams-taste.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`GovernorTimelockCompound`: reject proposals with duplicate actions before queueing to avoid creating proposals that can only fail later at queue time. diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index c3225f19865..6c96bb81a3b 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -58,6 +58,54 @@ abstract contract GovernorTimelockCompound is Governor { return true; } + /** + * @dev Check whether a proposal contains duplicate actions. + * + * Duplicate actions are identified by an exact match on target, value, and calldata. + */ + function _hasDuplicateActions( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas + ) internal pure returns (bool) { + 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] && + calldatas[i].length == calldatas[j].length && + keccak256(calldatas[i]) == keccak256(calldatas[j]) + ) { + return true; + } + } + } + + return false; + } + + /** + * @inheritdoc Governor + */ + 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 && + _hasDuplicateActions(targets, values, calldatas) + ) { + uint256 proposalId = getProposalId(targets, values, calldatas, keccak256(bytes(description))); + revert GovernorAlreadyQueuedProposal(proposalId); + } + + return super._propose(targets, values, calldatas, description, proposer); + } + /** * @dev Function to queue a proposal to the timelock. */ @@ -148,7 +196,7 @@ abstract contract GovernorTimelockCompound is Governor { * Note that if the timelock admin has been handed over in a previous operation, we refuse updates made through the * timelock if admin of the timelock has already been accepted and the operation is executed outside the scope of * governance. - + * * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. */ function updateTimelock(ICompoundTimelock newTimelock) public virtual onlyGovernance { 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 3912eefb8a7..af064f4c22e 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -148,16 +148,9 @@ describe('GovernorTimelockCompound', function () { }; const { id } = 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()) + await expect(this.helper.propose()) .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])); }); });