Panoptic - minhtrng's results

Lines of code

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

Vulnerability details

Impact

The protocol has an invariant that if someone wants to transfer SFPM-tokens of a certain ID to someone else, they have to send all tokens of that ID that they possess. The checks for this invariant are insufficient.

Proof of Concept

The registerTokenTransfer function performs the following check:

/// @notice update user position data following a token transfer
/// @dev token transfers are only allowed if you transfer your entire balance of a given tokenId and the recipient has none
function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
    ...
    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()
            )
        );

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

The check to validate that all balance is transferred is insufficient. An example:

1. user adds positionSize=200 (optionRatio=1) for tickRange a-b. s_accountLiq[from] will be 0-200 (0 left, 200 right). He gets minted 200 tokens (say tokenId 1 for simplicity)
2. user removes 100 positionSize by minting a long for same tickRange a-b. s_accountLiq[from] will be 100-100. He gets minted 100 tokens with (with tokenId 2)
3. user can now transfer 100 tokens of tokenId 1 as the validation fromLiq.rightSlot() == liquidityChunk.liquidity() passes, even though he owns 200 tokens with tokenId 1

Tools Used

Manual review

Recommended Mitigation Steps

One approach would be to disallow sending tokens where any of its legs has removed liquidity (where leftSlot() != 0). Though this may be more restrictive than the intended design.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Its possible to transfer liquidity that only has the left slot occupied to manipulate the premia calculation of a target that does not have liquidity for that range yet, without any additional cost.

Proof of Concept

Providing liquidity and removing it through minting a long position allows any user to create a LeftRight-liquidity of x-0 (x being amount in left slot and 0 in right slot). Doing a zero-transfer of any of the tokens minted will assign this liquidity to any target address that does not yet have a position in that tickRange:

//SFPM
function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
    ...
// 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();

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

Since the left slot, which represents the removed liquidity is important for the calculation of premia deltas, this can impact any upstream contracts that use them (e.g. the PanopticPool). This could for example take the form of frontrunning the initial mint on a tickRange of that pool to bypass the s_accountLiquidity[positionKey_to] != 0 check. That position would then start out with an amount x of supposedly removed liquidity.

Tools Used

Manual Review

Recommended Mitigation Steps

disallow transfers of positions with x-y liquidity where x != 0

Assessed type

Other

#[01] No validation of legs being sorted with longs first

The SFPM has the following code:

unchecked {
    // Reverse the order of the legs if this call is burning a position (LIFO)
    // We loop in reverse order if burning a position so that any dependent long liquidity is returned
    // to the pool first,
    // allowing the corresponding short liquidity to be removed
    _leg = _isBurn ? numLegs - leg - 1 : leg;
}

However when validating a position it is not enforced that longs or shorts are in any particular order, see TokenId.validate

[02] Tokens that contain legs with same tokenType and TickRange can not be transferred

registerTransfer reverts if the liquidity of positionKey_to is not 0:

if (
    (s_accountLiquidity[positionKey_to] != 0) ||
    (s_accountFeesBase[positionKey_to] != 0)
) revert Errors.TransferFailed();

Since the position key is determined by target address, tokenType and tickRange. This will disallow a token transfer of tokenIds that contain legs with same tokenType and TickRange. As the transfer of 1 leg in the loop will set that value, causing a revert of the next iteration

[03] Denial of tokenTransfers by frontRunning

Related to issue [2], a transfer of tokens can be denied by frontrunning, since registerTransfer revert if the positionKey_to already has liquidy.

[04] Negative amountToCollect will be casted to large number

_collectAndWritePositionData casts the left and right slot of amountToCollect to uint128:

(uint128 receivedAmount0, uint128 receivedAmount1) = univ3pool.collect(
    msg.sender,
    liquidityChunk.tickLower(),
    liquidityChunk.tickUpper(),
    uint128(amountToCollect.rightSlot()),
    uint128(amountToCollect.leftSlot())
);

As explained in _getFeesBase. The values can be negative:

// (feegrowth * liquidity) / 2 ** 128
/// @dev here we're converting the value to an int128 even though all values (feeGrowth, liquidity, Q128) 
// are strictly positive.
/// That's because of the way feeGrowthInside works in Uniswap v3, where it can underflow when stored for the
// first time.
/// This is not a problem in Uniswap v3 because the fees are always calculated by taking the difference of th
// feeGrowths,
/// so that the net different is always positive.
/// So by using int128 instead of uint128, we remove the need to handle extremely large underflows and simply 
//allow it to be negative
feesBase = int256(0)
    .toRightSlot(int128(int256(Math.mulDiv128(feeGrowthInside0LastX128, liquidity))))
    .toLeftSlot(int128(int256(Math.mulDiv128(feeGrowthInside1LastX128, liquidity))));

In practice there shouldnt be a negative impact, because collecting if the collected fees are negative will just yield 0. To be safe, only casting if the slot values are > 0 would probably be a good idea.