Allow all approves #47
Conversation
The approve function should not revert if it approves a non 0 value, even if it's already non 0. The ERC20 standard states that the interface should prevent it, not the contract. "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 compatibility with contracts deployed before" https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md This PR follows this issue: Giveth#45
|
Many tokens like Status or Aragon have this extra protection since the beginning. Until now, I never heard a backward compatibility issue for this. I strongly believe that it doesn't exist. Even if it would exists, it's much easier to update a UI than a Smart Contract. So at this point I fill more comfortable to maintain this extra safety protection. This enforces that the new UIs follow the standard recommendation. And we can say that the contract is safe even if the old UIs. The standard uses the verb "SHOULDN'T" instead of "MUST NOT" so I take it as a strong recommendation which I understand but I decided not to follow. Because of that, this extra protection cannot be interpreted as an incompatibility with the standard. |
|
Hi Jordy, The backward compatibility issue is not about the UI which can be updated. But about a smart contract owning tokens and making an approval (approval reverting could stuck the smart contract interacting with it). Preventing it at the contract level does not protect from the attack because someone can sign a TX to approve 0 and another one to approve X and still suffer from a an attacker spending the tokens before the first TX is included in a block. So to be safe, you need to make sure the first TX is mined before the 2nd and only the interface can do it. Not the smart contract. "Should not" does not mean "not required", it means it's not allowed. And the fact that the THOUGH is capitalize is to emphasis on the issue. The fact that some projects are violating the standard does not mean we should not care about it. So adding this check does not protect from the attack, poses backward compatibility issues and is a violation to the standard. You can obviously disagree with the standard (we can continue the discussion on the EIP repo), but if the MiniMeToken is not ERC20, I think it should be clearly stated. I'm actually interested in using the MiniMeToken while having a ERC20 tokens. But if you don't want to change that's fine, I can easily create a MiniMeTokenERC20 inherinting from the MiniMeToken and overwriting the approve function. |
|
Another thing, if you see the Implementation section in the standard, you will see Minime as an example of how to protect it. So I do think it's a good move to touch it now. Do you know about any old example that needs this functionality? |
|
Yes, because at some point the standard was advising to do so. After discussions, it was decided that it was breaking backward compatibility (and not providing additional security) and we should not do it: It should be removed when this pull request (ethereum/EIPs#789) will be approved. Well, I don't have a production contract with this problem in mind (I can make one example, but that would not be a real one) but that does not mean there is not, I haven't studied the code where it could matter (mostly decentralized exchanges). |
|
Well contrary to what I said below, according to the RFC:
So it can still be considered as standard. |
The approve function should not revert if it approves a non 0 value, even if it's already non 0. The ERC20 standard states that the interface should prevent it, not the contract.
"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 compatibility with contracts deployed before"
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md
This PR follows this issue: #45