From f54410208c4259d0c23b2683d5c90c53659a104d Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Wed, 12 Jun 2024 08:14:31 -0400 Subject: [PATCH 1/4] add back in returndatasize check it's not checked in `make_setter` when `needs_clamp` is False --- vyper/abi_types.py | 25 ------------------------- vyper/codegen/external_call.py | 13 ++++++++++++- 2 files changed, 12 insertions(+), 26 deletions(-) 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..12e38dc1ee 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 @@ -118,6 +119,16 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp b1.resolve(make_setter(return_buf, buf, hi=add_ofst(buf, payload_bound))) ) else: + # revert when returndatasize is not in bounds + # (except when return_override is provided.) + # make_setter checks returndatasize, except when needs_clamp is False. + 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 if call_kwargs.default_return_value is not None: From 7653155adbb953e978e3deb87af895b88d1a3430 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Wed, 12 Jun 2024 08:16:53 -0400 Subject: [PATCH 2/4] add a test contributed by @cyberthirst --------- Co-authored-by: cyberthirst --- .../builtins/codegen/test_abi_decode.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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() From 5b4c425eeb0ffe59454deee2a5cea99a4b8d2d9a Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Thu, 13 Jun 2024 07:03:28 -0400 Subject: [PATCH 3/4] flip a branch, add some comments --- vyper/codegen/external_call.py | 37 +++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/vyper/codegen/external_call.py b/vyper/codegen/external_call.py index 12e38dc1ee..7c671936e2 100644 --- a/vyper/codegen/external_call.py +++ b/vyper/codegen/external_call.py @@ -106,11 +106,32 @@ 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`. + 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"] ) @@ -118,18 +139,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: - # revert when returndatasize is not in bounds - # (except when return_override is provided.) - # make_setter checks returndatasize, except when needs_clamp is False. - 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 if call_kwargs.default_return_value is not None: # if returndatasize == 0: From e6cd05ea8334a9fca1929502822b4bbb304477f6 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Thu, 13 Jun 2024 12:11:00 -0400 Subject: [PATCH 4/4] update comment --- vyper/codegen/external_call.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vyper/codegen/external_call.py b/vyper/codegen/external_call.py index 7c671936e2..72fff5378f 100644 --- a/vyper/codegen/external_call.py +++ b/vyper/codegen/external_call.py @@ -119,7 +119,10 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp # 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`. + # 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]],