Kral01 - [H-02] SqrtPriceX96 is calculation is not done correctly which can lead to loss of funds.

[sherlock-admin2]

Kral01

high

[H-02] SqrtPriceX96 is calculation is not done correctly which can lead to loss of funds.

Summary

SqrtPriceX96 is calculation is done wrongly which can lead to loss of funds making repay function unusable in non-emergency cases.

Vulnerability Detail

While calling repay function in LiquidityBorrowingManager.sol during non-emergency cases the function calls LiquidityManager#_restoreLiquidity which calls LiquidityManager#_getCurrentSqrtPriceX96 which gives the value of SqrtPriceX96 , but this value can't be used as it is. Our protocol does this and this makes the _increaseLiquidity return totally wrong values for amount1 and 'amount0'. This makes the contract to make wrong calculations later on or totally revert the repay function most of the times.

Impact

Here is an end-end coded PoC that shows how the wrong value of SqrtPriceX96 that ultimately reverts the repay function everytime it is called in non-emergency conditions.

1. Create a Makefile and add your mainnet rpc and a recent block number.

   
   include .env

test_func: @forge test --fork-url ${MAINNET_RPC} --fork-block-number ${MAINNET_BLOCK} -vvvv --ffi --mt ${P}

2. Copy paste this code and run make test_func P=test_RepayRevert
```solidity

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import "forge-std/Test.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { IUniswapV3Pool } from "@uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol";
import { LiquidityBorrowingManager } from "contracts/LiquidityBorrowingManager.sol";
import { AggregatorMock } from "contracts/mock/AggregatorMock.sol";
import { HelperContract } from "../testsHelpers/HelperContract.sol";
import { INonfungiblePositionManager } from "contracts/interfaces/INonfungiblePositionManager.sol";

import { ApproveSwapAndPay } from "contracts/abstract/ApproveSwapAndPay.sol";

import { LiquidityManager } from "contracts/abstract/LiquidityManager.sol";

import { TickMath } from "../../contracts/vendor0.8/uniswap/TickMath.sol";

import { console } from "forge-std/console.sol";

contract ContractTest is Test, HelperContract {
    IERC20 WBTC = IERC20(0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599);
    IERC20 USDT = IERC20(0xdAC17F958D2ee523a2206206994597C13D831ec7);
    IERC20 WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    IUniswapV3Pool WBTC_WETH_500_POOL = IUniswapV3Pool(0x4585FE77225b41b697C938B018E2Ac67Ac5a20c0);
    IUniswapV3Pool WETH_USDT_500_POOL = IUniswapV3Pool(0x11b815efB8f581194ae79006d24E0d814B7697F6);
    address constant NONFUNGIBLE_POSITION_MANAGER_ADDRESS =
        0xC36442b4a4522E871399CD717aBDD847Ab11FE88;
    /// Mainnet, Goerli, Arbitrum, Optimism, Polygon
    address constant UNISWAP_V3_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984;
    /// Mainnet, Goerli, Arbitrum, Optimism, Polygon
    address constant UNISWAP_V3_QUOTER_V2 = 0x61fFE014bA17989E743c5F6cB21bF9697530B21e;
    bytes32 constant UNISWAP_V3_POOL_INIT_CODE_HASH =
        0xe34f199b19b2b4f47f68442619d555527d244f78a3297ea89325f843f87b8b54;
    /// Mainnet, Goerli, Arbitrum, Optimism, Polygon
    address constant alice = 0x70997970C51812dc3A010C7d01b50e0d17dc79C8;
    address constant bob = 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC;
    AggregatorMock aggregatorMock;
    LiquidityBorrowingManager borrowingManager;

    uint256 tokenId;

    uint256 deadline = block.timestamp + 15;

    function setUp() public {
        vm.createSelectFork("mainnet", 17_329_500);
        vm.label(address(WETH), "WETH");
        vm.label(address(USDT), "USDT");
        vm.label(address(WBTC), "WBTC");
        vm.label(address(WBTC_WETH_500_POOL), "WBTC_WETH_500_POOL");
        vm.label(address(WETH_USDT_500_POOL), "WETH_USDT_500_POOL");
        vm.label(address(this), "ContractTest");
        aggregatorMock = new AggregatorMock(UNISWAP_V3_QUOTER_V2);
        borrowingManager = new LiquidityBorrowingManager(
            NONFUNGIBLE_POSITION_MANAGER_ADDRESS,
            UNISWAP_V3_QUOTER_V2,
            UNISWAP_V3_FACTORY,
            UNISWAP_V3_POOL_INIT_CODE_HASH
        );
        vm.label(address(borrowingManager), "LiquidityBorrowingManager");
        vm.label(address(aggregatorMock), "AggregatorMock");
        deal(address(USDT), address(this), 1_000_000_000e6);
        deal(address(WBTC), address(this), 10e8);
        deal(address(WETH), address(this), 100e18);
        deal(address(USDT), alice, 1_000_000_000_000_000_000_000_000e6);
        deal(address(WBTC), alice, 1000e8);
        deal(address(WETH), alice, 100_000_000_000_000_000e18);

        deal(address(USDT), bob, 1_000_000_000e6);
        deal(address(WBTC), bob, 1000e8);
        deal(address(WETH), bob, 10_000e18);
        //deal eth to alice
        deal(alice, 10_000 ether);
        deal(bob, 1000 ether);

        // deal(address(USDT), address(borrowingManager), 1000000000e6);
        // deal(address(WBTC), address(borrowingManager), 10e8);
        // deal(address(WETH), address(borrowingManager), 100e18);

        _maxApproveIfNecessary(address(WBTC), address(borrowingManager), type(uint256).max);
        _maxApproveIfNecessary(address(WETH), address(borrowingManager), type(uint256).max);
        _maxApproveIfNecessary(address(USDT), address(borrowingManager), type(uint256).max);

        vm.startPrank(alice);
        _maxApproveIfNecessary(address(WBTC), address(borrowingManager), type(uint256).max);
        // _maxApproveIfNecessary(address(WETH), address(borrowingManager), type(uint256).max);
        IERC20(address(WETH)).approve(address(borrowingManager), type(uint256).max);
        IERC20(address(WBTC)).approve(address(borrowingManager), type(uint256).max);
        IERC20(address(WETH)).approve(
            address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max
        );
        IERC20(address(WBTC)).approve(
            address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max
        );
        _maxApproveIfNecessary(address(USDT), address(borrowingManager), type(uint256).max);
        _maxApproveIfNecessary(address(WBTC), address(this), type(uint256).max);
        _maxApproveIfNecessary(address(WETH), address(this), type(uint256).max);
        _maxApproveIfNecessary(address(USDT), address(this), type(uint256).max);

        // _maxApproveIfNecessary(
        //     address(WBTC),
        //     NONFUNGIBLE_POSITION_MANAGER_ADDRESS,
        //     type(uint256).max
        // );
        // _maxApproveIfNecessary(
        //     address(WETH),
        //     NONFUNGIBLE_POSITION_MANAGER_ADDRESS,
        //     type(uint256).max
        // );
        _maxApproveIfNecessary(
            address(USDT), NONFUNGIBLE_POSITION_MANAGER_ADDRESS, type(uint256).max
        );

        (tokenId,,,) = mintPositionAndApprove();
        vm.stopPrank();

        vm.startPrank(bob);
        IERC20(address(WETH)).approve(address(borrowingManager), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(address(borrowingManager));
        IERC20(address(WETH)).approve(
            address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max
        );
        IERC20(address(WBTC)).approve(
            address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max
        );
        vm.stopPrank();
    }

    function test_SetUpState() public {
        assertEq(WBTC_WETH_500_POOL.token0(), address(WBTC));
        assertEq(WBTC_WETH_500_POOL.token1(), address(WETH));
        assertEq(WETH_USDT_500_POOL.token0(), address(WETH));
        assertEq(WETH_USDT_500_POOL.token1(), address(USDT));
        assertEq(USDT.balanceOf(address(this)), 1_000_000_000e6);
        assertEq(WBTC.balanceOf(address(this)), 10e8);
        assertEq(WETH.balanceOf(address(this)), 100e18);
        assertEq(borrowingManager.owner(), address(this));
        assertEq(borrowingManager.dailyRateOperator(), address(this));
        assertEq(
            borrowingManager.computePoolAddress(address(USDT), address(WETH), 500),
            address(WETH_USDT_500_POOL)
        );
        assertEq(
            borrowingManager.computePoolAddress(address(WBTC), address(WETH), 500),
            address(WBTC_WETH_500_POOL)
        );
        assertEq(
            address(borrowingManager.underlyingPositionManager()),
            NONFUNGIBLE_POSITION_MANAGER_ADDRESS
        );
    }

    LiquidityManager.LoanInfo[] loans;

    function createBorrowParams(uint256 _tokenId)
        public
        returns (LiquidityBorrowingManager.BorrowParams memory borrow)
    {
        bytes memory swapData = "";

        LiquidityManager.LoanInfo memory loanInfo = LiquidityManager.LoanInfo({
            liquidity: 100,
            tokenId: _tokenId //5500 = 1319241402 500 = 119931036 10 = 2398620
         });

        loans.push(loanInfo);

        LiquidityManager.LoanInfo[] memory loanInfoArrayMemory = loans;

        borrow = LiquidityBorrowingManager.BorrowParams({
            internalSwapPoolfee: 500,
            saleToken: address(WBTC), //token1 - WETH
            holdToken: address(WETH), //token0 - WBTC
            minHoldTokenOut: 1,
            maxCollateral: 10e8,
            externalSwap: ApproveSwapAndPay.SwapParams({
                swapTarget: address(0),
                swapAmountInDataIndex: 0,
                maxGasForCall: 0,
                swapData: swapData
            }),
            loans: loanInfoArrayMemory
        });
    }

    function createRepayParams(bytes32 _borrowingKey)
        public
        pure
        returns (LiquidityBorrowingManager.RepayParams memory repay)
    {
        bytes memory swapData = "";

        repay = LiquidityBorrowingManager.RepayParams({
            isEmergency: false,
            internalSwapPoolfee: 0, //token1 - WETH
            externalSwap: ApproveSwapAndPay.SwapParams({
                swapTarget: address(0),
                swapAmountInDataIndex: 0,
                maxGasForCall: 0,
                swapData: swapData
            }),
            borrowingKey: _borrowingKey,
            swapSlippageBP1000: 0
        });
    }

    function mintPositionAndApprove()
        public
        returns (uint256 _tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
    {
        INonfungiblePositionManager.MintParams memory mintParams = INonfungiblePositionManager
            .MintParams({
            token0: address(WBTC),
            token1: address(WETH),
            fee: 3000,
            tickLower: 253_320, //TickMath.MIN_TICK,
            tickUpper: 264_600, //TickMath.MAX_TICK ,
            amount0Desired: 10e8,
            amount1Desired: 100e18,
            amount0Min: 0,
            amount1Min: 0,
            recipient: alice,
            deadline: block.timestamp
        });
        (_tokenId, liquidity, amount0, amount1) = INonfungiblePositionManager(
            NONFUNGIBLE_POSITION_MANAGER_ADDRESS
        ).mint{ value: 1 ether }(mintParams);
        INonfungiblePositionManager(NONFUNGIBLE_POSITION_MANAGER_ADDRESS).approve(
            address(borrowingManager), _tokenId
        );
    }

    function test_RepayRevert() public {
        vm.startPrank(alice);
        console.log("alice", alice);
        // console.log("Address test", address(this));
        // console.log("Address borrowinganager", address(borrowingManager));

        console.log("Before borrow");
        console.log(IERC20(address(WETH)).balanceOf(address(alice)));
        console.log(IERC20(address(WBTC)).balanceOf(address(alice)));

        LiquidityBorrowingManager.BorrowParams memory AliceBorrowing = createBorrowParams(tokenId);

        borrowingManager.borrow(AliceBorrowing, deadline);
        console.log("After borrow");
        console.log(IERC20(address(WETH)).balanceOf(address(alice)));
        console.log(IERC20(address(WBTC)).balanceOf(address(alice)));
        bytes32[] memory BorrowingKey = borrowingManager.getBorrowingKey(address(alice));

        //Make the time skip
        skip(96_400);
        vm.stopPrank();

        //      vm.startPrank(address(borrowingManager));
        //     IERC20(WBTC).approve(address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max);
        //     IERC20(WETH).approve(address(NONFUNGIBLE_POSITION_MANAGER_ADDRESS), type(uint256).max);

        vm.startPrank(bob);
        vm.expectRevert();
        LiquidityBorrowingManager.RepayParams memory bobRepaying =
            createRepayParams(BorrowingKey[0]);
        borrowingManager.repay(bobRepaying, deadline);
        vm.stopPrank();
    }
}

The test passes, we can see from the error logs in foundry:

 [12350] 0xC36442b4a4522E871399CD717aBDD847Ab11FE88::increaseLiquidity((512098 [5.12e5], 0, 6715283 [6.715e6], 0, 0, 1685033235 [1.685e9])) 
    │   │   ├─ [696] 0xCBCdF9626bC03E24f779434178A73a0B4bad62eD::slot0() [staticcall]
    │   │   │   └─ ← 0x000000000000000000000000000000000005db000f1598ba8f0e02e024506208000000000000000000000000000000000000000000000000000000000003ec8f000000000000000000000000000000000000000000000000000000000000006400000000000000000000000000000000000000000000000000000000000000c800000000000000000000000000000000000000000000000000000000000000c800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001
    │   │   ├─ [3747] 0xCBCdF9626bC03E24f779434178A73a0B4bad62eD::mint(0xC36442b4a4522E871399CD717aBDD847Ab11FE88, 253320, 264600, 0, 0x0000000000000000000000002260fac5e5542a773aa44fbcfedf7c193bc2c599000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000bb80000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b) 
    │   │   │   └─ ← "EvmError: Revert"
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"

The function reverts when calling increaseLiquidity ->mint. With sufficient console.log() messages we can find that due to wrong value of SqrtPriceX96 the calculated amount0 when calling increaseLiquidity is equal to 0 which is causing the issue and making the function revert.

This can make liquidation impossible and loss of liquidation bonus for liquidator.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/abstract/LiquidityManager.sol#L331C3-L343C1

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();
    }

Calculation for SqrtPriceX96 is done wrong.

Tool used

Manual Review

Recommendation

There are multiple ways to correctly calculate SqrtPriceX96: Please refer to this discussion in ethereum stackexchange - > link

[sherlock-admin2]

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

tsvetanovv commented:

Same root as slot0 vulnerability

[Abelaby]

This is not a duplicate of #109, that describes using slot0 to calculate SqrtPriceX96, this can't be used as it is, I had failing tests, when I used SqrtPriceX96 as it is in the code, thats because SqrtPriceX96 is not meant to be used as it is. If you read the discussion in here there are some post-processing to be done after fetching SqrtPriceX96. uint160 SqrtPriceX96 is supposed to be converted into uint256. Most protocols use an additional SqrtPriceX96ToUint function.

Here is a blog on this calculation

[midori-fuse]

Escalate

Escalating on behalf of @Abelaby

[sherlock-admin2]

Escalate

Escalating on behalf of @Abelaby

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Abelaby]

I want to further make this clear by adding that this issue makes _getHoldTokenAmountIn to return wrong values for amount0 and amount1 and will make the underlyingPositionManager.increaseLiquidity fail everytime when _restoreLiquidity calls increaseLiquidity.

In the case of the attached PoC in issue above, if you add console.log statements in LiquidityManager.sol using foundry like this;

function 
    _increaseLiquidity(
        address saleToken,
        address holdToken,
        LoanInfo memory loan,
        uint256 amount0,
        uint256 amount1
    ) private {
        // increase if not equal to zero to avoid rounding down the amount of restored liquidity.
        if (amount0 > 0) ++amount0; //@audit any benefit if reenter?
        if (amount1 > 0) ++amount1;
        // Call the increaseLiquidity function of underlyingPositionManager contract
        // with IncreaseLiquidityParams struct as argument
        console.log("before increaseLiquidity");
        console.log("loan.tokenId",loan.tokenId);
        console.log("amount0",amount0);
        console.log("amount1",amount1);
        console.log("Sender",msg.sender);
        console.log("address of pos manager",address(underlyingPositionManager));
        console.log("saletoken balance",IERC20(saleToken).balanceOf(address(this)));
        console.log("holdtoken balance",IERC20(holdToken).balanceOf(address(this)));
        console.log("Address of this", address(this));

        //IERC20(saleToken).approve(address(underlyingPositionManager), amount1);
        //IERC20(holdToken).approve(address(underlyingPositionManager), amount1);

        (uint128 restoredLiquidity, , ) = underlyingPositionManager.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams({
                tokenId: loan.tokenId,
                amount0Desired: amount0,
                amount1Desired: amount1,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp 
            })
        );

The call fails because amuont0 is 0 in this case which makes the call to fail; make amount0 any non-zero value and you can see that the call does not revert.

[Czar102]

I agree that it's not a duplicate, but I don't understand the root cause and the extent of the potential impact of this issue. Could you specify those? Also, what is the requirement for the function to incorrectly revert? I can see above that when amount0 is 0, the call seem to revert. Does it happen anytime else? Should the call succeed for amount0 == 0?

@Abelaby

[Abelaby]

Hey @Czar102

Whenever the amount0 is 0 the call will revert. It wont revert for non-zero values, starting from 1. The impact is that this makes liquidation impossible and loss of liquidation bonus for liquidator.

This is due to when increaseLiquidity is called, Uniswap calls mint and then uniswapv3MintCalback, I'm not sure why but some amount of amount0 token is used up during this.

I initially thought SqrtPriceX96 has to be post processed but that doesn't seem to be the case, a simple fix is that when ever amount0 is 0 make it a min value of 1 and and have the liquidator transfer it to the borrowingManager and it will work . After execution, the remaining amount0 is transferred back to the liquidator (Notice that the existing code already transfers the amount0 back to the liquidator, only need functionality to transfer the min amount0 to the borrowingManager ).

[Czar102]

Hi @Abelaby, thanks for more info on the issue. I have some more questions, though.

The impact is that this makes liquidation impossible (...)

So a position cannot be liquidated at all?

I initially thought SqrtPriceX96 has to be post processed but that doesn't seem to be the case (...)

So only the PoC part of the issue is correct and everything else should be disregarded?

What would be the fix?

[Abelaby]

@Czar102 Yes, the position can't be liquidated, and this was a standard position, I used the same parameters as an existing position from the UniswapPositionNFT tokenId = 7 to mint my position in PoC.

The fix is as I mentioned; when amount0 is 0 make it 1 (MIN_VALUE) and have the caller(liquidator) transfer it to the manager contract. This will fix the issue.

[Czar102]

I don't understand why would it fail for a zero amount. Can you look into why is a minimum amount needed?

[Abelaby]

@Czar102 Got it.

So when we call increaseLiquidity to positionManager it calls LiquidityManagement#addLiquidity and there is a call to LiquidityAmounts#getLiquidityForAmounts

function getLiquidityForAmounts(
        uint160 sqrtRatioX96,
        uint160 sqrtRatioAX96,
        uint160 sqrtRatioBX96,
        uint256 amount0,
        uint256 amount1
    ) internal pure returns (uint128 liquidity) {
        if (sqrtRatioAX96 > sqrtRatioBX96) (sqrtRatioAX96, sqrtRatioBX96) = (sqrtRatioBX96, sqrtRatioAX96);

        if (sqrtRatioX96 <= sqrtRatioAX96) {
            liquidity = getLiquidityForAmount0(sqrtRatioAX96, sqrtRatioBX96, amount0);
        } else if (sqrtRatioX96 < sqrtRatioBX96) {
            uint128 liquidity0 = getLiquidityForAmount0(sqrtRatioX96, sqrtRatioBX96, amount0);
            uint128 liquidity1 = getLiquidityForAmount1(sqrtRatioAX96, sqrtRatioX96, amount1);

            liquidity = liquidity0 < liquidity1 ? liquidity0 : liquidity1;
        } else {
            liquidity = getLiquidityForAmount1(sqrtRatioAX96, sqrtRatioBX96, amount1);
        }
    }

which returns the liquidity as 0 (This is caused by amount0 being 0).

Then the transaction calls pool.mint with liquidity as 0, this can seen in the transaction trace in terminal

0xCBCdF9626bC03E24f779434178A73a0B4bad62eD::mint(0xC36442b4a4522E871399CD717aBDD847Ab11FE88, 253320, 264600, 0, 0x0000000000000000000000002260fac5e5542a773aa44fbcfedf7c193bc2c599000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000bb80000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b) 
    │   │   │   └─ ← "EvmError: Revert"

The pool.mint is this -> UniswapV3Pool#mint and it has a check

 require(amount > 0);

Where amount = liquidity. This causes the call to fail. Hope its clear now.

[Czar102]

I don't think there should be transactions with zero liquidity being done here, though. You can't increase liquidity by 0, that would make sense imo.

[Abelaby]

@Czar102 Yes that's exactly the case,

TLDR of my above explanation;

when amount0 is zero, it ultimately makes the call to increaseLiquidity by 0, which makes it to fail.

In this function trace:

0xCBCdF9626bC03E24f779434178A73a0B4bad62eD::mint(0xC36442b4a4522E871399CD717aBDD847Ab11FE88, 253320, 264600, 0, 0x0000000000000000000000002260fac5e5542a773aa44fbcfedf7c193bc2c599000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000bb80000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b) │ │ │ └─ ← "EvmError: Revert"

Notice in above that mint(address recipient, int24 tickLower, int24 tickUpper, uint128 amount, bytes calldata data) is called with the 4th argument (amount / liqudity ) as 0. ie; minting with zero liqudity.

[Czar102]

Then I would be accepting the escalation and making this submission invalid. Don't see a bug or exploit there.

[Abelaby]

@Czar102 The bug is, when calling repay the amount0 will be zero (This is not controlled by the caller in any way) and the function will revert always in this case.

So liquidations are impossible, that is a valid bug.

[Czar102]

@cvetanovv @fann95 could you weigh in?

[fann95]

            isEmergency: false,
            internalSwapPoolfee: 0, //token1 - WETH
            externalSwap: ApproveSwapAndPay.SwapParams({
                swapTarget: address(0),
                swapAmountInDataIndex: 0,
                maxGasForCall: 0,
                swapData: swapData
            }),
            borrowingKey: _borrowingKey,
            swapSlippageBP1000: 0
        });

[fann95]

I think the problems are in the repay parameters. internalSwapPoolfee: 0 and swapTarget: address(0). Try setting the variable internalSwapPoolfee to 3000. since you are trying to make an exchange in a 0th pool and adjust the price in the 3000th. There was actually a bug in the code that prevented pools with different fees from being used, but we fixed it as part of the fix #109 although no one reported about it.

[Abelaby]

@fann95 thanks for your comment, when I looked through the flow of repay() function, the function reverts before ever using internalSwapPoolfee , params.fee is used in liquidityManager._restoreLiquidity() when if (holdTokenAmountIn > 0) is true , which is false in this case.

And I also tried setting internalSwapPoolfee to 3000 which still reverts.

[fann95]

And I also tried setting internalSwapPoolfee to 3000 which still reverts.

Ok, I'll try to take a closer look at this and let you know.

[Abelaby]

@Czar102 @fann95

Found a similar issue in Uniswap v3-periphery repo But no comments so far.

Here is the output when implementing the Fix I recommended.

The fix is as I mentioned; when amount0 is 0 make it 1 (MIN_VALUE) and have the caller(liquidator) transfer it to the manager contract. This will fix the issue.

For simplicity, I had already transferred the MIN_VALUE to manager .

[fann95]

The problem is the small amount of liquidity that the borrower chose It's hard to imagine that someone will spend more on gas than they borrow, but I think that the calculation of the minimum liquidity and verification must be present. Although this is not critical. Fixed: https://github.com/RealWagmi/wagmi-leverage/commit/b5ed9dd9f433722588db8f357801f2664933de81

[detectiveking123]

@Czar102 @cvetanovv Curious if you have opinions on whether this should be a valid low or medium? It feels like the only way this could happen is if the amount of liquidity chosen is extremely, extremely low (easily less than 1/10 of a cent, probably less than 1/100 of a cent). The low amount of liquidity chosen leads to the output amount being 0.

The impact was liquidation being impossible, but in this case, liquidation will never be profitable past gas costs anyways (no one is going to liquidate a 1/100 cent position profitably). As a result, I don't think this issue has any impact. Furthermore, I don't think anyone will attempt to create positions of sub 1/100 of a cent (again gas costs exist).

I would lean towards categorizing this as low for the reasons stated above. It doesn't feel like the requirements for a medium severity issue are met. That being said, the Watson did find an issue that was fixed by the sponsor, so I would not be opposed to giving Medium severity here. Up to your discretion.

[fann95]

The minimum size will depend on the tick range. The size of liquidity should be such that with any case extracted of liquidity (even when the current tick is close to the borders) we receive an amount of each token sufficient to complete a swap with a non-zero output. I have added a function for calculating the minimum liquidity, you can play with it.

[Abelaby]

@fann95 @detectiveking123

I want to point out a few things.

1. The liquidity that the borrower chose already passes through a check, ie; there is a MINIMUM_BORROWED_AMOUNT that the borrower is expected to clear, so this Liquidity is within the expected range,
2. Also for a sanity check, I tried adjusting the liquidity in the test, I was getting the mint() error up to liquidity of 5750 and for liquidity = 5800 , the borrow() reverts with TOO_BIG_COLLATERAL.

[Czar102]

As far as I understand: it's not possible to liquidate a position because there is insufficient liquidity in the uni v3 pool? This sounds like basic margin/leverage specification – you can't close a position if you can't swap the assets back.

In that case there is always an external swap option, right?

The problem is the small amount of liquidity that the borrower chose

So it's when the borrower's liquidity is "worth" zero tokens of both types? Or one type?

[detectiveking123]

@fann95 Yes, it is indeed dependent on the tick. However, consider that the pool should be roughly balanced in terms of price (if you put in $X, you should get approximately $X out, otherwise it would have been arbed.. these are live V3 LP positions). If you're putting in some amount and getting 0 out, it feels like there's very little liquidity (in terms of USD) in the pool.

@Czar102 To answer your questions, theoretically only one type, but if there's a nontrivial amount of money in the pool, it would have been arbed. So in reality both types.

Correct me if I'm wrong here (@Abelaby), but rereading the code, I believe the liquidation functionality is technically still possible through repay or takeOverDebt, just use the externalSwap as was mentioned. Then you can avoid amount0 or amount1 ever being 0.

"As far as I understand: it's not possible to liquidate a position because there is insufficient liquidity in the uni v3 pool? This sounds like basic margin/leverage specification – you can't close a position if you can't swap the assets back." -- yes I think this is correct, the built-in swap will give you terrible outputs, so you should just use your own swap in this case.

It also feels like cases like this are the very reason externalSwap exists.

[fann95]

As far as I understand: it's not possible to liquidate a position because there is insufficient liquidity in the uni v3 pool? This sounds like basic margin/leverage specification – you can't close a position if you can't swap the assets back.

In that case there is always an external swap option, right?

The problem is the small amount of liquidity that the borrower chose

So it's when the borrower's liquidity is "worth" zero tokens of both types? Or one type?

The problem is also in the NonfungiblePositionManager in which you need to transfer the amounts of tokens and not the amount of liquidity, as directly in the Uniswap pool.

[fann95]

@fann95 @detectiveking123

I want to point out a few things.

1. The liquidity that the borrower chose already passes through a check, ie; there is a MINIMUM_BORROWED_AMOUNT that the borrower is expected to clear, so this Liquidity is within the expected range,
2. Also for a sanity check, I tried adjusting the liquidity in the test, I was getting the mint() error up to liquidity of 5750 and for liquidity = 5800 , the borrow() reverts with TOO_BIG_COLLATERAL.

increase the minimum value in BorrowParams to fix this

[detectiveking123]

@fann95 Can you explain more what's going wrong specifically with the periphery NonfungiblePositionManager? I read the code and it looks like UniswapV3Pool requires the liquidity parameter in mint to be positive, but other than that (which I think is a non-issue) I don't see anything that would cause a revert.

My current understanding is that:

                    (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
                        cache.sqrtPriceX96,
                        TickMath.getSqrtRatioAtTick(cache.tickLower),
                        TickMath.getSqrtRatioAtTick(cache.tickUpper),
                        loan.liquidity
                    );

Can lead to amount0 = 0, for example. Which makes sense. But why does this lead to an issue with _increaseLiquidity?

My best guess is that perhaps these should be incremented even if amount0 = 0 or amount1 = 0?

        if (amount0 > 0) ++amount0;
        if (amount1 > 0) ++amount1;

(Perhaps the issue is that we are just rounding down on the returned liquidity)

Can you also comment on the impact here? My impression is that liquidations can still run fine through externalSwap.

[Czar102]

@detectiveking123 thank you. I agree, it seems to be a rounding issue. Joining on the questions above. @fann95 @Abelaby

[Abelaby]

@Czar102 @detectiveking123

amount0 is used as a parameter when calling increaseLiquidity

  (uint128 restoredLiquidity, , ) = underlyingPositionManager.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams({
                tokenId: loan.tokenId,
                amount0Desired: amount0, ///////////////////////// Currently equal to 0
                amount1Desired: amount1 ,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp 
            })

Which as I mentioned before in the call traces here causes the issue (To cause mint() to fail).

My best guess is that perhaps these should be incremented even if amount0 = 0 or amount1 = 0?

This was the fix that I recommended earlier. The current code:

        if (amount0 > 0) ++amount0;
        if (amount1 > 0) ++amount1;

It wont increment if amount0 = 0 or amount1 = 0.

And with sufficient console.log() messages, we can see that this call wont even go to the externalSwap sections, which is inside this code block.

increase the minimum value in BorrowParams to fix this

@fann95 I did this but the issue still persists (mint() issue).

[fann95]

If the current tick is within the range of the position, then neither amount 0 nor amount 1 can be equal to zero and must be rounded up after calculation.I made the changes and now everything works fine.At the time of liquidity extraction, they are checked for a minimum amount. So, it is impossible to take out a loan with liquidity that cannot be restored later.

[Czar102]

It seems the watson didn't manage to describe the issue properly. Even though the PoC presents a bug, the core issue wasn't described, which is what defines a valid submission. Planning to accept the escalation (not a duplicate), but this submission needs to be considered invalid. Otherwise, this would constitute a precedent where an invalid description is considered valid based on the fact that a valid finding was found based on the submission. It is watson's responsibility to fully understand the issue and describe it before the contest ends.

Thank you @Abelaby for escalating this issue and exploring the issue further. I'm sorry it won't yield any payout.

[Abelaby]

@Czar102 I understand your reasoning, but I was able to provide value to the sponsors and had to create a custom PoC in foundry for this one. And I had to follow up through this issue for weeks now which took considerable time and affected my allocated time for other ongoing contests.I believe this disregards the efforts put forth by Watsons o provide value to the sponsors, I was able to provide value and that should be regarded.

And @fann95 I can't see the fix, the repo is private I believe.

[Czar102]

I was able to provide value to the sponsors and had to create a custom PoC in foundry for this one

I definitely agree. But diluting the contest pot would be unfair with respect to other watsons and would break the rules set before the competition. I'm sorry it ends that way, but I don't see another solution. I'm sure the Sherlock team will find a solution to this issue in close future, but for now there seems to be nothing I can do.

[fann95]

@Czar102 I understand your reasoning, but I was able to provide value to the sponsors and had to create a custom PoC in foundry for this one. And I had to follow up through this issue for weeks now which took considerable time and affected my allocated time for other ongoing contests.I believe this disregards the efforts put forth by Watsons o provide value to the sponsors, I was able to provide value and that should be regarded.

And @fann95 I can't see the fix, the repo is private I believe.

Yes, unfortunately the repository is private for now. Thank you for your time on this issue.

[Abelaby]

@Czar102 @fann95

In case of non-obvious issues with complex vulnerabilities/attack paths, Watson must submit a valid POC for the issue to be considered valid and rewarded.

Can't this be considered under this category, the issue was non-obvious and was 'only' able to catch because of the PoC that I submitted.

@cvetanovv @Evert0x tagging you guys for visibility.

[Czar102]

This decision is on me, so no need to explicitly tag the sponsor or other people. I asked Evert to double check my decision here anyway.

In case of non-obvious issues with complex vulnerabilities/attack paths, Watson must submit a valid POC for the issue to be considered valid and rewarded.

Can't this be considered under this category, the issue was non-obvious and was 'only' able to catch because of the PoC that I submitted.

This is an additional requirement, meaning that the watson needs to additionally submit a PoC. It cannot be considered a substitute of a bug report.

[Abelaby]

@Czar102 I understand, tagged them because Sherlock rules states that judge has the final word.

I believe it's unfair to the Watson , the current reasoning was there from the start and if this was to be the final outcome I feel like I was knowingly 'exploited' into providing value to the project over these few weeks.

[detectiveking123]

Yes, it seems like this is the issue:

if (amount0 > 0) ++amount0;
if (amount1 > 0) ++amount1;

Just make this ++amount0 and ++amount1 instead, without the if statements.

@Abelaby can you explain why the code wouldn't get to the externalSwap section? I agree that is the correct code block. But _increaseLiquidity is only called afterwards.

[Abelaby]

@detectiveking123 there is a condition to be met, you can figure it out by using console log statements. And I think the sponsors already implemented a fix, the above one is not necessary and can't be used implicitly as you mentioned.

[Evert0x]

Result: Invalid Unique

The submitter just provided a clue in their submission, after a deep investigation by different parties the actual bug was discovered. The original report is of insufficient quality to be rewarded as a bug report.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: rejected

[Abelaby]

@Evert0x Agree with the decision, but the escalation by midori-fuse should be accepted I believe, because this is not a duplicate.

[midori-fuse]

It's fine, glad I could help securing the protocol. I'd say this is well worth it.

Besides I have like 20 escalations left anyway ¯\_(ツ)_/¯ If anything then you just owe me one ice cream.

[IAm0x52]

Fix looks good. amount0 and amount1 are no longer only incremented when they are zero. Instead calculations now always round up in favor of the LP.