Inconsistent and potentially reverting handling of tokens that do not allow an approval amount of 0

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L551

Vulnerability details

Impact

Common._swap() may revert for tokens that do not allow an approval amount of 0.

Proof of Concept

As indicated by Common._safeResetAndApprove() some tokens do not allow an approval amount of 0:

/// @dev some tokens require allowance == 0 to approve new amount
/// but some tokens does not allow approve ammount = 0
/// we try to set allowance = 0 before approve new amount. if it revert means that
/// the token not allow to approve 0, which means the following line code will work properly
function _safeResetAndApprove(IERC20 token, address _spender, uint256 _value) internal {
    /// @dev ommited approve(0) result because it might fail and does not break the flow
    address(token).call(abi.encodeWithSelector(token.approve.selector, _spender, 0));

    /// @dev value for approval after reset must greater than 0
    require(_value > 0);
    _safeApprove(token, _spender, _value);
}

However, inside Common._swap() such an approval of 0 is attempted at L551 _safeApprove(tokenIn, swapRouter, 0);, where _safeApprove() is:

function _safeApprove(IERC20 token, address _spender, uint256 _value) internal {
    bytes memory returnData = address(token).functionCall(abi.encodeWithSelector(token.approve.selector, _spender, _value));
    if (returnData.length > 0) { // Return data is optional
        require(abi.decode(returnData, (bool)), "SafeERC20: ERC20 operation did not succeed");
    }
}

This means that L551 may cause a revert with such a token.

Recommended Mitigation Steps

Take precautions similar to those in _safeResetAndApprove(), setting approval to 1 in case of failure.

Assessed type

ERC20

[3docSec]

Report of satisfactory quality and weird token behavior is in scope as per README

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[c4-judge]

3docSec marked the issue as duplicate of #10

[c4-judge]

3docSec marked the issue as not selected for report