Skip to content

Commit 049dbdc

Browse files
Merge pull request from GHSA-j2x6-9323-fp7h
This commit addresses two issues in validating returndata, both related to the inferred type of the external call return. First, it addresses an issue with interfaces imported from JSON. The JSON_ABI encoding type was added in 0.3.0 as part of the calling convention refactor to mimic the old code's behavior when the signature of a function had `is_from_json` toggled to True. However, both implementations were a workaround for the fact that in FunctionSignatures from JSON with Bytes return types, length is set to 1 as a hack to ensure they always typecheck - almost always resulting in a runtime revert. This commit removes the JSON_ABI encoding type, so that dynamic returndata from an interface defined with .json ABI file cannot result in a buffer overrun(!). To avoid the issue with always runtime reverting, codegen uses the uses the inferred ContractFunction type of the Call.func member (which is both more accurate than the inferred type of the Call expression, and the return type on the FunctionSignature!) to calculate the length of the external Bytes array. Second, this commit addresses an issue with validating call returns in complex expressions. In the following examples, the type of the call return is either inferred incorrectly or it takes a path through codegen which avoids generating runtime clamps: ``` interface Foo: def returns_int128() -> int128: view def returns_Bytes3() -> Bytes[3]: view foo: Foo ... x: uint256 = convert(self.foo.returns_int128(), uint256) y: Bytes[32] = concat(self.foo.returns_Bytes3(), b"") ``` To address this issue, if the type of returndata needs validation, this commit decodes the returndata "strictly" into a newly allocated buffer at the time of the call, to avoid unvalidated data accidentally getting into the runtime. This does result in extra memory traffic which is a performance hit, but the performance issue can be addressed at a later date with a zero-copy buffering scheme (parent Expr allocates the buffer). Additional minor fixes and cleanup: - fix compiler panic in new_type_to_old_type for Tuples - remove `_should_decode` helper function as it duplicates `needs_clamp` - minor optimization in returndatasize check - assert ge uses one fewer instruction than assert gt.
1 parent 228b5bd commit 049dbdc

File tree

6 files changed

+214
-80
lines changed

6 files changed

+214
-80
lines changed

Diff for: tests/parser/functions/test_interfaces.py

+165-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from vyper.builtin_interfaces import ERC20, ERC721
77
from vyper.cli.utils import extract_file_interface_imports
88
from vyper.compiler import compile_code, compile_codes
9-
from vyper.exceptions import InterfaceViolation, StructureException
9+
from vyper.exceptions import ArgumentException, InterfaceViolation, StructureException
1010

1111

1212
def test_basic_extract_interface():
@@ -308,6 +308,170 @@ def test():
308308
assert erc20.balanceOf(sender) == 1000
309309

310310

311+
# test data returned from external interface gets clamped
312+
@pytest.mark.parametrize("typ", ("int128", "uint8"))
313+
def test_external_interface_int_clampers(get_contract, assert_tx_failed, typ):
314+
external_contract = f"""
315+
@external
316+
def ok() -> {typ}:
317+
return 1
318+
319+
@external
320+
def should_fail() -> int256:
321+
return -2**255 # OOB for all int/uint types with less than 256 bits
322+
"""
323+
324+
code = f"""
325+
interface BadContract:
326+
def ok() -> {typ}: view
327+
def should_fail() -> {typ}: view
328+
329+
foo: BadContract
330+
331+
@external
332+
def __init__(addr: BadContract):
333+
self.foo = addr
334+
335+
336+
@external
337+
def test_ok() -> {typ}:
338+
return self.foo.ok()
339+
340+
@external
341+
def test_fail() -> {typ}:
342+
return self.foo.should_fail()
343+
344+
@external
345+
def test_fail2() -> {typ}:
346+
x: {typ} = self.foo.should_fail()
347+
return x
348+
349+
@external
350+
def test_fail3() -> int256:
351+
return convert(self.foo.should_fail(), int256)
352+
"""
353+
354+
bad_c = get_contract(external_contract)
355+
c = get_contract(
356+
code,
357+
bad_c.address,
358+
interface_codes={"BadCode": {"type": "vyper", "code": external_contract}},
359+
)
360+
assert bad_c.ok() == 1
361+
assert bad_c.should_fail() == -(2 ** 255)
362+
363+
assert c.test_ok() == 1
364+
assert_tx_failed(lambda: c.test_fail())
365+
assert_tx_failed(lambda: c.test_fail2())
366+
assert_tx_failed(lambda: c.test_fail3())
367+
368+
369+
# test data returned from external interface gets clamped
370+
def test_external_interface_bytes_clampers(get_contract, assert_tx_failed):
371+
external_contract = """
372+
@external
373+
def ok() -> Bytes[2]:
374+
return b"12"
375+
376+
@external
377+
def should_fail() -> Bytes[3]:
378+
return b"123"
379+
"""
380+
381+
code = """
382+
interface BadContract:
383+
def ok() -> Bytes[2]: view
384+
def should_fail() -> Bytes[2]: view
385+
386+
foo: BadContract
387+
388+
@external
389+
def __init__(addr: BadContract):
390+
self.foo = addr
391+
392+
393+
@external
394+
def test_ok() -> Bytes[2]:
395+
return self.foo.ok()
396+
397+
@external
398+
def test_fail() -> Bytes[3]:
399+
return self.foo.should_fail()
400+
"""
401+
402+
bad_c = get_contract(external_contract)
403+
c = get_contract(code, bad_c.address)
404+
assert bad_c.ok() == b"12"
405+
assert bad_c.should_fail() == b"123"
406+
407+
assert c.test_ok() == b"12"
408+
assert_tx_failed(lambda: c.test_fail())
409+
410+
411+
# test data returned from external interface gets clamped
412+
def test_json_abi_bytes_clampers(get_contract, assert_tx_failed, assert_compile_failed):
413+
external_contract = """
414+
@external
415+
def returns_Bytes3() -> Bytes[3]:
416+
return b"123"
417+
"""
418+
419+
should_not_compile = """
420+
import BadJSONInterface as BadJSONInterface
421+
@external
422+
def foo(x: BadJSONInterface) -> Bytes[2]:
423+
return slice(x.returns_Bytes3(), 0, 2)
424+
"""
425+
426+
code = """
427+
import BadJSONInterface as BadJSONInterface
428+
429+
foo: BadJSONInterface
430+
431+
@external
432+
def __init__(addr: BadJSONInterface):
433+
self.foo = addr
434+
435+
436+
@external
437+
def test_fail1() -> Bytes[2]:
438+
# should compile, but raise runtime exception
439+
return self.foo.returns_Bytes3()
440+
441+
@external
442+
def test_fail2() -> Bytes[2]:
443+
# should compile, but raise runtime exception
444+
x: Bytes[2] = self.foo.returns_Bytes3()
445+
return x
446+
447+
@external
448+
def test_fail3() -> Bytes[3]:
449+
# should revert - returns_Bytes3 is inferred to have return type Bytes[2]
450+
# (because test_fail3 comes after test_fail1)
451+
return self.foo.returns_Bytes3()
452+
453+
"""
454+
455+
bad_c = get_contract(external_contract)
456+
bad_c_interface = {
457+
"BadJSONInterface": {
458+
"type": "json",
459+
"code": compile_code(external_contract, ["abi"])["abi"],
460+
}
461+
}
462+
463+
assert_compile_failed(
464+
lambda: get_contract(should_not_compile, interface_codes=bad_c_interface), ArgumentException
465+
)
466+
467+
c = get_contract(code, bad_c.address, interface_codes=bad_c_interface)
468+
assert bad_c.returns_Bytes3() == b"123"
469+
470+
assert_tx_failed(lambda: c.test_fail1())
471+
assert_tx_failed(lambda: c.test_fail2())
472+
assert_tx_failed(lambda: c.test_fail3())
473+
474+
311475
def test_units_interface(w3, get_contract):
312476
code = """
313477
import balanceof as BalanceOf

Diff for: vyper/codegen/core.py

+10-13
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,7 @@ def _dynarray_make_setter(dst, src):
123123

124124
# for ABI-encoded dynamic data, we must loop to unpack, since
125125
# the layout does not match our memory layout
126-
should_loop = (
127-
src.encoding in (Encoding.ABI, Encoding.JSON_ABI)
128-
and src.typ.subtype.abi_type.is_dynamic()
129-
)
126+
should_loop = src.encoding == Encoding.ABI and src.typ.subtype.abi_type.is_dynamic()
130127

131128
# if the subtype is dynamic, there might be a lot of
132129
# unused space inside of each element. for instance
@@ -379,7 +376,7 @@ def _get_element_ptr_tuplelike(parent, key):
379376

380377
ofst = 0 # offset from parent start
381378

382-
if parent.encoding in (Encoding.ABI, Encoding.JSON_ABI):
379+
if parent.encoding == Encoding.ABI:
383380
if parent.location == STORAGE:
384381
raise CompilerPanic("storage variables should not be abi encoded") # pragma: notest
385382

@@ -449,7 +446,7 @@ def _get_element_ptr_array(parent, key, array_bounds_check):
449446
# NOTE: there are optimization rules for this when ix or bound is literal
450447
ix = IRnode.from_list([clamp_op, ix, bound], typ=ix.typ)
451448

452-
if parent.encoding in (Encoding.ABI, Encoding.JSON_ABI):
449+
if parent.encoding == Encoding.ABI:
453450
if parent.location == STORAGE:
454451
raise CompilerPanic("storage variables should not be abi encoded") # pragma: notest
455452

@@ -703,20 +700,20 @@ def _freshname(name):
703700
# returns True if t is ABI encoded and is a type that needs any kind of
704701
# validation
705702
def needs_clamp(t, encoding):
706-
if encoding not in (Encoding.ABI, Encoding.JSON_ABI):
703+
if encoding == Encoding.VYPER:
707704
return False
705+
if encoding != Encoding.ABI:
706+
raise CompilerPanic("unreachable") # pragma: notest
708707
if isinstance(t, (ByteArrayLike, DArrayType)):
709-
if encoding == Encoding.JSON_ABI:
710-
# don't have bytestring size bound from json, don't clamp
711-
return False
712-
return True
713-
if isinstance(t, BaseType) and t.typ not in ("int256", "uint256", "bytes32"):
714708
return True
709+
if isinstance(t, BaseType):
710+
return t.typ not in ("int256", "uint256", "bytes32")
715711
if isinstance(t, SArrayType):
716712
return needs_clamp(t.subtype, encoding)
717713
if isinstance(t, TupleLike):
718714
return any(needs_clamp(m, encoding) for m in t.tuple_members())
719-
return False
715+
716+
raise CompilerPanic("unreachable") # pragma: notest
720717

721718

722719
# Create an x=y statement, where the types may be compound

Diff for: vyper/codegen/external_call.py

+33-37
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66
check_assign,
77
check_external_call,
88
dummy_node_for_type,
9-
get_element_ptr,
9+
make_setter,
10+
needs_clamp,
1011
)
1112
from vyper.codegen.ir_node import Encoding, IRnode
1213
from vyper.codegen.types import InterfaceType, TupleType, get_type_for_exact_size
14+
from vyper.codegen.types.convert import new_type_to_old_type
1315
from vyper.exceptions import StateAccessViolation, TypeCheckFailure
1416

1517

@@ -59,22 +61,19 @@ def _pack_arguments(contract_sig, args, context):
5961
return buf, mstore_method_id + [encode_args], args_ofst, args_len
6062

6163

62-
def _returndata_encoding(contract_sig):
63-
if contract_sig.is_from_json:
64-
return Encoding.JSON_ABI
65-
return Encoding.ABI
64+
def _unpack_returndata(buf, contract_sig, skip_contract_check, context, expr):
65+
# expr.func._metadata["type"].return_type is more accurate
66+
# than contract_sig.return_type in the case of JSON interfaces.
67+
ast_return_t = expr.func._metadata["type"].return_type
6668

67-
68-
def _unpack_returndata(buf, contract_sig, skip_contract_check, context):
69-
return_t = contract_sig.return_type
70-
if return_t is None:
69+
if ast_return_t is None:
7170
return ["pass"], 0, 0
7271

72+
# sanity check
73+
return_t = new_type_to_old_type(ast_return_t)
74+
check_assign(dummy_node_for_type(return_t), dummy_node_for_type(contract_sig.return_type))
75+
7376
return_t = calculate_type_for_external_return(return_t)
74-
# if the abi signature has a different type than
75-
# the vyper type, we need to wrap and unwrap the type
76-
# so that the ABI decoding works correctly
77-
should_unwrap_abi_tuple = return_t != contract_sig.return_type
7877

7978
abi_return_t = return_t.abi_type
8079

@@ -88,25 +87,30 @@ def _unpack_returndata(buf, contract_sig, skip_contract_check, context):
8887
# revert when returndatasize is not in bounds
8988
ret = []
9089
# runtime: min_return_size <= returndatasize
91-
# TODO move the -1 optimization to IR optimizer
9290
if not skip_contract_check:
93-
ret += [["assert", ["gt", "returndatasize", min_return_size - 1]]]
91+
ret += [["assert", ["ge", "returndatasize", min_return_size]]]
9492

95-
# add as the last IRnode a pointer to the return data structure
93+
encoding = Encoding.ABI
9694

97-
# the return type has been wrapped by the calling contract;
98-
# unwrap it so downstream code isn't confused.
99-
# basically this expands to buf+32 if the return type has been wrapped
100-
# in a tuple AND its ABI type is dynamic.
101-
# in most cases, this simply will evaluate to ret.
102-
# in the special case where the return type has been wrapped
103-
# in a tuple AND its ABI type is dynamic, it expands to buf+32.
104-
buf = IRnode(buf, typ=return_t, encoding=_returndata_encoding(contract_sig), location=MEMORY)
95+
buf = IRnode.from_list(
96+
buf,
97+
typ=return_t,
98+
location=MEMORY,
99+
encoding=encoding,
100+
annotation=f"{expr.node_source_code} returndata buffer",
101+
)
105102

106-
if should_unwrap_abi_tuple:
107-
buf = get_element_ptr(buf, 0, array_bounds_check=False)
103+
assert isinstance(return_t, TupleType)
104+
# unpack strictly
105+
if needs_clamp(return_t, encoding):
106+
buf2 = IRnode.from_list(
107+
context.new_internal_variable(return_t), typ=return_t, location=MEMORY
108+
)
108109

109-
ret += [buf]
110+
ret.append(make_setter(buf2, buf))
111+
ret.append(buf2)
112+
else:
113+
ret.append(buf)
110114

111115
return ret, ret_ofst, ret_len
112116

@@ -145,7 +149,7 @@ def _external_call_helper(
145149
buf, arg_packer, args_ofst, args_len = _pack_arguments(contract_sig, args_ir, context)
146150

147151
ret_unpacker, ret_ofst, ret_len = _unpack_returndata(
148-
buf, contract_sig, skip_contract_check, context
152+
buf, contract_sig, skip_contract_check, context, expr
149153
)
150154

151155
sub += arg_packer
@@ -169,15 +173,7 @@ def _external_call_helper(
169173
if contract_sig.return_type is not None:
170174
sub += ret_unpacker
171175

172-
ret = IRnode.from_list(
173-
sub,
174-
typ=contract_sig.return_type,
175-
location=MEMORY,
176-
# set the encoding to ABI here, downstream code will decode and add clampers.
177-
encoding=_returndata_encoding(contract_sig),
178-
)
179-
180-
return ret
176+
return IRnode.from_list(sub, typ=contract_sig.return_type, location=MEMORY)
181177

182178

183179
def _get_special_kwargs(stmt_expr, context):

0 commit comments

Comments
 (0)