Metamask-extension: Warn users when sending tokens to the token address

[danfinlay]

This really is an extension of the whole issue where people are losing tokens b/c ERC-20 makes it easy to send to contracts that can't actually do anything with them.

This issue represents a single ERC-20 validation mitigation: Prevent sending tokens to token addresses, perhaps in general, maybe just to the same address as the token itself.

[mryellow]

This is an issue with the contract standard.

transfer() applies token balance to whichever address it is given. This address should be able to be any kind of user or smart contract without customised case-by-case interference from clients.

ERC223 https://github.com/ethereum/EIPs/issues/223 approaches this by adding a tokenFallback() method which gives the target an opportunity to reject the transfer (or store details about it).

Fortunately in the case of non-fungible tokens ERC721 has solidified around including a callback onERC721Received() which informs the receiving end and will revert if this function is missing unless a legacy transfer method is used.

https://github.com/ethereum/EIPs/pull/841

Besides these improvements token authors can mitigate the issue by providing methods where the owner can transfer out any tokens mistakenly sent to the contract.

[fulldecent]

If there is a strong interest in the MetaMask community (the people that have commit access) then I will create an ERC-20 replacement that includes this same feature from ERC-721.

[mryellow]

@daniel-jozsef made a really good point here:

There's an ERC for that. A proper one, that isn't a mess. Also de-fact accepted, implemented in OpenZeppelin. ERC827

https://github.com/ethereum/EIPs/issues/223#issuecomment-371471087

Makes it easy enough to create an ERC-20 with extended callback features where it might need specific "deposit" type behaviour. Figure there could be room for a standard which uses ERC-827 features to send an onERCXXXReceived() callback. Which I guess would need to be exposed to wallets with EIP-820 introspection.

This could supplant ERC-223 with a cleaner layered approach. Would be a good thing as 223 seems misplaced.

Otherwise you're left with a standard ERC-20 and approve/transferFrom pattern for many cases. With recovery remaining the role of the receiving contract. Which is probably fine also.

[bdresser]

As a really MVP solution re-using existing components, we could do this (below) with a small ? icon next to the error message to link out to a support article that explains a little bit about contract addresses etc. @danfinlay @cjeria sound good?

[bdresser]

some folks on Support are advocating for this to be a warning rather than fully preventing users from moving forward.

downside: people might not read the warning and still lose tokens.
upside: people won't get angry that we're preventing them from doing what they want

[bdresser]

let's link to this article on our support center

[mryellow]

let's link to this article on our support center

Looks to be some kind of signup form for another 3rd party forum of some kind.

Any way to see the content?

[bdresser]

sorry, published with wrong permissions @mryellow - try now

[mryellow]

You almost certainly do NOT want to send your tokens to the token contract

Positive of that? In all use-cases?

We maintain a list of known token contracts

Decentralised? Permissionless?

Not sure introducing centralised repositories of this kind of information is ever a good idea for blockchain type projects. Essentially it is a centralised blacklist denying token transfers to specific addresses.

If there is a blacklist, it should probably be opt-in, or at the very least have opt-out or the ability to disable that functionality. Possibly editing the list to remove an address on case-by-case basis if you so desire.

edit: Note all the added complexity. That only gets worse as these issues build on each other.

Honestly this really is a token issue. If tokens have problems they should die out and be replaced by better tokens without such problems. The world improves rather than stagnates under artificial protections.

If Metamask has to UX work-around every smart contract bug at every turn?

When does the client become responsible for ill-functioning code on the backend?

[bdresser]

All good questions @mryellow, thanks for your feedback. We get hundreds of support emails each month from folks who have accidentally burned their tokens, so the article is targeted at the very new folks among our userbase.

The list of known token contracts is used to auto-detect token balances and pull basic metadata like token images. It's an ugly solution, and we hope to someday have a better way of doing this, but it's a simple way to get basic token data in the extension.

There's an inaccuracy that I've clarified – we'd prevent/warn the user if they're sending to the contract address for the specific token they're sending, whether that address comes from our auto-detect list, or from the manual token addition form. The token list is not a block list.

I agree this is a token issue, but at this stage we'll save the team time & energy by adding this warning and reducing our support load.

[mryellow]

warning

Sweet, yeah it has to be fairly loose or there will be legacy generated here which is hard to escape.

I always imagine if there is some way to check for specific signatures (such as looking for symbol and digits) but that too seems messy and extra over-head on every single address entered.

[fulldecent]

I’m more concerned that when you send tokens the UI does not tell you contract you are interacting with.

—-

But regarding this issue, yes of course MetaMask should have a warning. When people lose tokens they are going to call MetaMask to complain before they call the contract author.

Trust me, people are writing crappy contracts that just barely work with the old draft version of whatever EIP they are implementing. And that’s not going to change.

MetaMask is the one that get a commission when you buy Ether and they are responsible for the customers’ experience.

[bdresser]

This warning got dropped with the introduction of the new Send flow - re-opening