Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lack of Price Impact & Slippage Protection in quoteExactInput #6

Closed
howlbot-integration bot opened this issue Nov 4, 2024 · 3 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_17_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/periphery/lens/MixedRouteQuoterV1.sol#L145

Vulnerability details

Summary

The quoteExactInput function in MixedRouteQuoterV1 lacks price impact and slippage protection mechanisms, potentially exposing users to significant financial loss through unfavorable quotes in low liquidity conditions or manipulated pools.

The function quotes prices across multiple pools but:

  • Uses no price impact checks
  • Has no slippage limits
  • Doesn't validate price movements between pools
  • Sets sqrtPriceLimitX96 to 0, effectively removing price bounds

Current Vulnerable Implementation

function quoteExactInput(bytes memory path, uint256 amountIn) public returns (
    uint256 amountOut,
    uint160[] memory v3SqrtPriceX96AfterList,
    uint32[] memory v3InitializedTicksCrossedList,
    uint256 v3SwapGasEstimate
) {
    // ... initialization ...

    while (true) {
        (address tokenIn, address tokenOut, uint24 fee) = path.decodeFirstPool();

        if (fee & flagBitmask != 0) {
            // V2 quote with no price impact check
            amountIn = quoteExactInputSingleV2(
                QuoteExactInputSingleV2Params({
                    tokenIn: tokenIn,
                    tokenOut: tokenOut,
                    amountIn: amountIn
                })
            );
        } else {
            // V3 quote with no price limits
            (uint256 _amountOut, uint160 _sqrtPriceX96After,,,) = 
                quoteExactInputSingleV3(
                    QuoteExactInputSingleV3Params({
                        tokenIn: tokenIn,
                        tokenOut: tokenOut,
                        fee: fee,
                        amountIn: amountIn,
                        sqrtPriceLimitX96: 0  // No price limit protection
                    })
                );
            amountIn = _amountOut;
        }
        // ... path handling ...
    }
}

Impact

  1. Financial Risk:

    • Users might receive quotes with extreme price impact
    • No protection against sandwich attacks
    • Potential for significant slippage in multi-hop routes
  2. Manipulation Vulnerability:

    • Attackers can manipulate low liquidity pools
    • Flash loan attacks could temporarily skew prices
    • No detection of abnormal price movements

Attack Scenarios

Scenario 1: Low Liquidity Manipulation

// Attacker temporarily removes liquidity from a pool
function attackLowLiquidity() {
    // 1. Remove large amount of liquidity
    pool.withdraw(largeAmount);
    
    // 2. Victim gets quote with terrible price
    quoter.quoteExactInput(path, amount);  // Returns unfavorable quote
    
    // 3. Restore liquidity
    pool.deposit(largeAmount);
}

Scenario 2: Multi-Hop Price Impact

// Path: TokenA -> TokenB -> TokenC where TokenB pool is thin
bytes path = encodePath([tokenA, tokenB, tokenC], [fee1, fee2]);
// Large trade causes severe price impact, but no checks prevent it
quoter.quoteExactInput(path, largeAmount);  // Extreme price impact

Fixed Implementation

struct PriceImpactParams {
    uint256 maxPriceImpactBps;
    uint160 minSqrtPriceX96;
    uint160 maxSqrtPriceX96;
    uint256 minAmountOut;
}

