Openzeppelin-contracts: 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.