diff --git a/.changeset/sixty-numbers-reply.md b/.changeset/sixty-numbers-reply.md new file mode 100644 index 000000000..4e6faa837 --- /dev/null +++ b/.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. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index d0ace4f03..1e1526ceb 100644 --- a/contracts/governance/Governor.sol +++ b/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); } /** diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 2eeedd313..20636e3ba 100644 --- a/contracts/governance/IGovernor.sol +++ b/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, diff --git a/hardhat.config.js b/hardhat.config.js index 6cb8b9144..8ee5d05e5 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -78,6 +78,7 @@ module.exports = { warnings: { 'contracts-exposed/**/*': { 'code-size': 'off', + 'initcode-size': 'off', }, '*': { 'code-size': withOptimizations, diff --git a/package-lock.json b/package-lock.json index a85611941..2e6296d24 100644 --- a/package-lock.json +++ b/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", diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 909c38686..f4a352c6d 100644 --- a/test/governance/Governor.test.js +++ b/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]); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 00936c505..fb58edaaf 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/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], + ); + }); }); }); } diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 665c21605..588aeb258 100644 --- a/test/helpers/governance.js +++ b/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); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 201a55f47..71be4313f 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/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)'],