The _swap is set to zero slippage by default and doesn't protect the users properly.

[hats-bug-reporter]

Github username: @catellaTech
Twitter username: catellatech
Submission hash (on-chain): 0xca61dcbf9bfe2303ceb41e6834f39e19dad96447ea165e96693e1683e906d4fd
Severity: medium

Description:

Severity: Medium

Description:

The _swap function is designed to facilitate token exchanges through a swap contract. However, its current implementation contains several vulnerabilities that can be exploited, particularly concerning slippage logic and transaction timing.

Identified Issues

1. Zero Slippage Setting:
   • Configuring slippage to zero can cause legitimate user transactions to fail if the price changes before their transaction is processed. Check the code:

    function _swap(address[] memory path, uint256[] memory flag) private {
        uint256 amountIn_;
        require(path.length - 1 == flag.length);
        for (uint256 i; i < flag.length; i++) {
            (address input, address output) = (path[i], path[i + 1]);
            (uint256 k, uint256 j, address swapContract) =
                SmartRouterHelper.getStableInfo(stableSwapFactory, input, output, flag[i]);
            if (input == ROSE) {
                amountIn_ = address(this).balance;
                IStableSwap(swapContract).exchange{ value: amountIn_ }(k, j, amountIn_, 0);
            }
            if (input != ROSE) {
                amountIn_ = IERC20(input).balanceOf(address(this));
                TransferHelper.safeApprove(input, swapContract, amountIn_); 
                // This means that anyone with enough capital can force arbitrarily large slippage by
                // sandwiching
                // transactions, close to 100%.

                IStableSwap(swapContract).exchange(k, j, amountIn_, 0); // @audit <-- slippage set to 0
            }

2. Frontrunning Vulnerability:
   • An attacker can frontrun a legitimate user's transaction, altering the expected amountOut and causing the transaction to fail due to the check require(amountOut >= amountOutMin).

Impact:

• Transaction Failures: Legitimate transactions may fail due to price fluctuations, leading to poor user experience.
• Frontrunning Exploitation: Attackers can manipulate transactions, resulting transactions may fail.

Proposed Improvements

To mitigate these vulnerabilities, the following improvements are recommended for the _swap function:

Improved Code for the _swap Function

Below is an enhanced version of the _swap function that incorporates these suggestions:

function _swap(address[] memory path, uint256[] memory flag, uint256 amountOutMin) private {
    uint256 amountIn_;
    require(path.length - 1 == flag.length, "Invalid path or flag length");

    for (uint256 i; i < flag.length; i++) {
        (address input, address output) = (path[i], path[i + 1]);
        (uint256 k, uint256 j, address swapContract) = SmartRouterHelper
            .getStableInfo(stableSwapFactory, input, output, flag[i]);

        if (input == ROSE) {
            amountIn_ = address(this).balance;
            IStableSwap(swapContract).exchange{value: amountIn_}(k, j, amountIn_, 0);
        } else {
            amountIn_ = IERC20(input).balanceOf(address(this));
            TransferHelper.safeApprove(input, swapContract, amountIn_);

            // Estimate the expected amount out before the swap
            uint256 estimatedAmountOut = IStableSwap(swapContract).getEstimatedAmountOut(k, j, amountIn_);

            // Execute the exchange 
            IStableSwap(swapContract).exchange(k, j, amountIn_, amountOutMin); // <- proper slippage setUp
        }
    }
}

Improvements in the exactInputStableSwap Function

Additionally, here is the adjusted exactInputStableSwap function to include slippage:

function exactInputStableSwap(
    address[] calldata path,
    uint256[] calldata flag,
    uint256 amountIn,
    uint256 amountOutMin,  // Minimum output tokens expected
    address to
) external payable nonReentrant returns (uint256 amountOut) {
    
    require(!isKill, "Contract is killed");
    
    // Perform the swap
    _swap(path, flag, amountOutMin);  // Pass the slippage to _swap

    // Obtain the output amount
    if (dstToken == ROSE) {
        amountOut = address(this).balance;
    } else {
        amountOut = IERC20(dstToken).balanceOf(address(this));
    }

    // Ensure the output amount is greater than or equal to the expected minimum
    require(amountOut >= amountOutMin, "Output amount too low");  // This line may cause the user's initial tx to fail if slippage is too high.

    // Adjust the destination address and execute the transfer
    if (to == Constants.MSG_SENDER) {
        to = msg.sender;
    } else if (to == Constants.ADDRESS_THIS) {
        to = address(this);
    }

    if (to != address(this)) {
        pay(dstToken, address(this), to, amountOut);
    }

    emit StableExchange(msg.sender, amountIn, path[0], amountOut, path[path.length - 1], to);
}