safeTxGas is not enforced

[fabiohild]

Reporting a bug recently found through a bug bounty in Solidified. We can say the overall consensus is that this is a bug, but opinions on risk vary greatly. The goal is to start a discussion around the topic, and hopefully arrive at a consensus on how to secure the now prevalent Meta TXs.

Description

The bug consists in the fact that the parameter safeTxGas, signed by the user, is not enforced by the smart contract, in some situations the gas provided to the safeTx can be lower than the one signed by the user.

Detailed explanation by user @wighawag : https://web.solidified.io/contract/5b4769b1e6c0d80014f3ea4e/bug/5c83d86ac2dd6600116381f9

EIP150, relevant for the discussion: http://eips.ethereum.org/EIPS/eip-150

Some of the possible impacts were already flagged by @wighawag, to sum up:

• Valid safeTxs (with just enough safeTxGas, calculated in SSADFASFA) can fail if the relayer send a low gas value. For larger Txs the relayer can obtain the refund (if the gas left is enough to finish execution).
• Transactions provided with excess gas being successfully executed, with a different gas amount (unlikely impactful).

The discussion right now is orbiting between including this in the documentation or fix this and enforce the gas amount signed by the user. We tend to favor the fix side, and think that although this can be a design decision, the following are fair assumptions:

• Gas is a relevant parameter of a tx, a user is interested in ensuring his transaction has enough gas, and possibly an exact gas amount.
• This parameter is already signed by the user, and a signature implies not only ensuring authenticity but also integrity.
• Fixing not only will meet these assumptions, but also take care of all edge cases discussed, including possible unknowns.

Other implementations

Most early implementations do not take gas into consideration. Gas amount is not signed nor enforced, but the external call is required to succeed in order for the payout to the relayer to be performed. In these implementations a fixed amount is refunded to the relayer, and he can opt not to submit Txs that revert or are not profitable.

• Austin Griffith: https://github.com/austintgriffith/bouncer-proxy/blob/master/BouncerProxy/BouncerProxy.sol
• Meta Cartel: https://github.com/Meta-tx/relayer/blob/master/contracts/PersonalWallet.sol
• uPort: https://github.com/uport-project/uport-identity/blob/develop/contracts/TxRelay.sol

EIP1077

EIP 1077 currently requires gasPrice and gasLimit to be signed, although it does not specify if they should be checked or enforced (https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1077.md).

Status identityGasRelay implementation (that inspired EIP1077) is very similar to Gnosis Safe implmentation, gasPrice and gasLimit are signed, and the limit is checked during execution (first check in callGasRelayed). This implementation also allows for transactions being executed with slight less gas than the limit (can be a problem for Txs with tight gas). https://github.com/status-im/contracts/blob/73-economic-abstraction/contracts/identity/IdentityGasRelay.sol

Universal Logins also implements the standard, have both gasLimit and gasPrice signed by the user, but do not verify it at all. The relayer in this scenario can cause any exteral tx to fail and still get refunded by providing anough gas for executeSigned to execute but less than the external call needs.
https://github.com/UniversalLogin/UniversalLoginSDK/blob/master/universal-login-contracts/contracts/ERC1077.sol

EIP 1776

Proposing native meta TXs, created by the original bug submitter @wighawag. Current version requires both inclusion of gas parameters in the user signed payload and also the need to verify them and enforce them in execution. Seems to be the first time this concern was brought up.
ethereum/EIPs#1776

It's reference implementation does not implement external calls: https://github.com/pixowl/thesandbox-contracts/blob/master/src/Sand/erc20/ERC20MetaTxExtension.sol

Conclusion

As we can see implementations are fairly similar, after looking at all of them I tend to think that two approaches are valid:

• Early approach of not requiring users to sign gas parameters, but requiring the external to succeed in order to provide the refund.
• If failed external TXs are to be refunded, have the user sign the gas amount and ensure that the external call has enough to execute the tx as signed.

