Deferred Evaluation Of Default_Return_Value

[cyberthirst]

credits: @anatomist (Whitehat) for Attackathon | Ethereum Protocol
reportid: #38169

Brief/Intro

Calls to external contracts has four additional kwargs that allow user to tune the behavior of the call. Within those kwargs, default_return_value specifies what to use as return value when the callee contract does not return anything. However, deferred evaluation of the expression for default_return_value may result in it being executed at the wrong time, or worse, skipped altogether along with any side effects it has.
Vulnerability Details

The _unpack_returndata is responsible for decoding data returned from an external call. It also injects the default_return_value as return data if the returndatasize == 0

def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, expr):
    ...
    if call_kwargs.default_return_value is not None:
        ...

        override_value = wrap_value_for_external_return(call_kwargs.default_return_value)
        stomp_return_buffer = ["seq"]
        if not call_kwargs.skip_contract_check:
            stomp_return_buffer.append(_extcodesize_check(contract_address))
        stomp_return_buffer.append(make_setter(return_buf, override_value))
        unpacker = ["if", ["eq", "returndatasize", 0], stomp_return_buffer, unpacker]       #use default_return_value if returndatasize == 0

    unpacker = ["seq", unpacker, return_buf]

    return unpacker, ret_ofst, ret_len

However, since default_return_value is not cached with cache_when_complex prior to the external call, the evaluation is deferred until expression value is actually used. This creates an interesting scenario, if the expr for default_return_value contains side effects, it might be skipped. For example, in the PoC below, get_default_id only gets called if the external call doesn't return any data, and the side effect of increasing external_call_counter might be skipped.

interface FooBar:
	def test() -> uint256: payable

external_call_counter: uint256

@deploy
def __init__(ext: FooBar):
	res: uint256 = extcall ext.test(default_return_value = self.get_default_id())   #get_default_id might not run at all

def get_default_id() -> uint256:
    counter: uint256 = self.external_call_counter
	self.external_call_counter += 1
	return counter

This implementation is confusing from language users' point of view, since it is reasonable to expect all arguments of a function to be evaluated prior to the function call itself. We believe the "correct" language design would be to cache the expr of default_return_value, so it gets executed before the external call, regardless of the returned result.

Additionally, we also noticed another small "mistake" in the default value assignment code, where the following "legal" code fails to compile.

interface FooBar:
    def test() -> (uint256, uint256): payable

@deploy
def __init__(ext: FooBar):
    x: uint256 = 2
    a: (uint256, uint256) = (x, x)
    b: (uint256, uint256) = extcall ext.test(default_return_value = a)  #fails to compile

This is due to make_setter not able to assign values from VYPER encoding pointers to ABI encoding pointers

def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, expr):
    ...
    encoding = Encoding.ABI
    ...
    buf.encoding = encoding
    ...
    if not needs_clamp(wrapped_return_t, encoding):
        ...
        return_buf = buf
    else:
        ...

    if call_kwargs.default_return_value is not None:
        ...
        override_value = wrap_value_for_external_return(call_kwargs.default_return_value)
        ...
        stomp_return_buffer.append(make_setter(return_buf, override_value))         #assign from VYPER encoding pointers to ABI encoding pointers

def make_setter(left, right, hi=None):
    ...
    with right.cache_when_complex("c_right") as (b1, right):
        ret = ["seq"]
        if hi is not None:
            item_end = add_ofst(right, right.typ.abi_type.static_size())
            len_check = ["assert", ["le", item_end, hi]]
            ret.append(len_check)

        ret.append(_complex_make_setter(left, right, hi=hi))
        return b1.resolve(IRnode.from_list(ret))


def _complex_make_setter(left, right, hi=None):
    ...
    if left.is_pointer and right.is_pointer and right.encoding == Encoding.VYPER:
        # both left and right are pointers, see if we want to batch copy
        # instead of unrolling the loop.
        assert left.encoding == Encoding.VYPER
        ...

[cyberthirst]

Lazy evaluation of the default_return_value is a deliberate design decision. It's similar to assert condition, expensive_reason_string() from Python. One use case of the current implementation is extcall foo.bar(..., default_return_value=baz.backup_value()) - ie we want to use the evm context after the extcall had been made.

The "fix" is just to properly define how default_return_value behaves.

[cyberthirst]

Note this crash:

Additionally, we also noticed another small "mistake" in the default value assignment code, where the following "legal" code fails to compile.

interface FooBar:
    def test() -> (uint256, uint256): payable

@deploy
def __init__(ext: FooBar):
    x: uint256 = 2
    a: (uint256, uint256) = (x, x)
    b: (uint256, uint256) = extcall ext.test(default_return_value = a)  #fails to compile