0xhacksmithh - Mechanism For Calculation Of `fee0` and `fee1` In `_withdraw()` Is Wrong.

[sherlock-admin]

0xhacksmithh

high

Mechanism For Calculation Of fee0 and fee1 In _withdraw() Is Wrong.

Summary

fee0 and fee1 calculation is wrong.

Vulnerability Detail

For calculation of fee0 and fee1, _Withdraw() doing following

1. It first get IUniswapV3Pool.position()
2. Then Burn withdrawable liquidity via IUniswapv3pool.burn()
3. Then again call IUniswapv3Pool.position()
4. To get fee0 amount (tokensOwed0After - amount0) - tokensOwed0Before same for fee1 as well

But while i exploring Iuniswapv3pool contract, i come to know that, positions() is a getter function for Position.info struct associated with a perticular bytes32 id, And burn() simply increase token0Owed and token1Owed with corresponding amount of amount0 and amount1 considering burnedLiquidity amount and return same amount0 and amount1

So For example ::

1. Consider when first positions() called, let token0beforeOwed=x and token1beforeOwed=y

2. Then burn() called with some liquidity amount, now let token0Owed=x+a and token1Owed=y+b in UnswapV3 PositionInfo struct, so returned value amount0=a, amount1=b

3. Then again positions() called which will return token0OwedAfter = x+a, token1OwedAfter = y+b

So while i want to calculate fee fee0 = (tokensOwed0After - amount0) - tokensOwed0Before = (x+a - a) - x = 0

So This will be 0 every time. Same for fee1

Uniswapv3pool Burn()

function burn(
        int24 tickLower,
        int24 tickUpper,
        uint128 amount
    ) external override lock returns (uint256 amount0, uint256 amount1) {
        (Position.Info storage position, int256 amount0Int, int256 amount1Int) =
            _modifyPosition(
                ModifyPositionParams({
                    owner: msg.sender,
                    tickLower: tickLower,
                    tickUpper: tickUpper,
                    liquidityDelta: -int256(amount).toInt128()
                })
            );

        amount0 = uint256(-amount0Int);
        amount1 = uint256(-amount1Int);

        if (amount0 > 0 || amount1 > 0) {
            (position.tokensOwed0, position.tokensOwed1) = (
                position.tokensOwed0 + uint128(amount0),
                position.tokensOwed1 + uint128(amount1)
            );
        }

        emit Burn(msg.sender, tickLower, tickUpper, amount, amount0, amount1);
    }

    function _withdraw(
        uint256 lpAmount,
        uint256 _totalSupply
    ) private returns (uint256 withdrawnAmount0, uint256 withdrawnAmount1) {
        assert(_totalSupply > 0);
        PositionInfo memory position;
        uint256 posNum = multiPosition.length;
        uint256 fee0;
        uint256 fee1;
        for (uint256 i = 0; i < posNum; ) {
            position = multiPosition[i];

            {
                (uint128 liquidity, , , , ) = IUniswapV3Pool(position.poolAddress).positions(
                    position.positionKey
                );

                uint128 liquidityToWithdraw = uint128(
                    (uint256(liquidity) * lpAmount) / _totalSupply
                );

                (, , , uint128 tokensOwed0Before, uint128 tokensOwed1Before) = IUniswapV3Pool( // @audit-info just a Getter function for public mapping (positions) available inside UniswapV3 contract
                    position.poolAddress
                ).positions(position.positionKey);

                (uint256 amount0, uint256 amount1) = IUniswapV3Pool(position.poolAddress).burn( // @audit-info it just update the tokenOwed part of position info 
                    position.lowerTick,
                    position.upperTick,
                    liquidityToWithdraw
                );

                withdrawnAmount0 += amount0;
                withdrawnAmount1 += amount1;

                (, , , uint128 tokensOwed0After, uint128 tokensOwed1After) = IUniswapV3Pool(
                    position.poolAddress
                ).positions(position.positionKey);

                fee0 += (tokensOwed0After - amount0) - tokensOwed0Before; // @audit-issue This line of code has no significance
                fee1 += (tokensOwed1After - amount1) - tokensOwed1Before;
            }

Impact

_upFeesGrowth() will get imapcted, which helps to get Protocol cut. As due to fee0, fee1 calculation bug Protocol revenue suffers.

Code Snippet

https://github.com/sherlock-audit/2023-06-real-wagmi/blob/main/concentrator/contracts/Multipool.sol#L526-L527

Tool used

Manual Review

Recommendation

Use some other method for calculating fee received amount

Duplicate of #88