From 44bb281ccaac89dc3bd66030702473c386bceae6 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Thu, 13 Jun 2024 09:22:58 -0700 Subject: [PATCH] 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 21f7172274e test case and poc contributed by @cyberthirst --------- Co-authored-by: cyberthirst --- .../builtins/codegen/test_abi_decode.py | 20 +++++++++++ vyper/abi_types.py | 25 -------------- vyper/codegen/external_call.py | 33 ++++++++++++++++--- 3 files changed, 48 insertions(+), 30 deletions(-) diff --git a/tests/functional/builtins/codegen/test_abi_decode.py b/tests/functional/builtins/codegen/test_abi_decode.py index 5773636add..9ae869c9cc 100644 --- a/tests/functional/builtins/codegen/test_abi_decode.py +++ b/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() diff --git a/vyper/abi_types.py b/vyper/abi_types.py index 24d6fe866a..a95930b16d 100644 --- a/vyper/abi_types.py +++ b/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 diff --git a/vyper/codegen/external_call.py b/vyper/codegen/external_call.py index b6ac180722..72fff5378f 100644 --- a/vyper/codegen/external_call.py +++ b/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,11 +106,35 @@ 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"] ) @@ -117,8 +142,6 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp 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: