fix: add extcodesize check when default_return_value is activated (#…

…2864)

in the branch where returndatasize == 0, add an extcodesize check. note
that the extcodesize check is still not foolproof in the presence of
selfdestructs

tests/parser/features/external_contracts/test_external_contract_calls.py

@@ -2333,11 +2333,24 @@ def transfer(receiver: address, amount: uint256):
     pass
     """
 
+    negative_transfer_code = """
+@external
+def transfer(receiver: address, amount: uint256) -> bool:
+    return False
+    """
+
+    self_destructing_code = """
+@external
+def transfer(receiver: address, amount: uint256):
+    selfdestruct(msg.sender)
+    """
+
     code = """
 from vyper.interfaces import ERC20
 @external
-def safeTransfer(erc20: ERC20, receiver: address, amount: uint256):
+def safeTransfer(erc20: ERC20, receiver: address, amount: uint256) -> uint256:
     assert erc20.transfer(receiver, amount, default_return_value=True)
+    return 7
 
 @external
 def transferBorked(erc20: ERC20, receiver: address, amount: uint256):
@@ -2349,7 +2362,22 @@ def transferBorked(erc20: ERC20, receiver: address, amount: uint256):
     # demonstrate transfer failing
     assert_tx_failed(lambda: c.transferBorked(bad_erc20.address, c.address, 0))
     # would fail without default_return_value
-    c.safeTransfer(bad_erc20.address, c.address, 0)
+    assert c.safeTransfer(bad_erc20.address, c.address, 0) == 7
+
+    # check that `default_return_value` does not stomp valid returndata.
+    negative_contract = get_contract(negative_transfer_code)
+    assert_tx_failed(lambda: c.safeTransfer(negative_contract.address, c.address, 0))
+
+    # default_return_value should fail on EOAs (addresses with no code)
+    random_address = "0x0000000000000000000000000000000000001234"
+    assert_tx_failed(lambda: c.safeTransfer(random_address, c.address, 1))
+
+    # in this case, the extcodesize check runs after the token contract
+    # selfdestructs. however, extcodesize still returns nonzero until
+    # later (i.e., after this transaction), so we still pass
+    # the extcodesize check.
+    self_destructing_contract = get_contract(self_destructing_code)
+    assert c.safeTransfer(self_destructing_contract.address, c.address, 0) == 7
 
 
 def test_default_override2(get_contract, assert_tx_failed):

vyper/codegen/external_call.py

@@ -75,7 +75,7 @@ def _pack_arguments(fn_type, args, context):
     return buf, pack_args, args_ofst, args_len
 
 
-def _unpack_returndata(buf, fn_type, call_kwargs, context, expr):
+def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, expr):
     ast_return_t = fn_type.return_type
 
     if ast_return_t is None:
@@ -130,7 +130,10 @@ def _unpack_returndata(buf, fn_type, call_kwargs, context, expr):
         #    do the other stuff
 
         override_value = wrap_value_for_external_return(call_kwargs.default_return_value)
-        stomp_return_buffer = make_setter(return_buf, override_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]
 
     unpacker = ["seq", unpacker, return_buf]
@@ -161,6 +164,10 @@ def _bool(x):
     return ret
 
 
+def _extcodesize_check(address):
+    return ["assert", ["extcodesize", address]]
+
+
 def ir_for_external_call(call_expr, context):
     from vyper.codegen.expr import Expr  # TODO rethink this circular import
 
@@ -182,7 +189,7 @@ def ir_for_external_call(call_expr, context):
     buf, arg_packer, args_ofst, args_len = _pack_arguments(fn_type, args_ir, context)
 
     ret_unpacker, ret_ofst, ret_len = _unpack_returndata(
-        buf, fn_type, call_kwargs, context, call_expr
+        buf, fn_type, call_kwargs, contract_address, context, call_expr
     )
 
     ret += arg_packer
@@ -194,7 +201,7 @@ def ir_for_external_call(call_expr, context):
         # when we _do_ expect return data because we later check
         # `returndatasize` (that check works even if the contract
         # selfdestructs).
-        ret.append(["assert", ["extcodesize", contract_address]])
+        ret.append(_extcodesize_check(contract_address))
 
     gas = call_kwargs.gas
     value = call_kwargs.value