Incorrect implementation of approve function

[sbuljac]

From GST1.sol

    // Spec: Allow `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`.
    function approve(address spender, uint256 value) public returns (bool success) {
        address owner = msg.sender;
        if (value != 0 && s_allowances[owner][spender] != 0) {
            return false;
        }
        s_allowances[owner][spender] = value;
        Approval(owner, spender, value);
        return true;
    }

From comment:

If this function is called again it overwrites the current allowance with value.

This is not true, if this function is called second time(or multiple time) it will not overwrite previus allowance unless it is 0.

Correct implementation would be:

  function approve(address spender, uint256 value) public returns (bool) {
    require(spender != address(0));

    s_allowances[msg.sender][spender] = value;
    emit Approval(msg.sender, spender, value);
    return true;
  }

[k06a]

@sbuljac this is for protection reason. That's why increaseAllowance and decreaseAllowance exist in OpenZeppelin ERC20 implementation: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20.sol

[k06a]

@sbuljac imagine:

1. I approved 20 tokens for you
2. You asked about additional 10 tokens
3. I send tx approve(@sbuljac, 30)
4. You send tx with higher gas price transferFrom(@k06a, @sbuljac, 20)
5. You got 20 tokens, then my transaction got mined
6. You send tx transferFrom(@k06a, @sbuljac, 30)

So you got 20+30 tokens instead of 30 tokens I allowed you to get.

[sbuljac]

Not sure what do you mean. If you meant to implement it this way than you should fix spec in the comment. Regarding OpenZeppelin implementation, the code I posted above is from OpenZeppeling ERC20 implementation.

[lorenzb]

@sbuljac You're correct. Our implementation of approve is not compliant with EIP-20. This was commonly done before EIP-20 was standardized. OpenZeppelin also used to do this in the past: https://github.com/OpenZeppelin/openzeppelin-solidity/issues/438

EIP-20 acknowledges this:

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

(Another example of this being an "old-school" ERC-20: Our methods return false instead of reverting when something goes wrong.)

@k06a Thanks for joining the discussion and explaining the reasoning behind this choice ❤️

---

I don't want to modify the documentation, since the contract is deployed on mainnet and it would probably be confusing if the version in the repo were different from the deployed one.

Closing this issue for now. Feel free to reopen if you'd like to anything.