diff --git a/.changeset/dull-games-fly.md b/.changeset/dull-games-fly.md new file mode 100644 index 00000000000..1b287649cb2 --- /dev/null +++ b/.changeset/dull-games-fly.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +Update `ERC2771Forwarder` `executeBatch` docs removing atomicity terminology diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 755150ee0d4..fd76ef02049 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,40 +143,34 @@ 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) { refundValue += requests[i].value; } @@ -189,9 +183,9 @@ 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 (atomic) revert ERC2771ForwarderFailureInAtomicBatch(); + 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 diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index ec8d20b47a5..12723aef5e5 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -228,18 +228,73 @@ 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); + 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, 'ERC2771ForwarderFailureInAtomicBatch'); + 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 () { + // Zero-value revert: nothing to refund, batch can proceed + await this.forgeRequest( + { value: 0n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') }, + this.accounts[requestCount], + ).then(extraRequest => this.requests.push(extraRequest)); + this.value = requestsValue(this.requests); + + const receipt = this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }); + + // The reverting request emits success == false; the others still execute normally + await expect(receipt) + .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'); + }); }); });