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

Should call (and family) check for extcodesize? #4823

Closed
chriseth opened this issue Aug 15, 2018 · 8 comments
Closed

Should call (and family) check for extcodesize? #4823

chriseth opened this issue Aug 15, 2018 · 8 comments
Labels
language design :rage4: Any changes to the language, e.g. new features

Comments

@chriseth
Copy link
Contributor

Currently, call does not fail if the target contract does not exist. The idea is that it is a low-level thing.

People might still not think about that.

On the other hand, this makes it impossible to use call for sending ether to external accounts.

On the third hand, you should use transfer for that anyway.

@chriseth chriseth added the language design :rage4: Any changes to the language, e.g. new features label Aug 15, 2018
@chriseth
Copy link
Contributor Author

Also note that extcodesize is 700 gas.

@theethernaut
Copy link
Contributor

theethernaut commented Aug 15, 2018

  1. If the callee was a contract, it would have to expose some part of its ABI as payable for it to be able to receive ether. That is, contracts need to explicitly declare that they can receive ether.
    Now, would you impose the same requirement on a regular account? What purposes does an account have other than representing an identity and receiving funds? So I don't see a reason for them to have to explicitly declare such an ability.

  2. What benefits do you see in adding this restriction?

@spalladino
Copy link

Is there a valid use case where you may want to send some data along with the ETH to an external account, to later retrieve that data from an app off-chain, and act based on it? (similar to the the data parameter present on the transfer methods of some token standards). If so, I'd not add the extcodesize check.

That said, an example of such use case doesn't come to mind now.

@frangio
Copy link
Contributor

frangio commented Aug 15, 2018

My gut feeling is that call should stay a low-level function, without additional checks. If this isn't desired I'd consider removing it in favor of just inline assembly.

@alcuadrado
Copy link
Member

I agree with @frangio, but that I don't think it should be removed. Dealing with with inline assembly in program analysis tools is much harder, so IMO the least it's used the better.

@spalladino I've seen extra data sent to a contract's default function for tracking purpose in ICOs. I wouldn't be surprised if this technique is also used for external accounts.

@chriseth
Copy link
Contributor Author

@ajsantander note that this is not about sending ether, but just about sending data. The problem is that if you send data to an account that does not have code, it will be successful, even though you would expect some code to execute.

I agree with keeping things simple, but we check for extcodesize with regular function calls and thus people might expect the same thing to happen for low-level calls. Note that this would also provide some kind of protection from "library" selfdestructs (note that libraries cannot selfdestruct anymore, you have to use the contract workaround that was actually also used in the two library-related bugs) combined with low-level delegatecall.

The main question would be which use-case is more edge-casy: deliberately sending data to non-contract accounts or assuming that call executed code just because it did not fail.

One resolution of this issue could also be: "call never checked for extcodesize so it should not start doing so now", especially if we cannot find strong arguments for either side.

@axic
Copy link
Member

axic commented Sep 5, 2018

I think with introducing .iscontract() on the address type would negate the need for this.

@axic
Copy link
Member

axic commented Sep 5, 2018

Agreed on todays meeting that this replaced #4910. The current behaviour is simpler and it isn't fully clear which option is better (having the check or not having the check), but by introducing a helper (as of #4910) the option is put in the hands of the user.

@ajsantander @frangio @spalladino @alcuadrado please comment if you would like to keep this issue open and discussed further.

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
Projects
None yet
Development

No branches or pull requests

6 participants