VIP: Disallow sending eth to 0x0

[ben-kaufman]

Preamble

VIP: 829
Title: Disallow sending eth to 0x0
Author: Ben Kaufman
Type: Standard Track
Status: Draft
Created: 2018-05-20

Simple Summary

Disallow sending eth to 0x0, either by default or with a new function.

Abstract

Disallow sending eth to 0x0, either by default or with a new function.

Motivation

In order to minimize the risk for ETH to get stuck, we should consider blocking sends to the 0x0 address.

Specification

We can either do this by disabling it by default or by adding a safe_send function.

Backwards Compatibility

This might be incompatible with cases which explicitly want to send ETH to 0x0, but it also might be compatible, depends on the implementation.

Copyright

Copyright and related rights waived via CC0

Cute Animal Picture

[fubuloubu]

I think in all scenarios this guard would operate against (as a compile time check), you would have to explicitly state the 0x0000....0000 as the recipient of the ether. If you are writing it out, I would think you might know why you're doing it and you should be allowed to burn your funds if you know you're doing it.

What is probably the concern here is uninitialized storage variables, which is very likely to allow you to do that unintentionally. To solve for this, we could add a run-time check against a zero address setting for sending ether. My question is why stop there? Tokens are valuable too, so is ownership. I don't think we could include all patterns where you would get burned.

My counterproposal is that all loads and stores to an address storage variable cannot be 0x0000....0000. This sounds draconian but I think it could be considered reasonable if you think of that address as "special", which it is, since that is the contract creation address. One modification that would need to take place to enable this would be changing to a specific burn address, something that would could enable as a compiler macro to a standard burn address e.g. BURN_ADDRESS = 0xFFFF....FFFF.

Thoughts?

[ben-kaufman]

Sounds good, I think this can work pretty good.

[dani-corie]

So... if I try to access an uninitialized address storage variable, eg. through a mapping, the transaction is reverted? Now that's okay for transactions I guess, but kinda painful for calls. Note how we don't really have try/catch... Also, calls and transactions behaving differently is bad and possibly impossible.

Really trying to make sense of this... I mean, for the most part, when accessing a mapping, we already do a check whether it's set, and revert if it isn't. But without try/catch, it will be impossible to tell if a certain mapping is initialized or not. If it's not, the function call will fail.

Add to that that Ethereum error messages are, in general, good to wipe our asses with, this sounds like a developer's nightmare. Not sure, but it strikes me as a bad thing.

[fubuloubu]

@daniel-jozsef a call is the same thing as a transaction, except it is only executed locally and not broadcast to the network. If you execute a state-modifying call that relies on information in the modified state during the call, the call will still be executed exactly the same as if it were a transaction.

The point being this suggestion would not operate any differently on call vs. transaction, that would not be helpful just as you have summarized.

[dani-corie]

Yes, it's not even really about calls vs. transactions... It's about getting errors (opaque errors, due to horrible Ethereum error handling) from trying to access unset mappings. Which is horrible. Mappings to addresses would have to be changed to mappings to {address, bool} structs, doubling storage costs.

I'm very much against this idea. The damage done by it would overshadow any potential gains by a huge margin. Instead we should focus on educating people about safe asset handling.

[fubuloubu]

I'm not sure I understand where you get "mappings to addresses would have to be changed to mappings to {address, book} structs". Could you explain further?

As far as I understand your concern, that is not the suggestion here. The suggestion is that attempting to read an address storage variable would throw an exception, so that any use in calls (either as the destination address or as an argument) would fail for uninitialized variables.

"Education" is the lowest impact, highest resource solution to a problem like this. If you have to educate people to prevent them from constantly making a mistake, then you've failed to design robust software.

[dani-corie]

As far as I understand your concern, that is not the suggestion here. The suggestion is that attempting to read an address storage variable would throw an exception

That IS my concern.

I'm not sure I understand where you get "mappings to addresses would have to be changed to mappings to {address, book} structs". Could you explain further?

Okay... So, for a very simple example, say you have a mapping from address to address, for an optional custodian address for each user address.

Currently, retrieving funds would look like this in pseudocode:

1. Get custodian address from mapping for msg.sender.
2. If set, forward Ether to custodian address.
3. If not set (== 0), forward Ether to msg.sender.

With the modification, it would have to look like this:

1. Get isSet boolean from mapping for msg.sender.
2. If isSet == true: 2.1. Get custodian address from mapping for msg.sender. 2.2. Forward Ether to custodian address. 2.3. Return
3. If isSet == false, forward Ether to msg.sender.

I guess I could live with it, but we should at least think about the implications. Especially since try/catch doesn't exist. We don't have "exceptions", we have reverts. And a revert is a final, unrecoverable, and opaque error.

[fubuloubu]

Oh, it would work like this:

1. You try a call to forward Ether to the custodian, it fails. You wonder why...
2. You remember you need to set a custodian, so you set a custodian
3. You successfully make a call to forward Ether to the custodian, so you broadcast that as a transaction to the network.

Currently, 1. would succeed and all your money would be gone because you forgot to set the custodian. You would need a Boolean in the current use case to know it was set. In the suggestion, this would not occur, the worst case scenario is you loose your gas money for forgetting to set the address as well as broadcasting it to the network without checking first.

This is why many ERC20 implemetations check for non-zero address on transfer, which is another example of this behavior, although it doesn't use a storage variable.

[dani-corie]

Currently, 1. would succeed and all your money would be gone because you forgot to set the custodian.

Not if the smart contract was written by someone with more than two brain cells. :) (And you did read the code, right? RIGHT? :D)

the worst case scenario is you loose your gas money for forgetting to set the address as well as broadcasting it to the network without checking first.

That's a pretty bad scenario. Especially in a time-critical application such as an ICO sale.

This is why many ERC20 implemetations check for non-zero address on transfer

That's actually a pretty good practice. My problem is with storage access catastrophically failing based on the value of the storage. But as I said, I can live with it if it's justified... But it would be nice to hear more thoughts on it. How about airing the question on ethresear.ch?

[fubuloubu]

Let me counter with a question: what use cases are there for reading a storage variable with a zero address setting other than 1) checking that it is not zero (which would revert anyways) or 2) sending to a burn address?

Currently, 1. would succeed and all your money would be gone because you forgot to set the custodian.

Not if the smart contract was written by someone with more than two brain cells. :) (And you did read the code, right? RIGHT? :D)

This is basically having to manually code in assert send_addr != 0x0 every time you do it (checking if non-zero)

the worst case scenario is you loose your gas money for forgetting to set the address as well as broadcasting it to the network without checking first.

That's a pretty bad scenario. Especially in a time-critical application such as an ICO sale.

It's better than losing tons of money. An ICO contract should probably not be programmed this way in the first place.

This is why many ERC20 implemetations check for non-zero address on transfer

That's actually a pretty good practice. My problem is with storage access catastrophically failing based on the value of the storage

Exactly, this falls under 1) check that it is not zero (again, so you don't lose your money)

---

Thinking about this more, I think it is not correct to add this implicit functionality for precisely the opposite reason you suggest, the functionality should be forced to be coded into every contract by throwing a compiler error or warning. If you're not doing zero address checks (which revert "catastrophically" as you said), then you're not programming it correctly and we should tell you about that.

For auditability's sake, forcing to have this appear in the code is clearer than handling it implicitly.

[dani-corie]

checking that it is not zero (which would revert anyways)

I gave a real-world example for a case when you check if it's zero, and DO NOT revert if it is (but instead do some arbitrary behavior as the contract logic demands).

But here's another use case, probably better than the previous one: conqueror game, you can conquer unowned parts of a map, or cryptokitties, or whatever. You check if an asset's owner is 0x0, and if it is, then you don't revert, but rather allow msg.sender to become the owner. You revert if it's not unset (ie. the asset is already owned).

Another one, say there's a contract with a number of roles defined (for purposes of authorization, event indexing, etc.) Each of these roles can be given to a signing address. Some might be optional. Logic might look like "once the chancellor has created a new decree, if there is no ombudsman, the monarch can sign it at once, but if the ombudsman role is set (not 0x0), they need to sign it first before it is passed on for royal review".

How would you do that with your solution? You'd need two variables for each role (as I said a bool and an address), which only complicates the contract, reduces readability, and makes it more error prone.

ps. Your core fallacy in this debate is assuming that an address in a smart contract is primarily stored for reasons of sending assets to that address. In my opinion, that overlooks all but the most simplistic and primitive contract use cases.

[ben-kaufman]

@daniel-jozsef I’m not sure about the rest of the conversation here but that’s not what the VIP is about. What I meant here was disallowing sending ETH to the 0x0 address. It’s just to make it harder to accidentally burn ETH. According to my suggestion your example will still be completely valid one. But if you try to send ETH to an unset (0x0) land owner this will fail. Of course there are lots of important use cases for the 0x0 address but very little ones which require sending it ETH.

_{Sent with GitHawk}

[dani-corie]

I’m not sure about the rest of the conversation here but that’s not what the VIP is about.

Please read the rest of the conversation. @fubuloubu said that your VIP is kinda pointless as value can be lost to 0x0 in ways other than sending Ether, so instead we should revert upon reading from address storage fields that have 0x0 as their value.

Your VIP is quite inoffensive, though I'm not entirely sure if it's warranted in this implicit fashion.

[fubuloubu]

@daniel-jozsef I wouldn't say pointless as Ether is the only protocol layer asset and is therefore much harder to recover as it requires a hard fork or a change in the process for handling ETH (e.g. EIP 867) to give other options for handling a loss. I think disallowing sends to 0x0 is good because someone could forget to populate a method input argument and if the designer didn't guard against that... bye bye ETH. This could arguably be handled as a Warning instead, asking the designer to design in a check for this if it is possible in their code design.

The implicitness of the approach of this VIP was something I was trying to build a more general case for, there are plenty of other assets where transferring to 0x0 may or may not be unintended behavior. I think the easiest solution to this is a Warning that an ETH send(), raw_call, or interface call to another account with 0x0 as an argument is potentially dangerous, and supply a compiler variable BURN_ADDRESS for designer use that is non-zero, so they can design in explicit burning flows that won't trigger the warning.

I had some other way around the behavior you had suggested, but I think you're right in that it is needlessly complicated and probably error-prone at some point, hence the Warning suggestion is probably the best solution to protect developers from themselves. We should still probably revert if sending ETH to 0x0 in this case, especially since we'll have this BURN_ADDRESS variable to use instead.