From 28869eed28d06bbcecec7c41173118a5b5e2a871 Mon Sep 17 00:00:00 2001 From: Gonzalo Othacehe Date: Mon, 16 Mar 2026 16:14:38 -0300 Subject: [PATCH 1/5] ERC2271Forwarder executeBatch revert on atomic batch with reverting zero value requests --- .changeset/dull-games-fly.md | 5 ++++ contracts/metatx/ERC2771Forwarder.sol | 3 +-- test/metatx/ERC2771Forwarder.test.js | 38 +++++++++++++++++++-------- 3 files changed, 33 insertions(+), 13 deletions(-) create mode 100644 .changeset/dull-games-fly.md diff --git a/.changeset/dull-games-fly.md b/.changeset/dull-games-fly.md new file mode 100644 index 00000000000..50822ae50f6 --- /dev/null +++ b/.changeset/dull-games-fly.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +ERC2271Forwarder executeBatch revert on atomic batch with reverting zero value requests diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 755150ee0d4..f49d66ab907 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -178,6 +178,7 @@ contract ERC2771Forwarder is EIP712, Nonces { requestsValue += requests[i].value; bool success = _execute(requests[i], atomic); if (!success) { + if (atomic) revert ERC2771ForwarderFailureInAtomicBatch(); refundValue += requests[i].value; } } @@ -191,8 +192,6 @@ contract ERC2771Forwarder is EIP712, Nonces { // Some requests with value were invalid (possibly due to frontrunning). // To avoid leaving ETH in the contract this value is refunded. if (refundValue != 0) { - if (atomic) revert ERC2771ForwarderFailureInAtomicBatch(); - // We know refundReceiver != address(0) && requestsValue == msg.value // meaning we can ensure refundValue is not taken from the original contract's balance // and refundReceiver is a known account. diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index ec8d20b47a5..4a487f50b8e 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -228,18 +228,34 @@ describe('ERC2771Forwarder', function () { } }); - it('atomic batch with reverting request reverts the whole batch', async function () { - // Add extra reverting request - await this.forgeRequest( - { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, - this.accounts[requestCount], - ).then(extraRequest => this.requests.push(extraRequest)); - // recompute total value with the extra request - this.value = requestsValue(this.requests); + describe('atomic batch with reverting request', function () { + it('positive value request reverting reverts the whole batch', async function () { + // Add extra reverting request + await this.forgeRequest( + { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, + this.accounts[requestCount], + ).then(extraRequest => this.requests.push(extraRequest)); + // recompute total value with the extra request + this.value = requestsValue(this.requests); + + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), + ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); + }); - await expect( - this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), - ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); + it('zero value request reverting reverts the whole batch', async function () { + // Add extra reverting request + await this.forgeRequest( + { value: 0n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, + this.accounts[requestCount], + ).then(extraRequest => this.requests.push(extraRequest)); + // recompute total value with the extra request + this.value = requestsValue(this.requests); + + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), + ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); + }); }); }); From 379815103a8dbc3d217cbe352004aebdf028602b Mon Sep 17 00:00:00 2001 From: Gonzalo Othacehe <86085168+gonzaotc@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:25:47 -0300 Subject: [PATCH 2/5] Update dull-games-fly.md --- .changeset/dull-games-fly.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/dull-games-fly.md b/.changeset/dull-games-fly.md index 50822ae50f6..006005ceef2 100644 --- a/.changeset/dull-games-fly.md +++ b/.changeset/dull-games-fly.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': patch --- -ERC2271Forwarder executeBatch revert on atomic batch with reverting zero value requests +Make `ERC2771Forwarder` `executeBatch` atomic batches revert upon zero value requests reverts From eb31b007879f5b295086f469537c9eb2e7510bd5 Mon Sep 17 00:00:00 2001 From: Gonzalo Othacehe Date: Thu, 26 Mar 2026 15:20:06 -0300 Subject: [PATCH 3/5] ERC2771Forwarder: revert with ERC2771ForwarderNoRefundReceiver on execution failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When refundReceiver == address(0) and a valid request's forwarded call fails while carrying value, revert with ERC2771ForwarderNoRefundReceiver instead of the former ERC2771ForwarderFailureInAtomicBatch. Removes the misleading "atomic"/"all-or-nothing" semantics — the error now precisely describes the issue: leftover ETH has no receiver. Made-with: Cursor --- .changeset/dull-games-fly.md | 2 +- contracts/metatx/ERC2771Forwarder.sol | 25 ++++++++---------- test/metatx/ERC2771Forwarder.test.js | 37 ++++++++------------------- 3 files changed, 21 insertions(+), 43 deletions(-) diff --git a/.changeset/dull-games-fly.md b/.changeset/dull-games-fly.md index 50822ae50f6..e950b064d45 100644 --- a/.changeset/dull-games-fly.md +++ b/.changeset/dull-games-fly.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': patch --- -ERC2271Forwarder executeBatch revert on atomic batch with reverting zero value requests +ERC2771Forwarder: `executeBatch` with `refundReceiver == address(0)` now refunds failed-execution value to `msg.sender` instead of reverting, preventing ETH from being burned diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index f49d66ab907..f219dac0707 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -76,9 +76,9 @@ contract ERC2771Forwarder is EIP712, Nonces { event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); /** - * @dev One of the calls in an atomic batch failed. + * @dev A request in the batch failed and no `refundReceiver` was set to handle the leftover value. */ - error ERC2771ForwarderFailureInAtomicBatch(); + error ERC2771ForwarderNoRefundReceiver(); /** * @dev The request `from` doesn't match with the recovered `signer`. @@ -143,42 +143,35 @@ contract ERC2771Forwarder is EIP712, Nonces { } /** - * @dev Batch version of {execute} with optional refunding and atomic execution. + * @dev Batch version of {execute} with optional refunding. * * In case a batch contains at least one invalid request (see {verify}), the * request will be skipped and the `refundReceiver` parameter will receive back the * unused requested value at the end of the execution. This is done to prevent reverting * the entire batch when a request is invalid or has already been submitted. * - * If the `refundReceiver` is the `address(0)`, this function will revert when at least - * one of the requests was not valid instead of skipping it. This could be useful if - * a batch is required to get executed atomically (at least at the top-level). For example, - * refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids - * including reverted transactions. + * If the `refundReceiver` is `address(0)`, the function will instead revert + * when any request is invalid or when a valid request's forwarded call fails while + * carrying value (since there is no receiver to refund the leftover ETH to). * * Requirements: * * - The sum of the requests' values should be equal to the provided `msg.value`. * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address. - * - * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for - * the first-level forwarded calls. In case a forwarded request calls to a contract with another - * subcall, the second-level call may revert without the top-level call reverting. */ function executeBatch( ForwardRequestData[] calldata requests, address payable refundReceiver ) public payable virtual { - bool atomic = refundReceiver == address(0); + bool requireValidRequests = refundReceiver == address(0); uint256 requestsValue; uint256 refundValue; for (uint256 i; i < requests.length; ++i) { requestsValue += requests[i].value; - bool success = _execute(requests[i], atomic); + bool success = _execute(requests[i], requireValidRequests); if (!success) { - if (atomic) revert ERC2771ForwarderFailureInAtomicBatch(); refundValue += requests[i].value; } } @@ -192,6 +185,8 @@ contract ERC2771Forwarder is EIP712, Nonces { // Some requests with value were invalid (possibly due to frontrunning). // To avoid leaving ETH in the contract this value is refunded. if (refundValue != 0) { + if (requireValidRequests) revert ERC2771ForwarderNoRefundReceiver(); + // We know refundReceiver != address(0) && requestsValue == msg.value // meaning we can ensure refundValue is not taken from the original contract's balance // and refundReceiver is a known account. diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 4a487f50b8e..28b599a1466 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -228,34 +228,17 @@ describe('ERC2771Forwarder', function () { } }); - describe('atomic batch with reverting request', function () { - it('positive value request reverting reverts the whole batch', async function () { - // Add extra reverting request - await this.forgeRequest( - { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, - this.accounts[requestCount], - ).then(extraRequest => this.requests.push(extraRequest)); - // recompute total value with the extra request - this.value = requestsValue(this.requests); - - await expect( - this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), - ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); - }); + it('reverts when a valid request fails execution with value and no refund receiver is set', async function () { + // Add a request whose forwarded call will revert (valid signature, but reverts on execution) + await this.forgeRequest( + { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, + this.accounts[requestCount], + ).then(extraRequest => this.requests.push(extraRequest)); + this.value = requestsValue(this.requests); - it('zero value request reverting reverts the whole batch', async function () { - // Add extra reverting request - await this.forgeRequest( - { value: 0n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, - this.accounts[requestCount], - ).then(extraRequest => this.requests.push(extraRequest)); - // recompute total value with the extra request - this.value = requestsValue(this.requests); - - await expect( - this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), - ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); - }); + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), + ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderNoRefundReceiver'); }); }); From 2bb7090588063e58f53d026f2772dfc6adb2a8f7 Mon Sep 17 00:00:00 2001 From: Gonzalo Othacehe Date: Thu, 26 Mar 2026 15:52:37 -0300 Subject: [PATCH 4/5] up --- contracts/metatx/ERC2771Forwarder.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index f219dac0707..fd76ef02049 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -183,7 +183,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } // Some requests with value were invalid (possibly due to frontrunning). - // To avoid leaving ETH in the contract this value is refunded. + // To avoid leaving ETH in the contract, this value is refunded. if (refundValue != 0) { if (requireValidRequests) revert ERC2771ForwarderNoRefundReceiver(); From 8cd8d06f48c37797a6343c9d3ccbcb0d05e58666 Mon Sep 17 00:00:00 2001 From: Gonzalo Othacehe Date: Thu, 26 Mar 2026 16:17:58 -0300 Subject: [PATCH 5/5] up tests --- test/metatx/ERC2771Forwarder.test.js | 59 +++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 35e5d329446..12723aef5e5 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -228,17 +228,44 @@ describe('ERC2771Forwarder', function () { } }); - describe('with no refund receiver set (refundReceiver == address(0))', function () { - it('reverts when a failing request carries value (no receiver for the leftover ETH)', async function () { - await this.forgeRequest( - { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, - this.accounts[requestCount], - ).then(extraRequest => this.requests.push(extraRequest)); - this.value = requestsValue(this.requests); + it('refunds value to refund receiver when a valid request fails execution', async function () { + const initialRefundReceiverBalance = await ethers.provider.getBalance(this.refundReceiver); + const initialNonce = await this.forwarder.nonces(this.requests[idx].from); + + // Replace one request with a valid-but-reverting one (signature/nonce/deadline all fine) + this.requests[idx] = await this.forgeRequest( + { + value: this.requests[idx].value, + data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason'), + }, + this.accounts[idx], + ); - await expect( - this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), - ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderNoRefundReceiver'); + 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', + ), + ); + + // All requests emit the event — the reverting one has success == false + expect(events).to.have.lengthOf(this.requests.length); + expect(events[idx].args.success).to.be.false; + + // Unlike a tampered request, the nonce is consumed since the request was valid + expect(await this.forwarder.nonces(this.requests[idx].from)).to.equal(initialNonce + 1n); + + // The value is refunded to refundReceiver + expect(await ethers.provider.getBalance(this.refundReceiver)).to.equal( + initialRefundReceiverBalance + this.requests[idx].value, + ); + }); + + describe('when the refund receiver is the zero address', function () { + beforeEach(function () { + this.refundReceiver = ethers.ZeroAddress; }); it('does not revert when a failing request carries no value', async function () { @@ -256,6 +283,18 @@ describe('ERC2771Forwarder', function () { .to.emit(this.forwarder, 'ExecutedForwardRequest') .withArgs(this.requests.at(-1).from, this.requests.at(-1).nonce, false); }); + + it('reverts when a failing request carries value', async function () { + await this.forgeRequest( + { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, + this.accounts[requestCount], + ).then(extraRequest => this.requests.push(extraRequest)); + this.value = requestsValue(this.requests); + + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), + ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderNoRefundReceiver'); + }); }); });