Add Governor signature nonces

contracts/governance/Governor.sol

@@ -12,6 +12,7 @@ import "../utils/math/SafeCast.sol";
 import "../utils/structs/DoubleEndedQueue.sol";
 import "../utils/Address.sol";
 import "../utils/Context.sol";
+import "../utils/Nonces.sol";
 import "./IGovernor.sol";
 
 /**
@@ -25,12 +26,15 @@ import "./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 {
@@ -519,17 +523,25 @@ 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))),
+        uint256 currentNonce = _useNonce(voter);
+
+        address signer = ECDSA.recover(
+            _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, currentNonce))),
             v,
             r,
             s
         );
-        return _castVote(proposalId, voter, support, "");
+
+        if (voter != signer) {
+            revert GovernorInvalidSigner(signer, voter);
+        }
+
+        return _castVote(proposalId, signer, support, "");
     }
 
     /**
@@ -538,19 +550,24 @@ 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(
+        uint256 currentNonce = _useNonce(voter);
+
+        address signer = ECDSA.recover(
             _hashTypedDataV4(
                 keccak256(
                     abi.encode(
                         EXTENDED_BALLOT_TYPEHASH,
                         proposalId,
                         support,
+                        voter,
+                        currentNonce,
                         keccak256(bytes(reason)),
                         keccak256(params)
                     )
@@ -561,7 +578,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.
      */
@@ -348,6 +353,7 @@ abstract contract IGovernor is IERC165, IERC6372 {
     function castVoteBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         uint8 v,
         bytes32 r,
         bytes32 s
@@ -361,6 +367,7 @@ abstract contract IGovernor is IERC165, IERC6372 {
     function castVoteWithReasonAndParamsBySig(
         uint256 proposalId,
         uint8 support,
+        address voter,
         string calldata reason,
         bytes memory params,
         uint8 v,

test/governance/Governor.test.js

@@ -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: 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,73 @@ 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.signature = (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,
+                }))
+                .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);
+
+            expectRevertCustomError(
+              this.helper.vote({
+                support: Enums.VoteType.For,
+                voter: this.voterBySig.address,
+                nonce,
+                signature: (...params) => {
+                  const sig = this.signature(...params);
+                  sig[69] ^= 0xff;
+                  return sig;
+                },
+              }),
+              'GovernorInvalidSigner',
+              [],
+            );
+          });
+
+          it('if vote nonce is incorrect', async function () {
+            const nonce = await this.mock.nonces(this.voterBySig.address);
+
+            expectRevertCustomError(
+              this.helper.vote({
+                support: Enums.VoteType.For,
+                voter: this.voterBySig.address,
+                nonce: nonce.addn(1),
+                signature: this.signature,
+              }),
+              // The signature check implies the nonce can't be tampered without changing the signer
+              'GovernorInvalidSigner',
+              [],
+            );
+          });
+        });
+
         describe('on execute', function () {
           it('if proposal does not exist', async function () {
             await expectRevertCustomError(this.helper.execute(), 'GovernorNonexistentProposal', [this.proposal.id]);

test/governance/extensions/GovernorWithParams.test.js

@@ -7,6 +7,7 @@ const { fromRpcSig } = require('ethereumjs-util');
 const Enums = require('../../helpers/enums');
 const { getDomain, domainType } = require('../../helpers/eip712');
 const { GovernorHelper } = require('../../helpers/governance');
+const { expectRevertCustomError } = require('../../helpers/customError');
 
 const Governor = artifacts.require('$GovernorWithParamsMock');
 const CallReceiver = artifacts.require('CallReceiverMock');
@@ -119,56 +120,102 @@ contract('GovernorWithParams', function (accounts) {
         expect(votes.forVotes).to.be.bignumber.equal(weight);
       });
 
-      it('Voting with params by signature is properly supported', async function () {
-        const voterBySig = Wallet.generate();
-        const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
-
-        const signature = (contract, message) =>
-          getDomain(contract)
-            .then(domain => ({
-              primaryType: 'ExtendedBallot',
-              types: {
-                EIP712Domain: domainType(domain),
-                ExtendedBallot: [
-                  { name: 'proposalId', type: 'uint256' },
-                  { name: 'support', type: 'uint8' },
-                  { name: 'reason', type: 'string' },
-                  { name: 'params', type: 'bytes' },
-                ],
-              },
-              domain,
-              message,
-            }))
-            .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data }))
-            .then(fromRpcSig);
+      describe('voting by signature', function () {
+        beforeEach(async function () {
+          this.voterBySig = Wallet.generate();
+          this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString());
+
+          this.signature = (contract, message) =>
+            getDomain(contract)
+              .then(domain => ({
+                primaryType: 'ExtendedBallot',
+                types: {
+                  EIP712Domain: domainType(domain),
+                  ExtendedBallot: [
+                    { name: 'proposalId', type: 'uint256' },
+                    { name: 'support', type: 'uint8' },
+                    { name: 'voter', type: 'address' },
+                    { name: 'nonce', type: 'uint256' },
+                    { name: 'reason', type: 'string' },
+                    { name: 'params', type: 'bytes' },
+                  ],
+                },
+                domain,
+                message,
+              }))
+              .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }))
+              .then(fromRpcSig);
+
+          await this.token.delegate(this.voterBySig.address, { from: voter2 });
+
+          // Run proposal
+          await this.helper.propose();
+          await this.helper.waitForSnapshot();
+        });
 
-        await this.token.delegate(voterBySigAddress, { from: voter2 });
+        it('is properly supported', async function () {
+          const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
 
-        // Run proposal
-        await this.helper.propose();
-        await this.helper.waitForSnapshot();
+          const nonce = await this.mock.nonces(this.voterBySig.address);
 
-        const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
+          const tx = await this.helper.vote({
+            support: Enums.VoteType.For,
+            voter: this.voterBySig.address,
+            nonce: nonce,
+            reason: 'no particular reason',
+            params: encodedParams,
+            signature: this.signature,
+          });
 
-        const tx = await this.helper.vote({
-          support: Enums.VoteType.For,
-          reason: 'no particular reason',
-          params: encodedParams,
-          signature,
+          expectEvent(tx, 'CountParams', { ...rawParams });
+          expectEvent(tx, 'VoteCastWithParams', {
+            voter: this.voterBySig.address,
+            proposalId: this.proposal.id,
+            support: Enums.VoteType.For,
+            weight,
+            reason: 'no particular reason',
+            params: encodedParams,
+          });
+
+          const votes = await this.mock.proposalVotes(this.proposal.id);
+          expect(votes.forVotes).to.be.bignumber.equal(weight);
+          expect(await this.mock.nonces(this.voterBySig.address)).to.be.bignumber.equal(nonce.addn(1));
         });
 
-        expectEvent(tx, 'CountParams', { ...rawParams });
-        expectEvent(tx, 'VoteCastWithParams', {
-          voter: voterBySigAddress,
-          proposalId: this.proposal.id,
-          support: Enums.VoteType.For,
-          weight,
-          reason: 'no particular reason',
-          params: encodedParams,
+        it('reverts if signature does not match signer', async function () {
+          const nonce = await this.mock.nonces(this.voterBySig.address);
+
+          expectRevertCustomError(
+            this.helper.vote({
+              support: Enums.VoteType.For,
+              voter: this.voterBySig.address,
+              nonce,
+              signature: (...params) => {
+                const sig = this.signature(...params);
+                sig[69] ^= 0xff;
+                return sig;
+              },
+            }),
+            'GovernorInvalidSigner',
+            [],
+          );
         });
 
-        const votes = await this.mock.proposalVotes(this.proposal.id);
-        expect(votes.forVotes).to.be.bignumber.equal(weight);
+        it('reverts if vote nonce is incorrect', async function () {
+          const nonce = await this.mock.nonces(this.voterBySig.address);
+
+          expectRevertCustomError(
+            this.helper.vote({
+              support: Enums.VoteType.For,
+              voter: this.voterBySig.address,
+              nonce: nonce.addn(1),
+              signature: this.signature,
+            }),
+            // The signature check implies the nonce can't be tampered without changing the signer
+            'GovernorInvalidSigner',
+            [],
+          );
+        });
       });
     });
   }