Liquidity provider loses Liquidity during collection initialization

Summary

The first liquidity provider initiating the collection through Locker::initializeCollection() loses ownership of their liquidity position due to how the UniswapImplementation::_unlockCallback() is executed. The owner of the liquidity position on Uniswap will incorrectly tracks UniswapImplementation as the owner of the liquidity.

Vulnerability Detail

When a collection is initialized, the UniswapImplementation::_unlockCallback() is used to add the first liquidity to the pool.

UniswapImplementation::_unlockCallback()

File: UniswapImplementation.sol
376:     function _unlockCallback(bytes calldata _data) internal override returns (bytes memory) {
377:         // Unpack our passed data
378:         CallbackData memory params = abi.decode(_data, (CallbackData));
379: 
380:         // As this call should only come in when we are initializing our pool, we
381:         // don't need to worry about `take` calls, but only `settle` calls.
382:@>         (BalanceDelta delta,) = poolManager.modifyLiquidity({
383:             key: params.poolKey,
384:             params: IPoolManager.ModifyLiquidityParams({
385:                 tickLower: MIN_USABLE_TICK,
386:                 tickUpper: MAX_USABLE_TICK,
387:                 liquidityDelta: int(uint(params.liquidityDelta)),
388:                 salt: ''
389:             }),
390:             hookData: ''
391:         });
---
420:     }

However, the v4-core/PoolManager uses msg.sender as the owner of the position.

v4-core/tickbitmap-overload/PoolManager::modifyLiquidity()

function modifyLiquidity(
    PoolKey memory key,
    IPoolManager.ModifyLiquidityParams memory params,
    bytes calldata hookData
) external onlyWhenUnlocked noDelegateCall returns (BalanceDelta 
---
        BalanceDelta principalDelta;
        (principalDelta, feesAccrued) = pool.modifyLiquidity(
            Pool.ModifyLiquidityParams({
                owner: msg.sender, //@note HERE
                tickLower: params.tickLower,
                tickUpper: params.tickUpper,
                liquidityDelta: params.liquidityDelta.toInt128(),
                tickSpacing: key.tickSpacing,
                salt: params.salt
            })
        );
---
}

In this case, UniswapImplementation contract becomes the owner of the liquidity position, not the user who initiated the Locker::initializeCollection().

As a result, the user who provided the initial liquidity has no control over the position they created.

Impact

The user who initiates the collection and provides the first liquidity will lose ownership of their liquidity position.

Code Snippet

UniswapImplementation::_unlockCallback()

File: UniswapImplementation.sol
376:     function _unlockCallback(bytes calldata _data) internal override returns (bytes memory) {
377:         // Unpack our passed data
378:         CallbackData memory params = abi.decode(_data, (CallbackData));
379: 
380:         // As this call should only come in when we are initializing our pool, we
381:         // don't need to worry about `take` calls, but only `settle` calls.
382:@>         (BalanceDelta delta,) = poolManager.modifyLiquidity({
383:             key: params.poolKey,
384:             params: IPoolManager.ModifyLiquidityParams({
385:                 tickLower: MIN_USABLE_TICK,
386:                 tickUpper: MAX_USABLE_TICK,
387:                 liquidityDelta: int(uint(params.liquidityDelta)),
388:                 salt: ''
389:             }),
390:             hookData: ''
391:         });
---
420:     }

v4-core/tickbitmap-overload/PoolManager::modifyLiquidity()

function modifyLiquidity(
    PoolKey memory key,
    IPoolManager.ModifyLiquidityParams memory params,
    bytes calldata hookData
) external onlyWhenUnlocked noDelegateCall returns (BalanceDelta 
---
        BalanceDelta principalDelta;
        (principalDelta, feesAccrued) = pool.modifyLiquidity(
            Pool.ModifyLiquidityParams({
                owner: msg.sender, //@note HERE
                tickLower: params.tickLower,
                tickUpper: params.tickUpper,
                liquidityDelta: params.liquidityDelta.toInt128(),
                tickSpacing: key.tickSpacing,
                salt: params.salt
            })
        );
---
}

UniswapImplementation::initializeCollection()

File: UniswapImplementation.sol
205:     function initializeCollection(address _collection, uint _amount0, uint _amount1, uint _amount1Slippage, uint160 _sqrtPriceX96) public override {
206:         // Ensure that only our {Locker} can call initialize
207:         if (msg.sender != address(locker)) revert CallerIsNotLocker();
---
225:         // Obtain the UV4 lock for the pool to pull in liquidity
226:         poolManager.unlock(
227:             abi.encode(CallbackData({
228:                 poolKey: poolKey,
229:                 liquidityDelta: LiquidityAmounts.getLiquidityForAmounts({
230:                     sqrtPriceX96: _sqrtPriceX96,
231:                     sqrtPriceAX96: TICK_SQRT_PRICEAX96,
232:                     sqrtPriceBX96: TICK_SQRT_PRICEBX96,
233:                     amount0: poolParams.currencyFlipped ? _amount1 : _amount0,
234:                     amount1: poolParams.currencyFlipped ? _amount0 : _amount1
235:                 }),
236:                 liquidityTokens: _amount1,
237:                 liquidityTokenSlippage: _amount1Slippage
238:             })
239:         ));
240:     }

Locker::initializeCollection()

File: Locker.sol
367:     function initializeCollection(address _collection, uint _eth, uint[] calldata _tokenIds, uint _tokenSlippage, uint160 _sqrtPriceX96) public virtual whenNotPaused collectionExists(_collection) {
368:         // Ensure the collection is not already initialised
---
384:         nativeToken.transferFrom(msg.sender, address(_implementation), _eth);
385: 
---
388:         _implementation.initializeCollection(_collection, _eth, tokens, _tokenSlippage, _sqrtPriceX96);
389: 
---
399:     }

Tool used

Manual Review

Recommendation

Given that v4-core/PoolManager::modifyLiquidity() uses msg.sender as the owner and the Uniswap contract cannot be altered, consider the following strategies to address ownership concerns:

Use a PositionManager to handle liquidity positions for different users, ensuring users retain control.
Use the salt parameter to differentiate same-range positions, helping manage positions for multiple users.
Set up an external tracking system and facilitate liquidity withdrawal to verify and manage ownership for initial liquidity providers.

sherlock-audit / 2024-08-flayer-judging

merlinboii - Liquidity provider loses Liquidity during collection initialization #737