Stack underflow in unreachable ERC20.transfer

[dHonerkamp]

Version Information

• vyper Version (output of vyper --version): 0.1.0b10
• OS: osx mojave 10.14.5
• Python Version (output of python --version): 3.7.3
• Environment (output of pip freeze):

appnope==0.1.0
asteval==0.9.14
attrs==19.1.0
backcall==0.1.0
beautifulsoup4==4.7.1
bleach==3.1.0
bs4==0.0.1
cachetools==3.1.1
certifi==2019.3.9
chardet==3.0.4
cycler==0.10.0
decorator==4.4.0
defusedxml==0.6.0
entrypoints==0.3
google-api-core==1.11.1
google-auth==1.6.3
google-cloud-bigquery==1.13.0
google-cloud-core==1.0.1
google-cloud-storage==1.16.1
google-resumable-media==0.3.2
googleapis-common-protos==1.6.0
idna==2.8
ipykernel==5.1.1
ipython==7.5.0
ipython-genutils==0.2.0
ipywidgets==7.4.2
jedi==0.13.3
Jinja2==2.10.1
joblib==0.13.2
jsonschema==3.0.1
jupyter==1.0.0
jupyter-client==5.2.4
jupyter-console==6.0.0
jupyter-core==4.4.0
kiwisolver==1.1.0
lmfit==0.9.13
MarkupSafe==1.1.1
matplotlib==3.1.0
mistune==0.8.4
nbconvert==5.5.0
nbformat==4.4.0
noisyopt==0.2.2
notebook==5.7.8
numpy==1.16.4
pandas==0.24.2
pandocfilters==1.4.2
parso==0.4.0
pexpect==4.7.0
pickleshare==0.7.5
prometheus-client==0.6.0
prompt-toolkit==2.0.9
protobuf==3.8.0
ptyprocess==0.6.0
pyasn1==0.4.5
pyasn1-modules==0.2.5
pycryptodome==3.8.2
Pygments==2.4.2
pyparsing==2.4.0
PyPortfolioOpt==0.3.1
pyrsistent==0.15.2
python-dateutil==2.8.0
pytz==2019.1
pyzmq==18.0.1
qtconsole==4.5.1
requests==2.22.0
rsa==4.0
scikit-learn==0.21.2
scipy==1.3.0
seaborn==0.9.0
selenium==4.0.0a1
Send2Trash==1.5.0
six==1.12.0
sklearn==0.0
sortedcontainers==2.1.0
soupsieve==1.9.1
terminado==0.8.2
testpath==0.4.2
tornado==6.0.2
traitlets==4.3.2
uncertainties==3.1.1
urllib3==1.25.3
vyper==0.1.0b10
wcwidth==0.1.7
webencodings==0.5.1
widgetsnbextension==3.4.2

What's your issue about?

Transfer to an ERC20 token returns stack underflow if it is in an if clause that never gets executed, but works without the if clause.

Contract ERC20DummyToken.sol:

pragma solidity ^0.5.0;

import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol";
import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol";

contract ERC20DumyToken is ERC20, ERC20Detailed {
    constructor(uint256 initialSupply) ERC20Detailed("Dummy", "DUM", 18) public {
        _mint(msg.sender, initialSupply);
    }
}

Contract vyperIssue.vy:

from vyper.interfaces import ERC20

deposits: public(map(address, uint256))
token: ERC20

@public
def __init__(_ERC20Address: address):
    self.token = ERC20(_ERC20Address)

@public
def bid(_amount: uint256):
    if False:
        self.token.transfer(msg.sender, _amount)

test.js:

const ERC20DummyToken = artifacts.require("ERC20DummyToken");
const auction = artifacts.require("vyperIssue");

contract("auction", async accounts => {
    it("do stuff", async () => {
        let _initialSupply = 100000
        let token = await ERC20DummyToken.new(_initialSupply)
        let instance = await auction.new(token.address);

        await token.transfer(instance.address, 10000)
        await instance.bid(500, {from: accounts[1]})
    });
});

Returns:

     Error: Returned error: VM Exception while processing transaction: stack underflow
      at Object.ErrorResponse (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/web3-core-requestmanager/~/web3-core-helpers/src/errors.js:29:1)
      at /Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/web3-core-requestmanager/src/index.js:140:1
      at /Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/packages/truffle-provider/wrapper.js:112:1
      at XMLHttpRequest.request.onreadystatechange (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/web3-providers-http/src/index.js:96:1)
      at XMLHttpRequestEventTarget.dispatchEvent (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request-event-target.js:34:1)
      at XMLHttpRequest._setReadyState (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:208:1)
      at XMLHttpRequest._onHttpResponseEnd (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:318:1)
      at IncomingMessage.<anonymous> (/Users/danielhonerkamp/.npm-packages/lib/node_modules/truffle/build/webpack:/~/xhr2-cookies/dist/xml-http-request.js:289:47)
      at endReadableNT (_stream_readable.js:1129:12)
      at process._tickCallback (internal/process/next_tick.js:63:19)

The test succeeds if I remove the if clause:

@public
def bid(_amount: uint256):
    self.token.transfer(msg.sender, _amount)

[dHonerkamp]

