[ETHEREUM-CONTRACTS] Trusted Forwarder behavior is not EIP-2771 compliant

[jtriley-eth]

Bug Description

The meta-transaction standard, EIP-2771, requires that if the isTrustedForwarder function returns false for a given msg.sender, the "signer" must fallback to the msg.sender of the call. The current forwardBatchCall implementation does not conform to this standard.

Steps To Reproduce

• Call forwardBatchCall from an EOA or a contract whose address, when passed as an argument to isTrustedForwarder, returns false.

Expected Behavior

Per the EIP-2771 standard, the expected behavior of forwardBatchCall is to fallback to msg.sender instead of signer when the msg.sender is not a trusted forwarder.

Actual Behavior

If the msg.sender is not a trusted forwarder, the call reverts entirely with revert string: "Not trusted forwarder".

Implementation

EIP-2771 Implementation:

function _msgSender() internal view returns (address payable signer) {
    signer = msg.sender;
    if (msg.data.length>=20 && isTrustedForwarder(signer)) {
        assembly {
            signer := shr(96,calldataload(sub(calldatasize(),20)))
        }
    }    
}

Superfluid Implementation:

function _getTransactionSigner() internal virtual view returns (address payable ret) {
    require(msg.data.length >= 24 && isTrustedForwarder(msg.sender), "Not trusted forwarder");
   assembly {
      ret := shr(96,calldataload(sub(calldatasize(),20)))
   }
}

[hellwolf]

Thanks.

It is was indeed modified from https://github.com/opengsn/forwarder/blob/master/contracts/BaseRelayRecipient.sol,

Apart from conforming to the standard, is there a legit case to be made to allow "non trusted forwarder" to call this function, hence maybe increase the attack surface? Does the standard have an engineering compromise to an use case that is not highlighted here?

[hellwolf]

CC @d10r for more thoughts

[jtriley-eth]

In the case of forwardBatchCall the trusted forwarder is the only address that's expected to call the function, so no, there is no other use case here.

However, making this EIP-2771 compliant would make meta-transactions possible on other relevant functions on the Host. For example, at the moment, the only function supporting meta-transactions is forwardBatchCall, but if the _getTransactionSigner() function is used in callAgreement, batchCall and callAppAction, then there would be no need for a forwardBatchCall and it would make things like one-off agreement calls or app action calls easier to use with meta-transactions.

[hellwolf]

I see, at the time, the thinking was that the only meta transaction entry point is forwardBatchCall. There ware also some ergonomic reason, in biconomy at the time, you need to whitelist the function you want to allow "forwarding", in that case listing one (forwardBatchCall) seemed a "clean" choice.

Gotta think about this again, we could enable that standard behavior for forwardBatchCall, seems harmless. But enabling more "forwardable" entry points in host seems in conflict with other approaches we are doing:

user/forwarder -> host.[callAgreement,callAppAction,batchCall]
or
user/forwarder -> agreement forwarder -> host.forwardBatchCall 
or
user -> token -> host.tokenBatchCall??

[d10r]

thx @JoshuaTrujillo15 for pointing out. The requirement indeed makes sense in the examples you describe.
I don't think however it's worth the effort to make forwardBatchCall compliant, would be a purely academic exercise w/o any practical benefit, given that it's there exclusively for calls via forwarder. Imo we should stick to the current interface and take a closer look at generic metatx support in the token centric interface. I'll consider already adding support to the agreement forwarder too.

[hellwolf]

1. It seems low benefit of adding this compliance back to the forwardBatchCall.
2. But @d10r will add the the EIP-2771 support in the #982 .

[hellwolf]

Didi 18:35 no, bcs the CFA forwarder ended up not being a forwarder itself, but just use that protocol feature. Which makes me think that "forwarder" may not be the best name for it (too focused on internal architecture vs what it is/does from an outside perspective), will see if I can find a better one.