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

Library for safe interaction with untrusted contracts #3544

Closed
pcaversaccio opened this issue Jul 13, 2022 · 4 comments
Closed

Library for safe interaction with untrusted contracts #3544

pcaversaccio opened this issue Jul 13, 2022 · 4 comments

Comments

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Jul 13, 2022

Background

Inspired by ExcessivelySafeCall

A low-level Solidity call will copy any amount of bytes to local memory. When bytes are copied from returndata to memory, the memory expansion cost is paid. This means that when using a standard Solidity call, the callee can "returnbomb" the caller, imposing an arbitrary gas cost. Because this gas is paid by the caller and in the caller's context, it can cause the caller to run out of gas and halt execution.

Details

OpenZeppelin provides a library for safe interactions with untrusted contracts. Specifically, starting with call and staticcall. This would be in line with the current safe libraries such as SafeERC20 or SafeCast.

@frangio
Copy link
Contributor

frangio commented Jul 14, 2022

Discussed before in #3168.

See our thoughts for not including this in #3168 (comment).

In summary, there are some contexts where this protection is useful, but we believe the vast majority of users are not affected by this attack and we do not want to encourage unnecessary use of the pattern.

Feel free to share if you disagree with this thinking. Will close as it has been discussed very recently.

@frangio frangio closed this as completed Jul 14, 2022
@frangio frangio reopened this Jul 14, 2022
@frangio frangio closed this as not planned Won't fix, can't repro, duplicate, stale Jul 14, 2022
@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Jul 15, 2022

@frangio I understand the arguments about the reach of use for this library.

Nonetheless, let me share my use case for this library: I've built a batch distributor which is essentially a helper smart contract for batch sending both native and ERC-20 tokens. Now, this contract is planned to be used by projects to distribute e.g. airdrops to pre-registered wallets, distribute funding via ETH to registered wallets (for devs), or to pay crypto salaries to employees in one batch, just to name a few (think about it as a modern version of disperse.app). The major reason why I use the low-level call call instead of transfer is that the 2,300 gas forward could be not enough for some cases where one of the addresses is a more complex e.g. multisig address or smart contract wallet. So to summarise, I tried to be as agnostic as possible and do not check for the existing code of an address.

Now in the context of my batch distributor, it can well be that you have a list of 100-200 addresses to send ETH or ERC20 tokens to. Assume one or two of the addresses try to returnbomb the ETH distribution and make the sender pay unnecessary money. There are two ways to resolve that:

  • Check each address if it's an EOA, and if not check for the receiving logic of the contract code. This can be really time-consuming and you don't know whether the logic is updated after checking in case the contract uses an upgradable pattern.
  • Use such a library for the low-level calls.

If the sender of a batch is not an advanced user, he/she might even trigger a couple of times the same transaction because Etherscan will just show an OOG error and probably he/she will not use e.g. Tenderly to debug the failing transaction in detail.

It might be a niche case, but I still wanted to share that with you. It's relevant for a use case where you send batches of ETH to a large list of "unknown" addresses.

@frangio
Copy link
Contributor

frangio commented Jul 15, 2022

In the case you describe you're not interested in the returndata at all, so it's not necessary to copy it to memory. If you don't copy it, the target contract can't cause any additional gas to be wasted.

This was addressed by the compiler in ethereum/solidity#12647, and the fix was released in 0.8.13 apparently (not in the changelog so I've asked to confirm ethereum/solidity#12684 (comment)).

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Jul 16, 2022

Ah very interesting, I was not aware that this was addressed by 0.8.13. Thanks for the reference @frangio. In this case (since I'm using 0.8.15) everything looks fine for my use case.

Update: I quickly tweeted about it here so maybe further people will be aware of this fix by 0.8.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants