Add Governor signature nonces

.changeset/sixty-numbers-reply.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`Governor`: Add `voter` and `nonce` parameters in signed ballots, to avoid forging signatures for random addresses, prevent signature replay, and allow invalidating signatures. Add `voter` as a new parameter in the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions.

contracts/governance/Governor.sol

@@ -12,6 +12,7 @@ import {SafeCast} from "../utils/math/SafeCast.sol";
 import {DoubleEndedQueue} from "../utils/structs/DoubleEndedQueue.sol";
 import {Address} from "../utils/Address.sol";
 import {Context} from "../utils/Context.sol";
+import {Nonces} from "../utils/Nonces.sol";
 import {IGovernor, IERC6372} from "./IGovernor.sol";
 
 /**
@@ -25,12 +26,15 @@ import {IGovernor, IERC6372} from "./IGovernor.sol";
  *
  * _Available since v4.3._
  */
-abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver {
+abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC721Receiver, IERC1155Receiver {
     using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
 
-    bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)");
+    bytes32 public constant BALLOT_TYPEHASH =
+        keccak256("Ballot(uint256 proposalId,uint8 support,address voter,uint256 nonce)");
     bytes32 public constant EXTENDED_BALLOT_TYPEHASH =
-        keccak256("ExtendedBallot(uint256 proposalId,uint8 support,string reason,bytes params)");
+        keccak256(
+            "ExtendedBallot(uint256 proposalId,uint8 support,address voter,uint256 nonce,string reason,bytes params)"
+        );
 
     // solhint-disable var-name-mixedcase
     struct ProposalCore {
@@ -514,17 +518,23 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     function castVoteBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         uint8 v,
         bytes32 r,
         bytes32 s
     ) public virtual override returns (uint256) {
-        address voter = ECDSA.recover(
-            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support))),
+        address signer = ECDSA.recover(
+            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),
             v,
             r,
             s
         );
-        return _castVote(proposalId, voter, support, "");
+
+        if (voter != signer) {
+            revert GovernorInvalidSigner(signer, voter);
+        }
+
+        return _castVote(proposalId, signer, support, "");
     }
 
     /**
@@ -533,19 +543,22 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     function castVoteWithReasonAndParamsBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         string calldata reason,
         bytes memory params,
         uint8 v,
         bytes32 r,
         bytes32 s
     ) public virtual override returns (uint256) {
-        address voter = ECDSA.recover(
+        address signer = ECDSA.recover(
             _hashTypedDataV4(
                 keccak256(
                     abi.encode(
                         EXTENDED_BALLOT_TYPEHASH,
                         proposalId,
                         support,
+                        voter,
+                        _useNonce(voter),
                         keccak256(bytes(reason)),
                         keccak256(params)
                     )
@@ -556,7 +569,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
             s
         );
 
-        return _castVote(proposalId, voter, support, reason, params);
+        if (voter != signer) {
+            revert GovernorInvalidSigner(signer, voter);
+        }
+
+        return _castVote(proposalId, signer, support, reason, params);
     }
 
     /**

contracts/governance/IGovernor.sol

@@ -80,6 +80,11 @@ abstract contract IGovernor is IERC165, IERC6372 {
      */
     error GovernorInvalidVoteType();
 
+    /**
+     * @dev The `voter` doesn't match with the recovered `signer`.
+     */
+    error GovernorInvalidSigner(address signer, address voter);
+
     /**
      * @dev Emitted when a proposal is created.
      */
