Skip to content

Commit

Permalink
fix: _abi_decode() validation (#3626)
Browse files Browse the repository at this point in the history
`_abi_decode()` does not validate input when it is nested in certain
expressions. the following example gets correctly validated (bounds
checked):

```vyper
    x: uint8 = _abi_decode(slice(msg.data, 4, 32), uint8)
```

however, the following example is not bounds checked:

```vyper
@external
def abi_decode(x: uint256) -> uint256:
    a: uint256 = convert(
        _abi_decode(
            slice(msg.data, 4, 32),
            (uint8)
        ),
        uint256
    )
    return a  # abi_decode(256) returns: 256
```

the issue is caused because the `ABIDecode()` builtin tags its output
with `encoding=Encoding.ABI`, but this does not result in validation
until that itself is passed to `make_setter` (which is called for
instance when generating an assignment or return statement).

the issue can be triggered by constructing an example where the output
of `ABIDecode()` is not internally passed to `make_setter` or other
input validating routine.

this commit fixes the issue by calling `make_setter` in `ABIDecode()`
before returning the output buffer, which causes validation to be
performed. note that this causes a performance regression in the common
(and majority of) cases where `make_setter` is immediately called on the
result of `ABIDecode()` because a redundant memory copy ends up being
generated (like in the aforementioned examples: in a plain assignment or
return statement). however, fixing this performance regression is left
to future work in the optimizer.
  • Loading branch information
charles-cooper committed Sep 26, 2023
1 parent e5c323a commit d438d92
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 28 deletions.
28 changes: 28 additions & 0 deletions tests/parser/functions/test_abi_decode.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,34 @@ def abi_decode(x: Bytes[96]) -> (uint256, uint256):
assert_tx_failed(lambda: c.abi_decode(input_))


def test_clamper_nested_uint8(get_contract, assert_tx_failed):
# check that _abi_decode clamps on word-types even when it is in a nested expression
# decode -> validate uint8 -> revert if input >= 256 -> cast back to uint256
contract = """
@external
def abi_decode(x: uint256) -> uint256:
a: uint256 = convert(_abi_decode(slice(msg.data, 4, 32), (uint8)), uint256)
return a
"""
c = get_contract(contract)
assert c.abi_decode(255) == 255
assert_tx_failed(lambda: c.abi_decode(256))


def test_clamper_nested_bytes(get_contract, assert_tx_failed):
# check that _abi_decode clamps dynamic even when it is in a nested expression
# decode -> validate Bytes[20] -> revert if len(input) > 20 -> convert back to -> add 1
contract = """
@external
def abi_decode(x: Bytes[96]) -> Bytes[21]:
a: Bytes[21] = concat(b"a", _abi_decode(x, Bytes[20]))
return a
"""
c = get_contract(contract)
assert c.abi_decode(abi.encode("(bytes)", (b"bc",))) == b"abc"
assert_tx_failed(lambda: c.abi_decode(abi.encode("(bytes)", (b"a" * 22,))))


@pytest.mark.parametrize(
"output_typ,input_",
[
Expand Down
52 changes: 24 additions & 28 deletions vyper/builtins/functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
get_type_for_exact_size,
ir_tuple_from_args,
make_setter,
needs_external_call_wrap,
promote_signed_int,
sar,
shl,
Expand Down Expand Up @@ -2367,8 +2366,6 @@ def build_IR(self, expr, args, kwargs, context):
class ABIEncode(BuiltinFunction):
_id = "_abi_encode" # TODO prettier to rename this to abi.encode
# signature: *, ensure_tuple=<literal_bool> -> Bytes[<calculated len>]
# (check the signature manually since we have no utility methods
# to handle varargs.)
# explanation of ensure_tuple:
# default is to force even a single value into a tuple,
# e.g. _abi_encode(bytes) -> _abi_encode((bytes,))
Expand Down Expand Up @@ -2529,24 +2526,11 @@ def build_IR(self, expr, args, kwargs, context):
)

data = ensure_in_memory(data, context)

with data.cache_when_complex("to_decode") as (b1, data):
data_ptr = bytes_data_ptr(data)
data_len = get_bytearray_length(data)

# Normally, ABI-encoded data assumes the argument is a tuple
# (See comments for `wrap_value_for_external_return`)
# However, we do not want to use `wrap_value_for_external_return`
# technique as used in external call codegen because in order to be
# type-safe we would need an extra memory copy. To avoid a copy,
# we manually add the ABI-dynamic offset so that it is
# re-interpreted in-place.
if (
unwrap_tuple is True
and needs_external_call_wrap(output_typ)
and output_typ.abi_type.is_dynamic()
):
data_ptr = add_ofst(data_ptr, 32)

ret = ["seq"]

if abi_min_size == abi_size_bound:
Expand All @@ -2555,18 +2539,30 @@ def build_IR(self, expr, args, kwargs, context):
# runtime assert: abi_min_size <= data_len <= abi_size_bound
ret.append(clamp2(abi_min_size, data_len, abi_size_bound, signed=False))

# return pointer to the buffer
ret.append(data_ptr)

return b1.resolve(
IRnode.from_list(
ret,
typ=output_typ,
location=data.location,
encoding=Encoding.ABI,
annotation=f"abi_decode({output_typ})",
)
to_decode = IRnode.from_list(
data_ptr,
typ=wrapped_typ,
location=data.location,
encoding=Encoding.ABI,
annotation=f"abi_decode({output_typ})",
)
to_decode.encoding = Encoding.ABI

# TODO optimization: skip make_setter when we don't need
# input validation

output_buf = context.new_internal_variable(wrapped_typ)
output = IRnode.from_list(output_buf, typ=wrapped_typ, location=MEMORY)

# sanity check buffer size for wrapped output type will not buffer overflow
assert wrapped_typ.memory_bytes_required == output_typ.memory_bytes_required
ret.append(make_setter(output, to_decode))

ret.append(output)
# finalize. set the type and location for the return buffer.
# (note: unwraps the tuple type if necessary)
ret = IRnode.from_list(ret, typ=output_typ, location=MEMORY)
return b1.resolve(ret)


class _MinMaxValue(TypenameFoldedFunction):
Expand Down

0 comments on commit d438d92

Please sign in to comment.