ERC2771Forwarder: consider not enforcing trustedForwarder

[sakulstra]

🧐 Motivation
When relaying transactions, you often end up in a situation where you need to relay a token permit in addition to some additional transaction(s). While the additional transaction(s) can be forwarded via ERC2771Forwarder - considering the target contract implements ERC2771Context- the permit can not because the token is not "trusting" this forwarder.
However, not trusting the forwarder is not a problem as the permit does not rely on altering the context.

For any flow that requires an approval in order to forward the rest of a transaction batch this would be very useful.

📝 Details
As far as i can tell the Forwarder itself is not part of the Spec, so i think it's worth considering to remove the isTrustedForwarder check as I don't actually see any downside of not having it - worst case the txn would revert. This is in line with how the previous MinimalForwarder worked.

edit: I see this was changed in #4502 (comment) specifically to prevent accidents caused by approving the forwarder, but the fix as lined out above, can actually cause problems.

[Amxx]

Hello @sakulstra and thank you for raising that issue.

The ERC2771Forwarder is probably not going to be developped any further:

ERC-7702, which should be live soon will provide a good alternative to ERC-2771 meta transaction. This will allow us to remove the cumbersome Context contract. At the same time we will deprecate/remove ERC2771Forwarder from our codebase.

We cannot do that now, as it would be a breaking change, but we will do that in the next major release (v6.0)

In the meantime, we are commited to not doing any breaking change. Changing the validation/trust of the forwarder would be a breaking change as far as security/behavior goes. If we wanted to ship that, we would probably wait until 6.0 ... but again we will lieklly remove the Forwarder entierly in 6.0

@ernestognw WDYT ?

[ernestognw]

With Account Abstraction and especially EIP-7702 it's likely that Context won't be relevant anymore. For now, I agree that we must keep backwards compatibility until 6.0 (we don't have plans for it yet).

In regards to ERC2771Forwarder, we can make the _isTrustedByTarget function internal so people can override it with their own definition.

We initially didn't do this because it's technically a footgun if a user approves the forwarder to spend tokens, but if I understand correctly, the use case is to relay a permit where you don't require the token to trust the forwarder and it's just batched along for convenience. Is that correct?

[sakulstra]

@ernestognw yes exactly.

The scenario is that i have a token A that i want to supply on some pool B.
While i am in control of B and I can implement 7702-context, for a supply to work i need to batch it with an A.permit(B).

[Amxx]

@sakulstra I designed this approach for batch operations a while back, and it does not check _isTrustedByTarget on any of the sub calls. So technically it would work in your case.

The nice part is it can be added "on top of" the existing code without needing any modification.

ForwardRequestData ---[relayer]--> Forwarder.execute(...) ---[verified trusted]--> Forwarder.atomicBatch(...) ---[unverified]--> Anything[]

It removed some of the _isTrustedByTarget safety, but honestly if the Forwarder does not hold any assets (or allowance) that should be just fine.

[Amxx]

contract ERC2771ForwarderWithAtomicBatch is ERC2771Context(address(this)), ERC2771Forwarder {
    error ERC2771ForwarderInvalidRelayLength(uint256 targets, uint256 calldatas, uint256 values);
    
    function atomicBatch(
        address[] calldata targets,
        uint256[] calldata values,
        bytes[] calldata calldatas
    ) public payable virtual {
        uint256 length = targets.length;
        if (length == 0 || values.length != length || calldatas.length != length) {
            revert ERC2771ForwarderInvalidRelayLength(length, calldatas.length, values.length);
        }

        // This will recover the caller is execution is the result of a relay though the Forwarder (this);
        address caller = _msgSender();
        uint256 remaining = msg.value;

        // relay the subcalls using the caller as the "from"
        for (uint256 i = 0; i < length; ++i) {
            (bool success, ) = targets[i].call{value: values[i]}(abi.encodePacked(calldatas[i], caller));
            if (!success) {
                revert Address.FailedInnerCall();
            }
            remaining -= values[i];
        }

        // if not all value is passed to subcalls.
        if (remaining != 0) {
            revert ERC2771ForwarderMismatchedValue(msg.value - remaining, msg.value);
        }
    }
}

[ernestognw]

Just opened #5416, which can make it into 5.3 if we feel it's the right decision.

[sakulstra]

@ernestognw, so the idea would be to override with a noop returning true, right?

One different thing I was wondering about (perhaps a bit off-topic) is why does the forwarder require a single permit per forwarded txn 😅. I see that it kinda makes sense when having essentially 1 user per txn, but for cases of e.g batching a supply & borrow of a single user it would be nice if they'd only need to sign a single permit. Perhaps with 7702 around the corner no longer worth it, but curious if there was a rational not to support this (or is the assumption to always use this with multicall and then sign the multicall)?

[ernestognw]

@ernestognw, so the idea would be to override with a noop returning true, right?

Right, just as:

function _isTrustedByTarget(address target) internal override returns (bool) {
  return true;
}

is why does the forwarder require a single permit per forwarded txn 😅

So you mean having a 1-signature per-batch mechanism, right?

We didn't look further into it because Account Abstraction was becoming more relevant when we wrote it and we weren't sure whether ERC2771 would remain relevant for end-user interactions.

We found out, though, that many of the use cases that were still relevant at the moment were automations (e.g. OpenZeppelin Relayers). I understand a single signature per batch would be desirable, but yeah I guess 7702 kinda replaces batching for the end-users.

We can consider adding this to the forwarder for 5.3, but I think 7702 will be the way to go.

Btw, feel free to check out our ERC7702 Account prototype we're actively working on in our community contracts repository 😄

[sakulstra]

So you mean having a 1-signature per-batch mechanism, right?

Not really, i think 🤔 a batch could still be multiple signatures.

Let's assume I operate a relayer for some erc20, that has a mint, transfer and stake.
The token implements ERC2771Context and the relayer implements ERC2771Forwarder.

Now i can batch multiple mint, transfer and stake txns from various users in a single txn, which is as desired.

But what do I do if a single user wants to mint & stake?
Currently, it's possible, but it would require the user to sign two txns with two nonces, which imo is not very good ux.
The alternative would be signing both txns in a single signature.

We can consider adding this to the forwarder for 5.3, but I think 7702 will be the way to go.

I kinda agree, but also not 😅 I think there's no exact date for pectra yet and even if there is it's not clear when L2s will catch up. ERC2771 on the other hand works on london.

[Amxx]

But what do I do if a single user wants to mint & stake?
Currently, it's possible, but it would require the user to sign two txns with two nonces, which imo is not very good ux.

That was the usecase I had in mind when writting the atomicBatch extensions. Not only does is use only one signature for multiple calls (by the same user) but it also guarantees that they are executed atomically (right now it is possible for a relayer to execute the mint part, and leave the stake part).

It was decided that it was not a priority, and that the complexity was possibly not worth it ... but I guess that can be reconsidered based on user requests

[sakulstra]

@Amxx fair point

Going back to my initial example (permit + deposit), i guess the desired behavior would be to not enforce atomicity though, i think. Otherwise, someone could front run the permit to make the txn fail.

[ernestognw]

Not really, i think 🤔 a batch could still be multiple signatures.

Yeah, true! I still got the point that it'll be convenient to have a way to batch forward requests in a single signature.

We can still implement this for 5.3 and it can be atomic using a flag (similar to how we check for refundReceiver == address(0) in executeBatch). We basically have three options:

1. To follow @Amxx's approach of wrapping a call to the a forwarder function called atomicBatch (though I'd rename it to batch(..., address refundReceiver)).
2. To create a new function (perhaps called batch(..., address refundReceiver) too) that does batch calls for a single user. This approach will require the user to sign a single EIP-712 message that nests a call to batch.

It's worth noting none of both is good in terms of readability. Another option could be another EIP-712 typehash:

 bytes32 internal constant _FORWARD_REQUEST_TYPEHASH =
        keccak256(
            "BatchedForwardRequest(ForwardRequest[] requests,bool atomic)ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
        )

Shall we consider this for 5.3 @Amxx?

[sakulstra]

Was experimenting a bit today with the current contract and am wondering if it would also make sense to extend "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" with an address relayer(or similar)?

When composing transactions it can be quite valuable to enforce a specific msg.sender - independent of the signer.