vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.84k stars 788 forks source link

Incorrect Buffer Size Passed to `abi_encode()` #4188

Closed ritzdorf closed 1 month ago

ritzdorf commented 4 months ago

Version Information

In external_call.py the function _pack_arguments encodes the arguments of the call as follow:

if len(args) != 0:
    pack_args.append(abi_encode(buf + 32, args_as_tuple, context, bufsz=buflen))

However, the buffer size passed is incorrect given that buflen is the length of the buffer allocated at buf but here the passed buffer starts at buf + 32.

No security implications were found for this issue.

cyberthirst commented 1 month ago

I don't think there are security implications.

In abi_encode the bufsz is only used as: https://github.com/vyperlang/vyper/blob/7d28a501cf128a878f5d434978506e5da7aec943/vyper/codegen/abi_encoder.py#L169

Thus it seems that the only problem could be a crash. Assume bufsz < size_bound. But size_bound = abi_t.size_bound() and the buflen is initially assigned: https://github.com/vyperlang/vyper/blob/44bb281ccaac89dc3bd66030702473c386bceae6/vyper/codegen/external_call.py#L50-L52

ie, buflen is always at least args_abi_t.size_bound() which means that the initial assumption can't happen.

cyberthirst commented 1 month ago

i misformulated myself in the analysis here, but the conclusion should still hold

I don't think there are security implications.

In abi_encode the bufsz is only used as: https://github.com/vyperlang/vyper/blob/7d28a501cf128a878f5d434978506e5da7aec943/vyper/codegen/abi_encoder.py#L169

Thus it seems that the only problem could be a crash. Assume bufsz < size_bound. But size_bound = abi_t.size_bound() and the buflen is initially assigned: https://github.com/vyperlang/vyper/blob/44bb281ccaac89dc3bd66030702473c386bceae6/vyper/codegen/external_call.py#L50-L52

ie, buflen is always at least args_abi_t.size_bound() which means that the initial assumption can't happen.

the problem is that we might not crash in abi_encode while we should, i.e., is it possible that bufsz-32 < size_bound while bufsz >= size_bound?

that is can the outcome of this branch change?

    if bufsz < size_bound:  # pragma: nocover
        raise CompilerPanic("buffer provided to abi_encode not large enough")

buflen follows 2 paths:

    if fn_type.return_type is not None:
        return_abi_t = calculate_type_for_external_return(fn_type.return_type).abi_type

        # we use the same buffer for args and returndata,
        # so allocate enough space here for the returndata too.
        buflen = max(args_abi_t.size_bound(), return_abi_t.size_bound())
    else:
        buflen = args_abi_t.size_bound()

and then for both paths, we do buflen += 32 # padding for the method id

we know that size_bound = abi_t.size_bound() and the abi_t is from ir_node which is args_as_tuple from _pack_arguments

  1. consider the if branch path:

buflen = max(args_abi_t.size_bound(), return_abi_t.size_bound()) + 32

is bufsz-32 < size_boundsatisfiable? max(args_abi_t.size_bound(), return_abi_t.size_bound()) + 32 - 32 < abi_t.size_bound() which is not satisfiable

  1. consider the else branch path: buflen = args_abi_t.size_bound() + 32

is bufsz-32 < size_boundsatisfiable? args_abi_t.size_bound() + 32 - 32 < abi_t.size_bound() which is not satisfiable