🚫Improper use of Open Zeppelins safeApprove() for Non-zero Allowance Increments

Book an audit with Zokyo

This vulnerability arises from incorrect usage of OpenZeppelin's safeApprove() function. The function is designed to mitigate race conditions in ERC20 token approvals. The implementation of safeApprove() is designed for setting initial allowances or resetting allowances to zero, and it will revert if called to change an allowance from a non-zero value to another non-zero value. This is due to a specific require statement in the function definition.

Here's the function definition for safeApprove:

function safeApprove(IERC20 token, address spender, uint256 value) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    // solhint-disable-next-line max-line-length
    require((value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

In this function, the require statement enforces that the new value is either zero, or that the current allowance is zero. This makes it inappropriate for use in situations where the current allowance is non-zero, and a different non-zero allowance is desired.

Several smart contracts have been found to misuse safeApprove() by attempting to use it to increase allowances, expecting this to work similarly to safeIncreaseAllowance(). But, this misuse of safeApprove() causes the function to revert if the current allowance is not zero, leading to potential denial of service and preventing successful token deposits.

function _depositIn() {
    // Check if the current allowance is less than the contract's token balance
    if (ERC20(token).allowance(address(this), address(vault)) < ERC20(token).balanceOf(address(this))) {
        // Use safeApprove() to give maximum allowance to the Yearn Vault
        // This will revert if the current allowance is not zero
        SafeERC20.safeApprove(IERC20(token), address(vault), uint256.max);
    }
}

In the code above, the developer is attempting to increase the allowance if it is less than the contract's token balance. However, the use of safeApprove() here will cause the function to revert if the current allowance is not zero.

Mitigation:

Use OpenZeppelin's safeIncreaseAllowance() function to safely increase allowances instead of safeApprove(). The safeIncreaseAllowance() function will not revert if the current allowance is not zero, and is therefore safer to use in scenarios where the allowance needs to be increased.

function _depositInVault() {
    // Check if the current allowance is less than the contract's token balance
    if (ERC20(token).allowance(address(this), address(vault)) < ERC20(token).balanceOf(address(this))) {
        // Use safeIncreaseAllowance() to give maximum allowance to the Yearn Vault
        SafeERC20.safeIncreaseAllowance(IERC20(token), address(vault), uint256.max);
    }
}

The corrected code above uses safeIncreaseAllowance() instead of safeApprove(). This avoids the problem of the function reverting when the current allowance is not zero, and fits the intended scenario of increasing an existing, non-zero allowance.