I would basically report anything that allows the relayer to influence on the outcome of the transaction, other than not sending it at all.

[wighawag]

@fabiohild thanks for bringing that here so the whole community can bring feedback.

As my report says there is indeed a clear benefit in fixing the bug since as you mentioned the parameter safeTxGas is signed by the user and it is logical to assume that the user will expect such parameter to be properly respected by the safe and would be surprised to learn that its meta transaction failed AND it lost funds even if it provided the correct value for the call to succeed.
Furthermore, if the bug is not fixed and a user provide a value for safeTxGas that is equal to the expected gas for the call to succeed, the success of the external call is dependent on the relayer who is able to get the refund in both case.

Now @tschubotz and the rest of your team, you decided to leave the bug as is, saying that this can be avoided by ensuring that the user sign a safeTxGas value higher than necessary, hence your choice of categorizing this bug as a documentation issue.

While as my report states, the bug can indeed be avoided on the interface side by ensuring that safeTxGas is inflated via I x 64/63 + E where I is the gas expected to be given to the external call and E a safe value to accommodate for the EVM operation between the gas check and the external call itself, this would mean that the smart contract security now relies on interfaces/users being aware of such requirements.
But such requirements are not obvious from the name of the parameter itself, nor from the contract code where at the require call that is supposed to check the value safeTxGas it says "Not enough gas to execute safe transaction" see https://github.com/gnosis/safe-contracts/blob/5a8b07326faf30c644361c9d690d57dbeb838698/contracts/GnosisSafe.sol#L101
This is clearly not true since as I showed the external call can be given less gas even if that check pass.

Then if I am correct you rejected the fix because you concluded it would be unreliable for 2 reasons :

1. it would be dependent on the behaviour of the EVM (specified by EIP-150) not changing
2. since E is dependent on the compiler used, it would need to be adjusted depending on the compiler.

While 1) is true, if the EVM do change on that front, this would not be worse that what you have to do now at the interface level to avoid the bug. As such this is to me not a convincing argument. Plus there is no indication that EVM will change in that regard.

For 2)
First of all you ll have to compute that value at the interface level even now anyway since the bug would appear if you underestimated E. As such the problem you raise is still a problem even if you don't fix the bug in the smart contract.
Second, a simple fix would be to set E to a high enough value that it will cover all possible case. Alternatively you can over estimate E with a sufficient margin based on the EVM operations that sit between the call to gasleft() and the external call itself.
I also wonder how much assembly would be necessary to come up with something safe for all case, independent of all compilers. A margin would still be necessary in that case though to accommodate for change in gas pricing.

There is also an alternative solution that do not require any gas prediction and would continue to work even if gas pricing change as long as EIP-150 behaviour remains the same. It involves doing the check after the external call. Indeed, since EIP-150 leave Math.floor(G/64) gas after the call, we can be sure that not enough gas was passed if the gas left after the call is : gasleft() < I/63
Note though that such check does not cover the case where the external call exit early based on code that check for something along the line of require(gasleft()>X). In that particular case, while the gas passed to the external was lower than I, the gas left after the call would be greater than I/63 even though the call failed due to having less gas than expected.

Finally safe owner could be given a mechanism by which the value E or its computation parameters or even the whole gas check mechanism could be modified to provide a future proof solution to the problem.

To conclude though, even if I believe the fixes I propose can solve the problem I also believe that such checks should be performed at the EVM level. In other words, the EVM should provide a mechanism that can ensure an external calls are given exactly the amount of gas specified and not just act as a maximum value as it does now.

I thus wrote a proposal to fix this here : ethereum/EIPs#1930

This way there would be no need to compute E nor assume the external call will not exit early based on the gas provided. It would also be independent of changes of the EVM in regard to EIP-150

Feel free to contribute to it and let's get this situation fixed once and for all.

[fabiohild]

Commit 62d4bd3 (merged into the latest release) fixes this. Thanks folks!