Remove .send and .transfer.

[MicahZoltu]

Description

Once again, necessary gas cost adjustments in the EVM are being contested because people incorrectly have made assumptions that gas costs are fixed rather than variable. I think a lot of this stems from the fact that Solidity actively encourages this behavior through .send and .transfer methods. The only input into deciding the gas cost for any given operation is the operational cost of that instruction relative to other EVM instructions. As seen with Constantinople, and now Istanbul, the operational costs of various operations can change (both up and down) over time as EVM implementations gain/lose optimizations.

The advice that is constantly doled out telling people to use .transfer and send to protect from reentrancy has resulted in Constantinople being cancelled and Petersburg having some silly code to deal with the fact that the community has been giving bad advice to new Solidity engineers for years. Similar advice is now causing pushback against the proposed gas cost changes in Istanbul because people have hard-coded things like, if (gasleft() < 2300) I suspect largely because of the .transfer and .send methods.

I know that writing secure code is hard, and I'm a huge advocate for making writing secure code easier. However, .transfer and .send are likely going to eventually cause security issues (like almost happened with Constantinople) because it creates a false sense of security. Also, while we may not ever be able to drop SSTORE costs down to below 2300 in ETH 1.x because of legacy code that depends on it, ETH 2.0 or any other new platform running the EVM who doesn't have to support legacy contracts can set gas costs appropriately and not have to worry about breaking legacy code. However, as long as .transfer and .send exist people will continue using them and even new EVM based platforms will continue being in this bad place where gas costs are not calculated the way they should be.

TL;DR: Please remove .transfer and .send from Solidity (can deprecate for a couple years first) and advise people to follow development strategies that do not rely on on gas costs being fixed.

[jochem-brouwer]

