Skip to content
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

Stack underflow in unreachable ERC20.transfer #1511

Closed
dHonerkamp opened this issue Jul 2, 2019 · 13 comments · Fixed by #1665
Labels
bug

Comments

@dHonerkamp
Copy link

@dHonerkamp dHonerkamp commented Jul 2, 2019

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)
@jacqueswww jacqueswww added the bug label Jul 6, 2019
@dHonerkamp

This comment has been minimized.

Copy link
Author

@dHonerkamp dHonerkamp commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Author

@dHonerkamp dHonerkamp commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Contributor

@brockelmore brockelmore commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor

@smarx smarx commented Sep 12, 2019

Perhaps a simpler repro:

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

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 12, 2019

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:

nonreentrant_post = [['pass']]

@smarx

This comment has been minimized.

Copy link
Contributor

@smarx smarx commented Sep 14, 2019

@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:

self.valency = self.args[1].valency

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

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 14, 2019

@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

This comment has been minimized.

Copy link
Contributor

@smarx smarx commented Sep 14, 2019

I left out some of the debugging, sorry.

This appears to be where the extra POP comes from:

o.append('POP')

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

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 14, 2019

@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

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 14, 2019

(related: #1468 (comment))

@smarx

This comment has been minimized.

Copy link
Contributor

@smarx smarx commented Sep 14, 2019

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

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 16, 2019

also related: #1154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.