feat[codegen]!: check returndatasize even when `skip_contract_check…

…` is set (#4148)

previously, when `skip_contract_check=True` was set, the ABI decoding
routine could result in garbage memory in the case that no contract is
at the target address, based on the logic that, if we don't care about
whether a contract exists at the target, we can skip the returndatasize
check. however, it makes sense for the ABI decoder to in any case
include the returndatasize check, to prevent the garbage data case as
previously mentioned in the docs.

this is a breaking change, since previously calls with
`skip_contract_set=True` could result in garbage memory - and now they
would revert instead.

this represents a slight performance regression in that case, but it is
easier to reason about and also ensures the user always decodes valid
data.

docs/interfaces.rst

@@ -85,10 +85,6 @@ The ``default_return_value`` parameter can be used to handle ERC20 tokens affect
     extcall IERC20(USDT).transfer(msg.sender, 1, default_return_value=True) # returns True
     extcall IERC20(USDT).transfer(msg.sender, 1) # reverts because nothing returned
 
-.. warning::
-
-   When ``skip_contract_check=True`` is used and the called function returns data (ex.: ``x: uint256 = SomeContract.foo(skip_contract_check=True)``, no guarantees are provided by the compiler as to the validity of the returned value. In other words, it is undefined behavior what happens if the called contract did not exist. In particular, the returned value might point to garbage memory. It is therefore recommended to only use ``skip_contract_check=True`` to call contracts which have been manually ensured to exist at the time of the call.
-
 Built-in Interfaces
 ===================
 

tests/functional/codegen/calling_convention/test_external_contract_calls.py

@@ -1441,22 +1441,29 @@ def get_lucky(gas_amount: uint256) -> int128:
         c2.get_lucky(50)  # too little gas.
 
 
-def test_skip_contract_check(get_contract):
+def test_skip_contract_check(get_contract, tx_failed):
     contract_2 = """
 @external
 @view
 def bar():
     pass
+
+# include fallback for sanity, make sure we don't get trivially rejected in
+# selector table
+@external
+def __default__():
+    pass
     """
     contract_1 = """
 interface Bar:
     def bar() -> uint256: view
     def baz(): nonpayable
 
 @external
-def call_bar(addr: address):
-    # would fail if returndatasize check were on
-    x: uint256 = staticcall Bar(addr).bar(skip_contract_check=True)
+def call_bar(addr: address) -> uint256:
+    # fails during abi decoding
+    return staticcall Bar(addr).bar(skip_contract_check=True)
+
 @external
 def call_baz():
     # some address with no code
@@ -1466,7 +1473,10 @@ def call_baz():
     """
     c1 = get_contract(contract_1)
     c2 = get_contract(contract_2)
-    c1.call_bar(c2.address)
+
+    with tx_failed():
+        c1.call_bar(c2.address)
+
     c1.call_baz()
 
 

vyper/codegen/external_call.py

@@ -109,8 +109,7 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp
 
     # unpack strictly
     if not needs_clamp(wrapped_return_t, encoding):
-        # revert when returndatasize is not in bounds, except when
-        # skip_contract_check is enabled.
+        # revert when returndatasize is not in bounds
         # NOTE: there is an optimization here: when needs_clamp is True,
         # make_setter (implicitly) checks returndatasize during abi
         # decoding.
@@ -125,14 +124,13 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp
         # another thing we could do instead once we have the machinery is to
         # simply always use make_setter instead of having this assertion, and
         # rely on memory analyser to optimize out the memory movement.
-        if not call_kwargs.skip_contract_check:
-            assertion = IRnode.from_list(
-                ["assert", ["ge", "returndatasize", min_return_size]],
-                error_msg="returndatasize too small",
-            )
-            unpacker.append(assertion)
-        return_buf = buf
+        assertion = IRnode.from_list(
+            ["assert", ["ge", "returndatasize", min_return_size]],
+            error_msg="returndatasize too small",
+        )
+        unpacker.append(assertion)
 
+        return_buf = buf
     else:
         return_buf = context.new_internal_variable(wrapped_return_t)