I just discussed this with Micah over chat. I think a step in the right direction would be to explicilty note in the solidity docs that developers should be aware that .send or .transfer to a contract MIGHT fail in the future due to changed gas costs in a fork (if the contract is meant to only transfer to EOA's then I think it is fine to use this - but the developer SHOULD BE aware of this).

I also think that noting that "use transfer or send to protect against re-entrancy attacks" should be removed because I do not think it is part of the EVM design that the gas stipend (the 2300 gas) should only be used for logging and that it can be assumed that no SSTOREs get executed.

In general Solidity is currently the language most new developers use. This is hence also the gate to learning how to develop on Ethereum. Hence solidity has an important position to make sure that developers are aware of the not-so-obvious differences between code on Ethereum and "normal" code.

[chriseth]

Before we can remove send and transfer, we need a new function that forwards all gas. I don't think that .call.value(x)() should be the recommended way to transfer ether.

I second the suggested changes to the documentation.

We should also note (if that is not already part of the "withdraw pattern" documentation) that you should deal with the fact that a "forward all gas" call can always get you stuck because it consumes all gas.

[jochem-brouwer]

Good point on the consume all gas. Keep in mind that you will always keep 1/64 of the available gas which you had before a call due to the call depth limit eip.

[MicahZoltu]

We should also note (if that is not already part of the "withdraw pattern" documentation) that you should deal with the fact that a "forward all gas" call can always get you stuck because it consumes all gas.

I think it is more general to assert that:

calling external code (which includes ETH transfers) can always trigger a revert and there is nothing you can do to prevent this.

For the same reason as mentioned above, I think it is a bad idea for contracts to try to do any sort of conditional logic based on gas remaining or to use gas limiting as a means of controlling the negative impact a contract can have on operation, or ensuring that you "have enough gas leftover to finish". The two exceptions I may make is for a contract to always spend a percentage of gas on a child call. This allows the contract to continue to function even in the face of of changing gas prices, or to allow a user-specified "leftover", so gas price changes can be accommodated through UI updates.

Really, the withdraw pattern is the way to go and contracts should simply not call untrusted external code outside of those contexts.

[chriseth]

If we are saying "the withdraw pattern is the way to go", then maybe we should make that more explicit by disallowing send and transfer and replacing them by a new function called withdraw (bad naming, we should come up with a better one) that does the following:

withdraw(x) sends x wei to msg.sender (forwarding all gas) and terminates the execution (unless the send reverted in which case it will revert).

Terminating the execution is important because you should do state change before that anyway.

There is still the problem that the containing function might need return values.

[MicahZoltu]

🤔 Terminal functions in general are a neat idea. It would be even cooler though if the compiler instead were able to guarantee that no storage writes occurred after the call, and any external function calls were STATIC_CALL. That would allow you to do things like logging, or prep data for returning.

Of course, this doesn't protect the user from reentrancy since we cannot enforce at the language level that the caller continues to follow those rules. I believe we would need a new EVM instruction that would set a transaction level flag that says "no state writes from this point on".

[jochem-brouwer]

@MicahZoltu Basically that is what STATICCALL does - it does not allow any state writes a level deeper in the call frame. If a static called contract re-enters the current contract and tries to modify state there, it will fail.

I think solidity should start using STATICCALL for transfers and sends. The only downside is that STATICCALL is strict - you also cannot LOG events. - does not work because STATICCALL cannot transfer value as this obv. changes state as pointed out by @MicahZoltu

[MicahZoltu]

@jochem-brouwer Can you attach value to a STATICCALL? I would expect not since moving ETH around is a state change.

[jochem-brouwer]

@MicahZoltu yes of course, you are right, the value is always zero. It would hence not work when transfering ETH.

[rmeissner]

Is there any plan how to move this forward. With the current changes to the sload gas costs and future changes to event gas costs (that might be coming), it would be really nice to avoid that the 2300 gas limit is further used.

[chriseth]

If even the events get more costly, then maybe the EIP process should consider either removing or increasing the stipend.

[MicahZoltu]

Changing the EVM stipend is much harder than changing Solidity's bytecode generation because of backward compatibility. Even with the EVM having a stipend, the .transfer and .send recommendations are still bad ideas.

[chriseth]

I'm perfectly fine with introducing new functions, but we have to find good names. Because of try/catch, one would actually suffice. What about allowing .gas() for now?

[MicahZoltu]

@chriseth Hmm, maybe we are talking about different things. I'm talking about removing .send and .transfer, or at least strongly discouraging them in the docs in favor of .call.value()(""). Are you saying that you would like to add a new method for .call.value()("") (since that syntax is kind of ugly)?

If that is what you mean, then my recommendation would be something like .sendEth(amount) or similar. It will help disambiguate it from contract function calls named send and transfer (e.g., ERC20/ERC777 tokens) and it would be unique from the existing methods.

[3esmit]

I dont see any problem in using .call.value, because I think that sending Ether is odd, and uncommon. Most of contracts will deal with L2 tokens, and Ether is pure gas.

In ERC20 tokens when you transfer you know that other contract dont executes anything, and instead we need to approveAndCall.

For Ether, the value transfer can also execute something... Maybe Ether should be only payable in a ERC20 like type, things would get easier for gas abstraction aswell.

[axic]

Are you saying that you would like to add a new method for .call.value()("") (since that syntax is kind of ugly)?

We are actually discussing new syntax for that under #2136. One of the likely candidates is call{value = 12 eth}()

[axic]

A relevant read on the topic: https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/

Given the date on it, it may have been the inspiration for this issue.

Two more length articles:

• https://ethereum.stackexchange.com/a/38642
• https://github.com/fravoll/solidity-patterns/blob/master/docs/secure_ether_transfer.md

[chriseth]

Since we have try/catch now, we could just modify the semantics of .transfer in the following way: .transfer will always forward all gas, unless specifed using e.g. .transfer{gas: 0}(...). We could introduce a warning now if the gas is not explicitly specified and change the behaviour in a breaking release.

[chriseth]

Instead of terminating after a call to withdraw, we could have the compiler check that there are no state-changing operations after the call to withdraw. The function could also be called returnEther or sendBack.

[GNSPS]

Since we have try/catch now, we could just modify the semantics of .transfer in the following way: .transfer will always forward all gas, unless specifed using e.g. .transfer{gas: 0}(...). We could introduce a warning now if the gas is not explicitly specified and change the behaviour in a breaking release.

I would very much prefer deprecation of the methods.
I think that the same way opcode repricing broke the "transfer is reentrancy-safe" assumption, this change would, too.

Generally, I would advocate keeping the functionality of the methods the same and just discourage its usage until deprecation. I could get behind a new keyword for different functionality, though, like withdraw, just think it won't cover a majority of the use cases for the deprecated transfer/send.

[rmeissner]

This might become more interesting with https://ethereum-magicians.org/t/eip-2929-gas-cost-increases-for-state-access-opcodes/4558

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[MicahZoltu]

Bump, I still believe this should happen.

[rmeissner]

Very much agree especially since EIP-2929 is now in place.

[ekpyron]

Staging this for discussion for inclusion in the next breaking release (which will also prevent the stale bot from attempting to close it again).