From a07c0a28e3e8a6706d246bd511207f2e95b49356 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 1 Dec 2021 17:56:31 +0100 Subject: [PATCH] Add a governor module to protect against late quorum (#2973) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 3 + contracts/governance/Governor.sol | 33 ++- contracts/governance/README.adoc | 4 + .../extensions/GovernorPreventLateQuorum.sol | 106 ++++++++ .../mocks/GovernorPreventLateQuorumMock.sol | 60 +++++ test/governance/GovernorWorkflow.behavior.js | 2 +- .../GovernorPreventLateQuorum.test.js | 247 ++++++++++++++++++ 7 files changed, 444 insertions(+), 11 deletions(-) create mode 100644 contracts/governance/extensions/GovernorPreventLateQuorum.sol create mode 100644 contracts/mocks/GovernorPreventLateQuorumMock.sol create mode 100644 test/governance/extensions/GovernorPreventLateQuorum.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index e45e1a3b158..204fbd72de4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## Unreleased + * `GovernorExtendedVoting`: add new module to ensure a minimum voting duration is available after the quorum is reached. ([#2973](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2973)) + ## Unreleased * `GovernorTimelockControl`: improve the `state()` function to have it reflect cases where a proposal has been canceled directly on the timelock. ([#2977](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2977)) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 8cc1a51d742..e2cdc9c8e75 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -110,23 +110,36 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { * @dev See {IGovernor-state}. */ function state(uint256 proposalId) public view virtual override returns (ProposalState) { - ProposalCore memory proposal = _proposals[proposalId]; + ProposalCore storage proposal = _proposals[proposalId]; if (proposal.executed) { return ProposalState.Executed; - } else if (proposal.canceled) { + } + + if (proposal.canceled) { return ProposalState.Canceled; - } else if (proposal.voteStart.getDeadline() >= block.number) { + } + + uint256 snapshot = proposalSnapshot(proposalId); + + if (snapshot == 0) { + revert("Governor: unknown proposal id"); + } + + if (snapshot >= block.number) { return ProposalState.Pending; - } else if (proposal.voteEnd.getDeadline() >= block.number) { + } + + uint256 deadline = proposalDeadline(proposalId); + + if (deadline >= block.number) { return ProposalState.Active; - } else if (proposal.voteEnd.isExpired()) { - return - _quorumReached(proposalId) && _voteSucceeded(proposalId) - ? ProposalState.Succeeded - : ProposalState.Defeated; + } + + if (_quorumReached(proposalId) && _voteSucceeded(proposalId)) { + return ProposalState.Succeeded; } else { - revert("Governor: unknown proposal id"); + return ProposalState.Defeated; } } diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index d946022510b..d94bde71e96 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -42,6 +42,8 @@ Other extensions can customize the behavior or interface in multiple ways. * {GovernorSettings}: Manages some of the settings (voting delay, voting period duration, and proposal threshold) in a way that can be updated through a governance proposal, without requiering an upgrade. +* {GovernorPreventLateQuorum}: Ensures there is a minimum voting period after quorum is reached as a security protection against large voters. + In addition to modules and extensions, the core contract requires a few virtual functions to be implemented to your particular specifications: * <>: Delay (in number of blocks) since the proposal is submitted until voting power is fixed and voting starts. This can be used to enforce a delay after a proposal is published for users to buy tokens, or delegate their votes. @@ -74,6 +76,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorSettings}} +{{GovernorPreventLateQuorum}} + {{GovernorCompatibilityBravo}} === Deprecated diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol new file mode 100644 index 00000000000..8b9573d99c1 --- /dev/null +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../Governor.sol"; +import "../../utils/math/Math.sol"; + +/** + * @dev A module that ensures there is a minimum voting period after quorum is reached. This prevents a large voter from + * swaying a vote and triggering quorum at the last minute, by ensuring there is always time for other voters to react + * and try to oppose the decision. + * + * If a vote causes quorum to be reached, the proposal's voting period may be extended so that it does not end before at + * least a given number of blocks have passed (the "vote extension" parameter). This parameter can be set by the + * governance executor (e.g. through a governance proposal). + * + * _Available since v4.5._ + */ +abstract contract GovernorPreventLateQuorum is Governor { + using SafeCast for uint256; + using Timers for Timers.BlockNumber; + + uint64 private _voteExtension; + mapping(uint256 => Timers.BlockNumber) private _extendedDeadlines; + + /// @dev Emitted when a proposal deadline is pushed back due to reaching quorum late in its voting period. + event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline); + + /// @dev Emitted when the {lateQuorumVoteExtension} parameter is changed. + event LateQuorumVoteExtensionSet(uint64 oldVoteExtension, uint64 newVoteExtension); + + /** + * @dev Initializes the vote extension parameter: the number of blocks that are required to pass since a proposal + * reaches quorum until its voting period ends. If necessary the voting period will be extended beyond the one set + * at proposal creation. + */ + constructor(uint64 initialVoteExtension) { + _setLateQuorumVoteExtension(initialVoteExtension); + } + + /** + * @dev Returns the proposal deadline, which may have been extended beyond that set at proposal creation, if the + * proposal reached quorum late in the voting period. See {Governor-proposalDeadline}. + */ + function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) { + return Math.max(super.proposalDeadline(proposalId), _extendedDeadlines[proposalId].getDeadline()); + } + + /** + * @dev Casts a vote and detects if it caused quorum to be reached, potentially extending the voting period. See + * {Governor-_castVote}. + * + * May emit a {ProposalExtended} event. + */ + function _castVote( + uint256 proposalId, + address account, + uint8 support, + string memory reason + ) internal virtual override returns (uint256) { + uint256 result = super._castVote(proposalId, account, support, reason); + + Timers.BlockNumber storage extendedDeadline = _extendedDeadlines[proposalId]; + + if (extendedDeadline.isUnset() && _quorumReached(proposalId)) { + uint64 extendedDeadlineValue = block.number.toUint64() + lateQuorumVoteExtension(); + + if (extendedDeadlineValue > proposalDeadline(proposalId)) { + emit ProposalExtended(proposalId, extendedDeadlineValue); + } + + extendedDeadline.setDeadline(extendedDeadlineValue); + } + + return result; + } + + /** + * @dev Returns the current value of the vote extension parameter: the number of blocks that are required to pass + * from the time a proposal reaches quorum until its voting period ends. + */ + function lateQuorumVoteExtension() public view virtual returns (uint64) { + return _voteExtension; + } + + /** + * @dev Changes the {lateQuorumVoteExtension}. This operation can only be performed by the governance executor, + * generally through a governance proposal. + * + * Emits a {LateQuorumVoteExtensionSet} event. + */ + function setLateQuorumVoteExtension(uint64 newVoteExtension) public virtual onlyGovernance { + _setLateQuorumVoteExtension(newVoteExtension); + } + + /** + * @dev Changes the {lateQuorumVoteExtension}. This is an internal function that can be exposed in a public function + * like {setLateQuorumVoteExtension} if another access control mechanism is needed. + * + * Emits a {LateQuorumVoteExtensionSet} event. + */ + function _setLateQuorumVoteExtension(uint64 newVoteExtension) internal virtual { + emit LateQuorumVoteExtensionSet(_voteExtension, newVoteExtension); + _voteExtension = newVoteExtension; + } +} diff --git a/contracts/mocks/GovernorPreventLateQuorumMock.sol b/contracts/mocks/GovernorPreventLateQuorumMock.sol new file mode 100644 index 00000000000..412337c08d5 --- /dev/null +++ b/contracts/mocks/GovernorPreventLateQuorumMock.sol @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../governance/extensions/GovernorPreventLateQuorum.sol"; +import "../governance/extensions/GovernorSettings.sol"; +import "../governance/extensions/GovernorCountingSimple.sol"; +import "../governance/extensions/GovernorVotes.sol"; + +contract GovernorPreventLateQuorumMock is + GovernorSettings, + GovernorVotes, + GovernorCountingSimple, + GovernorPreventLateQuorum +{ + uint256 private _quorum; + + constructor( + string memory name_, + ERC20Votes token_, + uint256 votingDelay_, + uint256 votingPeriod_, + uint256 quorum_, + uint64 voteExtension_ + ) + Governor(name_) + GovernorSettings(votingDelay_, votingPeriod_, 0) + GovernorVotes(token_) + GovernorPreventLateQuorum(voteExtension_) + { + _quorum = quorum_; + } + + function quorum(uint256) public view virtual override returns (uint256) { + return _quorum; + } + + function proposalDeadline(uint256 proposalId) + public + view + virtual + override(Governor, GovernorPreventLateQuorum) + returns (uint256) + { + return super.proposalDeadline(proposalId); + } + + function proposalThreshold() public view virtual override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function _castVote( + uint256 proposalId, + address account, + uint8 support, + string memory reason + ) internal virtual override(Governor, GovernorPreventLateQuorum) returns (uint256) { + return super._castVote(proposalId, account, support, reason); + } +} diff --git a/test/governance/GovernorWorkflow.behavior.js b/test/governance/GovernorWorkflow.behavior.js index 70319cd44d3..3256792cca9 100644 --- a/test/governance/GovernorWorkflow.behavior.js +++ b/test/governance/GovernorWorkflow.behavior.js @@ -65,7 +65,7 @@ function runGovernorWorkflow () { // vote if (tryGet(this.settings, 'voters')) { this.receipts.castVote = []; - for (const voter of this.settings.voters) { + for (const voter of this.settings.voters.filter(({ support }) => !!support)) { if (!voter.signature) { this.receipts.castVote.push( await getReceiptOrRevert( diff --git a/test/governance/extensions/GovernorPreventLateQuorum.test.js b/test/governance/extensions/GovernorPreventLateQuorum.test.js new file mode 100644 index 00000000000..e4ae5c17c81 --- /dev/null +++ b/test/governance/extensions/GovernorPreventLateQuorum.test.js @@ -0,0 +1,247 @@ +const { BN, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const Enums = require('../../helpers/enums'); + +const { + runGovernorWorkflow, +} = require('../GovernorWorkflow.behavior'); + +const Token = artifacts.require('ERC20VotesCompMock'); +const Governor = artifacts.require('GovernorPreventLateQuorumMock'); +const CallReceiver = artifacts.require('CallReceiverMock'); + +contract('GovernorPreventLateQuorum', function (accounts) { + const [ owner, proposer, voter1, voter2, voter3, voter4 ] = accounts; + + const name = 'OZ-Governor'; + // const version = '1'; + const tokenName = 'MockToken'; + const tokenSymbol = 'MTKN'; + const tokenSupply = web3.utils.toWei('100'); + const votingDelay = new BN(4); + const votingPeriod = new BN(16); + const lateQuorumVoteExtension = new BN(8); + const quorum = web3.utils.toWei('1'); + + beforeEach(async function () { + this.owner = owner; + this.token = await Token.new(tokenName, tokenSymbol); + this.mock = await Governor.new( + name, + this.token.address, + votingDelay, + votingPeriod, + quorum, + lateQuorumVoteExtension, + ); + this.receiver = await CallReceiver.new(); + await this.token.mint(owner, tokenSupply); + await this.token.delegate(voter1, { from: voter1 }); + await this.token.delegate(voter2, { from: voter2 }); + await this.token.delegate(voter3, { from: voter3 }); + await this.token.delegate(voter4, { from: voter4 }); + }); + + it('deployment check', async function () { + expect(await this.mock.name()).to.be.equal(name); + expect(await this.mock.token()).to.be.equal(this.token.address); + expect(await this.mock.votingDelay()).to.be.bignumber.equal(votingDelay); + expect(await this.mock.votingPeriod()).to.be.bignumber.equal(votingPeriod); + expect(await this.mock.quorum(0)).to.be.bignumber.equal(quorum); + expect(await this.mock.lateQuorumVoteExtension()).to.be.bignumber.equal(lateQuorumVoteExtension); + }); + + describe('nominal is unaffected', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [ this.receiver.address ], + [ 0 ], + [ this.receiver.contract.methods.mockFunction().encodeABI() ], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('1'), support: Enums.VoteType.For, reason: 'This is nice' }, + { voter: voter2, weight: web3.utils.toWei('7'), support: Enums.VoteType.For }, + { voter: voter3, weight: web3.utils.toWei('5'), support: Enums.VoteType.Against }, + { voter: voter4, weight: web3.utils.toWei('2'), support: Enums.VoteType.Abstain }, + ], + }; + }); + + afterEach(async function () { + expect(await this.mock.hasVoted(this.id, owner)).to.be.equal(false); + expect(await this.mock.hasVoted(this.id, voter1)).to.be.equal(true); + expect(await this.mock.hasVoted(this.id, voter2)).to.be.equal(true); + + await this.mock.proposalVotes(this.id).then(result => { + for (const [key, value] of Object.entries(Enums.VoteType)) { + expect(result[`${key.toLowerCase()}Votes`]).to.be.bignumber.equal( + Object.values(this.settings.voters).filter(({ support }) => support === value).reduce( + (acc, { weight }) => acc.add(new BN(weight)), + new BN('0'), + ), + ); + } + }); + + const startBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay); + const endBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay).add(votingPeriod); + expect(await this.mock.proposalSnapshot(this.id)).to.be.bignumber.equal(startBlock); + expect(await this.mock.proposalDeadline(this.id)).to.be.bignumber.equal(endBlock); + + expectEvent( + this.receipts.propose, + 'ProposalCreated', + { + proposalId: this.id, + proposer, + targets: this.settings.proposal[0], + // values: this.settings.proposal[1].map(value => new BN(value)), + signatures: this.settings.proposal[2].map(() => ''), + calldatas: this.settings.proposal[2], + startBlock, + endBlock, + description: this.settings.proposal[3], + }, + ); + + this.receipts.castVote.filter(Boolean).forEach(vote => { + const { voter } = vote.logs.find(Boolean).args; + expectEvent( + vote, + 'VoteCast', + this.settings.voters.find(({ address }) => address === voter), + ); + expectEvent.notEmitted( + vote, + 'ProposalExtended', + ); + }); + expectEvent( + this.receipts.execute, + 'ProposalExecuted', + { proposalId: this.id }, + ); + await expectEvent.inTransaction( + this.receipts.execute.transactionHash, + this.receiver, + 'MockFunctionCalled', + ); + }); + runGovernorWorkflow(); + }); + + describe('Delay is extended to prevent last minute take-over', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [ this.receiver.address ], + [ 0 ], + [ this.receiver.contract.methods.mockFunction().encodeABI() ], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('0.2'), support: Enums.VoteType.Against }, + { voter: voter2, weight: web3.utils.toWei('1.0') }, // do not actually vote, only getting tokens + { voter: voter3, weight: web3.utils.toWei('0.9') }, // do not actually vote, only getting tokens + ], + steps: { + wait: { enable: false }, + execute: { enable: false }, + }, + }; + }); + + afterEach(async function () { + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); + + const startBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay); + const endBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay).add(votingPeriod); + expect(await this.mock.proposalSnapshot(this.id)).to.be.bignumber.equal(startBlock); + expect(await this.mock.proposalDeadline(this.id)).to.be.bignumber.equal(endBlock); + + // wait until the vote is almost over + await time.advanceBlockTo(endBlock.subn(1)); + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); + + // try to overtake the vote at the last minute + const tx = await this.mock.castVote(this.id, Enums.VoteType.For, { from: voter2 }); + + // vote duration is extended + const extendedBlock = new BN(tx.receipt.blockNumber).add(lateQuorumVoteExtension); + expect(await this.mock.proposalDeadline(this.id)).to.be.bignumber.equal(extendedBlock); + + expectEvent( + tx, + 'ProposalExtended', + { proposalId: this.id, extendedDeadline: extendedBlock }, + ); + + // vote is still active after expected end + await time.advanceBlockTo(endBlock.addn(1)); + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); + + // Still possible to vote + await this.mock.castVote(this.id, Enums.VoteType.Against, { from: voter3 }); + + // proposal fails + await time.advanceBlockTo(extendedBlock.addn(1)); + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Defeated); + }); + runGovernorWorkflow(); + }); + + describe('setLateQuorumVoteExtension', function () { + beforeEach(async function () { + this.newVoteExtension = new BN(0); // disable voting delay extension + }); + + it('protected', async function () { + await expectRevert( + this.mock.setLateQuorumVoteExtension(this.newVoteExtension), + 'Governor: onlyGovernance', + ); + }); + + describe('using workflow', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [ this.mock.address ], + [ web3.utils.toWei('0') ], + [ this.mock.contract.methods.setLateQuorumVoteExtension(this.newVoteExtension).encodeABI() ], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('1.0'), support: Enums.VoteType.For }, + ], + }; + }); + afterEach(async function () { + expectEvent( + this.receipts.propose, + 'ProposalCreated', + { proposalId: this.id }, + ); + expectEvent( + this.receipts.execute, + 'ProposalExecuted', + { proposalId: this.id }, + ); + expectEvent( + this.receipts.execute, + 'LateQuorumVoteExtensionSet', + { oldVoteExtension: lateQuorumVoteExtension, newVoteExtension: this.newVoteExtension }, + ); + expect(await this.mock.lateQuorumVoteExtension()).to.be.bignumber.equal(this.newVoteExtension); + }); + runGovernorWorkflow(); + }); + }); +});