Sfoglia il codice sorgente

Add Governor signature nonces (#4378)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Sergei Tikhomirov <sergey.s.tikhomirov@gmail.com>
Co-authored-by: Renan Souza <renan.rodrigues.souza1@gmail.com>
Ernesto García 2 anni fa
parent
commit
6e21422737

+ 5 - 0
.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.

+ 25 - 8
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);
     }
 
     /**

+ 7 - 0
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,

+ 1 - 0
hardhat.config.js

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

+ 6 - 6
package-lock.json

@@ -7504,9 +7504,9 @@
       }
     },
     "node_modules/hardhat-ignore-warnings": {
-      "version": "0.2.8",
-      "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.8.tgz",
-      "integrity": "sha512-vPX94rJyTzYsCOzGIYdOcJgn3iQI6qa+CI9ZZfgDhdXJpda8ljpOT7bdUKAYC4LyoP0Z5fWTmupXoPaQrty0gw==",
+      "version": "0.2.9",
+      "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.9.tgz",
+      "integrity": "sha512-q1oj6/ixiAx+lgIyGLBajVCSC7qUtAoK7LS9Nr8UVHYo8Iuh5naBiVGo4RDJ6wxbDGYBkeSukUGZrMqzC2DWwA==",
       "dev": true,
       "dependencies": {
         "minimatch": "^5.1.0",
@@ -21441,9 +21441,9 @@
       }
     },
     "hardhat-ignore-warnings": {
-      "version": "0.2.8",
-      "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.8.tgz",
-      "integrity": "sha512-vPX94rJyTzYsCOzGIYdOcJgn3iQI6qa+CI9ZZfgDhdXJpda8ljpOT7bdUKAYC4LyoP0Z5fWTmupXoPaQrty0gw==",
+      "version": "0.2.9",
+      "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.9.tgz",
+      "integrity": "sha512-q1oj6/ixiAx+lgIyGLBajVCSC7qUtAoK7LS9Nr8UVHYo8Iuh5naBiVGo4RDJ6wxbDGYBkeSukUGZrMqzC2DWwA==",
       "dev": true,
       "requires": {
         "minimatch": "^5.1.0",

+ 95 - 6
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]);

+ 94 - 30
test/governance/extensions/GovernorWithParams.test.js

@@ -2,11 +2,12 @@ const { expectEvent } = require('@openzeppelin/test-helpers');
 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');
 const { GovernorHelper } = require('../../helpers/governance');
+const { expectRevertCustomError } = require('../../helpers/customError');
 
 const Governor = artifacts.require('$GovernorWithParamsMock');
 const CallReceiver = artifacts.require('CallReceiverMock');
@@ -119,56 +120,119 @@ 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());
+      describe('voting by signature', function () {
+        beforeEach(async function () {
+          this.voterBySig = Wallet.generate();
+          this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString());
 
-        const signature = (contract, message) =>
-          getDomain(contract)
-            .then(domain => ({
+          this.data = (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(voterBySig.getPrivateKey(), { data }))
-            .then(fromRpcSig);
+            }));
 
-        await this.token.delegate(voterBySigAddress, { from: voter2 });
+          this.signature = (contract, message) =>
+            this.data(contract, message)
+              .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }))
+              .then(fromRpcSig);
 
-        // Run proposal
-        await this.helper.propose();
-        await this.helper.waitForSnapshot();
+          await this.token.delegate(this.voterBySig.address, { from: voter2 });
 
-        const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
+          // Run proposal
+          await this.helper.propose();
+          await this.helper.waitForSnapshot();
+        });
 
-        const tx = await this.helper.vote({
-          support: Enums.VoteType.For,
-          reason: 'no particular reason',
-          params: encodedParams,
-          signature,
+        it('is properly supported', async function () {
+          const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
+
+          const nonce = await this.mock.nonces(this.voterBySig.address);
+
+          const tx = await this.helper.vote({
+            support: Enums.VoteType.For,
+            voter: this.voterBySig.address,
+            nonce,
+            reason: 'no particular reason',
+            params: encodedParams,
+            signature: this.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);
+
+          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;
+            },
+            reason: 'no particular reason',
+            params: encodedParams,
+          };
+
+          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,
+          ]);
         });
 
-        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);
+
+          const voteParams = {
+            support: Enums.VoteType.For,
+            voter: this.voterBySig.address,
+            nonce: nonce.addn(1),
+            signature: this.signature,
+            reason: 'no particular reason',
+            params: encodedParams,
+          };
+
+          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],
+          );
+        });
       });
     });
   }

+ 27 - 19
test/helpers/governance.js

@@ -91,26 +91,17 @@ class GovernorHelper {
     return vote.signature
       ? // if signature, and either params or reason →
         vote.params || vote.reason
-        ? vote
-            .signature(this.governor, {
-              proposalId: proposal.id,
-              support: vote.support,
-              reason: vote.reason || '',
-              params: vote.params || '',
-            })
-            .then(({ v, r, s }) =>
-              this.governor.castVoteWithReasonAndParamsBySig(
-                ...concatOpts([proposal.id, vote.support, vote.reason || '', vote.params || '', v, r, s], opts),
+        ? this.sign(vote).then(({ v, r, s }) =>
+            this.governor.castVoteWithReasonAndParamsBySig(
+              ...concatOpts(
+                [proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', v, r, s],
+                opts,
               ),
-            )
-        : vote
-            .signature(this.governor, {
-              proposalId: proposal.id,
-              support: vote.support,
-            })
-            .then(({ v, r, s }) =>
-              this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, v, r, s], opts)),
-            )
+            ),
+          )
+        : this.sign(vote).then(({ v, r, s }) =>
+            this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, v, r, s], opts)),
+          )
       : vote.params
       ? // otherwise if params
         this.governor.castVoteWithReasonAndParams(
@@ -122,6 +113,23 @@ class GovernorHelper {
       : this.governor.castVote(...concatOpts([proposal.id, vote.support], opts));
   }
 
+  sign(vote = {}) {
+    return vote.signature(this.governor, this.forgeMessage(vote));
+  }
+
+  forgeMessage(vote = {}) {
+    const proposal = this.currentProposal;
+
+    const message = { proposalId: proposal.id, support: vote.support, voter: vote.voter, nonce: vote.nonce };
+
+    if (vote.params || vote.reason) {
+      message.reason = vote.reason || '';
+      message.params = vote.params || '';
+    }
+
+    return message;
+  }
+
   async waitForSnapshot(offset = 0) {
     const proposal = this.currentProposal;
     const timepoint = await this.governor.proposalSnapshot(proposal.id);

+ 3 - 3
test/utils/introspection/SupportsInterface.behavior.js

@@ -66,7 +66,7 @@ const INTERFACES = {
     'execute(address[],uint256[],bytes[],bytes32)',
     'castVote(uint256,uint8)',
     'castVoteWithReason(uint256,uint8,string)',
-    'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)',
+    'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)',
   ],
   GovernorWithParams: [
     'name()',
@@ -87,8 +87,8 @@ const INTERFACES = {
     'castVote(uint256,uint8)',
     'castVoteWithReason(uint256,uint8,string)',
     'castVoteWithReasonAndParams(uint256,uint8,string,bytes)',
-    'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)',
-    'castVoteWithReasonAndParamsBySig(uint256,uint8,string,bytes,uint8,bytes32,bytes32)',
+    'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)',
+    'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,uint8,bytes32,bytes32)',
   ],
   GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'],
   GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'],