SwapAction's newly implemented Kyber swaps lack slippage protection

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L293-L305

Vulnerability details

Proof of Concept

Previously SwapAction only included 4 swap options, however with the current scope we now have five, see https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L21-L28

enum SwapProtocol {
    BALANCER,
    UNIV3,
    PENDLE_IN,
    PENDLE_OUT,
+   KYBER
}

Now all these previous implementations of querying the swap protocol includes a slippage protection, for .eg if uniswap is to be used we enforce it by and :
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L319-L353

    function uniV3Swap(
        SwapType swapType,
        address assetIn,
        uint256 amount,
        uint256 limit,//@audit slippage
        address recipient,
        uint256 deadline,
        bytes memory args
    ) internal returns (uint256) {
        if (swapType == SwapType.EXACT_IN) {
            IERC20(assetIn).forceApprove(address(uniRouter), amount);
            return
                uniRouter.exactInput(
                    ExactInputParams({
                        path: args,
                        recipient: recipient,
                        amountIn: amount,
                        amountOutMinimum: limit,//@audit slippage
                        deadline: deadline
                    })
                );
        } else {
            IERC20(assetIn).forceApprove(address(uniRouter), limit);
            return
                uniRouter.exactOutput(
                    ExactOutputParams({
                        path: args,
                        recipient: recipient,
                        amountOut: amount,
                        amountInMaximum: limit,//@audit slippage
                        deadline: deadline
                    })
                );
        }
    }

Similarly if it's a join via pendle, the minOut parameter is sued to protect users from unfair slippage, see https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L374-L382

    function pendleJoin(address recipient, uint256 minOut, bytes memory data) internal returns (uint256 netLpOut) {

..snip
        (netLpOut, , ) = pendleRouter.addLiquiditySingleToken(
            recipient,
            market,
            minOut,
            guessPtReceivedFromSy,
            input,
            limit
        );
        ..snip
    }

The above is the case for all other swap implementations in the swapAction.sol.

Issue however is that the new implementation of swaps via Kyber router do not include any slippage whatsoever, see https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L293-L305

    /// @notice Perform an swap using Kyber MetaAggregationRouterV2
    /// @param assetIn Token to swap from
    /// @param amountIn Amount of `assetIn` to swap
    /// @param payload tx calldata to use when calling the kyber router,
    /// this calldata can be generated using the kyber swap api
    /// @return _ Amount of tokens received from the swap
    function kyberSwap(address assetIn, uint256 amountIn, bytes memory payload) internal returns (uint256) {
        IERC20(assetIn).forceApprove(address(kyberRouter), amountIn);
        (bool success, bytes memory result) = kyberRouter.call(payload);
        if (!success) _revertBytes(result);
        return abi.decode(result, (uint256));
    }

The above method is used to perform a swap using Kyber's MetaAggregationRouterV2, and the amount of tokens received from the swap is returned via abi.decode(result, (uint256)) as hinted earlier on though, there is no check that this value is acceptable by the initiator of the swap just as with other swap implementations.

Impact

Loss of funds for swap initiators considering even if an unacceptable amount of tokens is returned after the swap on Kyber no reverts happen.

Alternatively check the received tokens against the limit in the swapParams: https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L36-L50

struct SwapParams {
    SwapProtocol swapProtocol;
    SwapType swapType;
    address assetIn;
    uint256 amount; // Exact amount in or exact amount out depending on swapType
    uint256 limit; // Min amount out or max amount in depending on swapType
    address recipient;
    address residualRecipient; // Address to send any residual tokens to
    uint256 deadline;
    /// @dev `args` can be used for protocol specific parameters
    /// For Balancer, it is the `poolIds` and `assetPath`
    /// For Uniswap v3, it is the `path` for the swap
    bytes args;
}

Recommended Mitigation Steps

Accept a minOut param for Kyber swaps to and check the value from abi.decode(result, (uint256)) to be above this minOut value.

Assessed type

Other