Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ERC-4337: Use ERC-7746 for userOp and PaymasterOp validation #631

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

peersky
Copy link
Contributor

@peersky peersky commented Sep 10, 2024

This PR proposes replacing validateUserOp and validatePaymasterOp with the generic middleware hook standard outlined in ERC-7746. This change introduces beforeCall and afterCall hooks, providing a more generalized approach to user operation and paymaster validations.

This approach is beneficial due to few reasons:

  • Better encapsulation: Current ERC is too big, referencing another ERC allows to reduce lines of code, ease understanding this standard and allow for smaller iterations
  • Better security: Use of 7746 provisions generic security constraints for middleware hooks, this includes afterCall on user operation validation, which is not included explicitly in current ERC, as well increased security as it specifies to revert within user/paymaster contract
  • Better standardization: Middleware hooks itself are widely needed in the industry. Example for such are Firewall solutions developed by IronBlocks & Forta.

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Sep 10, 2024

File ERCS/erc-4337.md

Requires 1 more reviewers from @drortirosh, @forshtat, @kristofgazso, @shahafn, @tjade273, @vbuterin, @yoavw

@eip-review-bot eip-review-bot changed the title Update ERC4337: Use ERC-7746 for userOp and PaymasterOp validation Update ERC-4337: Use ERC-7746 for userOp and PaymasterOp validation Sep 10, 2024
@shahafn
Copy link

shahafn commented Sep 11, 2024

Hi @peersky
This suggestion makes the contracts too generic, with an increased gas cost and bloated code, since instead of using the generated dispatcher by solc to determine the function executed, you'd need to implement a dispatcher in solidity for that. I'm not sure that I see the benefit here, that is worth breaking the current ABI and forcing a major change for.
Regarding your point that the ERC is too big - you're right! that's why we're extracting parts of it to other ercs, in #626 #627 #628 #629

@peersky
Copy link
Contributor Author

peersky commented Sep 12, 2024

Hi @peersky This suggestion makes the contracts too generic, with an increased gas cost and bloated code, since instead of using the generated dispatcher by solc to determine the function executed, you'd need to implement a dispatcher in solidity for that. I'm not sure that I see the benefit here, that is worth breaking the current ABI and forcing a major change for. Regarding your point that the ERC is too big - you're right! that's why we're extracting parts of it to other ercs, in #626 #627 #628 #629

Hi @shahafn

Thank you for your feedback! I'd like to clarify how my proposal relates to the potential dispatching issue you raised.

My proposed change focuses on modifying the interface (API) of an external contract, not altering the internal dispatching mechanism within the entry point contract. The Solidity compiler should continue to generate optimized dispatching code for function calls within the contract, regardless of how the external contract's API is structured. Thus, I don't anticipate my proposal introducing any gas inefficiencies related to custom dispatching within the entry point contract itself.

Regarding breaking changes:

I understand your concern, but given that this ERC is still in the Draft stage, I believe it's reasonable to expect that businesses building on this standard should be prepared for potential API changes.
The proposed change offers significant benefits by generalizing a common industry need for pre/post validation checks using external contracts.
This adaptability allows smart accounts to support future standards, even those that might deprecate or coexist with ERC-4337, without requiring modifications to the smart account API calls. It achieves this by enabling the identification of caller intents from the call data and signatures provided as arguments, offering greater flexibility compared to relying solely on the called method signature.

@shahafn
Copy link

shahafn commented Sep 12, 2024

Maybe I misunderstand something: for instance, you replaced validateUserOp() with beforeCall(). How would the wallet know what beforeCall() should do? Do you intend 4337 wallets to implement erc 7746 only for this purpose? That is, all calls to the wallet beforeCall() function is just a call to validateUserOp() ? Just a rename for them? if so, then how is erc7746 composable middleware? what if there's another dapp that the wallet interacts with, that needs hooks implemented? Would the wallet use beforeCall() for that as well? if so, then dispatching is needed here inside the beforeCall() function. Otherwise, it's just a rename.
Either way, not sure I see the added value right now.

Copy link
Contributor

@forshtat forshtat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peersky although the ERC-4337 is still in Draft stage, it has been pretty stable for a long time and is not expected to be changed unless there is a serious security threat discovered. If you still wish to propose an ERC-7746 derived Account Abstraction solution it can be done in a separate ERC, but not through modifying ERC-4337.
Thank you!

@peersky
Copy link
Contributor Author

peersky commented Sep 12, 2024

Maybe I misunderstand something: for instance, you replaced validateUserOp() with beforeCall(). How would the wallet know what beforeCall() should do? Do you intend 4337 wallets to implement erc 7746 only for this purpose? That is, all calls to the wallet beforeCall() function is just a call to validateUserOp() ? Just a rename for them? if so, then how is erc7746 composable middleware? what if there's another dapp that the wallet interacts with, that needs hooks implemented? Would the wallet use beforeCall() for that as well? if so, then dispatching is needed here inside the beforeCall() function. Otherwise, it's just a rename. Either way, not sure I see the added value right now.

  1. Flexibility, not just a rename: The intent is not simply to rename functions. Embracing ERC-7746 offers wallets the choice to implement more sophisticated middleware logic if needed, going beyond basic userOp validation. If a wallet prioritizes gas efficiency for a single use case, it can indeed keep the implementation simple, essentially mirroring the current behavior without additional overhead.
  2. Enhanced Security: Current standard does not define userOp post validation. A key advantage of adopting ERC-7746 is its ability to bridge this gap, enabling smart accounts to integrate seamlessly with essential security solutions, like IronBlocks Firewall, which necessitate both pre- and post-checks. The IronBlocks team has already expressed their willingness to adopt ERC-7746, further underscoring its value. While in execute() firewall-like solutions can be an option, it restricts the implementation of fine-grained entry point-specific firewall tx validators.
  3. Standardization and Collaboration: ERC-7746 represents a proactive step towards preventing further fragmentation in the Ethereum. Embracing this standard within ERC-4337 encourages wider adoption and fosters a more unified developer experience, for example it allows to provision a generic security oracles, embracing unifying APIs of vendors such as IronBlocks, Forta, and others who are going to provide users with wallet runtime protection. This ultimately benefits end-users by promoting interoperability and higher security guarantees across different smart contract vendors.

@peersky
Copy link
Contributor Author

peersky commented Sep 12, 2024

@peersky although the ERC-4337 is still in Draft stage, it has been pretty stable for a long time and is not expected to be changed unless there is a serious security threat discovered. If you still wish to propose an ERC-7746 derived Account Abstraction solution it can be done in a separate ERC, but not through modifying ERC-4337. Thank you!

@forshtat, while I appreciate your point about the relative stability of ERC-4337, I believe suggesting the creation of deprecation ERCs for a standard still in "Draft" status sets a concerning precedent. It undermines trust in the ecosystem, discourages active participation, and signals a lack of commitment to maintaining standards.

The very essence of standardization is to establish guidelines that endure for the long term, ensuring the competitiveness and longevity of Ethereum. In this context, prioritizing interests over the collective, long-term success of the ecosystem is counterproductive.

I firmly believe that embracing a collaborative and forward-thinking approach to standards development is crucial for the continued growth and success of Ethereum.

That said, the ultimate question lies in whether the proposed changes add sufficient value to justify any potential disruption. I've outlined my considerations in my previous reply, and in my view, they make a strong case for introducing changes that accommodate the evolving landscape of security oracles.

@shahafn
Copy link

shahafn commented Sep 17, 2024

Maybe I misunderstand something: for instance, you replaced validateUserOp() with beforeCall(). How would the wallet know what beforeCall() should do? Do you intend 4337 wallets to implement erc 7746 only for this purpose? That is, all calls to the wallet beforeCall() function is just a call to validateUserOp() ? Just a rename for them? if so, then how is erc7746 composable middleware? what if there's another dapp that the wallet interacts with, that needs hooks implemented? Would the wallet use beforeCall() for that as well? if so, then dispatching is needed here inside the beforeCall() function. Otherwise, it's just a rename. Either way, not sure I see the added value right now.

1. **Flexibility, not just a rename**:  The intent is not simply to rename functions. Embracing ERC-7746 offers wallets the _choice_ to implement more sophisticated middleware logic if needed, going beyond basic userOp validation. If a wallet prioritizes gas efficiency for a single use case, it can indeed keep the implementation simple, essentially mirroring the current behavior without additional overhead.

2. **Enhanced Security**:  Current standard does not define `userOp` post validation.  A key advantage of adopting ERC-7746 is its ability to bridge this gap, enabling smart accounts to integrate seamlessly with essential security solutions, like IronBlocks Firewall, which necessitate both pre- and post-checks. The IronBlocks team has already expressed their willingness to adopt ERC-7746, further underscoring its value. While in `execute()` firewall-like solutions can be an option, it restricts the implementation of fine-grained entry point-specific firewall tx validators.

3. **Standardization and Collaboration**: ERC-7746 represents a proactive step towards preventing further fragmentation in the Ethereum. Embracing this standard within ERC-4337 encourages wider adoption and fosters a more unified developer experience, for example it allows to provision a generic security oracles, embracing unifying APIs of vendors such as IronBlocks, Forta, and others who are going to provide users with wallet runtime protection. This ultimately benefits end-users by promoting interoperability and higher security guarantees across different smart contract vendors.
  1. So if the wallet wants more flexibility, it needs the gas overhead of implementing a dispatcher inside these functions. I'm not sure it's better than using a different function name.
    2-3. Current version of EntryPoint is already there. We don't see the benefit of breaking APIs for that specific change right now. If it becomes a standard in the future, we'll reconsider then, and as a separate ERC.

As @forshtat mentioned, while the erc is draft, it's already widely used, will be moved from draft soon, and we don't see the benefit of breaking the api for reasons that are not critical security risks.

@peersky
Copy link
Contributor Author

peersky commented Sep 18, 2024

So if the wallet wants more flexibility, it needs the gas overhead of implementing a dispatcher inside these functions. I'm not sure it's better than using a different function name.

ERC-7746 empowers USERS to directly enhance their wallet security without relying on wallet developers for updates or feature implementations. This can be done with great UX, by simply allowing, within a wallet, to configure destination where all of user operation checks are to be proxied. This also widens doors for general use of security oracles and other extensions that can be daisy-chained with same API.
As an example of such daisy chaining think of following: with 7746, paymaster validation interface could be completely extracted from this standard, leaving only user validation via 7746 and some payload format definitions (which are already in place). While this would reduce LoC of standard, it also would mean that user validation can daisy-chain call to paymaster from wallet itself based on configuration passed in. This ultimately puts end-user in ability to control which paymasters can pay his transactions, or even override default one passed in configuration. From batcher perspective this still is possible to simulate and verify which paymaster will be used. This enshrines paymasters to offer better end-user experience, like doing transaction security screening etc and ultimately is beneficial for the ecosystem as they compete for the users.

This flexibility is crucial as it allows users to adapt their security measures to evolving threats or personal preferences. In contrast, your proposed scenario would necessitate wallet developers to incorporate new function names, potentially leading to delays, compatibility issues, or even requiring users to migrate to updated wallet contracts.

Also, gas issues are going to be diminishing in the next 10 years as consensus technology improves. Standards defined today should prioritise long term, lasting security architecture rather than minor gas savings in short run, and given rollup-centric development of Ethereum, I think it's fair to assume that most of ERC-4337 accounts will operate on L2s or even App-chains, where gas costs are not an issue.

2-3. Current version of EntryPoint is already there. We don't see the benefit of breaking APIs for that specific change right now. If it becomes a standard in the future, we'll reconsider then, and as a separate ERC.

The current ERC standard creates an inconsistency:

  • Paymasters enjoy both pre- and post-execution validation (validatePaymasterUserOp and postOp), essentially a middleware-style security model.
  • User operations, however, are limited to a single pre-execution validation (validateUserOp).

This discrepancy raises concerns:

  • It implies a security gap where paymasters are inherently better protected than users. This is especially problematic in the context of account abstraction, which inherently expands the attack surface on the user.
  • Wallets aiming to implement granular security measures (e.g., spending limits for entry-point and/or paymaster paid txs) are forced to integrate these checks directly into their execute() function. This results in unnecessary gas overhead for all transactions, even those not originating from the 4337 entry point (back to @shahafn concern on dispatching gas costs).

