Implementation of approve method violates ERC20 standard

[3sGgpQ8H]

Currently, approve method in StandardToken.sol does not allow to set allowance to non-zero value if current allowance is non-zero. This directly violates ERC20 standard that says:

Allows _spender to withdraw from your account multiple times, up to the _value amount. If this function is called again it overwrites the current allowance with _value.

EIP-20 pull request adds the following to the description of approve method:

NOTE: To prevent attack vectors like the one described here and discussed here, clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender. THOUGH The contract itself shouldn't enforce it, to allow backwards compatilibilty with contracts deployed before

So current implementation of approve in StandardToken.sol violates EIP-20 as well.

[jakub-wojciechowski]

As the ERC20 is formalized now I think we should strictly implement the specification. How about moving the additional security check that enforces 0 allowance to an additional safeApprove method and leaving the old approve to fully implement ERC20 specification?

[phiferd]

Also, I don't see how this check actually protects against the attack vector described:

require((_value == 0) || (allowed[msg.sender][_spender] == 0));

In the scenario where Alice tries to update Bob's allowance from N to M, by the time the update to the allowance is processed, won't Bob's remaining allowance be 0 already (assuming his attack was successful), in which case the check will pass?

[3sGgpQ8H]

@phiferd This check tries to encourage Alice to change Bob's allowance from N to zero and then from zero to M, instead of changing it from N to M directly. Many believe that this way is more safe because it makes it impossible for Bob to transfer N+M tokens. Actually this way is only slightly safer, if safer at all. Here is original attack scenario:

1. Bob is allowed to transfer N Alice's tokens
2. Alice publishes transaction that changes Bob's allowance to M
3. Bob front runs Alice's transaction and transfers N Alice's tokens
4. Alice's transaction is mined so now Bob is allowed to transfer M Alice's tokens
5. Bob transfers M Alice's tokens

Finally Bob have transferred N+M Alice's tokens that is more than Alice ever wanted to allow Bob to transfer.

Once the attack allows Bob to transfer N+M tokens at most, many think that if either N or M is zero, than Alice is safe. That's why this check is here. Actually, even if Alice would change allowance from N to zero and then from zero to M, Bob still may transfer N+M of her tokens. Here is the scenario:

1. Bob is allowed to transfer N Alice's tokens
2. Alice publishes transaction that changes Bob's allowance to zero
3. Bob front runs Alice's transaction and transfers N Alice's tokens
4. Alice's transaction is mined
5. Alice sees that her transaction was mined successfully, that Bob's allowance is now zero and that proper Approval event was logged. This is exactly what she would see if Bob would not transfer any tokens from her, so she has no reason to think that Bob actually used his allowance before it was revoked
6. Now Alice publishes transaction that changes Bob's allowance to M
7. Alice's second transaction is mined so now Bob is allowed to transfer M Alice's tokens
8. Bob transfers M Alice's tokens

Again. Bob got N+M tokens which is more that Alice ever wanted to allow him to transfer.

One may argue that at step 5 Alice should notice that her token balance was decreased and Transfer event was logged. This is true, but if Bob had transferred tokens not to himself but to somebody else, then Transfer event will not be linked to Bob, and, if Alice's account is busy and many people are allowed to transfer from it, Alice may think that this transfer was probably performed by somebody else, not by Bob.

[phiferd]

I see. So the check is not there to stop the attack, but rather just to encourage people to get in the habit of N -> 0 then 0 -> M?

I do agree that a CAS approach would be better as you suggested in your original doc, but understand that this would not meet the existing spec which is used by many contracts in the wild.

@jakub-wojciechowski if the intention is to create a safeApprove function (outside of the ERC20 spec), why not just include the CAS approach suggested by @mikhail-vladimirov?

function safeApprove(
  address _spender,
  uint256 _currentValue,
  uint256 _value)
returns (bool success)

[jakub-wojciechowski]

Hi, @phiferd, we've kind of agreed to implement Compare And Swap in #437
The discussion was very similar to the one above.

I'm waiting a bit to hear opinions whether there is a point in having to separate decrease and increase methods for the approve function. I'd love to submit a PR with CAS asap.

[frangio]

Thanks for chiming in, everyone. I agree we should remove the non-compliant line.

[novaknole]

Hi all.

I was wondering what's the end solution with this problem ?

approve(0) first and approve(amount) later doesn't solve the problem if we automatically want to connect the contract to dapp as we can't programmatically figure out for Alice in front-end client if bob has front-ran her approve(0) tx. Since we can't do it, this approach seems pointless. It only helps with Alice does it through etherscan and uses EtherCamp (see what I mean).

increaseAllowance and decreaseAllowance have been discouraged from OZ.

So how do you approach the problem ? you just don't pay attention to it anymore and it's what it is or there're some great practices ?

Thanks.

[frangio]

IMO dapps should not be concerned about this problem at all. Dapps will either approve the exact amount that will be needed for a transaction, which will then be consumed entirely or almost entirely (due to slippage) and nullify the alleged problem, or they will use infinite allowance at which point worrying about approve frontrunning makes absolutely no sense.

[novaknole]

IMO dapps should not be concerned about this problem at all. Dapps will either approve the exact amount that will be needed for a transaction, which will then be consumed entirely or almost entirely (due to slippage) and nullify the alleged problem, or they will use infinite allowance at which point worrying about approve frontrunning makes absolutely no sense.

@frangio and who should be concerned then ? in case infinity approval is not safe for a specific project. then what party should be at least observant to make sure things go well ? I'm asking for the worldwide opinion now that if I want to integrate approval in dapp, I just write as i would write in the past and that's it ? whatever happens, happens ?

[frangio]

Essentially yes. Basically no real uses of approve are vulnerable to frontrunning. There are hypothetical exceptions, but no one has ever come across an exception to this in the real world.