AA-159 prevent recursive call into handleOps #257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
merge v0.5 to main
- NonceManager to handle 2d nonces - _validateAndUpdateNonce() called after validateUserOp - getNonce(sender, key) - remove nonce checking from BaseAccount - BaseAccount implements nonce() using ep.getNonce
create account doesn't increment nonce automatically. The nonce is incremented if it is created through a UserOp Side-effect: if account is created NOT through AA, then first UserOp will cost 20000 gas more. (if created as a userop, then this extra cost is part of the "creation" and not on a separate transaction.
nonce() is no longer "do-nothing" method. use instead account()
drortirosh
force-pushed
the
non-recursion
branch
from
4fbe233
to
a677a49
Compare
forshtat
reviewed
Apr 4, 2023
without this event, all events in validation are attributed to the first UserOperation. We can't attribute them to specific UseROp (unless there is exactly one) - but at least not always assign the to the first.
drortirosh
force-pushed
the
non-recursion
branch
from
703efa4
to
f2eb361
Compare
use ReentrancyGuard directly from OpenZeppelin, not a copy.
shahafn
approved these changes
Apr 9, 2023
ptescher
pushed a commit
to BitskiCo/account-abstraction
that referenced
this pull request
May 16, 2023
* AA-159 prevent recursive call into handleOps * added BeforeExecution event without this event, all events in validation are attributed to the first UserOperation. We can't attribute them to specific UseROp (unless there is exactly one) - but at least not always assign the to the first.
ptescher
pushed a commit
to BitskiCo/account-abstraction
that referenced
this pull request
Jun 14, 2023
* AA-159 prevent recursive call into handleOps * added BeforeExecution event without this event, all events in validation are attributed to the first UserOperation. We can't attribute them to specific UseROp (unless there is exactly one) - but at least not always assign the to the first.
kaiix
pushed a commit
to kaiix/account-abstraction
that referenced
this pull request
Nov 7, 2023
* AA-159 prevent recursive call into handleOps * added BeforeExecution event without this event, all events in validation are attributed to the first UserOperation. We can't attribute them to specific UseROp (unless there is exactly one) - but at least not always assign the to the first.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Problem:
Currently, it is possible to make recursive calls to handleOps() during UserOp execution.
In order to associate events with UserOps, we parse all events since the previous UserOperationEvent. This means that recursive handleOp will break parsing, potentially causing events to be associated with the wrong UserOp.
Note that there is no execution risk: everything is executed in order, as it should. It is only the EXTERNAL view on the events that gets split the wrong way.
Example:
Consider a “flashloan” contract that emits “BeforeLoan” and “AfterLoan” events.
A user submits a UserOp that calls the flashloan contract, with a transaction that recursively calls
handleOpswith another UserOp which emits a “Transfer” event.The events sent are:
When parsing the events, the transactions look like:
UserOp1: BeforeLoan, Transfer
UserOp2: AfterLoan
The above split is the way the bundlers (using getUserOperationReceipt) would currently return the events.
Anyone parsing the flashloan events, will get confused - a single atomic flashloan is broken over 2 UserOps.
Additional issues with recursion:
Solution:
One possible solution is to "enshrine" recursions, and provide mechanisms for bundlers and applications to detect them. However, this would put the burden of handling recursions, however rare, by each account and each paymaster implementation. In practice, accounts and paymasters might end up spending gas on a reentrancy guard in each UserOp.
A more efficient way to solve the issue, along with the additional issues above, is to prevent recursion entirely, by adding non-reentrancy guard in EntryPoint itself. The cost of the reentrancy guard is split among all UserOps rather than having each account spend gas on its own protection.