From 8c1b0ca82d2f30f5812078a7cf360f978a786be6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 27 Jan 2025 09:56:35 +0100 Subject: [PATCH] Add a governor extension that implements a proposal guardian (#5303) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Co-authored-by: Ernesto García --- .changeset/pretty-lobsters-tan.md | 5 + contracts/governance/Governor.sol | 16 ++- contracts/governance/IGovernor.sol | 10 +- contracts/governance/README.adoc | 4 + .../extensions/GovernorProposalGuardian.sol | 57 ++++++++ .../GovernorProposalGuardianMock.sol | 27 ++++ test/governance/Governor.test.js | 36 ++--- .../GovernorProposalGuardian.test.js | 132 ++++++++++++++++++ 8 files changed, 251 insertions(+), 36 deletions(-) create mode 100644 .changeset/pretty-lobsters-tan.md create mode 100644 contracts/governance/extensions/GovernorProposalGuardian.sol create mode 100644 contracts/mocks/governance/GovernorProposalGuardianMock.sol create mode 100644 test/governance/extensions/GovernorProposalGuardian.test.js diff --git a/.changeset/pretty-lobsters-tan.md b/.changeset/pretty-lobsters-tan.md new file mode 100644 index 000000000..d3b8644ff --- /dev/null +++ b/.changeset/pretty-lobsters-tan.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`GovernorProposalGuardian`: Add a governance extension that defines a proposal guardian who can cancel proposals at any stage in their lifecycle. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 10275686b..4b67b1995 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -484,11 +484,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 // changes it. The `getProposalId` duplication has a cost that is limited, and that we accept. uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash); - // public cancel restrictions (on top of existing _cancel restrictions). - _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending)); - if (_msgSender() != proposalProposer(proposalId)) { - revert GovernorOnlyProposer(_msgSender()); - } + address caller = _msgSender(); + if (!_validateCancel(proposalId, caller)) revert GovernorUnableToCancel(proposalId, caller); return _cancel(targets, values, calldatas, descriptionHash); } @@ -805,6 +802,15 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } } + /** + * @dev Check if the `caller` can cancel the proposal with the given `proposalId`. + * + * The default implementation allows the proposal proposer to cancel the proposal during the pending state. + */ + function _validateCancel(uint256 proposalId, address caller) internal view virtual returns (bool) { + return (state(proposalId) == ProposalState.Pending) && caller == proposalProposer(proposalId); + } + /** * @inheritdoc IERC6372 */ diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 36ef099a7..702d2beb7 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -39,11 +39,6 @@ interface IGovernor is IERC165, IERC6372 { */ error GovernorDisabledDeposit(); - /** - * @dev The `account` is not a proposer. - */ - error GovernorOnlyProposer(address account); - /** * @dev The `account` is not the governance executor. */ @@ -112,6 +107,11 @@ interface IGovernor is IERC165, IERC6372 { */ error GovernorInvalidSignature(address voter); + /** + * @dev The given `account` is unable to cancel the proposal with given `proposalId`. + */ + error GovernorUnableToCancel(uint256 proposalId, address account); + /** * @dev Emitted when a proposal is created. */ diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index bee3c2ae6..a9d298083 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -48,6 +48,8 @@ Other extensions can customize the behavior or interface in multiple ways. * {GovernorPreventLateQuorum}: Ensures there is a minimum voting period after quorum is reached as a security protection against large voters. +* {GovernorProposalGuardian}: Adds a proposal guardian that can cancel proposals at any stage in their lifecycle--this permission is passed on to the proposers if the guardian is not set. + In addition to modules and extensions, the core contract requires a few virtual functions to be implemented to your particular specifications: * <>: Delay (in ERC-6372 clock) 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. @@ -88,6 +90,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorStorage}} +{{GovernorProposalGuardian}} + == Utils {{Votes}} diff --git a/contracts/governance/extensions/GovernorProposalGuardian.sol b/contracts/governance/extensions/GovernorProposalGuardian.sol new file mode 100644 index 000000000..339024a45 --- /dev/null +++ b/contracts/governance/extensions/GovernorProposalGuardian.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {Governor} from "../Governor.sol"; + +/** + * @dev Extension of {Governor} which adds a proposal guardian that can cancel proposals at any stage in the proposal's lifecycle. + * + * NOTE: if the proposal guardian is not configured, then proposers take this role for their proposals. + */ +abstract contract GovernorProposalGuardian is Governor { + address private _proposalGuardian; + + event ProposalGuardianSet(address oldProposalGuardian, address newProposalGuardian); + + /** + * @dev Getter that returns the address of the proposal guardian. + */ + function proposalGuardian() public view virtual returns (address) { + return _proposalGuardian; + } + + /** + * @dev Update the proposal guardian's address. This operation can only be performed through a governance proposal. + * + * Emits a {ProposalGuardianSet} event. + */ + function setProposalGuardian(address newProposalGuardian) public virtual onlyGovernance { + _setProposalGuardian(newProposalGuardian); + } + + /** + * @dev Internal setter for the proposal guardian. + * + * Emits a {ProposalGuardianSet} event. + */ + function _setProposalGuardian(address newProposalGuardian) internal virtual { + emit ProposalGuardianSet(_proposalGuardian, newProposalGuardian); + _proposalGuardian = newProposalGuardian; + } + + /** + * @dev Override {Governor-_validateCancel} to implement the extended cancellation logic. + * + * * The {proposalGuardian} can cancel any proposal at any point. + * * If no proposal guardian is set, the {IGovernor-proposalProposer} can cancel their proposals at any point. + * * In any case, permissions defined in {Governor-_validateCancel} (or another override) remains valid. + */ + function _validateCancel(uint256 proposalId, address caller) internal view virtual override returns (bool) { + address guardian = proposalGuardian(); + + return + guardian == caller || + (guardian == address(0) && caller == proposalProposer(proposalId)) || + super._validateCancel(proposalId, caller); + } +} diff --git a/contracts/mocks/governance/GovernorProposalGuardianMock.sol b/contracts/mocks/governance/GovernorProposalGuardianMock.sol new file mode 100644 index 000000000..5ed45d6c9 --- /dev/null +++ b/contracts/mocks/governance/GovernorProposalGuardianMock.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Governor} from "../../governance/Governor.sol"; +import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; +import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol"; +import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; +import {GovernorProposalGuardian} from "../../governance/extensions/GovernorProposalGuardian.sol"; + +abstract contract GovernorProposalGuardianMock is + GovernorSettings, + GovernorVotesQuorumFraction, + GovernorCountingSimple, + GovernorProposalGuardian +{ + function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function _validateCancel( + uint256 proposalId, + address caller + ) internal view override(Governor, GovernorProposalGuardian) returns (bool) { + return super._validateCancel(proposalId, caller); + } +} diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index ea1c3a324..0e4283c3b 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -624,8 +624,8 @@ describe('Governor', function () { await this.helper.connect(this.proposer).propose(); await expect(this.helper.connect(this.owner).cancel('external')) - .to.be.revertedWithCustomError(this.mock, 'GovernorOnlyProposer') - .withArgs(this.owner); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.owner); }); it('after vote started', async function () { @@ -633,12 +633,8 @@ describe('Governor', function () { await this.helper.waitForSnapshot(1n); // snapshot + 1 block await expect(this.helper.cancel('external')) - .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') - .withArgs( - this.proposal.id, - ProposalState.Active, - GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]), - ); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.owner); }); it('after vote', async function () { @@ -647,12 +643,8 @@ describe('Governor', function () { await this.helper.connect(this.voter1).vote({ support: VoteType.For }); await expect(this.helper.cancel('external')) - .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') - .withArgs( - this.proposal.id, - ProposalState.Active, - GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]), - ); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.voter1); }); it('after deadline', async function () { @@ -662,12 +654,8 @@ describe('Governor', function () { await this.helper.waitForDeadline(); await expect(this.helper.cancel('external')) - .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') - .withArgs( - this.proposal.id, - ProposalState.Succeeded, - GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]), - ); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.voter1); }); it('after execution', async function () { @@ -678,12 +666,8 @@ describe('Governor', function () { await this.helper.execute(); await expect(this.helper.cancel('external')) - .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') - .withArgs( - this.proposal.id, - ProposalState.Executed, - GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]), - ); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.voter1); }); }); }); diff --git a/test/governance/extensions/GovernorProposalGuardian.test.js b/test/governance/extensions/GovernorProposalGuardian.test.js new file mode 100644 index 000000000..1741072c3 --- /dev/null +++ b/test/governance/extensions/GovernorProposalGuardian.test.js @@ -0,0 +1,132 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); + +const { impersonate } = require('../../helpers/account'); +const { GovernorHelper } = require('../../helpers/governance'); +const { ProposalState } = require('../../helpers/enums'); + +const TOKENS = [ + { Token: '$ERC20Votes', mode: 'blocknumber' }, + { Token: '$ERC20VotesTimestampMock', mode: 'timestamp' }, +]; +const name = 'Proposal Guardian Governor'; +const version = '1'; +const tokenName = 'MockToken'; +const tokenSymbol = 'MTKN'; +const tokenSupply = ethers.parseEther('100'); +const votingDelay = 4n; +const votingPeriod = 16n; +const value = ethers.parseEther('1'); + +describe('GovernorProposalGuardian', function () { + for (const { Token, mode } of TOKENS) { + const fixture = async () => { + const [owner, proposer, guardian, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); + const receiver = await ethers.deployContract('CallReceiverMock'); + + const token = await ethers.deployContract(Token, [tokenName, tokenSymbol, tokenName, version]); + const mock = await ethers.deployContract('$GovernorProposalGuardianMock', [ + name, // name + votingDelay, // initialVotingDelay + votingPeriod, // initialVotingPeriod + 0n, // initialProposalThreshold + token, // tokenAddress + 10n, // quorumNumeratorValue + ]); + + await impersonate(mock.target); + await owner.sendTransaction({ to: mock, value }); + await token.$_mint(owner, tokenSupply); + + const helper = new GovernorHelper(mock, mode); + await helper.connect(owner).delegate({ token, to: voter1, value: ethers.parseEther('10') }); + await helper.connect(owner).delegate({ token, to: voter2, value: ethers.parseEther('7') }); + await helper.connect(owner).delegate({ token, to: voter3, value: ethers.parseEther('5') }); + await helper.connect(owner).delegate({ token, to: voter4, value: ethers.parseEther('2') }); + + return { owner, proposer, guardian, voter1, voter2, voter3, voter4, other, receiver, token, mock, helper }; + }; + + describe(`using ${Token}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + + // default proposal + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.target, + value, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + }, + ], + '', + ); + }); + + it('deployment check', async function () { + await expect(this.mock.name()).to.eventually.equal(name); + await expect(this.mock.token()).to.eventually.equal(this.token); + await expect(this.mock.votingDelay()).to.eventually.equal(votingDelay); + await expect(this.mock.votingPeriod()).to.eventually.equal(votingPeriod); + }); + + describe('set proposal guardian', function () { + it('from governance', async function () { + const governorSigner = await ethers.getSigner(this.mock.target); + await expect(this.mock.connect(governorSigner).setProposalGuardian(this.guardian)) + .to.emit(this.mock, 'ProposalGuardianSet') + .withArgs(ethers.ZeroAddress, this.guardian); + await expect(this.mock.proposalGuardian()).to.eventually.equal(this.guardian); + }); + + it('from non-governance', async function () { + await expect(this.mock.connect(this.other).setProposalGuardian(this.guardian)) + .to.be.revertedWithCustomError(this.mock, 'GovernorOnlyExecutor') + .withArgs(this.other); + }); + }); + + it('cancel proposal during pending state from proposer when proposal guardian is non-zero', async function () { + await this.mock.$_setProposalGuardian(this.guardian); + await this.helper.connect(this.proposer).propose(); + await expect(this.helper.connect(this.proposer).cancel()) + .to.emit(this.mock, 'ProposalCanceled') + .withArgs(this.proposal.id); + }); + + describe('cancel proposal during active state', function () { + beforeEach(async function () { + await this.helper.connect(this.proposer).propose(); + await this.helper.waitForSnapshot(1n); + await expect(this.mock.state(this.proposal.id)).to.eventually.equal(ProposalState.Active); + }); + + it('from proposal guardian', async function () { + await this.mock.$_setProposalGuardian(this.guardian); + + await expect(this.helper.connect(this.guardian).cancel()) + .to.emit(this.mock, 'ProposalCanceled') + .withArgs(this.proposal.id); + }); + + it('from proposer when proposal guardian is non-zero', async function () { + await this.mock.$_setProposalGuardian(this.guardian); + + await expect(this.helper.connect(this.proposer).cancel()) + .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel') + .withArgs(this.proposal.id, this.proposer); + }); + + it('from proposer when proposal guardian is zero', async function () { + await this.mock.$_setProposalGuardian(ethers.ZeroAddress); + + await expect(this.helper.connect(this.proposer).cancel()) + .to.emit(this.mock, 'ProposalCanceled') + .withArgs(this.proposal.id); + }); + }); + }); + } +});