H-04: Update ERC2771Forwarder executeBatch docs removing atomicity terminology#6415
H-04: Update ERC2771Forwarder executeBatch docs removing atomicity terminology#6415gonzaotc wants to merge 8 commits intoOpenZeppelin:masterfrom
ERC2771Forwarder executeBatch docs removing atomicity terminology#6415Conversation
…ero value requests
🦋 Changeset detectedLatest commit: e1bbf07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR introduces a changeset for openzeppelin-solidity and modifies the Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/metatx/ERC2771Forwarder.test.js (1)
231-259: Good test coverage for both zero-value and positive-value scenarios.The tests correctly verify that atomic batches revert with
ERC2771ForwarderFailureInAtomicBatchfor both value cases.Consider adding balance assertions to verify no ETH changes occurred after the revert, which helps ensure there are no unintended state changes. Based on learnings: "In tests that exercise revert semantics, use negative balance assertions to verify that no ether balance changes occurred."
,
💡 Optional: Add balance assertions
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'); + ) + .to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch') + .and.to.not.changeEtherBalances([this.forwarder, this.receiver], [0n, 0n]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metatx/ERC2771Forwarder.test.js` around lines 231 - 259, Add balance assertions around the two tests that add a reverting request (the cases using forgeRequest, requestsValue, and executeBatch on forwarder) to ensure no ETH moved when the batch reverts with ERC2771ForwarderFailureInAtomicBatch: capture balances before the executeBatch call for relevant parties (the caller account from accounts[requestCount], the forwarder and optionally the receiver), run the expect(...).to.be.revertedWithCustomError assertion as-is, then assert balances are equal to the captured values afterward (use BigInt/ethers utilities already used elsewhere in tests) so the tests confirm no ether balance changes on revert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/dull-games-fly.md:
- Line 5: Update the typo "ERC2271Forwarder" to the correct contract name
"ERC2771Forwarder" in the changeset text (the string inside
.changeset/dull-games-fly.md) so the published changelog references the correct
ERC-2771 implementation; search and replace any other occurrences of
"ERC2271Forwarder" in this file and ensure the surrounding phrasing remains
valid.
---
Nitpick comments:
In `@test/metatx/ERC2771Forwarder.test.js`:
- Around line 231-259: Add balance assertions around the two tests that add a
reverting request (the cases using forgeRequest, requestsValue, and executeBatch
on forwarder) to ensure no ETH moved when the batch reverts with
ERC2771ForwarderFailureInAtomicBatch: capture balances before the executeBatch
call for relevant parties (the caller account from accounts[requestCount], the
forwarder and optionally the receiver), run the
expect(...).to.be.revertedWithCustomError assertion as-is, then assert balances
are equal to the captured values afterward (use BigInt/ethers utilities already
used elsewhere in tests) so the tests confirm no ether balance changes on
revert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ad85766-9721-4dc2-aa76-ad71f2f39ca2
📒 Files selected for processing (3)
.changeset/dull-games-fly.mdcontracts/metatx/ERC2771Forwarder.soltest/metatx/ERC2771Forwarder.test.js
gonzaotc
changed the title
ERC2271Forwarder.executeBatch revert atomic batch on zero value reverting requestERC2771Forwarder executeBatch atomic batches revert upon zero value requests reverts
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
The original reason the refundReceiver == address(0) mode exists is to protect the relayer, not to provide execution atomicity guarantees to request signers. The scenario it defends against is:
- Alice, Bob, and Carl sign requests
- A relayer submits
executeBatchwith all three requests - Eve frontruns the relayer by calling
executewith Bob's request directly - The relayer's batch now lands with Bob's request invalid (nonce already consumed), which without the refund mechanism would cause the entire batch to revert (griefing the relayer out of gas costs and leaving Alice and Carl unserved)
In #6391 we missed the original intent too and reintroduced this vector. I think a better solution is to refund the msg.sender if refundReceiver == address(0) since that allows the batch to execute cleanly as the forwarder expects. Also, let's get rid of the atomic and all-or-nothing terminology.
There was a problem hiding this comment.
| * If the `refundReceiver` is `address(0)`, the function will instead revert | |
| * when any request is invalid, and any value corresponding to requests that could | |
| * not be executed will be refunded to `msg.sender`. This is useful for relayers | |
| * that want to ensure all requests they submitted are valid, while still being | |
| * protected against losing value from requests that fail at execution time. |
| // 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(); |
There was a problem hiding this comment.
We discussed internally and noticed that this line doesn't reintroduce the frontrunning vector. In that case, the _execute will return false because the nonce is already consumed. For that case, it would've burned the ETH instead of reverting.
Still, it would be possible that an attacker prepares a valid request that calls a contract the attacker controls, so the success of the call depends on the state of the contract. Therefore the attacker can still frontrun the forwarder and set the target contract's state to force a revert.
Bottom line is that the code is currently fine but:
- If a relayer wants to avoid reverts, they must provide a
refundReceiver - Otherwise, the must only process requests they trust
…cution failures 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
gonzaotc
changed the title
ERC2771Forwarder executeBatch atomic batches revert upon zero value requests revertsERC2771Forwarder executeBatch docs removing atomicity terminology
There was a problem hiding this comment.
I think we don't need a changeset because there's no code change
There was a problem hiding this comment.
The CHANGELOG must indicate the custom error rename in the "breaking changes" section
| * - 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 |
There was a problem hiding this comment.
We should still keep a note about setting a refundReceiver to address(0).
According to our discussion, developers should only do this when the operations use 0 value or when they're the operation won't revert. For example, if they use a private RPC (or their own node)
The reason is that, if the relayer is not aware, they might still get a whole batch invalidated if they send an operation with value that's submitted first. Currently it's not clearly described imo
| 'openzeppelin-solidity': patch | ||
| --- | ||
|
|
||
| Update `ERC2771Forwarder` `executeBatch` docs removing atomicity terminology |
There was a problem hiding this comment.
| Update `ERC2771Forwarder` `executeBatch` docs removing atomicity terminology | |
| [BREAKING] `ERC2771Forwarder`: custom error `ERC2771ForwarderFailureInAtomicBatch` has been remaned to `ERC2771ForwarderNoRefundReceiver` |
There was a problem hiding this comment.
Agree. Instead of a changeset, it should go in the ### Breaking Changes section of the CHANGELOG
3 participants
Fixes #????
PR Checklist
npx changeset add)