Loss of precision in Math.getLiquidityForAmount1() function allows minting dust amounts of SFPM tokens unlinked from liquidity

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/libraries/Math.sol#L150-L163
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L586-L591
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L620-L630

Vulnerability details

Impact

Users can mint varying amounts of SFPM tokens for the same quantity of input tokens (token0 and token1) due to a lack of precision in Math.getLiquidityForAmount1().

The main issue in the system is a mismatch between user balances and liquidity. Although user balances are only used for transfers and do not affect their funds, it can pose a problem if SFPM is integrated with external protocols that rely on user balances to determine the amount of liquidity held.

Proof of Concept

Check out the PoC: https://gist.github.com/ustas-eth/e56c5cd27e91e0d02a06bcd0d495462a

The root of the problem is Math.getLiquidityForAmount1(). It acts in the same way as in Uniswap's NonfungiblePositionManager, but Uniswap doesn't use fungible balances for users (the reason why SemiFungiblePositionManager exists).

You can try to run the following contract in Remix with the Uniswap file, or you can use ./contracts/libraries/Math.sol with the same success:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.6;

import "@uniswap/v3-periphery/contracts/libraries/LiquidityAmounts.sol";
import "@uniswap/v3-core/contracts/libraries/TickMath.sol";

contract Test {
    function getLiquidityForAmount1(uint160 lowPriceX96, uint160 highPriceX96, uint256 amount) external pure returns (uint256) {
        return LiquidityAmounts.getLiquidityForAmount1(lowPriceX96, highPriceX96, amount);
    }

    function getAmount1ForLiquidity(uint160 lowPriceX96, uint160 highPriceX96, uint128 liquidity) external pure returns (uint256) {
        return LiquidityAmounts.getAmount1ForLiquidity(lowPriceX96, highPriceX96, liquidity);
    }

    function getSqrtRatioAtTick(int24 tick) external pure returns (uint256) {
        return TickMath.getSqrtRatioAtTick(tick);
    }
}

E.g., if you put 1057963989656675351166795586135635 (tick 190_000) as lowPriceX96 and 2875700509213056433999819962028363 (tick 210_000) as highPriceX96 it'll return liquidity with certain loss of precision:

amount	calculated liquidity
1000	0
10000	0
100000000	4358
100001111	4358
100005555	4358
100009999	4359

This allows users to call SFPM.mintTokenizedPosition() to get some x amount of SFPM tokens and do two things:

• Call SFPM.burnTokenizedPosition() with x - y, where y represents the portion of ghost tokens, get back all the tokens spent on the position, and leave y on the balance.
• Call SFPM.safeTransferFrom() with x - y, send all the liquidity to another address and leave y on the balance.

Users will only pay for x - y, the ghost tokens are free.

These leftover tokens then can be consolidated together to form y * 100, for example, but users won't be able to burn them since there's no liquidity underlying.

Tools Used

Manual review

Recommended Mitigation Steps

1. Explicitly allow only transfers of the entire balance of a user on registerTokenTransfer()

+   error InvalidAmountOfTransfer();

    function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
+       if (balanceOf[from][id] != 0) revert InvalidAmountOfTransfer(); // since this function is called after a token transfer, the balance must be 0 at this point

        // Extract univ3pool from the poolId map to Uniswap Pool
        IUniswapV3Pool univ3pool = s_poolContext[id.validate()].pool;

        uint256 numLegs = id.countLegs();

2. Mint SFPM tokens based on the minted liquidity instead of users' input in mintTokenizedPosition().

Assessed type

Math

[c4-sponsor]

dyedm1 (sponsor) disputed

[dyedm1]

It's true that you can have ghost tokens, but this is natural and expected inherent to the way we size positions. 1 position size unit does not correspond to exactly 1 liquidity unit because it's an amount of token0 or token1. The key is that those ghost tokens can't be exploited because of our strict liquidity accounting. Protocols who use Panoptic should consider this and ensure that their accounting cannot be gamed by accumulating ghost tokens - in Panoptic, for example, users are only allowed to mint and burn their entire token balance at once, so there is no opportunity for them to accumulate non-negligible ghost tokens that would affect any of the calculations. The tokens aren't meant to be transferred as part of a protocol's functions anyway and they otherwise have control over what can and cannot be minted and how the calculations are performed.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to QA as this more a warning for future integrators than a broken functionality in the current system.

[ustas-eth]

I believe this issue can be considered as a duplicate of #256 because the root cause (unchecked transfer) is the same and the proposed changes would have fixed both issues.

Mitigation steps from the #256:

At the moment, the protocol checks if the whole net liquidity is transferred by checking the right slot. However, this restriction is not enough and the situation of the left slot is not checked at all.

The transfer restriction should be widened and users should not be able to transfer if their removed liquidity (left slot) is greater than zero.

Mitigation steps from the current issue:

1. Explicitly allow only transfers of the entire balance of a user on registerTokenTransfer()

[dyedm1]

Not really. The key issue in 256 is that we're not checking the left slot of the liquidity being transferred. We are mitigating 256 by checking the removed liquidity as well during transfers, so you'll still be able to transfer ghost tokens (the key thing we look at is the liquidity, not the absolute balance).

As stated in my reasoning above, ghost tokens are expected behavior and they can be accumulated without doing any transfers.

[Picodes]

I agree with the sponsor on this one.