More generally, this seems to be an issue whenever I have something in the form of

if condition:
        self.token.transfer(msg.sender, _amount)

And the condition evaluates to False.

[fubuloubu]

More simply, it looks like it might be:

if condition:
    external_call()
# else:
    # nothing (stack underflow in this case)

Can anyone confirm this with a MWE that's not a token?

[dHonerkamp]

That seems to be correct. I've run the following example

from vyper.interfaces import ERC20

contract Example:
    def doStuff() -> bool: modifying

deposits: public(map(address, uint256))
token: ERC20
example: Example

@public
def __init__(_ERC20Address: address, _exampleAddress: address):
    self.token = ERC20(_ERC20Address)
    self.example = Example(_exampleAddress)

@public
def tokenTest(condition: bool, _amount: uint256):
    if condition:
        self.token.transfer(msg.sender, _amount)

@public
def exampleTest(condition: bool):
    if condition:
        self.example.doStuff()

with this Example.vy:

owner: address

@public
def __init__():
    self.owner = msg.sender

@public
def doStuff() -> bool:
    return True

And as expected, calls to tokenTest() and exampleTest() fail whenever I pass condition as false

[brockelmore]

Note on this - if the external call only transfers ETH, i.e.:

if condition:
    interface(addr).func1(value=msg.value)

I get no error. But, if using erc20:

if condition:
    interface(addr).func1(value=msg.value)
else:
    interface(addr).func1(SomeUint256)

and the else clause fires, I get stack underflow

[smarx]

Perhaps a simpler repro:

@public
@payable
def doit():
    if False:
        raw_call(msg.sender, b"", outsize=0, value=0, gas=msg.gas)

[charles-cooper]

the if statement translates to

JUMPI 
PUSH1 0 
ISZERO 
_sym_4
and _sym_4 is

_sym_4 
JUMPDEST 
POP  # <--  this doesn't seem right ..
STOP

this seems to be the source of that POP:
https://github.com/ethereum/vyper/blob/faacf1503e02b7af935a6dcf8eda4ec697ed2101/vyper/parser/function_definitions/utils.py#L47

[smarx]

@charles-cooper Why do you think the issue relates to nonreentrant_post? That doesn't seem to have any effect.

I think the issue has to do with valency. The external call has a valency of 1 because it pushes the memory location of the output to the stack.

So:

if False:
    # here we leave something on the stack
else:  # implicit
    # here we don't

This line means we treat the entire if as having valency 1:

https://github.com/ethereum/vyper/blob/4ab9ec18216d52df4a0d16b84ee767cc58cbde6b/vyper/parser/lll_node.py#L145

I don't have much experience with the Vyper compiler, so it's hard for me to suggest or implement a fix. Intuitively, I think if statements should always have valency 0, and each branch must do its own cleanup (e.g. each branch should POP n times at the end to get its valency down to 0).

[charles-cooper]

@smarx nice digging, that looks like you are on the right track. I don't have much time to look into this today but you could also take a look at vyper/compile_lll.py, there is a section which compiles if statements.

[smarx]

I left out some of the debugging, sorry.

This appears to be where the extra POP comes from:

https://github.com/ethereum/vyper/blob/4ab9ec18216d52df4a0d16b84ee767cc58cbde6b/vyper/compile_lll.py#L245

It's there because the valency of the sequence is 1 because the valency of the if statement is 1 as explained above.

I was testing this Vyper:

@public
@payable
def doit():
    if False:
        raw_call(msg.sender, b"", outsize=0, value=0, gas=msg.gas)

which compiles to this IR:

[seq,
  [return,
    0,
    [lll,
      [seq,
        [mstore, 28, [calldataload, 0]],
        [mstore, 32, 1461501637330902918203684832716283019655932542976],
        [mstore, 64, 170141183460469231731687303715884105727],
        [mstore, 96, -170141183460469231731687303715884105728],
        [mstore, 128, 1701411834604692317316873037158841057270000000000],
        [mstore, 160, -1701411834604692317316873037158841057280000000000],
        # Line 1
        [if,
          [eq, [mload, 0], '1297313763' <doit()>],
          [seq,
            # Line 4
            [if,
              0,
              [seq,
                # Line 5
                /* Memory copy */
                [with,
                  _source,
                  /* Create bytes[0]: b'' */ [seq, [mstore, 320, 0], 320],
                  [with,
                    _sz,
                    [add, 32, [mload, _source]],
                    [assert, [call, [add, 18, [div, _sz, 10]], 4, 0, _source, _sz, 384, _sz]]]],
                [assert, [call, gas, caller, 0, 416, [mload, 384], 480, 0]],
                [mstore, 448, 0],
                448]],
            # Line 1
            stop]],
        /* Default function */ [revert, 0, 0]],
      0]]]

[charles-cooper]

@smarx I think your approach is good. If you wanted, I think (not at a keyboard so this is off the top of my head) you could change the valency of LLL if to 0, and then where user if statements are compiled in stmt.pt, wrap both branches in a seq to ensure they are zerovalent.

[charles-cooper]

(related: #1468 (comment))

[smarx]

I believe both branches are already wrapped in a seq, but the valency of a seq is not always 0.

[charles-cooper]

also related: #1154