Add ERC2771Forwarder as an enhanced successor to MinimalForwarder by ernestognw · Pull Request #4346 · OpenZeppelin/openzeppelin-contracts (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation105 Commits36 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

Fixes #3884
Fixes #3664
Replaces #4065

Currently, MinimalForwarder is not considered production-ready since it was designed mainly for testing purposes in the library. However, it's not that far from a MinimalForwarder that's something we may recommend.

The new MinimalForwarder should have:

• A deadline for expiring transactions
• A msg.value mismatch check
• Redesign of gas forwarding check (in discussion)

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

I think I've finally nailed the gasLeft < request.gas / 63 and I also added batching.

Still need to adjust the tests (adding more) and I'm also thinking of fuzzing the execution.

Collaborator

Amxx left a comment

Choose a reason for hiding this comment

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

I have a concern with the batch part. Since all the request have their own signature, anyone could extract one of the request from the batch, and execute it alone. I don't think we want that to be possible.

The nonce would protect out of order execution, but by front running the relaying of the first one, an attacker would disrupt the relaying of the rest. Also, a user may want the request to be executed atomically.

This mostly applies to request that come from the same user. And I realise that it may be usefull for a relayer to batch requests from different users.

So my suggestion would be:

• To avoid someone batching multiple request from the same user, and hopping they are executed attomically, maybe the ForwardRequest should be

struct ForwardRequest { address from; address[] to; uint256[] value; bytes[] data; uint256[] gas; uint256 nonce; uint48 deadline; }

I have a concern with the batch part. Since all the request have their own signature, anyone could extract one of the request from the batch, and execute it alone. I don't think we want that to be possible.

The nonce would protect out of order execution, but by front running the relaying of the first one, an attacker would disrupt the relaying of the rest. Also, a user may want the request to be executed atomically.

This mostly applies to request that come from the same user. And I realise that it may be usefull for a relayer to batch requests from different users.

So my suggestion would be:

* To avoid someone batching multiple request from the same user, and hopping they are executed attomically, maybe the `ForwardRequest` should be

struct ForwardRequest { address from; address[] to; uint256[] value; bytes[] data; uint256[] gas; uint256 nonce; uint48 deadline; }

It's true that the current state allows an attacker to frontrun a relayer to force a revert on at least one of the requests, and I think that is the main problem to fix.

In regards to atomicity, you're right but I'm not sure if that requirement should fit into this contract (at least at the moment) since we don't have feedback yet. In any case, batching is a scalability strategy for the relayer more than an atomicity guarantee for a user.

According to what we discussed internally, we'll allow batching to continue even if there's an already processed nonce.

ernestognw changed the titleMake MinimalForwarder production-ready for 5.0 Make ERC2771Forwarder production-ready for 5.0

Jun 23, 2023

This was referenced

Sep 10, 2024

This was referenced

Sep 12, 2024

This was referenced

Nov 8, 2024

This was referenced

Nov 9, 2024