diff --git a/.changeset/some-dolls-shine.md b/.changeset/some-dolls-shine.md new file mode 100644 index 00000000000..b57a9ef2017 --- /dev/null +++ b/.changeset/some-dolls-shine.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC2771Forwarder`: Revert the entire atomic batch if a call with value fails. diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index a99dc553950..755150ee0d4 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -75,6 +75,11 @@ contract ERC2771Forwarder is EIP712, Nonces { */ event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); + /** + * @dev One of the calls in an atomic batch failed. + */ + error ERC2771ForwarderFailureInAtomicBatch(); + /** * @dev The request `from` doesn't match with the recovered `signer`. */ @@ -186,6 +191,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 (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 1a8bf2cd2c8..ec8d20b47a5 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -227,6 +227,20 @@ describe('ERC2771Forwarder', function () { expect(await this.forwarder.nonces(request.from)).to.equal(request.nonce + 1n); } }); + + 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); + + await expect( + this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }), + ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch'); + }); }); describe('with tampered requests', function () {