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

Allow all approves #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow all approves #47

wants to merge 1 commit into from

Conversation

clesaege
Copy link

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

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
@jbaylina
Copy link
Contributor

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.

@clesaege
Copy link
Author

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.

@jbaylina
Copy link
Contributor

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?

@clesaege
Copy link
Author

clesaege commented Dec 12, 2017

Yes, because at some point the standard was advising to do so.
See this commit ethereum/EIPs@7e6905c#diff-c846f31381e26d8beeeae24afcdf4e3e

After discussions, it was decided that it was breaking backward compatibility (and not providing additional security) and we should not do it:
ethereum/EIPs@09dc057#diff-c846f31381e26d8beeeae24afcdf4e3e
But in this commit, they forget to remove it from the Implementation section.

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).

@clesaege
Copy link
Author

Well contrary to what I said below, according to the RFC:
http://www.rfc-base.org/txt/rfc-2119.txt

SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.

So it can still be considered as standard.
I still think that there no "particular circumstances" for the MiniMe to not follow this recommendation.

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

Successfully merging this pull request may close these issues.

2 participants