0xJuda - Absence of Slippage Protection in LiquidityBorrowingManager#repay

[sherlock-admin2]

0xJuda

high

Absence of Slippage Protection in LiquidityBorrowingManager#repay

Summary

The repay function within LiquidityBorrowingManager currently lacks robust safeguards against slippage, potentially exposing borrowers to losses in their profits.

Vulnerability Detail

Consider a scenario where a borrower's position is profitable, and after repaying their debt, they anticipate receiving the remaining tokens following liquidity restoration. This occurs at the conclusion of the LiquidityBorrowingManager repay function.

LiquidityBorrowingManager.sol#L669-L670

// Pay a profit to a msg.sender
_pay(borrowing.holdToken, address(this), msg.sender, holdTokenBalance);
_pay(borrowing.saleToken, address(this), msg.sender, saleTokenBalance);

The issue arises from the fact that the borrower is paid whatever remains in the contract. This could be zero, even if the borrower initiated the repayment transaction with full awareness of their profitable position. The first part of the problem lies in the calculation of the liquidity restoration amount, which is determined based on stored liquidity figures and the current state of the pool ratio. The second part is that the borrower cannot specify the quantity of tokens they expect to receive.

Current ratio for each lone position is calculated here in _getCurrentSqrtPriceX96.

LiquidityManager.sol#L331-L342

function _getCurrentSqrtPriceX96(
    bool zeroForA,
    address tokenA,
    address tokenB,
    uint24 fee
) private view returns (uint160 sqrtPriceX96) {
    if (!zeroForA) {
        (tokenA, tokenB) = (tokenB, tokenA);
    }
    address poolAddress = computePoolAddress(tokenA, tokenB, fee);
    (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(poolAddress).slot0(); // @audit Slot0 is easily manipulated
}

This function retrieves the current pool state from slot0, which is susceptible to changes between the creation of the borrower's repayment transaction and its processing. The 'sqrtPriceX96' value plays a crucial role in the subsequent call to QuoterV2, used to estimate the amount of sale tokens the contract will receive after swapping hold tokens.

LiquidityManager.sol#L246-L256

// Quote exact input single for swap
uint256 saleTokenAmountOut;
(saleTokenAmountOut, cache.sqrtPriceX96, , ) = underlyingQuoterV2
    .quoteExactInputSingle(
        IQuoterV2.QuoteExactInputSingleParams({
            tokenIn: cache.holdToken,
            tokenOut: cache.saleToken,
            amountIn: holdTokenAmountIn,
            fee: params.fee,
            sqrtPriceLimitX96: 0
        })
    );

The saleTokenAmountOut is later used for amountOutMinimum value when the swap is executed. Because the saleTokenAmountOut represents the outcome of this swap calculated via QuoterV2, slippage protection won't work. It is the same as using block.timestamp in smart contracts for deadline protection.

LiquidityManager.sol#L259-L287

// Perform external swap if external swap target is provided
if (externalSwap.swapTarget != address(0)) {
    _patchAmountsAndCallSwap(
        cache.holdToken,
        cache.saleToken,
        externalSwap,
        holdTokenAmountIn,
        (saleTokenAmountOut * params.slippageBP1000) / Constants.BPS // @audit Slippage protection won't work.
    );
} else {
    // ...
    // Perform v3 swap exact input and update sqrtPriceX96
    _v3SwapExactInput(
        v3SwapExactInputParams({
            fee: params.fee,
            tokenIn: cache.holdToken,
            tokenOut: cache.saleToken,
            amountIn: holdTokenAmountIn,
            amountOutMinimum: (saleTokenAmountOut * params.slippageBP1000) / // @audit Slippage protection won't work.
                Constants.BPS
        })
    );
    // ...

In another scenario, a borrower may not realize a profit, yet they should still receive their remaining collateral and liquidation bonus. Due to market condition changes that can occur between the creation of their transaction and its processing, these assets could also be at risk.

PoC

I have updated the first repay test to demonstrate how output can change when ratio changes. Results for this demonstration aren't as big as they could be because WBTC/WETH is a big pool. This problem would be more severe for pools with lower liquidity because it is easier to move with the price.

Demonstration logs values of two scenarios.

1. Borrower expects a profit, repays loan and gets it.
2. Borrower expects a profit, creates a transaction, another transaction is proccessed before his and because of it he gets less.

WagmiLeverageTests.ts#L424-L452

it("repay borrowing and restore liquidity (long position WBTC zeroForSaleToken = false) will be successful", async () => {
    // ...
+   await hackDonor(DONOR_ADDRESS,[alice.address],[{ tokenAddress: WETH_ADDRESS, amount: ethers.utils.parseEther('1000') },{ tokenAddress: WBTC_ADDRESS, amount: ethers.utils.parseUnits('100', 8) },]);
+   await maxApprove(alice, router.address, [USDT_ADDRESS, WETH_ADDRESS, WBTC_ADDRESS]);

+   // After next transaction the user should end up in profit
+   await router.connect(alice).exactInputSingle({ tokenIn: WETH_ADDRESS, tokenOut: WBTC_ADDRESS, fee: 500, amountIn: ethers.utils.parseEther('500'), amountOutMinimum: 0, recipient: alice.address, deadline: (await time.latest()) + 60, sqrtPriceLimitX96: 0 });

+   // Image the next swap happens after the user initiates transaction and before it is processed
+   // Uncomment next line to log different values than user expects
+   await router.connect(alice).exactInputSingle({ tokenIn: WBTC_ADDRESS, tokenOut: WETH_ADDRESS, fee: 500, amountIn: ethers.utils.parseUnits('50', 8), amountOutMinimum: 0, recipient: alice.address, deadline: (await time.latest()) + 60, sqrtPriceLimitX96: 0 });

    params = {
        isEmergency: false,
        internalSwapPoolfee: 500,
        externalSwap: swapParams,
        borrowingKey: borrowingKey,
        swapSlippageBP1000: 990, //1%
    };

+   const wbtc = await ethers.getContractAt("IERC20", WBTC_ADDRESS);
+   console.log('Bob\' WBTC balance before', (await wbtc.balanceOf(bob.address)).toString());
+   console.log('Bob\' WETH balance before', (await WETH.balanceOf(bob.address)).toString());

    await borrowingManager.connect(bob).repay(params, deadline);
    const rateInfo = await borrowingManager.getHoldTokenDailyRateInfo(WETH_ADDRESS, WBTC_ADDRESS);
    expect(rateInfo[1].totalBorrowed).to.be.equal(0);
    await time.increase(86400);

+   console.log('Bob\' WBTC balance after', (await wbtc.balanceOf(bob.address)).toString());
+   console.log('Bob\' WETH balance after', (await WETH.balanceOf(bob.address)).toString());
});

Transaction in the middle	Token	Before	After
No	WETH	99000000000000000018	99012225608862363471
Yes	WETH	99000000000000000018	99005404442822551938

As you can see the outcomes, in the second scenario user expected 99012225608862363471 tokens, but received only 99005404442822551938. The difference is 6,821,166,039,811,533 wei ~ 0.00682 ether.

In current market conditions, it is around 12 USD (1676 USD for 1 ETH). In the time of market's all-time high where prices are even more volatile this would equal 33 USD (4891 for 1 ETH).

Impact

The repay function fails to offer adequate protection during token swaps from hold tokens to sale tokens. Furthermore, borrowers lack the ability to specify the minimum token amounts they anticipate, resulting in potential losses of profit, collateral, and liquidation bonus.

Code Snippet

LiquidityBorrowingManager.sol#L532

LiquidityBorrowingManager.sol#L669-L670

LiquidityManager.sol#L331-L342

LiquidityManager.sol#L246-L256

LiquidityManager.sol#L259-L287

WagmiLeverageTests.ts#L424-L452

Tool used

Manual Review

Recommendation

To address this issue, it is advised to integrate Uniswap TWAP (Time-Weighted Average Price) for precise slippage protection during token swaps. Additionally, a parameter should be introduced to enable borrowers to specify the minimum token quantities they expect to receive, thus enhancing transparency and security.

I found these params in test where slippage revert should be simulated.

let params: LiquidityBorrowingManager.RepayParamsStruct = {
    isEmergency: false,
    internalSwapPoolfee: 500,
    externalSwap: swapParams,
    borrowingKey: borrowingKey,
    swapSlippageBP1000: 1001, //<=slippage simulated
};

The 1001 slippage value should revert every time because it basically says that you expect more than 100% of what you should get. I would suggest finding another way to simulate slippage revert.

Duplicate of #109