search

near / NEPs

The Near Enhancement Proposals repository
https://nomicon.io
216 stars 140 forks source link

Advanced Fungible Token Standard #110

Open miohtama opened 4 years ago

miohtama commented 4 years ago

As per discussion #dev-contract channel in Discord, I am opening a discussion here for an alternative for NEP-21. I have been involved in Ethereum and EOS token development for the last four years and would like to highlight some issue and solutions NEAR community should consider before pushing forward with a token standard.

I am willing to champion this issue through, as long as I know there exists enough community acceptance and the backing for the proposal.

Advanced Fungible Token Standard

Summary

General-purpose fungible token standard aiming for the better developer and user experience.

Motivation

Currently NEP-21 is blindly copying the most widespread smart contract token standard, ERC-20. ERC-20 was initiated back in 2015, then formalized starting 2016. However the time has proven that there were many mistakes made with ERC-20 and it would be foolish to copy those mistakes to NEAR token standard when one can start from the clean slate.

Below I go through ERC-20 shortcomings one-by-one and also have some links as a reference material. Some post-Ethereum networks, like EOS, have already addressed technicalities and have more user-friendly approach to tokens. Also Ethereum has addressed issues in the form of later standards, like ERC-777, but due to ossification they have not been adopted (more to below).

Smart contracts cannot reject transfers

Because of the lack of standardized token receive hook, smart contracts cannot reject token transfers on them. If someone accidentally sends ERC-20 to a smart contract address they are likely lost. This error is a common and happens especially when copy-pasting addresses around: tokens are send to the token contract itself.

There is a Twitter account tweeting these mistakes:

https://twitter.com/TokenOops

Root cause: ERC-20 has different transfer() and transferFrom() semantics when dealing with normal accounts and smart contract accounts.

Account cannot express if it can receive tokens

Similar to one above, a common mistake is to send tokens to a centralised exchange address that cannot handle them. For example, Bittrex charged $5000 for "token recovery" in one point to give back the tokens that the user deposited to the exchange if the exchange did not have an active order book for them.

For example, there is worth of $772M tokens in 0x0 address. Some of them are token burns, but most of them are accidental sends and wallet input field failures: https://etherscan.io/address/0x0000000000000000000000000000000000000000

Root cause: Accounts cannot express what tokens they support

Hot wallets cannot interact with smart contracts

Centralised exchanges and other custodial use hot wallets where each receiving address belongs to an user, but withdraw address comes from a pooled wallet. Because most smart contract operations use msg.sender as the author, any reverse payments for msg.sender would go to the hot wallet pooled address directly. Because the transfer is not tripped through the receiving address of the user, the hot wallet accounting cannot mark this reverse payment deposit belonging to the user.

As a hack workaround, centralised exchanges like Kraken and Coinbase set the gas limit for the token transfers very low, hoping that the gas limit prevents any smart contract interaction from hot wallet direct withdrawals.

Root cause: transfer() does not provide alternative address as the return address

Different transfer semantics for account and smart contract interaction

People expect transfer() to work with a smart contract, like it works with normal accounts. However this is not the case. Any direct transfer() and not approve() + transferFrom() pair to a smart contract address usually leads to loss of the tokens, because smart contract cannot account tokens to msg.sender correctly.

https://mobile.twitter.com/moo9000/status/1300167829929459713

Native asset is treated differently from tokens

In Ethereum Defi world, the native asset ETH must be wrapped to WETH ERC-20 token to interact with many of the smart contracts. This causes extra work for developers, as they need to write double code with ifs to all deposits and withdrawals. This will also confuse users, as they see both the native asset and the wrapped asset in their wallet and wallets do not account them as one item.

As a side note, Solana copied this design mistake. However, for example in the case of EOS, all assets are treated similarly and any token asset can added to any payable transaction.

https://mobile.twitter.com/ProjectSerum/status/1300633211932868610

Lack of native relayers and gas fee markets

To transact with ERC-20 tokens, the user needs to have both the token and Ethers on the same account. This is very confusing for the users who are there only for the token, for example in gaming scenarios, and could not less care about cryptocurrency.

ERC-20 lacks native mechanisms for fee markets and relayers who would be willing to pay the gas fee on the behalf of the user and take a fee cut in the token amount. The history has proven that adding this functionality afterwards is especially complicate. Multiple smart contract wallets (Argent, Pillar, etc.) have come up with incompatible, proprietary, solutions.

Root cause: Lack of gas fee market design when ERC-20 was launched

Lack of metadata

ERC-20 only provides information for name, symbol and token supply. Even amount of decimals is an add-on. This has created a cottage industry of different "token lists" that supplement this information. A common elements to add would be at least homepage, icon and relayer information (for gas market transactions). Metadata often also contains various discussion forums, support email, officially author information (foundation, corporation) and such. Wallets could consume this information directly.

For example, the following applications maintain their incompatible lists just to get a token icon visible in the wallet: MyEtherWallet, TrustWallet, MetaMask, Parity. Then the services maintain their own lists: Uniswap, Loopring, IDEX.

Root cause: Blockchain persistent storage was deemed too expensive for this, community inability to come together for a common standard

Lack of notifications

Because how ERC-20 transfer events are implemented, wallets usually need to run extra infrastructure and servers to detect incoming token transfers. Developers lack generic "notify me for all incoming transfers for this address" event. (Furthermore it is even worse for ETH itself as it does not have any notifications and the only way to see balance changes is polling or heavily instrumented custom node.)

This makes it expensive to build wallets as you need to invest to the server-side infrastructure a lot, which is against the point of decentralisation.

Standard UX rules how the user finds out incoming transfer

ERC-20 wallets like MetaMask does not display incoming transfers by default if the ERC-20 token is not whitelisted in the MetaMask source code. This is to avoid airdrop spam attacks where desperate marketers send a small amount of tokens to everyone in the hope the user does a web search for this token and proceeds to buy or sell.

However it also makes it impossible to send any tokens to new users. Because MetaMask wallet silently ignores all incoming transfers that are not whitelisted either by MetaMask team or the user itself ("Add custom token") the first question of novice user if their tokens were lost.

Root cause: Community inability to come together for a common standard

Re-entrancy implementation guidelines

Because of re-entrancy issues on badly written smart contracts, people are afraid of moving away from ERC-20 even though this issues would have been already addressed. This has caused ossification of Ethereum token development, as ERC-20 is barely good enough, but users and developers will be suffering for the years to come.

Whereas this issue was addressed in late code examples and is now even highlighted in Solidity developer manual, the community has still not yet moved over this.

Root cause: Community ossification, psychological change resistance

Guide-level explanation

Here I propose that NEAR does not repeat the past mistake in the form of rolling out ERC-20 clone, but has a solid token standard since day zero.

The standard should cover

Reference-level explanation

TODO

Drawbacks

We should definitely do this.

Rationale and alternatives

TODO

Unresolved questions

TODO

Future possibilities

TODO

mikedotexe commented 4 years ago

Great points, thank you for adding this! Am soaking it in

kobuta23 commented 4 years ago

It's very easy to think ERC-20 is a good standard just because it was so successful.

Great post @miohtama going through the main points of why it's really not that good.

I would also add the inherently horrible UX of approve() and transferFrom() also makes for an extremely insecure environment.

Visit any Dapp, and one of the first things you will be asked to do is "Allow dapp X to spend your tokens. Do you trust them?". Yes, they can spend your tokens. No, I don't trust them. But there's no way around this, due to ERC-20.

It gets worse. You either sign an extra transaction every time, approving a new amount, or you use "unlimited approve", giving the spending address unlimited access to your funds, to avoid future approve() transactions.

This aspect is extremely insecure. Scam plan: create a uniswap clone website, ask users to approve your address. Immediately drain 100% of their wallet's balance.

By contrast, using a payable function, as implemented in ERC-777, your potential loss would be limited to the amount you actually send the contract.

ilblackdragon commented 4 years ago

We actually were trying to remove need of approve + transferFrom originally in NEP-21 via async calls and callbacks, but end up going back on that. @evgenykuzyakov have more context on the runtime implications of that.

Agree that with meta transaction type of thing, there is a lot more can be done.

We are also adding EVM with native meta-transaction support (e.g. msg.sender will be correct).

MaksymZavershynskyi commented 4 years ago

Ethereum community was trying to deprecate ERC20 for a long time, but there is a benefit of having a dead-simple standard, because it is hard to misuse. For example when they tried to improve ERC20 by introducing ERC777 there were reentrancy attacks. We have to be extremely conservative with token standards, and make sure that the change is absolutely necessary and not just merely nice to have.

miohtama commented 4 years ago

@nearmax re-entract attacks on ERC-777 are not true today. There has not been re-entrancy attack vectors on any major protocol or smart contract since 0x protocol was launched early 2018, as I investigated a lot of services and protocols for this vulnerability. Some Chinese copy-paste team got screwed because they did not know what there were doing, but generally Ethereum community knows very well today how to avoid re-entrancy issues.

You can find more insight here:

https://twitter.com/moo9000/status/1278426988236156931

And here:

https://twitter.com/moo9000/status/1299647432817606656

Moreso, NEAR has a clean start so it can make re-entrancy attacks ineffective by default either on VM, compiler or code example level.

Also please read comment from @kobuta23 - in a bigger picture approve() is unsafed, as users do not know how to use it and have higher chances to screw this up. Many services just do infinite approves because otherwise usability would be horrible. This is dangerous especially with a blockchain like NEAR where code update is enabled by default; You just switch your smart contract to one that does transferFrom() for all user tokens who have ever done approve().

evgenykuzyakov commented 4 years ago

Originally we explored the idea of having something like: transferWithConfirmation(receiver_id: ValidAccountId, amount: U128, callback_args: CallbackArgs) It would transfer tokens to the receiver_id and call on_receive on the receiver_id contract and also pass some extra arguments from callback_args then it might expect on_receive to return true if it indeed received the tokens or throw an error in case it doesn't want your tokens.

It made interface more complicated and created some Gas/promise issues, including handling of the callback. So we went with simpler path or mimicking the simplest possible implementation.

I think we can revisit this decision and remove allowance completely. It may make token storage much smaller, since it doesn't need to store allowance anymore. It will also make interface simpler, but then the just transferred tokens can't be immediately spent, because the callback relies on rollback.

robert-zaremba commented 4 years ago

@miohtama - do you have any working proposal?

I hope most of us agree that allowance is not secure - we can't say how and where the spender can spend the funds.

I was also thinking about a better standard for NEAR Fungible Tokens with callback for the NEARswap project I'm doing. However the async calls really complicate the things and you end up with much longer chain of receipts. This makes things harder to correctly handle edge cases on the dev side.

Probably the principal question we want to ask ourselves here is: Do we want a token standard, which will underline most of the blockchain services (!!!) to be:

If we care about financial services then the latter options with more scrutiny on writing and auditing is more safe long term.

evgenykuzyakov commented 4 years ago

Please take a look at https://github.com/nearprotocol/NEPs/issues/122 It addresses some points.

How meta-transactions prevent repeating the same transfer twice. Do we need to have a nonce on the token account?

miohtama commented 4 years ago

Hi @evgenykuzyakov Here is now the full submission to the hackathon:

https://github.com/miohtama/advanced-fungible

This is basically the data structure I used:

pub struct Ledger {

    // Total balances, including locked, for each user
    pub balances: LookupMap<AccountId, Balance>,

    /// Account has a pending promise chain in progress
    /// and balance locked is this chain cannot be withdawn.
    /// If a promise chain is succesful free the locked balance.
    /// If a promise chain fails, then the send() gets undoed
    pub locked_balances: LookupMap<AccountId, Balance>,

    /// Total supply of the token
    pub total_supply: Balance,

    /// Helper counter for testing to diagnose
    /// how many rollbacks have occured
    pub rollbacks: u64,
}

Every time a send() is executed, the balance is locked in the sender contract and the locked balance is not available to other transactions until the promise calling on_token_received is resolved.

evgenykuzyakov commented 4 years ago

https://github.com/miohtama/advanced-fungible

This implementation still doesn't guard against invalid transfers to the contracts that don't expect the token. So they are still going to be locked on the contracts that don't expect them.

I think there are 3 scenarios where the tokens are transferred:

For user to contract scenario, the front-end should handle the correct method name, e.g. transfer_to_contract. For contract to contract, the developer will also use the correct method. But for user -> user, the wallet can use the correct method e.g. transfer_unsafe

miohtama commented 4 years ago

This implementation still doesn't guard against invalid transfers to the contracts that don't expect the token.

The implementation does is_receiver() which is "an interface check" in very limited sense.

It is not excellent, but it should prevent accidentally sending tokens to a contract that does not want them. However as the comments say there are two problems with NEAR run-time:

1 ....currently NEAR run-time does not expose information about if accounts are code accounts, so the check is not perfect. However I am working on the assumption this can be made possible in the future versions of NEAR, as I do not see the blocker for it.

2 Alternatively the failure reason of promise is not exposed to the contract, which could be also used to weed out transfers between code and non-code accounts.