function quoteExactInput(
    bytes memory path,
    uint256 amountIn,
    PriceImpactParams memory priceParams
) public returns (
    uint256 amountOut,
    uint160[] memory v3SqrtPriceX96AfterList,
    uint32[] memory v3InitializedTicksCrossedList,
    uint256 v3SwapGasEstimate
) {
    require(priceParams.maxPriceImpactBps > 0, "Invalid price impact param");
    require(priceParams.maxPriceImpactBps <= 10000, "Price impact too high"); // max 100%
    
    v3SqrtPriceX96AfterList = new uint160[](path.numPools());
    v3InitializedTicksCrossedList = new uint32[](path.numPools());

    uint256 startingAmount = amountIn;
    uint256 i = 0;

    while (true) {
        (address tokenIn, address tokenOut, uint24 fee) = path.decodeFirstPool();

        // Get oracle price for comparison
        uint256 oraclePrice = getOraclePrice(tokenIn, tokenOut);
        
        if (fee & flagBitmask != 0) {
            // V2 quote with price impact check
            uint256 reserveIn;
            uint256 reserveOut;
            (reserveIn, reserveOut) = KatanaV2Library.getReserves(factoryV2, tokenIn, tokenOut);
            
            // Check minimum liquidity
            require(reserveIn > minLiquidity && reserveOut > minLiquidity, 
                    "Insufficient liquidity");

            uint256 amountOut = quoteExactInputSingleV2(
                QuoteExactInputSingleV2Params({
                    tokenIn: tokenIn,
                    tokenOut: tokenOut,
                    amountIn: amountIn
                })
            );

            // Calculate and validate price impact
            uint256 executionPrice = (amountOut * 1e18) / amountIn;
            uint256 priceImpactBps = calculatePriceImpactBps(
                executionPrice, 
                oraclePrice
            );
            
            require(priceImpactBps <= priceParams.maxPriceImpactBps, 
                    "V2 price impact too high");
            
            amountIn = amountOut;
        } else {
            // V3 quote with price limits
            (uint256 _amountOut, uint160 _sqrtPriceX96After, 
             uint32 _initializedTicksCrossed, uint256 _gasEstimate) =
                quoteExactInputSingleV3(
                    QuoteExactInputSingleV3Params({
                        tokenIn: tokenIn,
                        tokenOut: tokenOut,
                        fee: fee,
                        amountIn: amountIn,
                        sqrtPriceLimitX96: priceParams.maxSqrtPriceX96
                    })
                );

            // Validate V3 price movement
            require(_sqrtPriceX96After >= priceParams.minSqrtPriceX96, 
                    "Price below minimum");
            require(_sqrtPriceX96After <= priceParams.maxSqrtPriceX96, 
                    "Price above maximum");

            // Calculate and validate price impact
            uint256 priceImpactBps = calculateV3PriceImpactBps(
                _sqrtPriceX96After,
                getV3OracleSqrtPrice(tokenIn, tokenOut)
            );
            
            require(priceImpactBps <= priceParams.maxPriceImpactBps, 
                    "V3 price impact too high");

            v3SqrtPriceX96AfterList[i] = _sqrtPriceX96After;
            v3InitializedTicksCrossedList[i] = _initializedTicksCrossed;
            v3SwapGasEstimate += _gasEstimate;
            amountIn = _amountOut;
        }
        
        i++;

        if (!path.hasMultiplePools()) {
            // Final output validation
            require(amountIn >= priceParams.minAmountOut, 
                    "Insufficient output amount");
            
            // Validate total route price impact
            uint256 totalPriceImpactBps = calculateTotalPriceImpactBps(
                startingAmount,
                amountIn,
                path
            );
            require(totalPriceImpactBps <= priceParams.maxPriceImpactBps, 
                    "Total price impact too high");
                    
            return (
                amountIn,
                v3SqrtPriceX96AfterList,
                v3InitializedTicksCrossedList,
                v3SwapGasEstimate
            );
        }
        path = path.skipToken();
    }
}

function calculatePriceImpactBps(
    uint256 executionPrice,
    uint256 oraclePrice
) internal pure returns (uint256) {
    if (executionPrice >= oraclePrice) {
        return 0;
    }
    return ((oraclePrice - executionPrice) * 10000) / oraclePrice;
}

function calculateV3PriceImpactBps(
    uint160 executionSqrtPrice,
    uint160 oracleSqrtPrice
) internal pure returns (uint256) {
    // Convert sqrt prices to regular prices for comparison
    uint256 executionPrice = uint256(executionSqrtPrice) * 
                            uint256(executionSqrtPrice) / 2**192;
    uint256 oraclePrice = uint256(oracleSqrtPrice) * 
                         uint256(oracleSqrtPrice) / 2**192;
    return calculatePriceImpactBps(executionPrice, oraclePrice);
}

The fix adds comprehensive price impact and slippage protection while maintaining the core quoting functionality.

Assessed type

Invalid Validation

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_17_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Nov 4, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@khangvv
Copy link

khangvv commented Nov 4, 2024

MixedRouteQuoterV1 is designed to closely simulate on-chain swap execution, providing users with reliable quote amounts. This approach allows users to make informed decisions and set slippage or price impact checks later in the actual transaction. Based on this, I believe the concerns raised in this issue are not applicable.

Just as a note, the implementation also follows Uniswap’s original design.

@khangvv khangvv added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 4, 2024
@alex-ppg
Copy link

This submission is similar to #27 and the same rationale applies here; namely, manipulation of read-only calls is not really feasible as the contract is meant to be invoked off-chain similarly to the original Uniswap V3 implementation.

@c4-judge
Copy link

alex-ppg marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_17_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants