New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

failed calls due to address.call() gas computation #2999

Closed
lorenzb opened this Issue Sep 30, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@lorenzb

lorenzb commented Sep 30, 2017

Solidity version

Observed on 4.17+commit.bdeb9e52.Emscripten.clang. The current nightly build (0.4.18-nightly.2017.9.29+commit.b9218468.Emscripten.clang) is also affected.

Issue description

We came up with a minimal example demonstrating the issue and deployed it on the mainnet. The example consists of two contracts:

contract Callee {
    event ReceivedCall();
    
    function () {
        ReceivedCall();
    }
}
contract Caller {
    function callAddress(address a) {
        a.call();
    }
}

The issue occurs in transactions that invoke Caller.callAddress(<address of Callee>):
Everything works when the transaction is run with between 24402 and 48574 gas.
The internal transaction/call fails when it is run with between 48575 and 49388 gas. When the transaction is run with at least 49389 gas, the internal call succeeds again.

This behaviour is highly counterintuitive: If the transaction succeeds with x gas, it should also succeed with >x gas.

Cause

The cause of this behaviour is the code that solc generates for a.call():
For the EVM's CALL opcode, the top of the stack specifies the maximum amount of gas that should be available in the new callframe. The generated binary computes this amount by doing

PUSH2 0x646e
GAS
SUB

, i.e. as <amount of available gas> - 25710.

This code in ExpressionCompiler.cpp seems to be responsible for generating the above code.

In the above example, the call in the transaction with 48574 gas succeeds because the amount of available gas before the call is 25709; the subtraction hence underflows and provides an upper limit of 2**256-1 gas to the call.

When the amount of gas for the transaction lies between 48575 and 49388, the internal call runs out of gas because the subtraction no longer underflows and the maximum amount of gas hence lies between 0 and 813, while the call requires at least 814 gas to succeed.

Once the amount of gas for the transaction is at least 49389, the internal call succeeds again because now a sufficient amount of gas is supplied for the call even after subtracting 25710.

Suggested fixes

Most importantly, the behavior of address.call() is undocumented. We feel that users should be made aware of this behavior.

It is not clear to us what the benefit of retaining 25710 gas in the calling context is. The comment in ExpressionCompiler.cpp

send all gas except the amount needed to execute "SUB" and "CALL"

leads us to believe that solc wants to make sure that the maximum amount of gas supplied to the call isn't higher than the amount of gas available in the calling context. However, since the EVM will happily truncate the amount of gas down to what is actually available, this seems unnecessary.

Finally, if retaining the gas for the calling context is actually desirable, then the underflow issue should be fixed.

Attribution

This issue was jointly discovered by

  • Lorenz Breidenbach
  • Phil Daian
  • Florian Tramer
@axic

This comment has been minimized.

Member

axic commented Oct 1, 2017

Thanks for the detailed investigation!

That particular piece of code originates way before the Tangerine Whistle hardfork which introduced the new rules for gas calculation in messages. Before, one had to make sure that enough gas is left to cover the possible account creation and value transfer fees, after subtracting the message gas. After the fork, due to truncation rules, this is not needed anymore.

See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md where compustate.gas is the actual gas left, gas is the argument supplied, submsg_gas is the final limit passed to the call and extra_gas is the additional required gas.

The 25710 comprises of 25000 for possible account creation, 700 for the call and 10 for the cost of PUSH, GAS, SUB.

If we consider the last Solidity version to only output Tangerine Whistle compatible code, we could reduce that code path to a single instruction of GAS. Otherwise we'd need to introduce a setting to chose the EVM version, such as #1117.

@vbuterin

This comment has been minimized.

Contributor

vbuterin commented Oct 2, 2017

Reducing the code path to GAS is exactly what Viper does; I'm definitely in favor of Solidity doing the same.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 4, 2017

I agree, we can assume tangerine whistle, the semantics are much more friendly at least. Also note that there might be some built-in lll functions that have the same behaviour. Finally, we might completely remove .call in 0.5.0.

@axic

This comment has been minimized.

Member

axic commented Oct 16, 2017

Actually we've realised in a call with @chriseth that shouldn't we just use a large number, such as -1 (all bits set) instead of gas as that will ultimately result in the same effect?

It can also be optimised as PUSH 0 NOT and as a benefit more optimisations can happen around that piece of code where the usage of the gas opcode would prohibit it.

@vbuterin any thoughts?

@bejavu

This comment has been minimized.

bejavu commented Oct 20, 2017

Great bug research,
That answer my question:
https://ethereum.stackexchange.com/questions/28840/why-address-call-function-saves-unnecessary-gas-for-after-the-internal-execu

Is there any workaround for the time being? Maybe an older compiler version that works?

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 20, 2017

@bejavu use pragma experimental "0.5.0"; in Solidity 0.4.18.

@tal-beja

This comment has been minimized.

tal-beja commented Oct 20, 2017

MultiSigWallet.sol:3:1: SyntaxError: Unsupported experimental feature name. pragma experimental "0.5.0";

@tal-beja

This comment has been minimized.

tal-beja commented Oct 20, 2017

nvm, its supposed to be "v0.5.0"

@chriseth

This comment has been minimized.

Contributor

chriseth commented Oct 20, 2017

Ah yes, sorry!

@tal-beja

This comment has been minimized.

tal-beja commented Oct 20, 2017

Great it worked!
https://ropsten.etherscan.io/tx/0xf096c54824c852de722361c8af9464b9ecca6ce0ad7aa8fc1b82e7c52370b104

one small problem:
I couldn't verify the contract in etherscan.

yaronvel added a commit to KyberNetwork/smart-contracts that referenced this issue Jul 3, 2018

Merge pull request #260 from KyberNetwork/ilan/fixGetDecimalSafe
fix get decimal safe.
bug was detected while testing on mainnet here https://etherscan.io/tx/0x1bbf3bfaefb1b11ad71bed23ef977fbc84d5000e286ee8969f9cc6996057bca4

Root cause of the bug is compiler issue described here ethereum/solidity#2999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment