Bug: can't assert result of external calls to modifiable functions

[haydenadams]

Version Information

• vyper Version (output of vyper --version): 0.1.0b10

What's your issue about?

Upgrading Uniswap from vyper 0.1.0b4 to 0.1.0b10 and noticed my tests were failing on asserting external contract calls that return bools.

This fails (reverts):

assert self.token.transferFrom(msg.sender, self, token_amount)

This succeeds:

success: bool = self.token.transferFrom(msg.sender, self, token_amount)
assert success

The interface was defined as:

contract Token():
    def transferFrom(_from : address, _to : address, _value : uint256) -> bool: modifying

[fubuloubu]

Doesn't seem to be call data causing the issue:

>>> import vyper
>>> c_interface = vyper.compile_code("""
contract Foo:
    def baz() -> bool: modifying

foo: Foo

@public
def __init__(_foo: address):
    self.foo = Foo(_foo)

@public
def call_foo():
    assert self.foo.baz()
""", output_formats=['abi', 'bytecode'])

>>> foo_interface = vyper.compile_code("""
@public
def baz() -> bool:
    return True
""", output_formats=['abi', 'bytecode'])

>>> from web3 import Web3, EthereumTesterProvider
>>> w3 = Web3(EthereumTesterProvider())
>>> txn_hash = w3.eth.contract(**foo_interface).constructor().transact()
>>> address = w3.eth.waitForTransactionReceipt(txn_hash)['contractAddress']
>>> foo = w3.eth.contract(address, **foo_interface)

>>> txn_hash = w3.eth.contract(**c_interface).constructor(foo.address).transact()
>>> address = w3.eth.waitForTransactionReceipt(txn_hash)['contractAddress']
>>> contract = w3.eth.contract(address, **c_interface)

>>> contract.functions.call_foo().transact()
HexBytes('0x13583b7cb86379c320c2358df34a144e2e9f028aa1c8980af1ae14461f38deac')

[jacqueswww]

Same bug as #1377

[charles-cooper]

I inspected the IR of both ways, and assert <external call> uses staticcall, then I remembered #1150. So it is correct in using static call but incorrect in not weeding out the program.

[charles-cooper]

It's not just having to do with the semantics of assert, the compiler does not catch calls to modifying functions in @constant functions, either. So instead of compile failure, get a runtime failure when the modifying function gets called within staticcall context. repro:

contract Token:
    def transferFrom(_from : address, _to : address, _value : uint256) -> bool: modifying

token: public(address)

@public
@constant
def passes() :
    success: bool = Token(self.token).transferFrom(msg.sender, self, 37)
    assert success

[fubuloubu]

Good find!

[haydenadams]

Just wanna add that forcing all ERC20 transfers into two lines (catch return and assert) is:

1. much uglier
2. more likely to lead to people forgetting to assert their transfers

I'm hoping there can be a more elegant solution the issues with assert-ing functions that can modify state.

[charles-cooper]

That's true but the ERC20 standard having a return code is pretty much a historical artifact of not having REVERT at the time. Nowadays everybody just REVERTs if the transfer preconditions are not met.

[haydenadams]

Not asserting is not a solution here at all.

• There are popular ERC20 contracts live today that do not REVERT
• It's not part of the ERC20 standard that they have to revert
• Any auditor ever will tell you to add asserts to ERC20 calls

Historically Vyper has tried to produce clean, easily audit-able code that is as difficult to mess up as possible. This seems like a bit of a departure. Imo, if there are alternate solutions such as only preventing modifying state on asserting internal function calls its worth exploring.

[charles-cooper]

So I see a couple possibilities here

1. Roll back VIP1150
2. Go with ret: bool = external_call(..); assert ret and just steer users towards this usage
3. Require users to put the result of function calls in a variable, that way they are less likely to forget

I don't think that rolling back VIP1150 in part as suggested will work.

... such as only preventing modifying state on asserting internal function calls ...

This is a semantic inconsistency which can lead to even subtler bugs. For instance, assert other.external_call where external_call calls a modifying function on self.

[fubuloubu]

I think requiring the handling of return values will assuage your concern. As a sort of opt-out behavior, we can just have a blank assignment if you really don't wait it, i.e. _ = Foo(addr).ext_call()

If the developer had a return call, they had it for a reason and you should probably handle it.

[haydenadams]

If the developer had a return call, they had it for a reason and you should probably handle it.

The most common use case in Ethereum is ERC20 token transfers, and asserting transfer and transferFrom returned true is the standard and safe way of interacting with them.

[fubuloubu]

Yes, so forcing developers to handle returns by setting to a local variable would solve the problem. The compiler could optimize it out if it's determined not to be necessary (but the opposite isn't true)

[haydenadams]

yes but having to set a local variable is still significantly uglier and more verbose than just assert token.transfer(address, amount) imo

[fubuloubu]

I don't disagree, but we have to balance with the issues we found from allowing state-modifying functions in assert statements e.g. #1150.

This represents a compromise that, while a little annoying and uglier, improves security and also ensures devs are using other projects more effectively. It would actually be harder to misuse ERC20 in this case!

---

We should turn this into a VIP if this is the path we want to take to solve this problem.

[haydenadams]

Sure, but #1150 seems like a very rare/minor edge case which is still not that confusing (although I agree not ideal) whereas external token transfers are the most common thing on Ethereum.

[fubuloubu]

success: bool = token.transfer(...)
assert success

seems clearer, more understandable, and in line with the values of readability than:

assert token.transfer(...)

---

Also forcing the handling of return statements makes this clearer, as the compiler wouldn't let you compile the following:

token.transfer(...) # unhandled return value!

[charles-cooper]

yes but having to set a local variable is still significantly uglier and more verbose than just assert token.transfer(address, amount) imo

If it's about aesthetics, the developer is entitled to write a beautifying function,

def beautifulTransfer(tok, to, amount) : 
    ok: bool = tok.transfer(to, amount)
    assert ok

Or more generically,

def assert_allow_modify(expr: bool, fail_message: bytes32 = "") : 
    ok: bool = expr
    assert ok, fail_message

And if there's demand, we could even put assert_allow_modify in the standard library or make it a language keyword. Or as mentioned above, we could roll back #1150. But I think correctness is much more important than subjective assessments of aesthetics. As @fubuloubu pointed out, forcing the developer to handle return values is actually a more direct and robust solution to 'developers sometimes forgetting to handle the return value' than making it easy to handle it on one line.

[jakerockland]

Adding my 2 wei, I very much like requiring functions that return a value to have that value assigned to a parameter and throwing a warning at the very least if that value isn't used. As @fubuloubu mentioned above, I'm also I'm a big fan of blank assignment to make explicit where something is not used _ = ...