Lack of slippage protection can lead to a significant loss of user funds

Summary

The protocol interacts with Uniswap, but the parameters were configured incorrectly by default without slippage protection, giving rise to MEV bot attacks.

Vulnerability Detail

The main functions are borrow() and repay() which are available to the user. If a user wants to give a loan the borrow() function: _precalculateBorrowing() is called, then _extractLiquidity() and finally _decreaseLiquidity().

When _decreaseLiquidity() is called, the protocol interacts with Uniswap but amount0Min: 0 and amount1Min: 0 are set to 0. This means that it is the minimum amount that the person accepts when the exchange is made. liquidity, would be the expected tokens.

INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: tokenId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })

The result goes for a final check:

 if (amount0 == 0 && amount1 == 0) {
            revert InvalidBorrowedLiquidity(tokenId);
        }

But this is not enough, one of the two values can be zero, close to zero or both close to zero. And there is no other validation for these results.

This loss of funds is possible in the case of a Sandwich attack.

Impact

A malicious attack can decrease the expected amount of token expected when extract their liquidity. Bringing losses to the user.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/abstract/LiquidityManager.sol#L356-L358

Tool used

Manual Review

Recommendation

Use parameters amount0Min, amount1Min and deadline correctly to avoid loss of funds. https://uniswapv3book.com/docs/milestone_3/slippage-protection/#slippage-protection-in-swaps

sherlock-admin2 commented 1 year ago

1 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

We have slippage check in parent function

pinalikefruit commented 1 year ago

ESCALATED

In the main function, the result of the exchange for the sale token is verified. _precalculateBorrowing()

        // Ensure that the received holdToken balance meets the minimum required
        if (cache.holdTokenBalance < params.minHoldTokenOut) {
            revert TooLittleReceivedError(params.minHoldTokenOut, cache.holdTokenBalance);
        }

But it does not check when you want to extract liquidity and store the borrowed amount:

 if (amount0 == 0 && amount1 == 0) {
            revert InvalidBorrowedLiquidity(tokenId);
        }

How you can see, this validation is not good enough.

sherlock-admin2 commented 1 year ago

ESCALATED

In the main function, the result of the exchange for the sale token is verified. _precalculateBorrowing()
        // Ensure that the received holdToken balance meets the minimum required
        if (cache.holdTokenBalance < params.minHoldTokenOut) {
            revert TooLittleReceivedError(params.minHoldTokenOut, cache.holdTokenBalance);
        }
But it does not check when you want to extract liquidity and store the borrowed amount:
 if (amount0 == 0 && amount1 == 0) {
            revert InvalidBorrowedLiquidity(tokenId);
        }
How you can see, this validation is not good enough.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

sherlock-audit / 2023-10-real-wagmi-judging

pinalikefruit - Lack of slippage protection can lead to a significant loss of user funds #157