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

external contract can waste caller's gas by return value. #12647

Closed
drortirosh opened this issue Feb 8, 2022 · 6 comments · Fixed by #12684
Closed

external contract can waste caller's gas by return value. #12647

drortirosh opened this issue Feb 8, 2022 · 6 comments · Fixed by #12684
Labels
language design :rage4: Any changes to the language, e.g. new features optimizer

Comments

@drortirosh
Copy link

When calling a contract, the compiler always assigns a buffer to hold entire returned data, even if not in use.
even when calling a "void" function, the generated yul code always does

    finalize_allocation(_2, returndatasize())

regardless if there is a code to use the data or not.

This command, by itself, doesn't cause any significant gas usage. however, any future memory operation will start at higher offset, and the caller contract pays for the entire storage in use, up to the highest offset.

Consider an inner call that does

    return (0,1000000)

Not only the inner call takes a lot of gas (as expected, to copy this large buffer), but also the calling code now wastes more gas on its own memory allocations.

Suggestion:

  1. optimize out this allocation when calling a "void" function
  2. when calling a function with static return size (e.g. returns (uint)), use the return buffer size parameter of the call itself, instead of returndatasize(). This way, the caller expects (and uses) an exact amount of memory (it is also slightly cheaper than returndatacopy(memptr, returndatasize()), but this cost saving is marginal)
  3. handling a function with dynamic return (e.g. returns ( string memory)) is more complex. This would probably require a language change, to signal the max expected size

try/catch blocks

The same rules apply to:

    try methodCall() { }
    catch {
    }
@chriseth
Copy link
Contributor

chriseth commented Feb 8, 2022

Since the ABI decoder only verifies that returned data is not too short, we could limit the return size to the max size, if the max size is a constant and there are no pointers involved when decoding.

In general, I'm not too keen on guarding against hostile called contracts with regards to wasting gas, this is rather complicated and, at least to me, the benefit is not too big.

@drortirosh
Copy link
Author

it is an attack surface that was used.
https://twitter.com/transmissions11/status/1487532348467912704

it will take more effort to protect dynamic-sized return values, but at least avoid exposing staticly-sized return values (like void)

@cameel
Copy link
Member

cameel commented Feb 8, 2022

optimize out this allocation when calling a "void" function

#12306 would cover this case, though it's a bit more general. And harder to solve. A special case for when there is no data returned at all is much simpler - @chriseth do we want to have such a special case or should it be handled as a part of that more general mechanism for optimizing out unused allocations?

@cameel cameel added enhancement language design :rage4: Any changes to the language, e.g. new features optimizer labels Feb 8, 2022
@frangio
Copy link
Contributor

frangio commented Feb 9, 2022

I felt the same way as @chriseth when I first saw this (didn't really consider it an "attack") but there's something interesting about this compared to other possible "gas wasting attacks".

When one writes (, bytes res) = target.call{gas: G}(data) one expects that G will be an upper bound on the gas consumed by the call (± some constant), but in fact the target contract can cause "a lot" more gas to be consumed by copying large returndata.

Note that this is specifically about .call with a gas parameter though, so it does not affect high level function calls, but it may be an argument to introduce a new call parameter that would act as a cap on the memory that will be copied from returndata.

As for high level function calls, I think this makes sense as an optimization, but I would not consider it critical (based on the arguments I've seen so far).

@chriseth
Copy link
Contributor

As far as I see it, direct access to gas turned out to be a bad idea, not only because gas costs have changed several times over the past years. Because of that, I would not want to add more options to .call. Instead, if people want full control, they should use inline assembly.

@drortirosh
Copy link
Author

As far as I see it, direct access to gas turned out to be a bad idea

I fully agree. the compiler shouldn't do any gas manipulation.

What I do think it should is do LESS UNEEDED, UNEXPECTED work: if a method call returns nothing (or fixed 32 bytes), don't attempt to use/allocate returndatasize() dynamic data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language design :rage4: Any changes to the language, e.g. new features optimizer
Projects
None yet
4 participants