Add ERC2771Forwarder as an enhanced successor to MinimalForwarder

[ernestognw]

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)

[changeset-bot]

🦋 Changeset detected

Latest commit: 62d2342

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Major

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

[ernestognw]

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.

[Amxx]

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;
}

[ernestognw]

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]

This looks good to me. Let me know when that PR moves out of draft and it requires a formal approval.

Thanks! I didn't move it before because there are some issues with Slither but seems like false positives from my side

[frangio]

LGTM!

[Amxx]

Minor comment about code inlining. Otherwize LGTM.

[frangio]

The coverage report didn't get uploaded correctly this last run but we've seen it pass before in 95bcb57.