Panoptic - tapir's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L476-L533 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L848-L1067 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1200-L1329

Vulnerability details

Impact

When minting or burning a position attacker can take advantage of reentrancy to make the pool has more "removedLiquidity" than it has. This would make the owed premium to be mistakenly very high value. The issue root cause is reentrancy on an uninitialized pool in SFPM. SFPM initialize pool function explicitly makes the "locked" flag to false which can be leveraged for the attacker to reenter and burn the position before the mint goes through fully. This will lead to an underflow in the removedLiquidity part of the chunk. Thus, the premium owed will be calculated as a very big number since the removed liquidity is used to track the premiums owed.

Proof of Concept

It's best to explain this issue using an example due to its complexity:

First, the attacker is a contract that has a custom onERC1155Received implemented. In this case, the attacker checks for uninitialized pools in SFPM. Let's say the USDC-ETH 0.05% pool is not yet initialized. The attacker initiates a call to mintTokenizedPosition with a "short" type. Normally, this action would remove specific liquidity from Uniswapv3 deposited through SFPM. Attempting this without a deposit in that range would normally throw an error in this part of the code. Also, note that although the underlying uniswap pool is not registered mintTokenizedPosition does not reverts since the attacker will initialize the pool in the ERC1155 callback!

The attacker proceeds with the mint function despite expecting it to revert. The tokenId's Uniswap pool part (first 64 bits) remains uninitialized. The attacker takes advantage of this and, once the _mint is called internally in ERC1155, the attacker can execute their custom onERC1155Received in their contract.

As we can see here, before any execution on state and storage the _mint is called so attacker will has its onERC1155Received callback function called by SFPM: https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L521

Now, attacker will have the following onERC1155Received function:

function onERC1155Received(*/ address caller, address to */ , uint tokenId, uint amount, bytes calldata data) return bytes4 {
   SFPM.initializeAMMPool(USDC, WETH, FEE_TIER);
   SFPM.burnTokenizedPosition(tokenId, tokenId.positionSize(), slippageTickLimitLow, slippageTickLimitHigh);

   return ERC1155Holder.onERC1155Received.selector;
}

The first step here is initializing the pool. This action unlocks the pool's reentrancy lock, allowing re-entry into the functions for that pool link.

The second call burns the token. As this was a long position and has the "burn" flag, it flips the long to short link.

When reaching this part of the code, "isLong" equals 0, initiating an unchecked scope, resulting in an underflow of removedLiquidity, leading to an immensely large number!

Continuing with the function, these lines execute, minting liquidity using 1 ETH from the attacker, inadvertently increasing actual liquidity without a prior deposit.

The removedLiquidity and netLiquidity are then written to storage.

Despite the attacker's tokenId being burnt and nonexistent due to the onERC1155Received function execution, the process continues. Which means we can now continue with the initial mintTokenizedPosition function

Since the previous call updated removedLiquidity to an enormous number via underflow, and increasing netLiquidity with chunk liquidity, leads to no reversion in these lines.

Afterward, the position is successfully burned from Uniswap. Continuing with the function, at this point, the rightSlot holds the net liquidity from previous storage balance. Hence, we will execute the _collectAndWritePositionData function.

However, we have a problem arising in the _updateStoredPremia function which is called inside the _collectAndWritePositionData. link. Here, the removed liquidity is used to calculate the premium owed. Due to the underflow from previous storage balance in removedLiquidity (leftSlot), the resulting premiumOwed becomes immensely high. This impacts the premium owed to be an immensely big number, returning a false statement. Furthermore, users intending to use the USDC-ETH 0.05% pool may encounter difficulties due to messed-up premiums in one range position, potentially affecting other positions relying on these premiums within the external contract.

Tools Used

Manual

Recommended Mitigation Steps

Validate the uniswap pool ID before the _mint OR do not explicitly set the locked value to false when initializing the pool

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L971-L980 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1147-L1153

Vulnerability details

Impact

When someone opens a long position they trigger the SFPM to remove its liquidity from the Uniswap. When the same person wants to close the long position then they trigger the SFPM to replenish the same Uniswap position. To do so, the user has to provide liquidity to the same ticks that he/she removed the liquidity previously. However, since Uniswap pools has such thing called tickSpacingToMaxLiquidityPerTick it might not be possible to close the long position. This is simply that the every tick in Uniswap can hold up to certain amount of liquidity which is called tickSpacingToMaxLiquidityPerTick. If the liquidity to be added makes the existing tick liquidity exceed the max liquidity per tick value, then it will be impossible the mint the liquidity in the given range hence, the long position will not be able to be closed. https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L406-L429 https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/Tick.sol#L128

Proof of Concept

Assume a DAI-USDC 0.05% pool. Since this is a stable pool the majority of the liquidity is pretty concentraded in few ticks the liquidity that the specific ticks are extremely liquid. Also, the lesser the tick spacing (fee) the lesser value on the maxLiquidityPerTick because of how Uniswap calculates the max liquidity per tick.

Assume Alice opens a long position by removing from a position in SPFM which corresponds to the ticks x-y. Those ticks x-y has lots of liquidity and they are almost equal to the max liquidity per tick value. After Alice removes the liquidity she will be decrementing the total liquidity in those ticks by the liquidity that she removed. After some time, some people LP's the same ticks and now the Alice can't actually mint the same liquidity because the current liquidity on the tick x or y + the Alice's borrowed liquidity will exceed the max liquidity per tick. Hence, Alice needs to wait for that tick to have available liquidity room to restore her liquidity and close her long position.

This can also be used by an attacker, if the attacker is also short in that same tick range and the current liquidity on x or y ticks are very close to max liquidity per tick, then attacker can top up some liquidity to make the ticks have max liquidity hence effectively preventing Alice to close her long position.

Tools Used

Recommended Mitigation Steps

This is a very broad and unlikely issue for volatile pools. However, for stable pools that has lots of liquidity this issue can be realistic. In such cases, preventing this issue fully is probably impossible because SFPM does not controls the Uniswap pool. However, SFPM can control the liquidity provisioning happening through itself. So, if there are some people closing long position and someone opening short position, SFPM can check whether the opening of the short position will make the Uniswap tick value to exceed and reverts.

If there are some liquidity that are borrowed by longers then whenever someone mints (shorts) through SFPM, SFPM can check this invariant: liquidityBorrowedByLongersThroughSFPM + currentLiquidityInThatTickForAllUniswapLpers + requestedShortPositionsMintLiquidity < maxLiquidityPerTick

Assessed type

Other