Use invalid opcode to consume all gas in MinimalForwarder

[Amxx]

Fixes #2864

PR Checklist

• Tests
• Documentation
• Changelog entry

[frangio]

Thanks!

[lastperson]

@Amxx hey, I'm trying to understand the issue solved here. Could you explain why burning all gas is important and what it prevents from happening?

[frangio]

Explained in https://ipfs.io/ipfs/QmbbYTGTeot9ic4hVrsvnvVuHw4b5P7F5SeMSNX9TYPGjY/blog/ethereum-gas-dangers/

[lastperson]

@frangio the mentioned fix is only helpful for Forwarder calling another Forwarder. Though it still doesn't guarantee that the target contract gets specified amount of gas.

[frangio]

Can you elaborate?

[lastperson]

Sure. Consider the target contract

contract Target {
    function win(address who) external {
        require(gasleft() > 900000);
        IERC20(0x1234...1234).transfer(who, 100e18);
    }
}

User creates a meta tx request with req.gas = 1000000. Relayer picks it up and sends it with 500000 gas.
Target.win() reverts but gasleft() > req.gas / 63 passes.

[Amxx]

Its a bit subtle, but here is what happens:

• The forwarder is designed to check at the call in it, in case it run out of gas, was given the amount specified.
• You target contract, does not revert because it ran out of gas (and was unable to execute because of that), it reverts because a require failed.

You contract check if there is a given amount of gas, but if that is not the case, it doesn't not trigger an out of gas exception.
I'm pretty sure your Target contract is an example for the sake of discussion, but IMO it showcases possible bad design by def.

IMO, contract should not check gas that way.

• They should try to perform operation, and fail with out of gas if that was not enough.
• If they want to guarantee that a subcall has enough gas, they should replicate the Forwarder design (send gas, and if faillure and not enough left, do an invalid call)
• If you really want to ensure gasleft() > 900000, don't do it in a require, do if (gasleft() < 900000) assembly { invalid() }