From e473bcf859e1ac4b6fd736d8e29b462a6192705c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Nov 2023 03:24:21 +0100 Subject: [PATCH] Migrate metatx tests to ethers.js (#4737) Co-authored-by: ernestognw --- test/helpers/account.js | 8 +- test/helpers/math.js | 1 + test/metatx/ERC2771Context.test.js | 153 +++--- test/metatx/ERC2771Forwarder.test.js | 664 ++++++++++++--------------- test/utils/Context.behavior.js | 48 +- test/utils/Context.test.js | 19 +- 6 files changed, 408 insertions(+), 485 deletions(-) diff --git a/test/helpers/account.js b/test/helpers/account.js index 8c0ea130b..96874b16b 100644 --- a/test/helpers/account.js +++ b/test/helpers/account.js @@ -4,10 +4,10 @@ const { impersonateAccount, setBalance } = require('@nomicfoundation/hardhat-net // Hardhat default balance const DEFAULT_BALANCE = 10000n * ethers.WeiPerEther; -async function impersonate(account, balance = DEFAULT_BALANCE) { - await impersonateAccount(account); - await setBalance(account, balance); -} +const impersonate = (account, balance = DEFAULT_BALANCE) => + impersonateAccount(account) + .then(() => setBalance(account, balance)) + .then(() => ethers.getSigner(account)); module.exports = { impersonate, diff --git a/test/helpers/math.js b/test/helpers/math.js index 2bc654c51..134f8b045 100644 --- a/test/helpers/math.js +++ b/test/helpers/math.js @@ -1,6 +1,7 @@ module.exports = { // sum of integer / bignumber sum: (...args) => args.reduce((acc, n) => acc + n, 0), + bigintSum: (...args) => args.reduce((acc, n) => acc + n, 0n), BNsum: (...args) => args.reduce((acc, n) => acc.add(n), web3.utils.toBN(0)), // min of integer / bignumber min: (...args) => args.slice(1).reduce((x, y) => (x < y ? x : y), args[0]), diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index b0ebccca8..bb6718ce2 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -1,134 +1,117 @@ -const ethSigUtil = require('eth-sig-util'); -const Wallet = require('ethereumjs-wallet').default; -const { getDomain, domainType } = require('../helpers/eip712'); -const { MAX_UINT48 } = require('../helpers/constants'); - -const { expectEvent } = require('@openzeppelin/test-helpers'); +const { ethers } = require('hardhat'); const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const ERC2771ContextMock = artifacts.require('ERC2771ContextMock'); -const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); -const ContextMockCaller = artifacts.require('ContextMockCaller'); +const { impersonate } = require('../helpers/account'); +const { getDomain } = require('../helpers/eip712'); +const { MAX_UINT48 } = require('../helpers/constants'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); -contract('ERC2771Context', function (accounts) { - const [, trustedForwarder] = accounts; +async function fixture() { + const [sender] = await ethers.getSigners(); + const forwarder = await ethers.deployContract('ERC2771Forwarder', []); + const forwarderAsSigner = await impersonate(forwarder.target); + const context = await ethers.deployContract('ERC2771ContextMock', [forwarder]); + const domain = await getDomain(forwarder); + const types = { + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, + { name: 'data', type: 'bytes' }, + ], + }; + + return { sender, forwarder, forwarderAsSigner, context, domain, types }; +} + +describe('ERC2771Context', function () { beforeEach(async function () { - this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); - this.recipient = await ERC2771ContextMock.new(this.forwarder.address); - - this.domain = await getDomain(this.forwarder); - this.types = { - EIP712Domain: domainType(this.domain), - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'deadline', type: 'uint48' }, - { name: 'data', type: 'bytes' }, - ], - }; + Object.assign(this, await loadFixture(fixture)); }); it('recognize trusted forwarder', async function () { - expect(await this.recipient.isTrustedForwarder(this.forwarder.address)).to.equal(true); + expect(await this.context.isTrustedForwarder(this.forwarder)).to.equal(true); }); it('returns the trusted forwarder', async function () { - expect(await this.recipient.trustedForwarder()).to.equal(this.forwarder.address); + expect(await this.context.trustedForwarder()).to.equal(this.forwarder.target); }); - context('when called directly', function () { - beforeEach(async function () { - this.context = this.recipient; // The Context behavior expects the contract in this.context - this.caller = await ContextMockCaller.new(); - }); - - shouldBehaveLikeRegularContext(...accounts); + describe('when called directly', function () { + shouldBehaveLikeRegularContext(); }); - context('when receiving a relayed call', function () { - beforeEach(async function () { - this.wallet = Wallet.generate(); - this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.data = { - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - }; - }); - + describe('when receiving a relayed call', function () { describe('msgSender', function () { it('returns the relayed transaction original sender', async function () { - const data = this.recipient.contract.methods.msgSender().encodeABI(); + const nonce = await this.forwarder.nonces(this.sender); + const data = this.context.interface.encodeFunctionData('msgSender'); const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.nonces(this.sender)).toString(), - deadline: MAX_UINT48, + from: await this.sender.getAddress(), + to: await this.context.getAddress(), + value: 0n, data, + gas: 100000n, + nonce, + deadline: MAX_UINT48, }; - req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: { ...this.data, message: req }, - }); + req.signature = await this.sender.signTypedData(this.domain, this.types, req); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req); - await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); + await expect(this.forwarder.execute(req)).to.emit(this.context, 'Sender').withArgs(this.sender.address); }); it('returns the original sender when calldata length is less than 20 bytes (address length)', async function () { - // The forwarder doesn't produce calls with calldata length less than 20 bytes - const recipient = await ERC2771ContextMock.new(trustedForwarder); - - const { receipt } = await recipient.msgSender({ from: trustedForwarder }); - - await expectEvent(receipt, 'Sender', { sender: trustedForwarder }); + // The forwarder doesn't produce calls with calldata length less than 20 bytes so `this.forwarderAsSigner` is used instead. + await expect(this.context.connect(this.forwarderAsSigner).msgSender()) + .to.emit(this.context, 'Sender') + .withArgs(this.forwarder.target); }); }); describe('msgData', function () { it('returns the relayed transaction original data', async function () { - const integerValue = '42'; - const stringValue = 'OpenZeppelin'; - const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); + const args = [42n, 'OpenZeppelin']; + + const nonce = await this.forwarder.nonces(this.sender); + const data = this.context.interface.encodeFunctionData('msgData', args); const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.nonces(this.sender)).toString(), - deadline: MAX_UINT48, + from: await this.sender.getAddress(), + to: await this.context.getAddress(), + value: 0n, data, + gas: 100000n, + nonce, + deadline: MAX_UINT48, }; - req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: { ...this.data, message: req }, - }); + req.signature = this.sender.signTypedData(this.domain, this.types, req); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req); - await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); + await expect(this.forwarder.execute(req)) + .to.emit(this.context, 'Data') + .withArgs(data, ...args); }); }); it('returns the full original data when calldata length is less than 20 bytes (address length)', async function () { - // The forwarder doesn't produce calls with calldata length less than 20 bytes - const recipient = await ERC2771ContextMock.new(trustedForwarder); + const data = this.context.interface.encodeFunctionData('msgDataShort'); - const { receipt } = await recipient.msgDataShort({ from: trustedForwarder }); - - const data = recipient.contract.methods.msgDataShort().encodeABI(); - await expectEvent(receipt, 'DataShort', { data }); + // The forwarder doesn't produce calls with calldata length less than 20 bytes so `this.forwarderAsSigner` is used instead. + await expect(await this.context.connect(this.forwarderAsSigner).msgDataShort()) + .to.emit(this.context, 'DataShort') + .withArgs(data); }); }); }); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 0e0998832..a665471f3 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -1,245 +1,217 @@ -const ethSigUtil = require('eth-sig-util'); -const Wallet = require('ethereumjs-wallet').default; -const { getDomain, domainType } = require('../helpers/eip712'); -const { expectRevertCustomError } = require('../helpers/customError'); - -const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); +const { ethers } = require('hardhat'); const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); -const CallReceiverMockTrustingForwarder = artifacts.require('CallReceiverMockTrustingForwarder'); +const { getDomain } = require('../helpers/eip712'); +const { bigint: time } = require('../helpers/time'); +const { bigintSum: sum } = require('../helpers/math'); -contract('ERC2771Forwarder', function (accounts) { - const [, refundReceiver, another] = accounts; +async function fixture() { + const [sender, refundReceiver, another, ...accounts] = await ethers.getSigners(); - const tamperedValues = { - from: another, - value: web3.utils.toWei('0.5'), - data: '0x1742', + const forwarder = await ethers.deployContract('ERC2771Forwarder', ['ERC2771Forwarder']); + const receiver = await ethers.deployContract('CallReceiverMockTrustingForwarder', [forwarder]); + const domain = await getDomain(forwarder); + const types = { + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, + { name: 'data', type: 'bytes' }, + ], }; - beforeEach(async function () { - this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); - - this.domain = await getDomain(this.forwarder); - this.types = { - EIP712Domain: domainType(this.domain), - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'deadline', type: 'uint48' }, - { name: 'data', type: 'bytes' }, - ], + const forgeRequest = async (override = {}, signer = sender) => { + const req = { + from: await signer.getAddress(), + to: await receiver.getAddress(), + value: 0n, + data: receiver.interface.encodeFunctionData('mockFunction'), + gas: 100000n, + deadline: (await time.clock.timestamp()) + 60n, + nonce: await forwarder.nonces(sender), + ...override, }; + req.signature = await signer.signTypedData(domain, types, req); + return req; + }; - this.alice = Wallet.generate(); - this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); - - this.timestamp = await time.latest(); - this.receiver = await CallReceiverMockTrustingForwarder.new(this.forwarder.address); - this.request = { - from: this.alice.address, - to: this.receiver.address, - value: '0', - gas: '100000', - data: this.receiver.contract.methods.mockFunction().encodeABI(), - deadline: this.timestamp.toNumber() + 60, // 1 minute - }; - this.requestData = { - ...this.request, - nonce: (await this.forwarder.nonces(this.alice.address)).toString(), - }; - - this.forgeData = request => ({ - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - message: { ...this.requestData, ...request }, + const estimateRequest = request => + ethers.provider.estimateGas({ + from: forwarder, + to: request.to, + data: ethers.solidityPacked(['bytes', 'address'], [request.data, request.from]), + value: request.value, + gasLimit: request.gas, }); - this.sign = (privateKey, request) => - ethSigUtil.signTypedMessage(privateKey, { - data: this.forgeData(request), - }); - this.estimateRequest = request => - web3.eth.estimateGas({ - from: this.forwarder.address, - to: request.to, - data: web3.utils.encodePacked({ value: request.data, type: 'bytes' }, { value: request.from, type: 'address' }), - value: request.value, - gas: request.gas, - }); - this.requestData.signature = this.sign(this.alice.getPrivateKey()); + return { + sender, + refundReceiver, + another, + accounts, + forwarder, + receiver, + forgeRequest, + estimateRequest, + domain, + types, + }; +} + +// values or function to tamper with a signed request. +const tamperedValues = { + from: ethers.Wallet.createRandom().address, + to: ethers.Wallet.createRandom().address, + value: ethers.parseEther('0.5'), + data: '0x1742', + signature: s => { + const t = ethers.toBeArray(s); + t[42] ^= 0xff; + return ethers.hexlify(t); + }, +}; + +describe('ERC2771Forwarder', function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); }); - context('verify', function () { - context('with valid signature', function () { + describe('verify', function () { + describe('with valid signature', function () { it('returns true without altering the nonce', async function () { - expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( - web3.utils.toBN(this.requestData.nonce), - ); - expect(await this.forwarder.verify(this.requestData)).to.be.equal(true); - expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( - web3.utils.toBN(this.requestData.nonce), - ); + const request = await this.forgeRequest(); + expect(await this.forwarder.nonces(request.from)).to.be.equal(request.nonce); + expect(await this.forwarder.verify(request)).to.be.equal(true); + expect(await this.forwarder.nonces(request.from)).to.be.equal(request.nonce); }); }); - context('with tampered values', function () { + describe('with tampered values', function () { for (const [key, value] of Object.entries(tamperedValues)) { it(`returns false with tampered ${key}`, async function () { - expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message)).to.be.equal(false); + const request = await this.forgeRequest(); + request[key] = typeof value == 'function' ? value(request[key]) : value; + + expect(await this.forwarder.verify(request)).to.be.equal(false); }); } - it('returns false with an untrustful to', async function () { - expect(await this.forwarder.verify(this.forgeData({ to: another }).message)).to.be.equal(false); - }); - - it('returns false with tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.requestData.signature); - tamperedsign[42] ^= 0xff; - this.requestData.signature = web3.utils.bytesToHex(tamperedsign); - expect(await this.forwarder.verify(this.requestData)).to.be.equal(false); - }); - it('returns false with valid signature for non-current nonce', async function () { - const req = { - ...this.requestData, - nonce: this.requestData.nonce + 1, - }; - req.signature = this.sign(this.alice.getPrivateKey(), req); - expect(await this.forwarder.verify(req)).to.be.equal(false); + const request = await this.forgeRequest({ nonce: 1337n }); + expect(await this.forwarder.verify(request)).to.be.equal(false); }); it('returns false with valid signature for expired deadline', async function () { - const req = { - ...this.requestData, - deadline: this.timestamp - 1, - }; - req.signature = this.sign(this.alice.getPrivateKey(), req); - expect(await this.forwarder.verify(req)).to.be.equal(false); + const request = await this.forgeRequest({ deadline: (await time.clock.timestamp()) - 1n }); + expect(await this.forwarder.verify(request)).to.be.equal(false); }); }); }); - context('execute', function () { - context('with valid requests', function () { - beforeEach(async function () { - expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( - web3.utils.toBN(this.requestData.nonce), - ); - }); - + describe('execute', function () { + describe('with valid requests', function () { it('emits an event and consumes nonce for a successful request', async function () { - const receipt = await this.forwarder.execute(this.requestData); - await expectEvent.inTransaction(receipt.tx, this.receiver, 'MockFunctionCalled'); - await expectEvent.inTransaction(receipt.tx, this.forwarder, 'ExecutedForwardRequest', { - signer: this.requestData.from, - nonce: web3.utils.toBN(this.requestData.nonce), - success: true, - }); - expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( - web3.utils.toBN(this.requestData.nonce + 1), - ); + const request = await this.forgeRequest(); + + expect(await this.forwarder.nonces(request.from)).to.equal(request.nonce); + + await expect(this.forwarder.execute(request)) + .to.emit(this.receiver, 'MockFunctionCalled') + .to.emit(this.forwarder, 'ExecutedForwardRequest') + .withArgs(request.from, request.nonce, true); + + expect(await this.forwarder.nonces(request.from)).to.be.equal(request.nonce + 1n); }); it('reverts with an unsuccessful request', async function () { - const req = { - ...this.requestData, - data: this.receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), - }; - req.signature = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []); + const request = await this.forgeRequest({ + data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason'), + }); + + await expect(this.forwarder.execute(request)).to.be.revertedWithCustomError(this.forwarder, 'FailedInnerCall'); }); }); - context('with tampered request', function () { + describe('with tampered request', function () { for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with tampered ${key}`, async function () { - const data = this.forgeData({ [key]: value }); - await expectRevertCustomError( - this.forwarder.execute(data.message, { - value: key == 'value' ? value : 0, // To avoid MismatchedValue error - }), - 'ERC2771ForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data, sig: this.requestData.signature }), data.message.from], - ); + const request = await this.forgeRequest(); + request[key] = typeof value == 'function' ? value(request[key]) : value; + + const promise = this.forwarder.execute(request, { value: key == 'value' ? value : 0 }); + if (key != 'to') { + await expect(promise) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner') + .withArgs(ethers.verifyTypedData(this.domain, this.types, request, request.signature), request.from); + } else { + await expect(promise) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771UntrustfulTarget') + .withArgs(request.to, this.forwarder.target); + } }); } - it('reverts with an untrustful to', async function () { - const data = this.forgeData({ to: another }); - await expectRevertCustomError(this.forwarder.execute(data.message), 'ERC2771UntrustfulTarget', [ - data.message.to, - this.forwarder.address, - ]); - }); - - it('reverts with tampered signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.requestData.signature); - tamperedSig[42] ^= 0xff; - this.requestData.signature = web3.utils.bytesToHex(tamperedSig); - await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ - ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), - this.requestData.from, - ]); - }); - it('reverts with valid signature for non-current nonce', async function () { - // Execute first a request - await this.forwarder.execute(this.requestData); + const request = await this.forgeRequest(); - // And then fail due to an already used nonce - await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ - ethSigUtil.recoverTypedSignature({ - data: this.forgeData({ ...this.requestData, nonce: this.requestData.nonce + 1 }), - sig: this.requestData.signature, - }), - this.requestData.from, - ]); + // consume nonce + await this.forwarder.execute(request); + + // nonce has changed + await expect(this.forwarder.execute(request)) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner') + .withArgs( + ethers.verifyTypedData( + this.domain, + this.types, + { ...request, nonce: request.nonce + 1n }, + request.signature, + ), + request.from, + ); }); it('reverts with valid signature for expired deadline', async function () { - const req = { - ...this.requestData, - deadline: this.timestamp - 1, - }; - req.signature = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [ - this.timestamp - 1, - ]); + const request = await this.forgeRequest({ deadline: (await time.clock.timestamp()) - 1n }); + + await expect(this.forwarder.execute(request)) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderExpiredRequest') + .withArgs(request.deadline); }); it('reverts with valid signature but mismatched value', async function () { - const value = 100; - const req = { - ...this.requestData, - value, - }; - req.signature = this.sign(this.alice.getPrivateKey(), req); - await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderMismatchedValue', [0, value]); + const request = await this.forgeRequest({ value: 100n }); + + await expect(this.forwarder.execute(request)) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderMismatchedValue') + .withArgs(request.value, 0n); }); }); it('bubbles out of gas', async function () { - this.requestData.data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.requestData.gas = 1_000_000; - this.requestData.signature = this.sign(this.alice.getPrivateKey()); + const request = await this.forgeRequest({ + data: this.receiver.interface.encodeFunctionData('mockFunctionOutOfGas'), + gas: 1_000_000n, + }); - const gasAvailable = 100_000; - await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable })); + const gasLimit = 100_000n; + await expect(this.forwarder.execute(request, { gasLimit })).to.be.revertedWithoutReason(); - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + const { gasUsed } = await ethers.provider + .getBlock('latest') + .then(block => block.getTransaction(0)) + .then(tx => ethers.provider.getTransactionReceipt(tx.hash)); - expect(gasUsed).to.be.equal(gasAvailable); + expect(gasUsed).to.be.equal(gasLimit); }); it('bubbles out of gas forced by the relayer', async function () { + const request = await this.forgeRequest(); + // If there's an incentive behind executing requests, a malicious relayer could grief // the forwarder by executing requests and providing a top-level call gas limit that // is too low to successfully finish the request after the 63/64 rule. @@ -247,294 +219,252 @@ contract('ERC2771Forwarder', function (accounts) { // We set the baseline to the gas limit consumed by a successful request if it was executed // normally. Note this includes the 21000 buffer that also the relayer will be charged to // start a request execution. - const estimate = await this.estimateRequest(this.request); + const estimate = await this.estimateRequest(request); // Because the relayer call consumes gas until the `CALL` opcode, the gas left after failing // the subcall won't enough to finish the top level call (after testing), so we add a // moderated buffer. - const gasAvailable = estimate + 2_000; + const gasLimit = estimate + 2_000n; // The subcall out of gas should be caught by the contract and then bubbled up consuming // the available gas with an `invalid` opcode. - await expectRevert.outOfGas(this.forwarder.execute(this.requestData, { gas: gasAvailable })); + await expect(this.forwarder.execute(request, { gasLimit })).to.be.revertedWithoutReason(); - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + const { gasUsed } = await ethers.provider + .getBlock('latest') + .then(block => block.getTransaction(0)) + .then(tx => ethers.provider.getTransactionReceipt(tx.hash)); // We assert that indeed the gas was totally consumed. - expect(gasUsed).to.be.equal(gasAvailable); + expect(gasUsed).to.be.equal(gasLimit); }); }); - context('executeBatch', function () { - const batchValue = requestDatas => requestDatas.reduce((value, request) => value + Number(request.value), 0); + describe('executeBatch', function () { + const requestsValue = requests => sum(...requests.map(request => request.value)); + const requestCount = 3; + const idx = 1; // index that will be tampered with beforeEach(async function () { - this.bob = Wallet.generate(); - this.bob.address = web3.utils.toChecksumAddress(this.bob.getAddressString()); - - this.eve = Wallet.generate(); - this.eve.address = web3.utils.toChecksumAddress(this.eve.getAddressString()); - - this.signers = [this.alice, this.bob, this.eve]; - - this.requestDatas = await Promise.all( - this.signers.map(async ({ address }) => ({ - ...this.requestData, - from: address, - nonce: (await this.forwarder.nonces(address)).toString(), - value: web3.utils.toWei('10', 'gwei'), - })), - ); - - this.requestDatas = this.requestDatas.map((requestData, i) => ({ - ...requestData, - signature: this.sign(this.signers[i].getPrivateKey(), requestData), - })); - - this.msgValue = batchValue(this.requestDatas); - - this.gasUntil = async reqIdx => { - const gas = 0; - const estimations = await Promise.all( - new Array(reqIdx + 1).fill().map((_, idx) => this.estimateRequest(this.requestDatas[idx])), - ); - return estimations.reduce((acc, estimation) => acc + estimation, gas); - }; + this.forgeRequests = override => + Promise.all(this.accounts.slice(0, requestCount).map(signer => this.forgeRequest(override, signer))); + this.requests = await this.forgeRequests({ value: 10n }); + this.value = requestsValue(this.requests); }); - context('with valid requests', function () { - beforeEach(async function () { - for (const request of this.requestDatas) { + describe('with valid requests', function () { + it('sanity', async function () { + for (const request of this.requests) { expect(await this.forwarder.verify(request)).to.be.equal(true); } - - this.receipt = await this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }); }); it('emits events', async function () { - for (const request of this.requestDatas) { - await expectEvent.inTransaction(this.receipt.tx, this.receiver, 'MockFunctionCalled'); - await expectEvent.inTransaction(this.receipt.tx, this.forwarder, 'ExecutedForwardRequest', { - signer: request.from, - nonce: web3.utils.toBN(request.nonce), - success: true, - }); + const receipt = this.forwarder.executeBatch(this.requests, this.another, { value: this.value }); + + for (const request of this.requests) { + await expect(receipt) + .to.emit(this.receiver, 'MockFunctionCalled') + .to.emit(this.forwarder, 'ExecutedForwardRequest') + .withArgs(request.from, request.nonce, true); } }); it('increase nonces', async function () { - for (const request of this.requestDatas) { - expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1)); + await this.forwarder.executeBatch(this.requests, this.another, { value: this.value }); + + for (const request of this.requests) { + expect(await this.forwarder.nonces(request.from)).to.be.equal(request.nonce + 1n); } }); }); - context('with tampered requests', function () { - beforeEach(async function () { - this.idx = 1; // Tampered idx - }); - + describe('with tampered requests', function () { it('reverts with mismatched value', async function () { - this.requestDatas[this.idx].value = 100; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }), - 'ERC2771ForwarderMismatchedValue', - [batchValue(this.requestDatas), this.msgValue], - ); + // tamper value of one of the request + resign + this.requests[idx] = await this.forgeRequest({ value: 100n }, this.accounts[1]); + + await expect(this.forwarder.executeBatch(this.requests, this.another, { value: this.value })) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderMismatchedValue') + .withArgs(requestsValue(this.requests), this.value); }); - context('when the refund receiver is the zero address', function () { + describe('when the refund receiver is the zero address', function () { beforeEach(function () { - this.refundReceiver = constants.ZERO_ADDRESS; + this.refundReceiver = ethers.ZeroAddress; }); for (const [key, value] of Object.entries(tamperedValues)) { it(`reverts with at least one tampered request ${key}`, async function () { - const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + this.requests[idx][key] = typeof value == 'function' ? value(this.requests[idx][key]) : value; - this.requestDatas[this.idx] = data.message; - - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), - 'ERC2771ForwarderInvalidSigner', - [ - ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), - data.message.from, - ], - ); + const promise = this.forwarder.executeBatch(this.requests, this.refundReceiver, { value: this.value }); + if (key != 'to') { + await expect(promise) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner') + .withArgs( + ethers.verifyTypedData(this.domain, this.types, this.requests[idx], this.requests[idx].signature), + this.requests[idx].from, + ); + } else { + await expect(promise) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771UntrustfulTarget') + .withArgs(this.requests[idx].to, this.forwarder.target); + } }); } - it('reverts with at least one untrustful to', async function () { - const data = this.forgeData({ ...this.requestDatas[this.idx], to: another }); - - this.requestDatas[this.idx] = data.message; - - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), - 'ERC2771UntrustfulTarget', - [this.requestDatas[this.idx].to, this.forwarder.address], - ); - }); - - it('reverts with at least one tampered request signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); - tamperedSig[42] ^= 0xff; - - this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); - - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), - 'ERC2771ForwarderInvalidSigner', - [ - ethSigUtil.recoverTypedSignature({ - data: this.forgeData(this.requestDatas[this.idx]), - sig: this.requestDatas[this.idx].signature, - }), - this.requestDatas[this.idx].from, - ], - ); - }); - it('reverts with at least one valid signature for non-current nonce', async function () { // Execute first a request - await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + await this.forwarder.execute(this.requests[idx], { value: this.requests[idx].value }); // And then fail due to an already used nonce - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), - 'ERC2771ForwarderInvalidSigner', - [ - ethSigUtil.recoverTypedSignature({ - data: this.forgeData({ ...this.requestDatas[this.idx], nonce: this.requestDatas[this.idx].nonce + 1 }), - sig: this.requestDatas[this.idx].signature, - }), - this.requestDatas[this.idx].from, - ], - ); + await expect(this.forwarder.executeBatch(this.requests, this.refundReceiver, { value: this.value })) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderInvalidSigner') + .withArgs( + ethers.verifyTypedData( + this.domain, + this.types, + { ...this.requests[idx], nonce: this.requests[idx].nonce + 1n }, + this.requests[idx].signature, + ), + this.requests[idx].from, + ); }); it('reverts with at least one valid signature for expired deadline', async function () { - this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); - await expectRevertCustomError( - this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), - 'ERC2771ForwarderExpiredRequest', - [this.timestamp.toNumber() - 1], + this.requests[idx] = await this.forgeRequest( + { ...this.requests[idx], deadline: (await time.clock.timestamp()) - 1n }, + this.accounts[1], ); + + await expect(this.forwarder.executeBatch(this.requests, this.refundReceiver, { value: this.amount })) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderExpiredRequest') + .withArgs(this.requests[idx].deadline); }); }); - context('when the refund receiver is a known address', function () { + describe('when the refund receiver is a known address', function () { beforeEach(async function () { - this.refundReceiver = refundReceiver; - this.initialRefundReceiverBalance = web3.utils.toBN(await web3.eth.getBalance(this.refundReceiver)); - this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from); + this.initialRefundReceiverBalance = await ethers.provider.getBalance(this.refundReceiver); + this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requests[idx].from); }); for (const [key, value] of Object.entries(tamperedValues)) { it(`ignores a request with tampered ${key} and refunds its value`, async function () { - const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + this.requests[idx][key] = typeof value == 'function' ? value(this.requests[idx][key]) : value; - this.requestDatas[this.idx] = data.message; + const events = await this.forwarder + .executeBatch(this.requests, this.refundReceiver, { value: requestsValue(this.requests) }) + .then(tx => tx.wait()) + .then(receipt => + receipt.logs.filter( + log => log?.fragment?.type == 'event' && log?.fragment?.name == 'ExecutedForwardRequest', + ), + ); - const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { - value: batchValue(this.requestDatas), - }); - expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + expect(events).to.have.lengthOf(this.requests.length - 1); }); } it('ignores a request with a valid signature for non-current nonce', async function () { // Execute first a request - await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + await this.forwarder.execute(this.requests[idx], { value: this.requests[idx].value }); this.initialTamperedRequestNonce++; // Should be already incremented by the individual `execute` // And then ignore the same request in a batch due to an already used nonce - const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { - value: this.msgValue, - }); - expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + const events = await this.forwarder + .executeBatch(this.requests, this.refundReceiver, { value: this.value }) + .then(tx => tx.wait()) + .then(receipt => + receipt.logs.filter( + log => log?.fragment?.type == 'event' && log?.fragment?.name == 'ExecutedForwardRequest', + ), + ); + + expect(events).to.have.lengthOf(this.requests.length - 1); }); it('ignores a request with a valid signature for expired deadline', async function () { - this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], + this.requests[idx] = await this.forgeRequest( + { ...this.requests[idx], deadline: (await time.clock.timestamp()) - 1n }, + this.accounts[1], ); - const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { - value: this.msgValue, - }); - expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + const events = await this.forwarder + .executeBatch(this.requests, this.refundReceiver, { value: this.value }) + .then(tx => tx.wait()) + .then(receipt => + receipt.logs.filter( + log => log?.fragment?.type == 'event' && log?.fragment?.name == 'ExecutedForwardRequest', + ), + ); + + expect(events).to.have.lengthOf(this.requests.length - 1); }); afterEach(async function () { // The invalid request value was refunded - expect(await web3.eth.getBalance(this.refundReceiver)).to.be.bignumber.equal( - this.initialRefundReceiverBalance.add(web3.utils.toBN(this.requestDatas[this.idx].value)), + expect(await ethers.provider.getBalance(this.refundReceiver)).to.be.equal( + this.initialRefundReceiverBalance + this.requests[idx].value, ); // The invalid request from's nonce was not incremented - expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq( - web3.utils.toBN(this.initialTamperedRequestNonce), - ); + expect(await this.forwarder.nonces(this.requests[idx].from)).to.be.equal(this.initialTamperedRequestNonce); }); }); it('bubbles out of gas', async function () { - this.requestDatas[this.idx].data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.requestDatas[this.idx].gas = 1_000_000; - this.requestDatas[this.idx].signature = this.sign( - this.signers[this.idx].getPrivateKey(), - this.requestDatas[this.idx], - ); + this.requests[idx] = await this.forgeRequest({ + data: this.receiver.interface.encodeFunctionData('mockFunctionOutOfGas'), + gas: 1_000_000n, + }); - const gasAvailable = 300_000; - await expectRevert.assertion( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { - gas: gasAvailable, - value: this.requestDatas.reduce((acc, { value }) => acc + Number(value), 0), + const gasLimit = 300_000n; + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { + gasLimit, + value: requestsValue(this.requests), }), - ); + ).to.be.revertedWithoutReason(); - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + const { gasUsed } = await ethers.provider + .getBlock('latest') + .then(block => block.getTransaction(0)) + .then(tx => ethers.provider.getTransactionReceipt(tx.hash)); - expect(gasUsed).to.be.equal(gasAvailable); + expect(gasUsed).to.be.equal(gasLimit); }); it('bubbles out of gas forced by the relayer', async function () { // Similarly to the single execute, a malicious relayer could grief requests. // We estimate until the selected request as if they were executed normally - const estimate = await this.gasUntil(this.requestDatas, this.idx); + const estimate = await Promise.all(this.requests.slice(0, idx + 1).map(this.estimateRequest)).then(gas => + sum(...gas), + ); // We add a Buffer to account for all the gas that's used before the selected call. // Note is slightly bigger because the selected request is not the index 0 and it affects // the buffer needed. - const gasAvailable = estimate + 10_000; + const gasLimit = estimate + 10_000n; // The subcall out of gas should be caught by the contract and then bubbled up consuming // the available gas with an `invalid` opcode. - await expectRevert.outOfGas( - this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }), - ); + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { + gasLimit, + value: requestsValue(this.requests), + }), + ).to.be.revertedWithoutReason(); - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + const { gasUsed } = await ethers.provider + .getBlock('latest') + .then(block => block.getTransaction(0)) + .then(tx => ethers.provider.getTransactionReceipt(tx.hash)); // We assert that indeed the gas was totally consumed. - expect(gasUsed).to.be.equal(gasAvailable); + expect(gasUsed).to.be.equal(gasLimit); }); }); }); diff --git a/test/utils/Context.behavior.js b/test/utils/Context.behavior.js index 08f7558d7..9e986db7a 100644 --- a/test/utils/Context.behavior.js +++ b/test/utils/Context.behavior.js @@ -1,38 +1,46 @@ -const { BN, expectEvent } = require('@openzeppelin/test-helpers'); +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); -const ContextMock = artifacts.require('ContextMock'); +async function fixture() { + return { contextHelper: await ethers.deployContract('ContextMockCaller', []) }; +} +function shouldBehaveLikeRegularContext() { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); -function shouldBehaveLikeRegularContext(sender) { describe('msgSender', function () { it('returns the transaction sender when called from an EOA', async function () { - const receipt = await this.context.msgSender({ from: sender }); - expectEvent(receipt, 'Sender', { sender }); + await expect(this.context.connect(this.sender).msgSender()) + .to.emit(this.context, 'Sender') + .withArgs(this.sender.address); }); - it('returns the transaction sender when from another contract', async function () { - const { tx } = await this.caller.callSender(this.context.address, { from: sender }); - await expectEvent.inTransaction(tx, ContextMock, 'Sender', { sender: this.caller.address }); + it('returns the transaction sender when called from another contract', async function () { + await expect(this.contextHelper.connect(this.sender).callSender(this.context)) + .to.emit(this.context, 'Sender') + .withArgs(this.contextHelper.target); }); }); describe('msgData', function () { - const integerValue = new BN('42'); - const stringValue = 'OpenZeppelin'; - - let callData; - - beforeEach(async function () { - callData = this.context.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI(); - }); + const args = [42n, 'OpenZeppelin']; it('returns the transaction data when called from an EOA', async function () { - const receipt = await this.context.msgData(integerValue, stringValue); - expectEvent(receipt, 'Data', { data: callData, integerValue, stringValue }); + const callData = this.context.interface.encodeFunctionData('msgData', args); + + await expect(this.context.msgData(...args)) + .to.emit(this.context, 'Data') + .withArgs(callData, ...args); }); it('returns the transaction sender when from another contract', async function () { - const { tx } = await this.caller.callData(this.context.address, integerValue, stringValue); - await expectEvent.inTransaction(tx, ContextMock, 'Data', { data: callData, integerValue, stringValue }); + const callData = this.context.interface.encodeFunctionData('msgData', args); + + await expect(this.contextHelper.callData(this.context, ...args)) + .to.emit(this.context, 'Data') + .withArgs(callData, ...args); }); }); } diff --git a/test/utils/Context.test.js b/test/utils/Context.test.js index f372f7420..b766729e2 100644 --- a/test/utils/Context.test.js +++ b/test/utils/Context.test.js @@ -1,17 +1,18 @@ -require('@openzeppelin/test-helpers'); - -const ContextMock = artifacts.require('ContextMock'); -const ContextMockCaller = artifacts.require('ContextMockCaller'); +const { ethers } = require('hardhat'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { shouldBehaveLikeRegularContext } = require('./Context.behavior'); -contract('Context', function (accounts) { - const [sender] = accounts; +async function fixture() { + const [sender] = await ethers.getSigners(); + const context = await ethers.deployContract('ContextMock', []); + return { sender, context }; +} +describe('Context', function () { beforeEach(async function () { - this.context = await ContextMock.new(); - this.caller = await ContextMockCaller.new(); + Object.assign(this, await loadFixture(fixture)); }); - shouldBehaveLikeRegularContext(sender); + shouldBehaveLikeRegularContext(); });