Add a governor extension that implements a proposal guardian (#5303)

Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
Co-authored-by: Ernesto García <ernestognw@gmail.com>

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

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).
+        address caller = _msgSender();
-        _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
+        if (!_validateCancel(proposalId, caller)) revert GovernorUnableToCancel(proposalId, caller);
-        if (_msgSender() != proposalProposer(proposalId)) {
-            revert GovernorOnlyProposer(_msgSender());
-        }
 
         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
      */

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.
      */

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:
 
 * <<Governor-votingDelay-,`votingDelay()`>>: 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}}

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);
+    }
+}

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);
+    }
+}

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')
+              .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
-              .withArgs(this.owner);
+              .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')
+              .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
-              .withArgs(
+              .withArgs(this.proposal.id, this.owner);
-                this.proposal.id,
-                ProposalState.Active,
-                GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
-              );
           });
 
           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')
+              .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
-              .withArgs(
+              .withArgs(this.proposal.id, this.voter1);
-                this.proposal.id,
-                ProposalState.Active,
-                GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
-              );
           });
 
           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')
+              .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
-              .withArgs(
+              .withArgs(this.proposal.id, this.voter1);
-                this.proposal.id,
-                ProposalState.Succeeded,
-                GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
-              );
           });
 
           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')
+              .to.be.revertedWithCustomError(this.mock, 'GovernorUnableToCancel')
-              .withArgs(
+              .withArgs(this.proposal.id, this.voter1);
-                this.proposal.id,
-                ProposalState.Executed,
-                GovernorHelper.proposalStatesToBitMap([ProposalState.Pending]),
-              );
           });
         });
       });

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'),
+            },
+          ],
+          '<proposal description>',
+        );
+      });
+
+      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);
+        });
+      });
+    });
+  }
+});