Add Governor extension `GovernorNoncesKeyed` to use `NoncesKeyed` for vote by sig (#5574)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: ernestognw <ernestognw@gmail.com>

.changeset/popular-geese-tan.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`GovernorNoncesKeyed`: Extension of `Governor` that adds support for keyed nonces when voting by sig.

contracts/governance/Governor.sol

@@ -538,16 +538,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         address voter,
         bytes memory signature
     ) public virtual returns (uint256) {
-        bool valid = SignatureChecker.isValidSignatureNow(
-            voter,
-            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),
-            signature
-        );
-
-        if (!valid) {
+        if (!_validateVoteSig(proposalId, support, voter, signature)) {
             revert GovernorInvalidSignature(voter);
         }
-
         return _castVote(proposalId, voter, support, "");
     }
 
@@ -560,31 +553,56 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         bytes memory params,
         bytes memory signature
     ) public virtual returns (uint256) {
-        bool valid = SignatureChecker.isValidSignatureNow(
-            voter,
-            _hashTypedDataV4(
-                keccak256(
-                    abi.encode(
-                        EXTENDED_BALLOT_TYPEHASH,
-                        proposalId,
-                        support,
-                        voter,
-                        _useNonce(voter),
-                        keccak256(bytes(reason)),
-                        keccak256(params)
-                    )
-                )
-            ),
-            signature
-        );
-
-        if (!valid) {
+        if (!_validateExtendedVoteSig(proposalId, support, voter, reason, params, signature)) {
             revert GovernorInvalidSignature(voter);
         }
-
         return _castVote(proposalId, voter, support, reason, params);
     }
 
+    /// @dev Validate the `signature` used in {castVoteBySig} function.
+    function _validateVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        bytes memory signature
+    ) internal virtual returns (bool) {
+        return
+            SignatureChecker.isValidSignatureNow(
+                voter,
+                _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),
+                signature
+            );
+    }
+
+    /// @dev Validate the `signature` used in {castVoteWithReasonAndParamsBySig} function.
+    function _validateExtendedVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        string memory reason,
+        bytes memory params,
+        bytes memory signature
+    ) internal virtual returns (bool) {
+        return
+            SignatureChecker.isValidSignatureNow(
+                voter,
+                _hashTypedDataV4(
+                    keccak256(
+                        abi.encode(
+                            EXTENDED_BALLOT_TYPEHASH,
+                            proposalId,
+                            support,
+                            voter,
+                            _useNonce(voter),
+                            keccak256(bytes(reason)),
+                            keccak256(params)
+                        )
+                    )
+                ),
+                signature
+            );
+    }
+
     /**
      * @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve
      * voting weight using {IGovernor-getVotes} and call the {_countVote} internal function. Uses the _defaultParams().

contracts/governance/README.adoc

@@ -54,6 +54,8 @@ Other extensions can customize the behavior or interface in multiple ways.
 
 * {GovernorSuperQuorum}: Extension of {Governor} with a super quorum. Proposals that meet the super quorum (and have a majority of for votes) advance to the `Succeeded` state before the proposal deadline.
 
+* {GovernorNoncesKeyed}: An extension of {Governor} with support for keyed nonces in addition to traditional nonces when voting by signature.
+
 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.
@@ -100,6 +102,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you
 
 {{GovernorSuperQuorum}}
 
+{{GovernorNoncesKeyed}}
+
 == Utils
 
 {{Votes}}

contracts/governance/extensions/GovernorNoncesKeyed.sol

@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.24;
+
+import {Governor} from "../Governor.sol";
+import {Nonces} from "../../utils/Nonces.sol";
+import {NoncesKeyed} from "../../utils/NoncesKeyed.sol";
+import {SignatureChecker} from "../../utils/cryptography/SignatureChecker.sol";
+
+/**
+ * @dev An extension of {Governor} that extends existing nonce management to use {NoncesKeyed}, where the key is the first 192 bits of the `proposalId`.
+ * This is useful for voting by signature while maintaining separate sequences of nonces for each proposal.
+ *
+ * NOTE: Traditional (un-keyed) nonces are still supported and can continue to be used as if this extension was not present.
+ */
+abstract contract GovernorNoncesKeyed is Governor, NoncesKeyed {
+    function _useCheckedNonce(address owner, uint256 nonce) internal virtual override(Nonces, NoncesKeyed) {
+        super._useCheckedNonce(owner, nonce);
+    }
+
+    /**
+     * @dev Check the signature against keyed nonce and falls back to the traditional nonce.
+     *
+     * NOTE: This function won't call `super._validateVoteSig` if the keyed nonce is valid.
+     * Side effects may be skipped depending on the linearization of the function.
+     */
+    function _validateVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        bytes memory signature
+    ) internal virtual override returns (bool) {
+        if (
+            SignatureChecker.isValidSignatureNow(
+                voter,
+                _hashTypedDataV4(
+                    keccak256(
+                        abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, nonces(voter, uint192(proposalId)))
+                    )
+                ),
+                signature
+            )
+        ) {
+            _useNonce(voter, uint192(proposalId));
+            return true;
+        } else {
+            return super._validateVoteSig(proposalId, support, voter, signature);
+        }
+    }
+
+    /**
+     * @dev Check the signature against keyed nonce and falls back to the traditional nonce.
+     *
+     * NOTE: This function won't call `super._validateExtendedVoteSig` if the keyed nonce is valid.
+     * Side effects may be skipped depending on the linearization of the function.
+     */
+    function _validateExtendedVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        string memory reason,
+        bytes memory params,
+        bytes memory signature
+    ) internal virtual override returns (bool) {
+        if (
+            SignatureChecker.isValidSignatureNow(
+                voter,
+                _hashTypedDataV4(
+                    keccak256(
+                        abi.encode(
+                            EXTENDED_BALLOT_TYPEHASH,
+                            proposalId,
+                            support,
+                            voter,
+                            nonces(voter, uint192(proposalId)),
+                            keccak256(bytes(reason)),
+                            keccak256(params)
+                        )
+                    )
+                ),
+                signature
+            )
+        ) {
+            _useNonce(voter, uint192(proposalId));
+            return true;
+        } else {
+            return super._validateExtendedVoteSig(proposalId, support, voter, reason, params, signature);
+        }
+    }
+}

contracts/mocks/governance/GovernorNoncesKeyedMock.sol

@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.24;
+
+import {Governor, Nonces} 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";
+import {GovernorNoncesKeyed} from "../../governance/extensions/GovernorNoncesKeyed.sol";
+
+abstract contract GovernorNoncesKeyedMock is
+    GovernorSettings,
+    GovernorVotesQuorumFraction,
+    GovernorCountingSimple,
+    GovernorNoncesKeyed
+{
+    function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) {
+        return super.proposalThreshold();
+    }
+
+    function _validateVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        bytes memory signature
+    ) internal virtual override(Governor, GovernorNoncesKeyed) returns (bool) {
+        return super._validateVoteSig(proposalId, support, voter, signature);
+    }
+
+    function _validateExtendedVoteSig(
+        uint256 proposalId,
+        uint8 support,
+        address voter,
+        string memory reason,
+        bytes memory params,
+        bytes memory signature
+    ) internal virtual override(Governor, GovernorNoncesKeyed) returns (bool) {
+        return super._validateExtendedVoteSig(proposalId, support, voter, reason, params, signature);
+    }
+
+    function _useCheckedNonce(address owner, uint256 nonce) internal virtual override(Nonces, GovernorNoncesKeyed) {
+        super._useCheckedNonce(owner, nonce);
+    }
+}

test/governance/Governor.test.js

@@ -198,31 +198,35 @@ describe('Governor', function () {
       });
 
       describe('vote with signature', function () {
-        it('votes with an EOA signature', async function () {
+        it('votes with an EOA signature on two proposals', async function () {
           await this.token.connect(this.voter1).delegate(this.userEOA);
 
-          const nonce = await this.mock.nonces(this.userEOA);
-
-          // Run proposal
-          await this.helper.propose();
-          await this.helper.waitForSnapshot();
-          await expect(
-            this.helper.vote({
-              support: VoteType.For,
-              voter: this.userEOA.address,
-              nonce,
-              signature: signBallot(this.userEOA),
-            }),
-          )
-            .to.emit(this.mock, 'VoteCast')
-            .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), '');
-
-          await this.helper.waitForDeadline();
-          await this.helper.execute();
+          for (let i = 0; i < 2; i++) {
+            const nonce = await this.mock.nonces(this.userEOA);
 
-          // After
-          expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true;
-          expect(await this.mock.nonces(this.userEOA)).to.equal(nonce + 1n);
+            // Run proposal
+            await this.helper.propose();
+            await this.helper.waitForSnapshot();
+            await expect(
+              this.helper.vote({
+                support: VoteType.For,
+                voter: this.userEOA.address,
+                nonce,
+                signature: signBallot(this.userEOA),
+              }),
+            )
+              .to.emit(this.mock, 'VoteCast')
+              .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), '');
+
+            // After
+            expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true;
+            expect(await this.mock.nonces(this.userEOA)).to.equal(nonce + 1n);
+
+            // Update proposal to allow for re-propose
+            this.helper.description += ' - updated';
+          }
+
+          await expect(this.mock.nonces(this.userEOA)).to.eventually.equal(2n);
         });
 
         it('votes with a valid EIP-1271 signature', async function () {

test/governance/extensions/GovernorNoncesKeyed.test.js

@@ -0,0 +1,244 @@
+const { ethers } = require('hardhat');
+const { expect } = require('chai');
+const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
+
+const { GovernorHelper } = require('../../helpers/governance');
+const { getDomain, Ballot, ExtendedBallot } = require('../../helpers/eip712');
+const { VoteType } = require('../../helpers/enums');
+const { shouldBehaveLikeNoncesKeyed } = require('../../utils/Nonces.behavior');
+
+const name = 'OZ-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');
+
+const signBallot = account => (contract, message) =>
+  getDomain(contract).then(domain => account.signTypedData(domain, { Ballot }, message));
+const signExtendedBallot = account => (contract, message) =>
+  getDomain(contract).then(domain => account.signTypedData(domain, { ExtendedBallot }, message));
+
+describe('GovernorNoncesKeyed', function () {
+  const fixture = async () => {
+    const [owner, proposer, voter1, voter2, voter3, voter4, userEOA] = await ethers.getSigners();
+    const receiver = await ethers.deployContract('CallReceiverMock');
+
+    const token = await ethers.deployContract('$ERC20Votes', [tokenName, tokenSymbol, tokenName, version]);
+    const mock = await ethers.deployContract('$GovernorNoncesKeyedMock', [
+      name, // name
+      votingDelay, // initialVotingDelay
+      votingPeriod, // initialVotingPeriod
+      0n, // initialProposalThreshold
+      token, // tokenAddress
+      10n, // quorumNumeratorValue
+    ]);
+
+    await owner.sendTransaction({ to: mock, value });
+    await token.$_mint(owner, tokenSupply);
+
+    const helper = new GovernorHelper(mock, 'blocknumber');
+    await helper.connect(owner).delegate({ token: token, to: voter1, value: ethers.parseEther('10') });
+    await helper.connect(owner).delegate({ token: token, to: voter2, value: ethers.parseEther('7') });
+    await helper.connect(owner).delegate({ token: token, to: voter3, value: ethers.parseEther('5') });
+    await helper.connect(owner).delegate({ token: token, to: voter4, value: ethers.parseEther('2') });
+
+    return {
+      owner,
+      proposer,
+      voter1,
+      voter2,
+      voter3,
+      voter4,
+      userEOA,
+      receiver,
+      token,
+      mock,
+      helper,
+    };
+  };
+
+  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('vote with signature', function () {
+    for (const nonceType of ['default', 'keyed']) {
+      describe(`with ${nonceType} nonce`, function () {
+        beforeEach(async function () {
+          await this.helper.propose();
+
+          const maskedProposalId = BigInt(this.helper.id) & (2n ** 192n - 1n);
+
+          this.getNonce = async address => {
+            return await (nonceType === 'default'
+              ? this.mock.nonces(address)
+              : this.mock['nonces(address,uint192)'](address, maskedProposalId));
+          };
+        });
+
+        it('votes with an EOA signature', async function () {
+          await this.token.connect(this.voter1).delegate(this.userEOA);
+
+          const nonce = await this.getNonce(this.userEOA);
+
+          await this.helper.waitForSnapshot();
+          await expect(
+            this.helper.vote({
+              support: VoteType.For,
+              voter: this.userEOA.address,
+              nonce,
+              signature: signBallot(this.userEOA),
+            }),
+          )
+            .to.emit(this.mock, 'VoteCast')
+            .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), '');
+
+          await this.helper.waitForDeadline();
+          await this.helper.execute();
+
+          // After
+          expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true;
+          expect(await this.getNonce(this.userEOA)).to.equal(nonce + 1n);
+        });
+
+        it('votes with an EOA signature with reason', async function () {
+          await this.token.connect(this.voter1).delegate(this.userEOA);
+
+          const nonce = await this.getNonce(this.userEOA);
+
+          await this.helper.waitForSnapshot();
+          await expect(
+            this.helper.vote({
+              support: VoteType.For,
+              voter: this.userEOA.address,
+              nonce,
+              reason: 'This is an example reason',
+              signature: signExtendedBallot(this.userEOA),
+            }),
+          )
+            .to.emit(this.mock, 'VoteCast')
+            .withArgs(
+              this.userEOA,
+              this.proposal.id,
+              VoteType.For,
+              ethers.parseEther('10'),
+              'This is an example reason',
+            );
+
+          await this.helper.waitForDeadline();
+          await this.helper.execute();
+
+          // After
+          expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true;
+          expect(await this.getNonce(this.userEOA)).to.equal(nonce + 1n);
+        });
+
+        it('votes with a valid EIP-1271 signature', async function () {
+          const wallet = await ethers.deployContract('ERC1271WalletMock', [this.userEOA]);
+
+          await this.token.connect(this.voter1).delegate(wallet);
+
+          const nonce = await this.getNonce(wallet.target);
+
+          await this.helper.waitForSnapshot();
+          await expect(
+            this.helper.vote({
+              support: VoteType.For,
+              voter: wallet.target,
+              nonce,
+              signature: signBallot(this.userEOA),
+            }),
+          )
+            .to.emit(this.mock, 'VoteCast')
+            .withArgs(wallet, this.proposal.id, VoteType.For, ethers.parseEther('10'), '');
+          await this.helper.waitForDeadline();
+          await this.helper.execute();
+
+          // After
+          expect(await this.mock.hasVoted(this.proposal.id, wallet)).to.be.true;
+          expect(await this.getNonce(wallet)).to.equal(nonce + 1n);
+        });
+
+        afterEach('no other votes are cast', async function () {
+          expect(await this.mock.hasVoted(this.proposal.id, this.owner)).to.be.false;
+          expect(await this.mock.hasVoted(this.proposal.id, this.voter1)).to.be.false;
+          expect(await this.mock.hasVoted(this.proposal.id, this.voter2)).to.be.false;
+        });
+      });
+    }
+  });
+
+  describe('on vote by signature', function () {
+    beforeEach(async function () {
+      await this.token.connect(this.voter1).delegate(this.userEOA);
+
+      // Run proposal
+      await this.helper.propose();
+      await this.helper.waitForSnapshot();
+    });
+
+    it('if signature does not match signer', async function () {
+      const nonce = await this.mock.nonces(this.userEOA);
+
+      function tamper(str, index, mask) {
+        const arrayStr = ethers.getBytes(str);
+        arrayStr[index] ^= mask;
+        return ethers.hexlify(arrayStr);
+      }
+
+      const voteParams = {
+        support: VoteType.For,
+        voter: this.userEOA.address,
+        nonce,
+        signature: (...args) => signBallot(this.userEOA)(...args).then(sig => tamper(sig, 42, 0xff)),
+      };
+
+      await expect(this.helper.vote(voteParams))
+        .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature')
+        .withArgs(voteParams.voter);
+    });
+
+    for (const nonceType of ['default', 'keyed']) {
+      it(`if vote nonce is incorrect with ${nonceType} nonce`, async function () {
+        const nonce = await (nonceType === 'default'
+          ? this.mock.nonces(this.userEOA)
+          : this.mock['nonces(address,uint192)'](this.userEOA, BigInt(this.helper.id) & (2n ** 192n - 1n)));
+
+        const voteParams = {
+          support: VoteType.For,
+          voter: this.userEOA.address,
+          nonce: nonce + 1n,
+          signature: signBallot(this.userEOA),
+        };
+
+        await expect(this.helper.vote(voteParams))
+          .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature')
+          .withArgs(voteParams.voter);
+      });
+    }
+  });
+
+  shouldBehaveLikeNoncesKeyed();
+});