SafeERC20.safeApprove() Has unnecessary and unsecure added behavior

[BrendanChou]

SafeERC20.safeApprove() should not have the added functionality of checking the existing allowance of the token. It should simply call token.approve().

The purpose of this added code is to check that a user is either setting their allowance to zero, or setting it from zero. This is so that the user doesn't try to re-set their allowance from one non-zero number to another non-zero number where they can get front-run by the approved address using their allowance in-between those two transactions. However, this code doesn't actually solve the problem as the approval will still pass if the sandwich attack uses the rest of the remaining allowance.

Such behavior lends itself to bugs, unnecessary gas usage, and a false sense of security. In addition, there is no provided method in SafeERC20 that replicates the behavior of approve for tokens that are not ERC20 compliant, making this problem even more frustrating.

SafeERC20 already has safeIncreaseAllowance and safeDecreaseAllowance to solve this problem. Neither of which can be front-run.

Lastly, this allowance issue should not even be solved within this file. The intention of this file should solely be to wrap non-compliant ERC20 functions which was the original intention of this file.

 * @dev Wrappers around ERC20 operations that throw on failure (when the token
 * contract returns false). Tokens that return no value (and instead revert or
 * throw on failure) are also supported, non-reverting calls are assumed to be
 * successful.

[abcoathup]

Hi @BrendanChou! Thanks for the suggestion, it is really appreciated.

The project owner should review your suggestion during the next week.

[BrendanChou]

Thanks. Another possibility (if you don't want breaking changes) is to add another function to call approve in a "safe" manner without all the extra code.

[BrendanChou]

The reason this code is so bug-prone is because this behavior is not natively present when calling approve() and can therefore easily create unintended reverts that lock funds in smart contracts that use this code. Based on the documentation in the readmes and at the top of the file, one would assume that it is doing a safer, more-compatible version of the approve call (when it is in fact doing a more-restrictive version of the function-call)

[frangio]

Thanks for the detailed report @BrendanChou. Unfortunately "safe" is not a precise term but it has stuck. In this case it has lead to a deviation from the original purpose as you point out.

I wouldn't say that the library should be strictly restricted to wrapping non-compliant tokens, because I think safeIncreaseAllowance and safeDecreaseAllowance are also valuable. But I do agree that the checks in safeApprove are out of place. FWIW, @nventuro mentions that even in the scenario that you describe the gas estimation would fail for the end user attempting to send the transaction if the allowance is not already zero at that moment, which mitigates the problem and could be seen as an improvement over plain approve.

That's not enough, though, and I agree that use of safeApprove as it is now should be discourgaed. Unfortunately it's a breaking change to remove it, but we can deprecate it and place a warning in the documentation. We're not sure that adding an additional function that only does the wrapping is a good idea, given that approve is discouraged and the increment/decrement functions seem to be a good alternative. Is using those functions acceptable in your case?