Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect Buffer Size Passed to abi_encode() #4188

Closed
ritzdorf opened this issue Apr 25, 2024 · 2 comments · Fixed by #4202
Closed

Incorrect Buffer Size Passed to abi_encode() #4188

ritzdorf opened this issue Apr 25, 2024 · 2 comments · Fixed by #4202
Milestone

Comments

@ritzdorf
Copy link
Contributor

Version Information

  • vyper Version (output of vyper --version OR linkable commit hash vyperlang/vyper@): b43ffac

Issue Description

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 cyberthirst transferred this issue from another repository Jul 16, 2024
@cyberthirst
Copy link
Collaborator

I don't think there are security implications.

In abi_encode the bufsz is only used as:

if bufsz < size_bound: # pragma: nocover

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:

buflen = max(args_abi_t.size_bound(), return_abi_t.size_bound())
else:
buflen = args_abi_t.size_bound()

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

@cyberthirst
Copy link
Collaborator

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:

if bufsz < size_bound: # pragma: nocover

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:

buflen = max(args_abi_t.size_bound(), return_abi_t.size_bound())
else:
buflen = args_abi_t.size_bound()

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

@cyberthirst cyberthirst transferred this issue from another repository Jul 29, 2024
@cyberthirst cyberthirst added this to the v0.4.1 milestone Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants