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

Evaluate usage of transferFrom, approve #16

Closed
berndbohmeier opened this issue Jul 21, 2017 · 4 comments
Closed

Evaluate usage of transferFrom, approve #16

berndbohmeier opened this issue Jul 21, 2017 · 4 comments

Comments

@berndbohmeier
Copy link
Contributor

Discuss why and if we need it and then implement it properly. At the moment it does not make sense.

@ice09 ice09 added this to the Trustlines M1 milestone Jul 25, 2017
@heikoheiko
Copy link

heikoheiko commented Jul 26, 2017

It's needed for smart contract interoperability. It's the standard pattern to pay another contract with ERC20 tokens.

Example: When buying tokens at an exchange with trustlines money, you'd first approve a certain amount of trustline money for the seller at the exchange, register the path buyer to seller, then call the buy function of the exchange contract, which would transfer the tokens if it can do a successful transferFrom(buyer, seller), approve is called again with zero to release storage. All within the same transaction, so it's atomic.

Are there other suggestions how, prove of payment to contracts could work?

@heikoheiko heikoheiko modified the milestone: Trustlines PoC Jul 26, 2017
@heikoheiko
Copy link

Gas cost considerations?

@ice09
Copy link
Contributor

ice09 commented Aug 15, 2017

we want to support ERC223 instead of ERC20 for the reasons described here: ethereum/EIPs#223

@Dexaran
Copy link

Dexaran commented Aug 15, 2017

approve / transferFrom / allowance were included in ERC20 token standard to resolve EthereumVM related issues with "call stack depth attack". They should be considered deprecated and superfluous now. These functions doesn't introduce any functionality that could not be implemented without them. Call stack depth attack was resolved at the EIP150 (as you can see it happened a bit later than EIP 20 was created).

You should keep in mind that ERC20 token standard functions are leading to monetary losses. I wrote a number of articles about it (for example this one).

The main problem of ERC20 token is that it has two ways of transferring funds:

  1. transfer
  2. approve + transferFrom

which assumes that first pattern is processed without handling by the receiver and the second pattern simulates transaction handling.

The purpose of approve + transferFrom was described in solidity documentation at page 39:

Warning: There are some dangers in using send: The transfer fails if the call stack depth is at 1024 (this can always be forced by the caller) and it also fails if the recipient runs out of gas. So in order to make safe Ether transfers, always check the return value of send, use transfer or even better: use a pattern where the recipient withdraws the money

This was relevant at the moment of creating ERC20 token standard, but the stack depth attack was fixed through EIP150 . As the result of this approve + transferFrom must be considere deprecated now.

I think that there is no need to develop and implement the approve / allowance mechanism because:

  1. approve + transferFrom is leading to monetary losses since it assumes another way to transfer tokens (And an alternative way to execute a transaction often does not implement handling)
  2. vulnerability to re-approval attack is described here
  3. approve is superfluous since common pattern of transactions handling is fallback function execution. It is already described in ERC223.

I'd like to recommend to remove decApprove / setApprove / getAllowance from the token standard because of described problems of this approach.

ice09 pushed a commit that referenced this issue Aug 15, 2017
@ice09 ice09 closed this as completed Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants