Omniscia Steer Protocol Audit
PoolSharkMultiPositionLiquidityManager Manual Review Findings
PoolSharkMultiPositionLiquidityManager Manual Review Findings
PSM-01M: Potential Denial-of-Service of Liquidity Manager (Poolshark Integration)
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | PoolSharkMultiPositionLiquidityManager.sol:L394-L400, L421 |
Description:
The Poolshark AMM will burn an NFT ID if its position is withdrawn in full. As such, a PoolSharkMultiPositionLiquidityManager::_burnAndCollect function call that would result in all shares being burned would cause the NFT ID to be burned and thus to cause several functions of the system inoperable, such as PoolSharkMultiPositionLiquidityManager::getTotalAmounts, due to relying on proper positionIds values.
Impact:
While the documentation acknowledges this flaw and specifies it should never happen, its consequences would be devastating to the operation of the PoolSharkMultiPositionLiquidityManager and as such we advise this trait to be prevented at the code level.
Example:
381function _burnAndCollect(382 uint256 shares,383 uint256 totalShares384) internal override returns (uint256 t0, uint256 t1) {385 // First, fetch current positions, Only tend() and withdraw() call this function,386 // and neither uses these positions elsewhere (tend uses updated ones).387 LiquidityPositions[] memory _positions = positions;388
389 // For each position, burn() and then withdraw correct amount of liquidity.390 uint256 fees0;391 uint256 fees1;392 uint256 positionCount = _positions.length;393 for (uint256 i; i != positionCount; ++i) {394 //when called from tend shares and total shares are 1 and 1 so liquidityPercentageToBurn becomes 1e38 which just burning the whole position395 //when called from withdraw shares should never be greater than total shares so liquidityPercentageToBurn <= 1e38396 uint256 liquidityPercentageToBurn = FullMath.mulDiv(397 1e38,398 shares,399 totalShares400 );401
402 // amountsOwed are always pulled with liquidity in this contract,403 // so if position liquidity is 0, no need to withdraw anything.404 if (liquidityPercentageToBurn > 0) {405 // Amount burned in each position.406
407 PoolsharkStructs.BurnRangeParams memory burnParams = PoolsharkStructs408 .BurnRangeParams({409 to: address(this),410 positionId: positionIds[i],411 burnPercent: 1 // passing 1 to just claim the fees412 });413 (int256 collect0, int256 collect1) = pool.burnRange(414 burnParams415 ); //@todo check that pssing 1 as burnPercent returns the claimed fees0 and fees1416
417 // Corresponds to amount of liquidity withdrawn (i.e. doesn't include fees).418 burnParams = PoolsharkStructs.BurnRangeParams({419 to: address(this),420 positionId: positionIds[i],421 burnPercent: uint128(liquidityPercentageToBurn) // this should always be less than 1e38422 });423 (int256 posBurned0, int256 posBurned1) = pool.burnRange(424 burnParams425 );Recommendation:
We advise the system to subtract 1 from the liquidityPercentageToBurn unless shares is equal to totalShares which in turn is equal to 1, in which case a burn operation should be performed per the PoolSharkMultiPositionLiquidityManager::_setBins function.
Alternatively, we advise the PoolSharkMultiPositionLiquidityManager::_burnAndCollect function to properly update the positionIds array if a position ID was burned permitting all operations to be performed safely.
Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):
The code was updated to erase the positionIds when a full withdrawal is expected, properly maintaining the data points of the PoolSharkMultiPositionLiquidityManager contract and alleviating this exhibit.