Swapping logic would be broken for some supported tokens

[c4-bot-8]

Lines of code

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

Vulnerability details

Proof of Concept

First take a look at this excerpt from the readMe https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/README.md#L103-L106

| Question                                                                                                                                                   | Answer                                      |
| ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------- |
| Chains the protocol will be deployed on                                                                                                                    | Ethereum,Arbitrum,Base,BSC,Optimism,Polygon |
| ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ------                                      |
| [Revert on zero value approvals](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#revert-on-zero-value-approvals)                                    | Yes                                         |

From the above we can conclude that tokens like BNB that revert on zero valu approvals are to be integrated within protocol.

Now take a look at https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L537-L567

    function _swap(IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 amountOutMin, bytes memory swapData) internal returns (uint256 amountInDelta, uint256 amountOutDelta) {
        if (amountIn != 0 && swapData.length != 0 && address(tokenOut) != address(0)) {
            uint256 balanceInBefore = tokenIn.balanceOf(address(this));
            uint256 balanceOutBefore = tokenOut.balanceOf(address(this));

            // approve needed amount
            _safeApprove(tokenIn, swapRouter, amountIn);
            // execute swap
            (bool success,) = swapRouter.call(swapData);
            if (!success) {
                revert ("swap failed!");
            }

            // reset approval
            //@audit resetting the approval would never work for these tokens
            _safeApprove(tokenIn, swapRouter, 0);

            uint256 balanceInAfter = tokenIn.balanceOf(address(this));
            uint256 balanceOutAfter = tokenOut.balanceOf(address(this));

            amountInDelta = balanceInBefore - balanceInAfter;
            amountOutDelta = balanceOutAfter - balanceOutBefore;

            // amountMin slippage check
            if (amountOutDelta < amountOutMin) {
                revert SlippageError();
            }

            // event for any swap with exact swapped value
            emit Swap(address(tokenIn), address(tokenOut), amountInDelta, amountOutDelta);
        }
    }

This is the general swap function that eventually gets called which uses the external router with off-chain calculated swap instruction, issue howver is that after approving the initial amount needed to the router, there is a need to reset these approvals, however resetting these approvals whereas would work for most tokens would not work for a token like BNB that's to be supported, see minimalistic coded POC here, which then translates to a direct break in swapping for these tokens and other instances where _swap() gets called across protocol.

Additionally, it seem the _safeResetAndApprove() function was coded to help sort this issue, but is not being used in _swap(), https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L713-L724

    /// @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);
    }

Impact

As hinted under Proof of Concept, swaps would be completely broken for tokens that revert on zero value approvals, due to a reversion that always occurs on this line.

Recommended Mitigation Steps

Consider try/catching the attempt to safeApprove in _swap() and in the case it reverts, query _safeResetAndApprove() instead. Alternatively, do not support tokens that revert on zero value approvals.

Assessed type

Context

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as primary issue

[c4-judge]

3docSec marked the issue as selected for report

[3docSec]

Behavior is in scope as per README