fix[codegen]: add back in returndatasize check (#4144)

add back in `returndatasize` check for external calls in the case
that `make_setter()` is not called (i.e. when `needs_clamp()` is
`True`). the check was removed (i.e. there was a regression) in
21f7172

test case and poc contributed by @cyberthirst

---------

Co-authored-by: cyberthirst <[email protected]>

tests/functional/builtins/codegen/test_abi_decode.py

@@ -1421,3 +1421,23 @@ def foo(a:Bytes[1000]):
 
     with tx_failed():
         c.foo(_abi_payload_from_tuple(payload))
+
+
+# returndatasize check for uint256
+def test_returndatasize_check(get_contract, tx_failed):
+    code = """
+@external
+def bar():
+    pass
+
+interface A:
+    def bar() -> uint256: nonpayable
+
+@external
+def run() -> uint256:
+    return extcall A(self).bar()
+    """
+    c = get_contract(code)
+
+    with tx_failed():
+        c.run()

vyper/abi_types.py

@@ -24,11 +24,6 @@ def embedded_dynamic_size_bound(self):
             return 0
         return self.size_bound()
 
-    def embedded_min_dynamic_size(self):
-        if not self.is_dynamic():
-            return 0
-        return self.min_size()
-
     # size (in bytes) of the static section
     def static_size(self):
         raise NotImplementedError("ABIType.static_size")
@@ -42,14 +37,6 @@ def dynamic_size_bound(self):
     def size_bound(self):
         return self.static_size() + self.dynamic_size_bound()
 
-    def min_size(self):
-        return self.static_size() + self.min_dynamic_size()
-
-    def min_dynamic_size(self):
-        if not self.is_dynamic():
-            return 0
-        raise NotImplementedError("ABIType.min_dynamic_size")
-
     # The canonical name of the type for calculating the function selector
     def selector_name(self):
         raise NotImplementedError("ABIType.selector_name")
@@ -158,9 +145,6 @@ def static_size(self):
     def dynamic_size_bound(self):
         return self.m_elems * self.subtyp.embedded_dynamic_size_bound()
 
-    def min_dynamic_size(self):
-        return self.m_elems * self.subtyp.embedded_min_dynamic_size()
-
     def selector_name(self):
         return f"{self.subtyp.selector_name()}[{self.m_elems}]"
 
@@ -187,9 +171,6 @@ def dynamic_size_bound(self):
         # length word + data
         return 32 + ceil32(self.bytes_bound)
 
-    def min_dynamic_size(self):
-        return 32
-
     def selector_name(self):
         return "bytes"
 
@@ -222,9 +203,6 @@ def dynamic_size_bound(self):
         # length + size of embedded children
         return 32 + subtyp_size * self.elems_bound
 
-    def min_dynamic_size(self):
-        return 32
-
     def selector_name(self):
         return f"{self.subtyp.selector_name()}[]"
 
@@ -245,9 +223,6 @@ def static_size(self):
     def dynamic_size_bound(self):
         return sum([t.embedded_dynamic_size_bound() for t in self.subtyps])
 
-    def min_dynamic_size(self):
-        return sum([t.embedded_min_dynamic_size() for t in self.subtyps])
-
     def is_complex_type(self):
         return True
 

vyper/codegen/external_call.py

@@ -86,8 +86,9 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp
 
     abi_return_t = wrapped_return_t.abi_type
 
+    min_return_size = abi_return_t.static_size()
     max_return_size = abi_return_t.size_bound()
-    assert 0 <= max_return_size
+    assert 0 < min_return_size <= max_return_size
 
     ret_ofst = buf
     ret_len = max_return_size
@@ -105,20 +106,42 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp
     assert isinstance(wrapped_return_t, TupleT)
 
     # unpack strictly
-    if needs_clamp(wrapped_return_t, encoding):
+    if not needs_clamp(wrapped_return_t, encoding):
+        # revert when returndatasize is not in bounds, except when
+        # skip_contract_check is enabled.
+        # NOTE: there is an optimization here: when needs_clamp is True,
+        # make_setter (implicitly) checks returndatasize during abi
+        # decoding.
+        # since make_setter is not called in this branch, we need to check
+        # returndatasize here, but we avoid a redundant check by only doing
+        # the returndatasize check inside of this branch (and not in the
+        # `needs_clamp==True` branch).
+        # in the future, this check could be moved outside of the branch, and
+        # instead rely on the optimizer to optimize out the redundant check,
+        # it would need the optimizer to do algebraic reductions (along the
+        # lines of `a>b and b>c and a>c` reduced to `a>b and b>c`).
+        # 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
+
+    else:
         return_buf = context.new_internal_variable(wrapped_return_t)
 
         # note: make_setter does ABI decoding and clamps
-
         payload_bound = IRnode.from_list(
             ["select", ["lt", ret_len, "returndatasize"], ret_len, "returndatasize"]
         )
         with payload_bound.cache_when_complex("payload_bound") as (b1, payload_bound):
             unpacker.append(
                 b1.resolve(make_setter(return_buf, buf, hi=add_ofst(buf, payload_bound)))
             )
-    else:
-        return_buf = buf
 
     if call_kwargs.default_return_value is not None:
         # if returndatasize == 0: