From 2ed9be3e3c02d5084c22ddbcdebef4b3aff11bd6 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 29 Jun 2023 12:57:33 -0600 Subject: [PATCH] Fix tests --- test/governance/Governor.test.js | 89 ++++++++------- .../extensions/GovernorWithParams.test.js | 101 ++++++++++-------- test/helpers/governance.js | 53 ++++----- 3 files changed, 135 insertions(+), 108 deletions(-) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 2c1cbf50a98..f4a352c6d8c 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'); @@ -311,22 +311,24 @@ contract('Governor', function (accounts) { 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) => - 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.data(contract, message) .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data })) .then(fromRpcSig); @@ -340,35 +342,46 @@ contract('Governor', function (accounts) { 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', - [], - ); + 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); - expectRevertCustomError( - this.helper.vote({ - support: Enums.VoteType.For, - voter: this.voterBySig.address, - nonce: nonce.addn(1), - signature: this.signature, - }), + 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], ); }); }); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 1a0d242acda..fb58edaaf57 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -2,7 +2,7 @@ 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'); @@ -125,24 +125,26 @@ contract('GovernorWithParams', function (accounts) { this.voterBySig = Wallet.generate(); this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString()); + 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, + })); + 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, - })) + this.data(contract, message) .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data })) .then(fromRpcSig); @@ -185,39 +187,50 @@ contract('GovernorWithParams', function (accounts) { 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; - }, - reason: 'no particular reason', - params: encodedParams, - }), - 'GovernorInvalidSigner', - [], - ); + 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, + ]); }); 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, - reason: 'no particular reason', - params: encodedParams, - }), + 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 333904dd8c1..588aeb258c0 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -91,33 +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, - voter: vote.voter, - nonce: vote.nonce, - reason: vote.reason || '', - params: vote.params || '', - }) - .then(({ v, r, s }) => - this.governor.castVoteWithReasonAndParamsBySig( - ...concatOpts( - [proposal.id, vote.support, vote.voter, 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, - voter: vote.voter, - nonce: vote.nonce, - }) - .then(({ v, r, s }) => - this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, 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( @@ -129,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);