@@ -355,6 +360,7 @@ abstract contract IGovernor is IERC165, IERC6372 {
     function castVoteBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         uint8 v,
         bytes32 r,
         bytes32 s
@@ -368,6 +374,7 @@ abstract contract IGovernor is IERC165, IERC6372 {
     function castVoteWithReasonAndParamsBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         string calldata reason,
         bytes memory params,
         uint8 v,

hardhat.config.js

@@ -78,6 +78,7 @@ module.exports = {
   warnings: {
     'contracts-exposed/**/*': {
       'code-size': 'off',
+      'initcode-size': 'off',
     },
     '*': {
       'code-size': withOptimizations,

test/governance/Governor.test.js

@@ -2,7 +2,7 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-hel
 const { expect } = require('chai');
 const ethSigUtil = require('eth-sig-util');
 const Wallet = require('ethereumjs-wallet').default;
-const { fromRpcSig } = require('ethereumjs-util');
+const { fromRpcSig, toRpcSig } = require('ethereumjs-util');
 
 const Enums = require('../helpers/enums');
 const { getDomain, domainType } = require('../helpers/eip712');
@@ -166,7 +166,7 @@ contract('Governor', function (accounts) {
         expect(await web3.eth.getBalance(this.receiver.address)).to.be.bignumber.equal(value);
       });
 
-      it('vote with signature', async function () {
+      it('votes with signature', async function () {
         const voterBySig = Wallet.generate();
         const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
 
@@ -179,6 +179,8 @@ contract('Governor', function (accounts) {
                 Ballot: [
                   { name: 'proposalId', type: 'uint256' },
                   { name: 'support', type: 'uint8' },
+                  { name: 'voter', type: 'address' },
+                  { name: 'nonce', type: 'uint256' },
                 ],
               },
               domain,
@@ -189,13 +191,19 @@ contract('Governor', function (accounts) {
 
         await this.token.delegate(voterBySigAddress, { from: voter1 });
 
+        const nonce = await this.mock.nonces(voterBySigAddress);
+
         // Run proposal
         await this.helper.propose();
         await this.helper.waitForSnapshot();
-        expectEvent(await this.helper.vote({ support: Enums.VoteType.For, signature }), 'VoteCast', {
-          voter: voterBySigAddress,
-          support: Enums.VoteType.For,
-        });
+        expectEvent(
+          await this.helper.vote({ support: Enums.VoteType.For, voter: voterBySigAddress, nonce, signature }),
+          'VoteCast',
+          {
+            voter: voterBySigAddress,
+            support: Enums.VoteType.For,
+          },
+        );
         await this.helper.waitForDeadline();
         await this.helper.execute();
 
@@ -204,6 +212,7 @@ contract('Governor', function (accounts) {
         expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false);
         expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false);
         expect(await this.mock.hasVoted(this.proposal.id, voterBySigAddress)).to.be.equal(true);
+        expect(await this.mock.nonces(voterBySigAddress)).to.be.bignumber.equal(nonce.addn(1));
       });
 
       it('send ethers', async function () {
@@ -297,6 +306,86 @@ contract('Governor', function (accounts) {
           });
         });
 
+        describe('on vote by signature', function () {
+          beforeEach(async function () {
+            this.voterBySig = Wallet.generate();
+            this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString());
+
+            this.data = (contract, message) =>
+              getDomain(contract).then(domain => ({
+                primaryType: 'Ballot',
+                types: {
+                  EIP712Domain: domainType(domain),
+                  Ballot: [
+                    { name: 'proposalId', type: 'uint256' },
+                    { name: 'support', type: 'uint8' },
+                    { name: 'voter', type: 'address' },
+                    { name: 'nonce', type: 'uint256' },
+                  ],
+                },
+                domain,
+                message,
+              }));
+
+            this.signature = (contract, message) =>
+              this.data(contract, message)
+                .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }))
+                .then(fromRpcSig);
+
+            await this.token.delegate(this.voterBySig.address, { from: voter1 });
+
+            // 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.voterBySig.address);
+
+            const voteParams = {
+              support: Enums.VoteType.For,
+              voter: this.voterBySig.address,
+              nonce,
+              signature: async (...params) => {
+                const sig = await this.signature(...params);
+                sig.s[12] ^= 0xff;
+                return sig;
+              },
+            };
+
+            const { r, s, v } = await this.helper.sign(voteParams);
+            const message = this.helper.forgeMessage(voteParams);
+            const data = await this.data(this.mock, message);
+
+            await expectRevertCustomError(this.helper.vote(voteParams), 'GovernorInvalidSigner', [
+              ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }),
+              voteParams.voter,
+            ]);
+          });
+
+          it('if vote nonce is incorrect', async function () {
+            const nonce = await this.mock.nonces(this.voterBySig.address);
+
+            const voteParams = {
+              support: Enums.VoteType.For,
+              voter: this.voterBySig.address,
+              nonce: nonce.addn(1),
+              signature: this.signature,
+            };
+
+            const { r, s, v } = await this.helper.sign(voteParams);
+            const message = this.helper.forgeMessage(voteParams);
+            const data = await this.data(this.mock, { ...message, nonce });
+
+            await expectRevertCustomError(
+              this.helper.vote(voteParams),
+              // The signature check implies the nonce can't be tampered without changing the signer
+              'GovernorInvalidSigner',
+              [ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }), voteParams.voter],
+            );
+          });
+        });
+
         describe('on execute', function () {
           it('if proposal does not exist', async function () {
             await expectRevertCustomError(this.helper.execute(), 'GovernorNonexistentProposal', [this.proposal.id]);