If this design choice is intentional, a clear and detailed explanation is needed:

  • Why do paymasters require middleware-like security while users don't?
  • What are the technical or philosophical reasons behind this distinction?

The absence of such an explanation suggests either an oversight in the standard's design or a lack of consideration for the security needs of users in the account abstraction paradigm.

@drortirosh
Copy link
Contributor

The API was designed to provide the minimal required functions.

  • Account validation is the minimal API required to approve payment for the transaction (usually, owner's signature check). While account MAY use it for more sophisticated validations, they are not required to.
  • any higher level validation (pre/post hooks, etc) are expected to be handled by the account itself, in its execute method. Please see various implementations in the modular accounts models, ERC-6900 and ERC-7579
  • Paymasters perform payment, and as such require the pre/post pattern. Again, they MAY do more than that in the validation/postOp function, but are not required to.
  • The API is currently widely used (by over 15M accounts), and unless there is a critical security flaw, it is unlikely to change.

@peersky
Copy link
Contributor Author

peersky commented Sep 18, 2024

The API was designed to provide the minimal required functions.

  • Account validation is the minimal API required to approve payment for the transaction (usually, owner's signature check). While account MAY use it for more sophisticated validations, they are not required to.
  • any higher level validation (pre/post hooks, etc) are expected to be handled by the account itself, in its execute method. Please see various implementations in the modular accounts models, ERC-6900 and ERC-7579
  • Paymasters perform payment, and as such require the pre/post pattern. Again, they MAY do more than that in the validation/postOp function, but are not required to.
  • The API is currently widely used (by over 15M accounts), and unless there is a critical security flaw, it is unlikely to change.
  1. No it's not. Standard defines validateUserOp should "pay the fee if the account considers the operation valid", it's not a minimal required for approving.
  2. I am aware of these, see my response above.
  3. Please elaborate, from security standpoint, how is that different for validateUserOp paying the fee for himself (or shall I say, acting as paymaster for self)?
  4. Should I be surprised by this number? There are more then 5 billion users in the internet. In either case, these 15M users, (btw, out of which there are only 2M monthly active users), will continue using their wallets happily whatever ERC standard tells.

On your last point:
I'll reiterate once again, this standard is in Draft status. Anyone considering using it in production should be aware that changes may occur. The purpose of standards is not to blindly retroactively fit existing practices, but to guide the future development of the ecosystem. If there's room for immediate improvement, those improvements should be made before the standard is finalized.

Please elaborate, who has the authority to demand that changes to a Draft standard MUST be security-critical, simply because it's been around for a year or two and its complexity has stalled progress? Where is that written?

It's well-known that web3 user experience is poor [1], and improving UX has been a goal of @vbuterin from the start. I've already explained how this change not only enhances security but also paves the way for better UX in the long run.

Therefore, in my opinion, EITHER this argument is irrelevant, OR it must be acknowledged that keeping this standard in Draft status is hindering its adoption. The metric you mentioned (15M wallets) would likely be higher, not lower, if the standard was in a more advanced stage like "Review" or "Last Call."

@shahafn
Copy link

shahafn commented Sep 19, 2024

ERC-7746 empowers USERS to directly enhance their wallet security without relying on wallet developers for updates or feature implementations. This can be done with great UX, by simply allowing, within a wallet, to configure destination where all of user operation checks are to be proxied. This also widens doors for general use of security oracles and other extensions that can be daisy-chained with same API. As an example of such daisy chaining think of following: with 7746, paymaster validation interface could be completely extracted from this standard, leaving only user validation via 7746 and some payload format definitions (which are already in place). While this would reduce LoC of standard, it also would mean that user validation can daisy-chain call to paymaster from wallet itself based on configuration passed in. This ultimately puts end-user in ability to control which paymasters can pay his transactions, or even override default one passed in configuration. From batcher perspective this still is possible to simulate and verify which paymaster will be used. This enshrines paymasters to offer better end-user experience, like doing transaction security screening etc and ultimately is beneficial for the ecosystem as they compete for the users.

This flexibility is crucial as it allows users to adapt their security measures to evolving threats or personal preferences. In contrast, your proposed scenario would necessitate wallet developers to incorporate new function names, potentially leading to delays, compatibility issues, or even requiring users to migrate to updated wallet contracts.

  1. Having wallets get used to the possibility of an implementation upgrade isn't necessarily a bad idea.
  2. You still rely on those third party security oracles or extensions, as you mentioned, to be developed. So USERS still rely on devs to.. develop. Whether it's better to have third party sdks like that, or wallet devs themselves doing it, is not clear and we don't hold our breath for a futuristic adoption of standard, we have a working standard that is implemented and widely used. Again, as I said before, I'm not against 7746, if it gains enough traction and there's demand from different projects for having a 7746 compatible 4337-like solution, we will adjust. In the future.
    This is a different discussion that we shouldn't have here though.

Also, gas issues are going to be diminishing in the next 10 years as consensus technology improves. Standards defined today should prioritise long term, lasting security architecture rather than minor gas savings in short run, and given rollup-centric development of Ethereum, I think it's fair to assume that most of ERC-4337 accounts will operate on L2s or even App-chains, where gas costs are not an issue.

The whole point of this ERC is not to be compatible with a vague idea of a project/ecosystem in 10 years, but to be ready to be used now. If gas costs are going to be diminishing in 10 years, and 7746 is widely used, we can adjust in the future.

The current ERC standard creates an inconsistency:

* Paymasters enjoy both pre- and post-execution validation (`validatePaymasterUserOp` and `postOp`), essentially a middleware-style security model.

* User operations, however, are limited to a single pre-execution validation (`validateUserOp`).

This discrepancy raises concerns:

* It implies a security gap where paymasters are inherently better protected than users. This is especially problematic in the context of account abstraction, which inherently expands the attack surface on the user.

* Wallets aiming to implement granular security measures (e.g., spending limits for entry-point and/or paymaster paid txs) are forced to integrate these checks directly into their `execute()` function. This results in unnecessary gas overhead for all transactions, even those not originating from the 4337 entry point (back to @shahafn concern on dispatching gas costs).

If this design choice is intentional, a clear and detailed explanation is needed:

* Why do paymasters require middleware-like security while users don't?

* What are the technical or philosophical reasons behind this distinction?

The absence of such an explanation suggests either an oversight in the standard's design or a lack of consideration for the security needs of users in the account abstraction paradigm.

I'm not sure that you understood the whole motivation of the ERC, the differences between users and paymasters, and therefore the security and functionality considerations for the different decisions for them. This decision was not made unintentionally as an oversight. I'm not criticizing you for it, as the ERC may not be explaining that well, but assuming lack of consideration for security needs isn't true here. I don't think that we should get to an argument here about the pros and cons for every decision we made behind the ERC, as this PR suggests a concrete change that we rejected for other reasons anyway.

@shahafn
Copy link

shahafn commented Sep 19, 2024

On your last point: I'll reiterate once again, this standard is in Draft status. Anyone considering using it in production should be aware that changes may occur. The purpose of standards is not to blindly retroactively fit existing practices, but to guide the future development of the ecosystem. If there's room for immediate improvement, those improvements should be made before the standard is finalized.

We don't consider your suggestion as an improvement. We understand your points and your objections, but as explained several times, the benefit doesn't outweigh the cost here in our opinion.

Please elaborate, who has the authority to demand that changes to a Draft standard MUST be security-critical, simply because it's been around for a year or two and its complexity has stalled progress? Where is that written?

From my understanding of the EIP/ERC processes, authorship implies authority of the content of the document. Thus, as authors of the ERC, we decided, considering the state of the ecosystem and projects within it (that you may not be aware of), that changes to the ERC should be for security-critical reasons. With all due respect, you're coming late here, demanding changes to the ERC, and expecting us to oblige.

It's well-known that web3 user experience is poor [1], and improving UX has been a goal of @vbuterin from the start. I've already explained how this change not only enhances security but also paves the way for better UX in the long run.

This change, as I said, should be in a different ERC.

Therefore, in my opinion, EITHER this argument is irrelevant, OR it must be acknowledged that keeping this standard in Draft status is hindering its adoption. The metric you mentioned (15M wallets) would likely be higher, not lower, if the standard was in a more advanced stage like "Review" or "Last Call."

Thank you for your input.

Copy link

@shahafn shahafn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If relevant, should be a new ERC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants