Limit

Proof of concept: PoC

When burning a zeroForOne position, ticks in the TickMap are unset deAcknowledged on if there is a liquidityDelta of 0 at that tick in the ticks0 mapping. However, there may be a nonzero liquidityDelta on that tick in the ticks1 mapping.

This leads to ticks being unset from one zeroForOne side when they are critical for the other. Therefore these ticks will not be crossed during a swap which invalidates the protocol accounting.

Adopt two TickMaps and EpochMaps to solve this particular failure case as well as to avoid any additional potential logical errors related to sharing a TickMap and EpochMap.

Poolshark Team: The issue was resolved in commit 0e40b90.

Proof of concept: PoC

During a swap a position’s end tick may become unset, leaving a position with no upper tick that is set in the TickMap. The pool price can then be undercut such that price is below the position’s end tick.

The user is then able to claim at the current pool price and pass the claims validation as the TickMap.next yields the max tick which carries an unset epochLast.

Ultimately this allows the user to withdraw token0 and token1 balances from the contract that do not correspond to their actual fill amount, invalidating the system's accounting.

Always validate the position’s end tick epochLast in the claims validation logic with the following:

uint32 endTickAccumEpoch = EpochMap.get(params.zeroForOne ? params.upper : params.lower,
tickMap, constants);
if (endTickAccumEpoch > cache.position.epochLast) {
    require (false, 'WrongTickClaimedAt5()');
}

Poolshark Team: The issue was resolved in commit 0e40b90.

Proof of concept: PoC

The pool state is only saved if a position is minted, e.g. if params.amount > 0 && params.lower < params.upper is satisfied. However, liquidity for the pool can be unlocked without a position actually being minted when the params.amount is swapped entirely and the below case is entered.

if (cache.pool.liquidity == 0) {
   /// @dev - this makes sure to have liquidity unlocked if undercutting
   (cache, cache.pool) = Ticks.unlock(cache, cache.pool, ticks, tickMap, params.zeroForOne);
}

As a result, liquidity can be unlocked in the pool yet never saved. Once the liquidity is unlocked, the tick where the liquidity was stashed is cleared. This leads to catastrophic consequences as the liquidityDelta on the cleared tick will not be crossed and added to the pool.

Ultimately, there will be liquidity underflow when the corresponding negative liquidityDelta is added to the pool.liquidity during Ticks.unlock or Ticks._cross. This breaks a key invariant that pool.liquidity should never underflow, disrupting all LimitPool operations.

Save the pool state even when a position is not minted in MintCall.perform.

Poolshark Team: The recommendation was implemented in commit 00db8d3.

Proof of concept: PoC

When the Positions.remove function is called, the updated cache.position is not returned.

Therefore at the end of the perform function all updates are overwritten.

if ((params.zeroForOne ? params.claim != params.upper
                              : params.claim != params.lower))
            params.zeroForOne
                ? positions[msg.sender][params.claim][params.upper] = cache.position
                : positions[msg.sender][params.lower][params.claim] = cache.position;

Consequently, this duplicates the amounts of a position, leaving liquidity in the position when there should not be any since the state of the position prior to the burn is recorded.

Return the updated cache.position from Positions.remove and save this updated position state.

Poolshark Team: The recommendation was implemented in commit 5a30bbf.

Proof of concept: PoC

When a position has been fully filled and a user is claiming with a params.amount of 0 the position is resized to a range spanning 0 ticks, e.g. [100, 100], however the position remains at those ticks with a nonzero liquidity.

The user is now able to burn this remaining null position with a nonzero params.amount, however now the params.lower == params.upper == params.claim.

Therefore for zeroForOne positions, when cache.pool.price < cache.priceLower, the cache.removeLower will be assigned to true.

And for !zeroForOne positions, when cache.pool.price > cache.priceLower, cache.removeUpper will be assigned to true.

This results in the position’s liquidity being subtracted from the start tick, when the liquidity had already previously been removed when the position was originally crossed into and filled.

Therefore the startTick.liquidityDelta will be more negative than the pool.liquidity at that price and result in the entire pool being bricked upon reaching the startTick.

When users are claiming a fully filled position where the params.claim == endTick, always zero out the position liquidity among other attributes as the position no longer exists.

Poolshark Team: The recommendation was implemented in commit 3682ae9.

Proof of concept: PoC

In the Ticks.insertSingle function, the pool.liquidity is often zeroed out and stashed onto the tickToSave. However in some cases the pool.liquidity will not be zeroed out, and yet it is still stashed on the tickToSave.

This means that the pool.liquidity is immediately double counted in both the active liquidity and the tick.liquidityDelta. Now if this tick is crossed this liquidityDelta will be added to the pool.liquidity and the pool will have double the liquidity than it ought to at the current price.

Therefore users will be able to swap using more liquidity than exists at the current range, and they will draw from tokens in positions that are far away from the current pool price. This is catastrophic, as users will not be able to burn their positions as their underlying token amounts have been improperly swapped at the current price, leaving a lack of tokenIn to withdraw.

Ensure that whenever the liquidityDelta is updated on the tickToSave, the pool.liquidity is zeroed out, every time. Include the zeroing out of the pool.liquidity exactly where the tick.liquidityDelta is incremented on line 490.

Poolshark Team: The recommendation was implemented in commit d5443ff.

The poolsharkSwapCallback function on the PoolRouter contract does not validate that the msg.sender is a real Poolshark limit pool and yet gives the msg.sender the ability to transfer from any address to the msg.sender.

Therefore any user that has approved the PoolRouter contract may have their approval amounts stolen by an arbitrary address invoking the poolsharkSwapCallback.

Validate that the msg.sender is indeed a registered Poolshark LimitPool upon invocation of the poolsharkSwapCallback function.

Poolshark Team: The recommendation was implemented in commit 593ff6a.

Proof of concept: PoC

Because the TickMap can round up when performing tick += tickSpacing / 2, there is potential for the crossTick to be set to the current pool price rather than the next inserted tick when a position is minted. Thus, when a trader attempts to swap, the swap is performed from the current price to the cross price which are equivalent, leading to no swap at all although liquidity is available. This goes against the core functionality of the pool where a swap should be able to be performed when liquidity is available and users can get a fill.

Consider the following scenario where LPs mint oneForZero:

1. Bob mints a position with ticks [-100, 100] which shifts the price to tick 100.
2. Alice mints a position with ticks [-100, 100] which calls insertSingle on tick 100 since liquidity is

now available in the pool due to Bob’s mint.

1. Swapper comes in to swap and the cross tick is calculated to be TickMap.previous(tickMap,

pool.tickAtPrice, cache.constants.tickSpacing, true) with tickAtPrice being 100.

1. Due to roundUp being true, the tick in Ticks.previous becomes tick 105. The previous tick from tick

105 is now tick 100.

1. Because the crossTick is tick 100 rather than -100, no swap occurs as no ticks are crossed.

Do not round up when calculating the crossTick.

Poolshark Team: The issue was resolved in commit 27c03bb.

Proof of concept: PoC

During MintCall.perform, liquidity is unlocked in the swap pool in the case there is no liquidity. When the pool.tickAtPrice is negative, the tick rounds up when fetching the TickMap.next() tick. However rounding up allows the pool to skip over a tick with nonzero liquidityDelta.

Consequently, the ticks[pool.tickAtPrice].liquidityDelta can be negative with larger magnitude than the current liquidity, which will lead to silent underflow when casting the int128 value to a uint128 value uint128(ticks[pool.tickAtPrice].liquidityDelta).

As a result, the liquidity of the swap pool becomes severely inflated due to underflow which leads to significant loss of funds for users as traders are able to swap near infinite amounts at the current pool price.

Round back in TickMap.next() when the tick is negative.

Poolshark Team: The recommendation was implemented in commit 614db68.

Proof of concept: PoC

It is possible to enter the case if (cache.amountLeft < amountMax) during a swap, but have the newPrice be represented by a tick that is lower than the cache.crossTick. The tick of the newPrice can be less than the cache.crossTick because when Ticks.insertSingle is called, the priceAt attribute is set onto a rounded tick for the current pool price, e.g. current pool price at tick 4 is saved on the rounded tick 5.

Consequently, the tick at the newPrice may jump over a tick where liquidity is stashed (liquidityDelta > 0) during a swap. Because no cross is performed when inside if (cache.amountLeft < amountMax), the liquidity is never activated.

Since the activated pool liquidity is smaller than it should be, pool.liquidity underflows when a future cross occurs and a tick with liquidityDelta < 0 is crossed. This breaks a key invariant that pool.liquidity should never underflow, disrupting a all LimitPool operations.

In the case that the tick at the newPrice is smaller/larger than the cache.crossTick (jumped over a tick where liquidity is stashed) in the zeroForOne/oneForZero case, set cross = True so that the stashed liquidity delta does get activated.

Poolshark Team: The recommendation was implemented in commit 89cce40.

The initialize function on the LimitPool has unrestricted access and can be called after initialization has already occurred.

Add an onlyInitializer modifier to the initialize function so that the it cannot be called after initialization.

Poolshark Team: The recommendation was implemented in commit 80501d6.

Proof of concept: PoC

When a new tick is inserted in Positions.add during a position mint the new tick is initialized with an epoch of 0. However the claim validation logic in Claims.validate relies on the epoch of the next initialized tick to determine whether or not the user should be able to claim at the supplied claim tick.

When a new position is created and a new upper and/or lower tick is created, the new tick(s) will invalidate the Claims.validate logic as any user can now claim at a tick that is directly previous to a newly initialized tick.

Therefore users are able to claim at the current pool price even when it is not the furthest claim tick they should be claiming at. When a user claims at the current pool price and burns liquidity, that liquidity will be removed from the pool.liquidity value.

However a user’s position can have their liquidity stashed at a higher tick upon undercutting and therefore not have active liquidity when they are claiming at the current pool price.

This leads to users removing liquidity from the active pool.liquidity that should not have been and invalidates the pool accounting system leading to locked positions among other catastrophic consequences.

When a new tick is inserted during Positions.add, do not initialize it with an epoch of 0. Instead initialize new ticks with the same epoch as the tick further along in the zeroForOne or !zeroForOne direction.

Poolshark Team: The recommendation was implemented in commit 8a6c3bf.

Proof of concept: PoC

When an undercut is performed, the pool’s liquidity is stashed on a half tick with the current price.

It is crucial when swapping that this stashed liquidity is kicked into the pool, otherwise the negative liquidityDelta on the end tick of a position will exceed the pool.liquidity, causing underflow. However, it is possible for an iteration of a swap to skip a half tick where liquidity is stashed as the cache.crossTick can jump to the limitTickAhead in _iterate().

For example, the cache.crossTick may equal 5 after:

(cache.crossTick,) = TickMap.roundHalf(cache.crossTick, cache.constants, cache.price);

But the limitTickAhead may be 0 after:

int24 limitTickAhead = TickMap.previous(limitTickMap, cache.crossTick,
cache.constants.tickSpacing, inclusive);
cache.crossTick = limitTickAhead;

Therefore, the tick 5 stashed liquidity is never activated since it isn’t crossed.

Add the liquidity on the half tick before going to the tick ahead and then clear the tick’s liquidity delta.

Poolshark Team: The recommendation was implemented in commit b633427.

Proof of concept: PoC

In the Ticks.unlock function, the ticks[pool.tickAtPrice] is zeroed out before the following logic is executed to update the pool.price:

uint160 priceAt = ticks[pool.tickAtPrice].priceAt;
if (priceAt > 0) {
    pool.price = priceAt;
    pool.tickAtPrice = ConstantProduct.getTickAtPrice(priceAt, cache.constants);
}

Since the ticks[pool.tickAtPrice] is always zeroed out before this logic, the if (priceAt > 0) case will never be entered and the pool.price will never be updated. As a result, the pool.tickAtPrice will not agree with the pool.price. Ultimately this will cause the amountMax to be significantly larger than it should be in the quoteSingle call during a swap, as it relies on the pool.price and the crossPrice which relies on the tickAtPrice.

Zero out the ticks[pool.tickAtPrice] after performing the pool.price update logic which depends on the ticks[pool.tickAtPrice].

Poolshark Team: The recommendation was implemented in commit 1bf9eaa.

In the quoteSingle function, a swap cannot occur if the pool.price becomes cache.constants.bounds.min or cache.constants.bounds.max. Therefore a user can create a zeroForOne position with minimal liquidity and a lower tick of cache.constants.bounds.min to completely brick the pool and halt all !zeroForOne swaps from occurring.

This renders the pool useless, an attacker can exercise this on every Poolshark’s LimitPool to completely shut down the protocol. Pools of the same tokens and tickSpacing cannot be re-deployed as they are already registered under the same key.

Check the pool.price against the cache.constants.bounds.min and cache.constants.bounds.max dependent on the swap direction. If a swap is zeroForOne, the swap should early return if the price is cache.constants.bounds.min and if a swap is !zeroForOne, the swap should early return if the price is cache.constants.bounds.max.

Poolshark Team: The recommendation was implemented in commit 5dc6b2b.

Proof of concept: PoC

When a user claims for their partially filled position, they are able to claim at a half tick that is not an even multiple of their tickSpacing. This allows users to claim fills dependent on the stashed priceAt on the half tick, however it also results in the position getting resized to a start tick that happens to be that half tick.

Positions with a half tick as a boundary break a fundamental invariant of the protocol and potentially lead to severe issues and manipulation.

Do not resize positions to the boundary of a half tick, instead round the new boundary tick back to the previous full tick.

Poolshark Team: The recommendation was implemented in commit 0e40b90.

The fees function never sets token0Fees and token1Fees variables. Therefore, the owner will never be able to collect fees.

Set token0Fees and token1Fees prior to zeroing out the protocol fees.

Poolshark Team: The recommendation was implemented in commit b7ebe31.

In the roundAheadWithPrice function, if zeroForOne and the roundedTick is negative, the tickSpacing is subtracted from the rounded tick. Otherwise if !zeroForOne and the roundedTick is positive, the tickSpacing is added to the rounded tick. Therefore for the zeroForOne case a positive roundedTick is rounded down and not adjusted upwards, meanwhile a negative roundedTick is adjusted to be more negative. Both of these are rounding back rather than rounding ahead for a zeroForOne position. For the !zeroForOne case a positive roundedTick is adjusted upwards, meanwhile a negative roundedTick is rounded to be less negative and is not adjusted to be more negative. Both of these are rounding back rather than rounding ahead for a !zeroForOne position. Ultimately this results in the beginning of a position being resized to the roundedBack tick rather than the roundedAhead tick which unexpectedly alters the user’s overall execution price and unexpectedly sets the latest swapEpoch on the roundedBack tick. Additionally, the roundAhead function implements incorrect tick rounding where for zeroForOne cases where the roundedTick is negative it is rounded up twice. First the magnitude of the negative tick is reduced with rounding and then it is further adjusted up by the tickSpacing. On the other hand, positive roundedTicks are rounded down and not adjusted up. The inverses are true for the !zeroForOne case.

Use the following cases to accurately round ahead:

if (zeroForOne && (roundedTick > 0 || (roundedTick == 0 && tick > 0))) roundedTick +=
tickSpacing;
else if (!zeroForOne && (roundedTick < 0 || (roundedTick == 0 && tick < 0))) roundedTick -=
tickSpacing;

Poolshark Team: The recommendation was implemented in commit 4666f91.

Proof of concept: PoC

When an odd tick spacing is used in a LimitPool, tick rounding errors can cause swaps to have access to more liquidity at the current market price than they should.

Specifically in the TickMap.previous or TickMap.next functions the previous or next tick may be errantly rounded in the _tick function such that it yields a tick that was never set in the TickMap. This affects many areas of the protocol but invalidates the accounting system and leads to direct loss of funds when swappers are able to swap for an extra tick length using the same liquidity.

Do not allow an odd tickSpacing to be used as it is incompatible with the system.

Poolshark Team: Odd tick spacings are now disallowed in the LimitPoolManager contract.

Proof of concept: PoC

When computing the value of fees for a position the priceLower or priceUpper is used as the currentPrice, however the priceLower or priceUpper will rarely be accurate to the current price.

cache.liquidityOnPosition = ConstantProduct.getLiquidityForAmounts(
     cache.priceLower,
     cache.priceUpper,
     position.amount0 > 0 ? cache.priceLower : cache.priceUpper,
     position.amount1,
     position.amount0
)

A malicious actor can leverage this inaccuracy to mint a position where the fees are undervalued from the liquidityOnPosition calculation and subsequently burn to receive the full value of the fees with the calculation in the remove function.

params.amount = uint128(uint256(params.amount) * cache.totalSupply /
(uint256(position.liquidity - params.amount) + cache.liquidityOnPosition));
/// @dev - if there are fees on the position we mint less positionToken

Convert the position accounting logic to an ERC721 implementation rather than an ERC1155 implementation to avoid unnecessary complexity with fee valuation and potential manipulation.

Poolshark Team: The recommendation was implemented in commit 3356f37.

When params.amount == 0 the pool.liquidityGlobal is decremented when the params.claim != params.lower and params.claim == params.lower for zeroForOne and when the params.claim != params.upper and params.claim == params.upper for !zeroForOne.

These conditions are unsatisfiable, therefore the pool.liquidityGlobal will not be decremented for positions that are fully filled and ought to have their liquidity removed from pool.liquidityGlobal. This way an attacker can continuously open positions and remove them until the pool.liquidityGlobal reaches the maximum and users are unable to mint positions rendering the pool useless.

Appropriately decrement the pool.liquidityGlobal when users are claiming at their end tick with params.amount == 0.

Poolshark Team: The recommendation was implemented in commit f0d52ad.

Proof of concept: PoC

It is possible for a user’s position to get overwritten at the claim tick because the positions mapping is accessed with the wrong ticks when performing claim tick validation.

For a zeroForOne position, the new position should span from the claim tick to the upper tick. For a !zeroForOne position, the new position should span from the lower tick to the claim tick. However, that is not how the validation is checking the user’s position.

// prevent position overwriting at claim tick
if (params.zeroForOne) {
    if (positions[params.owner][params.lower][params.claim].liquidity > 0) {
        require (false, string.concat('UpdatePositionFirstAt(', String.from(params.lower), ',
                 ', String.from(params.claim), ')'));
    }
} else {
    if (positions[params.owner][params.claim][params.upper].liquidity > 0) {
        require (false, string.concat('UpdatePositionFirstAt(', String.from(params.lower), ',
                 ', String.from(params.claim), ')'));
    }
}

This can lead to a trader losing their funds, because any deltas for another position they have may be overwritten when burning one of their positions.

Modify the validation such that zeroForOne checks the position spanning from the params.claim to the params.upper and !zeroForOne checks the position spanning from the params.lower to the params.claim, which will prevent a claim tick that leads to an overwritten position.

Poolshark Team: The recommendation was implemented in commit c9a3a42.

When a user's position is undercut and liquidity is stashed on a half tick, they are required to claim at that half tick. However the initial priceClaim for a half tick is assigned to the price at that half tick rather than the priceAt for the half tick. When the pool.price is ahead of the priceClaim then the params.claim will be set to the earlier full tick.

However this tick is not a valid tick to claim at for the user, since the validation will fetch the next tick, the half tick which their position is stashed on, and check the epoch and see that the half tick is a valid claim tick and therefore revert.

This prevents users from burning their liquidity when the position is in this state, a malicious actor can abuse this to prevent others from burning from their positions and keeping them trapped.

Initialize the cache.priceClaim to be the priceAt for the half tick rather than the price at the half tick.

Poolshark Team: The recommendation was implemented in commit 0e40b90.

The amountLeft for !exactIn is increased in accordance with the swapFee in order to give the user exactly their specified amount. However the swapFee will likely not apply to the entire amountOut, as the swapFee applies only to the portion of amountOut that was a direct result of the range pool liquidity.

Therefore users specifying an amount for !exactIn will in most cases receive more than their defined amount out, which invalidates the definition of !exactIn.

Compute and apply the fees to the amountIn for the !exactIn case.

Poolshark Team: The recommendation was implemented in commit 90fb6e9.

When assigning fees with the fees function, the LimitPoolManager will provide a protocolFee0 and a protocolFee1, however the protocolFee0 will always be overwritten with the protocolFee1.

globalState.protocolFee = protocolFee0;
globalState.protocolFee = protocolFee1;

Therefore the protocolFee1 will always apply instead of the protocolFee0 which will result in unexpected fees being applied.

Create protocolFee0 and protocolFee1 attributes on the ILimitPoolStructs.GlobalState struct to store each protocolFee.

Poolshark Team: The recommendation was implemented in commit 80501d6.

The LimitPoolFactory allows for permissionless creation of a LimitPool with any tokenIn and tokenOut. Therefore a token0 or token1 with fee-on-transfer or rebase mechanisms may be supplied.

In the transferIn function there is logic to handle fee-on-transfer and rebase tokens. However in the mint call the returned value is not used in the mint process.

Therefore even though there is logic built in to support fee on transfer tokens, it is not used.

Refactor the mintCall and other relevant functions to rely on the returned value from transferIn to account for fee on transfer tokens. Otherwise make it well documented that fee-on-transfer and rebase tokens are not compatible with the system.

Poolshark Team: Acknowledged.

When minting a position with a lower tick at or below tick -665460, mints will begin to revert with the priceOutOfBounds error. This is because the getLiquidityForAmounts function returns 0 when the product of priceLower and priceUpper is less than Q96.

When liquidityMinted is zero it causes the priceLimit to be 0 as well. Therefore failing the validation in the getTickAtPrice function as 0 is less than the price limit.

if (price < constants.bounds.min || price >= constants.bounds.max)
          require (false, 'PriceOutOfBounds()');

If the price of a pool were in this range user’s ability to mint positions would be limited as they would have to increase the range well beyond market price to mint successfully.

Consider implementing a solution for the getLiquidityForAmounts function when the product of the priceLower and priceUpper are less than Q96. Otherwise ensure this behavior is clearly documented for traders and users deploying pools.

Poolshark Team: Acknowledged.

When positions are resized and the priceLimit is not past the current pool price the resulting position is still resized to the priceLimit, rather than the current market price.

This is unexpected for the user and does not allow their position to immediately begin filling as price moves in their direction.

Add the following after fetching the cache.priceLimit in order to resize the user to the current market price in the event that a swap cannot occur.

if (ConstantProduct.withinBounds(cache.swapPool.price, cache.constants) &&
(params.zeroForOne ? cache.priceLimit > cache.swapPool.price : cache.priceLimit <
cache.swapPool.price)) { cache.priceLimit = cache.swapPool.price; }

Poolshark Team: The recommendation was implemented in commit 501f609.

The Burn functionality allows users to burn a fully filled position with a nonzero params.amount. In this case the regular burn params.amount > 0 logic is entered and the position.liquidity is decremented by the params.amount.

Subsequently the position removal case is entered and the params.amount is updated to be the position.liquidity after the position.liquidity has been reduced by the original params.amount.

The params.amount is then used to emit the BurnLimit event as the liquidityBurned parameter.

Therefore a user can mislead other parties relying on the BurnLimit event by providing a nonzero params.amount when burning their fully filled position. In the worst case this event may emit a liquidityBurned of 0, while the entire original position.liquidity was actually burned.

Allowing users to provide a nonzero params.amount when they are burning a fully filled position introduces more avenues for exploitation and inconsistency. Do not allow users to pass a nonzero params.amount when they are burning their fully filled position, or auto update the amount to be 0 in Claims.validate.

Poolshark Team: The recommendation was implemented in commit 0e40b90.

Proof of concept: PoC

It is possible to insert a position into the positions mapping with 0 liquidity. When minting a position, the initial cache.liquidityMinted in Positions.resize may be greater than 0, bypassing the if (cache.liquidityMinted == 0) require (false, 'PositionLiquidityZero()') check.

However, when the cache.liquidityMinted is recalculated in the below snippet, it can become 0 after accounting for the swap.

if (params.amount > 0 && params.lower < params.upper)
               cache.liquidityMinted = ConstantProduct.getLiquidityForAmounts(
                   cache.priceLower,
                   cache.priceUpper,
                   params.zeroForOne ? cache.priceLower : cache.priceUpper,
                   params.zeroForOne ? 0 : uint256(params.amount),
                   params.zeroForOne ? uint256(params.amount) : 0
               );

This causes the user slight loss as any leftover params.amount will be unclaimable.

Check if the cache.liquidityMinted == 0 on recalculation, and revert if so.

Poolshark Team: The recommendation was implemented in commit 5dc6b2b.

The cache.feeAmount is increased by the locals.feeAmount after the locals.feeAmount is decreased by the locals.protocolFeesAccrued.

locals.feeAmount -= locals.protocolFeesAccrued;
// add to total fees paid for swap
cache.feeAmount += locals.feeAmount.toUint128();

As a result, the cache.feeAmount emitted in the Swap event does not truly represent how much the user paid in fees after swapping, since the protocol fees are not accounted for.

Include the protocol fees the user paid as part of the cache.feeAmount.

Poolshark Team: The recommendation was implemented in commit c312551.

In the case that the tickAtPrice is past the half tick, calling next() with inclusive=True does not return the the half tick when it is set. This could potentially lead to problems with liquidity not being activated which causes further negative consequences such as underflow, swap failure, among other things.

Amend the inclusive logic such that a tick that is past the half tick but not yet at the full tick rounds back to the half tick by reducing the magnitude of the tick by half of the tickSpacing.

Poolshark Team: The recommendation was implemented in commit 3356f37.

In the perform function the Collect.burnLimit function which transfers tokens is executed before saving the state of the position.

Poolshark allows the creating of a LimitPool with any tokenIn and tokenOut, some of these tokens will have callback capabilities. In the event that tokens with callbacks are used the state of the position should be saved before executing any token transfers, this way avoiding read-only reentrancy risks for systems interacting with Poolshark.

Save the state of the position before transferring out tokens to the receiver.

Poolshark Team: The recommendation was implemented in commit 77f9c26.

The quote and snapshot view functions lack reentrancy protection and therefore allow a malicious user to execute a flashloan swap from the LimitPool and manipulate the output of quote/snapshot to exploit systems interacting with the LimitPool and relying on these view functions.

Add reentrancy protections to the quote and snapshot functions such that these functions revert if the system has already been entered during the transaction.

Poolshark Team: The recommendation was implemented in commit 9458d42.

The mintFungible function lacks a checkERC1155Support modifier to validate that the _account address which will receive the minted tokens can handle the ERC1155 tokens.

Add a checkERC1155Support(_account) modifier to the mintFungible function.

Poolshark Team: The recommendation was implemented in commit 97d3d50.

The PoolRouter contract lacks any explicit slippage controls such as minimum output amount or maximum input amount. The priceLimit parameter for the LimitPool.swap function serves as user’s only form of protection from sandwich attacks, however user’s may desire additional explicit slippage controls such as minimum output or maximum input.

Consider implementing minimum output and maximum input slippage controls in the PoolRouter contract.

Poolshark Team: Acknowledged.

When creating a new LimitPool with the createLimitPool function there is no validation that tokenIn ≠ tokenOut or that neither tokenIn nor tokenOut are address(0).

Certainly pools where token1 == token0 are invalid. Additionally, pools where token0 is address(0) will attempt to use native ether via the functions in SafeTransfers.sol, however no functions are payable so clearly native ether is incompatible with the system.

Add validation to check that tokenIn ≠ tokenOut as well as that token0 ≠ address(0). Otherwise if token0 should be allowed to be address(0) and native ether is indeed meant to be compatible with the system, make the appropriate functions payable to allow native ether to be used in the system.

Poolshark Team: The suggested validations were implemented in the createLimitPool function.

The finalTick is read from storage before being immediately assigned back to storage.

{
    // update max deltas
    ILimitPoolStructs.Tick memory finalTick =
    ticks[params.zeroForOne ? params.lower : params.upper];
    ticks[params.zeroForOne ? params.lower : params.upper] = finalTick;
}

Remove the read and write for the finalTick as there is no net effect.

Poolshark Team: The recommendation was implemented in commit 3682ae9.

In the remove function, on line 225 an if case performs validation on the position's liquidity and reverts if the case is entered.

The following logic of the function is nested within an else case, however this else case is unnecessary as the contents of the if case will always revert.

Move the subsequent logic outside of the else case to improve code readability and style.

Poolshark Team: The recommendation was implemented in commit 3682ae9.

In the Positions.sol and Ticks.sol files there is a Q96 constant, but it is never used.

Remove the Q96 constant from these files.

Poolshark Team: The recommendation was implemented in commit 3682ae9.

The following comment contains a typo:

wouuld be smart to protect against the case of epochs crossing

would be smart to protect against the case of epochs crossing

Poolshark Team: The recommendation was implemented in commit 3682ae9.

In the Ticks.insert function, ternary operators are used as conditionals where one case is always true. However these ternaries can be simplified in the following way:

params.zeroForOne ? cache.priceLower > cache.pool.price : true

→

!params.zeroForOne || cache.priceLower > cache.pool.price

params.zeroForOne ? true : cache.priceUpper < cache.pool.price

→ params.zeroForOne || cache.priceLower > cache.pool.price.

Implement the above suggested simplifications.

Poolshark Team: The recommendation was implemented in commit 91b0f3d.

When assigning the token0 and token1 for a new limit pool, two ternary operators are used to determine each token. However a single ternary operator can be used like so: (address token0, address token1) = tokenIn < tokenOut ? (tokenIn, tokenOut) : (tokenIn, tokenOut)

Implement the above suggested optimization.

Poolshark Team: The recommendation was implemented in commit 91b0f3d.

Throughout the codebase the GlobalState globalState storage variable is used however the GlobalState struct only contains an unlocked variable to facilitate reentrancy locks. However the reentrancy lock currently implemented by the system is a more error prone and less efficient version of OpenZeppelin’s ReentrancyGuard.

Remove the globalState variable and use OpenZeppelin’s ReentrancyGuard.

Poolshark Team: The GlobalState storage variable is now used for more than a reentrancy lock.

In the Positions.resize function, the params.mintPercent is treated as a percentage with 1e28 decimals, however this is inconsistent with the decimals of 1e38 used for liquidity percent conversions in the _convert function.

Standardize on either 1e28 or 1e38 for percentage decimals for consistency.

Poolshark Team: Acknowledged.

There is no check for params.to == address(0) when calling the mint or swap functions in LimitPool.

Implement the following check in the mint and swap functions: if (params.to == address(0)) revert CollectToZeroAddress();

Poolshark Team: Acknowledged.

The canonicalOnly modifier is misspelled as canoncialOnly.

Replace all instances of canoncialOnly with canonicalOnly.

Poolshark Team: Acknowledged.

The _tick function is implemented in both the EpochMap and the TickMap files. Only the _tick implementation in the TickMap is used, therefore the implementation in the EpochMap can be removed.

Remove the _tick function from the TickMap.

Poolshark Team: Acknowledged.

Throughout the codebase the RebaseLibrary.sol file is not used.

Remove the unnecessary library.

Poolshark Team: Acknowledged.

The cache.claimTick is assigned to in several cases throughout the Claims.validate function, however the cache.claimTick is never referenced after this assignment.

Remove the assignments in the Claims.validate function.

Poolshark Team: Acknowledged.

The comment 0 -> 1 positions price moves up so nextFullTick is lesser is not accurate on line 535 as the relevant tick is the previousFullTick rather than the nextFullTick.

The comment 0 -> 1 positions price moves up so nextFullTick is lesser is not accurate on line 551 as this is the !zeroForOne case.

Update the comments in the insertSingle function.

Poolshark Team: Acknowledged.

The claimTickEpoch is assigned a value inside of the inner if cases on lines 43 and 61 but then re-assigned to the same exact value right after.

Remove the assignments to claimTickEpoch on these lines as they have no effect.

Poolshark Team: Acknowledged.

The Checks library includes save and balance functions which may be useful throughout the codebase, however this library is never used.

Either implement usage of the Checks library or remove it from the codebase.

Poolshark Team: Acknowledged.

The if statement beginning on line 391 is unreachable as it requires the params.claim ≠ lower as well as params.claim == lower which is unsatisfiable.

Remove this if statement as it is unreachable and unnecessary.

Poolshark Team: Acknowledged.

The early return logic in Claims.validate is unreachable. The conditions that satisfy the early return case in Claims.validate constitute calling Positions.remove instead.

To enter update (for zeroForOne):

cache.position.claimPriceLast != 0 || params.claim != params.lower || epochLower > positionEpoch

To enter the early return case inside of update (for zeroForOne):

claimPriceLast == 0 && params.claim == params.lower && epochLower <= positionEpoch

Never will these two conditions both be satisfiable, therefore this early return case can never be reached.

Remove all of the early return logic as it is unreachable.

Poolshark Team: Acknowledged.

There is a positions mapping write in the Positions.remove function, however it will always write to the same lower and upper tick boundaries as the subsequent write in the BurnCall.perform function.

Remove the extraneous positions write in the Positions.remove function, additionally add a check in the Positions.remove function that the claim is always exactly the params.lower for zeroForOne and params.upper for !zeroForOne, to be explicitly safe.

Poolshark Team: Acknowledged.

Once a tickSpacing or implementation is configured in the LimitPoolManager contract they cannot be disabled.

If an issue is discovered with any particular tickSpacing or implementation then it cannot be removed and pools will still be allowed to be created with that misguided configuration.

Implement functions to disable a particular tickSpacing or implementation in the LimitPoolManager.

Poolshark Team: Acknowledged.

In the remove function, it is validated that the params.amount is less than or equal to 1e38. However this validation is already performed in the _convert function.

Remove the first instance of the validation and perform the conversion at the beginning of the remove function.

Poolshark Team: Acknowledged.

There is a refundTo parameter on the MintParams struct, however it is never initialized or referenced.

Either implement logic for the refundTo address or remove it from the MintParams struct.

Poolshark Team: Acknowledged.

The smart contracts in the project use floating version of Solidity (^0.8.13). There is a list of known bugs in Solidity versions, many of them can impact smart contract functionality in unexpected way:

https://github.com/ethereum/solidity/blob/develop/docs/bugs.json.

Furthermore, if compiled with 0.8.20 there may be unexpected reverts when deployed as some chains still do not support PUSH0.

Consider utilizing a static version.

Poolshark Team: The recommendation was implemented in 0dd6a82.

Throughout the codebase several for loops are used without caching the length of the array they iterate over or performing an unchecked { ++i } for optimization.

Consider caching the length of the array to be iterated over and incrementing the index with unchecked { ++i }.

Poolshark Team: The recommendation was implemented in 91ab44a.

The EpochMap.get(params.lower, tickMap, constants) > cache.position.epochLast and EpochMap.get(params.upper, tickMap, constants) > cache.position.epochLast conditions on lines 234 and 249 are unsatisfiable as these are conditions to enter Positions.update rather than Positions.remove.

In fact if these conditions were satisfiable a critical bug would arise where the position.liquidity is decremented from the pool.liquidity as well as the position’s upper and lower ticks.

Remove the logic from lines 232 to 261.

Poolshark Team: Acknowledged.

Throughout the codebase, casting operations are performed. Downcasting does not revert on overflow, therefore it would be prudent to use OpenZeppelin’s SafeCast to revert in these cases.

Consider using OpenZeppelin’s SafeCast to protect against undetected overflow.

Poolshark Team: Acknowledged.

Malicious actors are allowed to create many small range positions with only a few wei of liquidity in order to initialize many ticks and make swaps more gas consumptive. Additionally, position’s with minute liquidity and narrow ranges serve little purpose but to potentially manipulate the LimitPool in unforseen ways.

Consider implementing a minimum liquidity for position’s to avoid potential manipulation or griefing, potentially based on the width of the position.

Poolshark Team: Acknowledged.

As a side effect of the positions[params.owner][params.claim][params.upper].liquidity > 0 check in the claims validation, a malicious actor can front-run a user’s burn and mint a dust position for the user such that this claim validation check fails. The user would then have to burn the dust position before being able to claim their existing position.

Consider removing the ability for users to mint for other users or ensure a minimum liquidity amount is implemented such that any griefing attacks pose a non-trivial loss to the actors.

Poolshark Team: Acknowledged.

The RangePositions.validate function only performs validation that the liquidityMinted is less than the max int128 value, however fails to validate that the liquidityGlobal + liquidityMinted is less than the max uint128 value.

This validation is later performed in the RangeTicks.insert function, however the disparity in validation poses risk for any future code changes.

Include the liquidityGlobal validation in the RangePositions.validate function.

Poolshark Team: Acknowledged.

In the _cross function the cache.liquidity amount is adjusted for the LIMIT_TICK and LIMIT_POOL cross statuses. However the cache.liquidity is immediately overwritten in the _iterate function, therefore the cache.liquidity writes in _cross have no effect.

Remove the unnecessary cache.liquidity adjustments in the _cross function.

Poolshark Team: Acknowledged.

The claim tick is rounded back to the earlier full tick after performing the position removal check on line 417. However in the old position would not need to be cleared when the params.claim is a half tick ahead of the position's lower tick.

Perform params.claim = TickMap.roundBack(params.claim, constants, params.zeroForOne, cache.priceClaim) before checking if clearing the original position is necessary on line 417.

Poolshark Team: Acknowledged.

The validation logic aims to prevent any burns which would result in no net change to the existing position with a NoPositionUpdates error.

However a burn with an amount of 0 and a claimTick which is just tickSpacing/2 ahead of the position's start tick will result in no net change to the existing position yet get past the validation logic.

Do not allow burns with an amount of 0 for claims where the claimTick rounds back to the start tick of the position.

Poolshark Team: Acknowledged.

Throughout the Ticks._quoteSingle function the cache.price variable is cast as uint256. However these variables are already uint256 variables and so therefore do not need to be cast as such.

Do not cast the cache.price variable as a uint256.

Poolshark Team: Acknowledged.

The _empty functions in RangeTicks and LimitTicks use an if condition to return true or false.

However the _empty function can simply return the result of the condition rather than using an if.

Return the result of the liquidityAbsolute != 0 check directly.

Poolshark Team: Acknowledged.

The Positions.validate call can be performed before the Positions.update as any update would be invalid if the provided lower/upper ticks are invalid.

Perform Positions.validate before Positions.update.

Poolshark Team: Acknowledged.

The contents of the else case on lines 95 and 130 are exactly that of the above if case where the tick already existed.

Rather than introducing another separate case with the same logic, adjust the original if cases to include the lower > tickAtPrice and upper > tickAtPrice cases.

Poolshark Team: Acknowledged.

The clones deployed with the cloneDeterministic function implement a receive function however neither the LimitPool nor the RangePoolIERC1155 implementations have methods to withdraw Ether that may be errantly sent to the clone.

Implement safety Ether withdrawal functions in case Ether is errantly transferred to the cloned contracts.

Poolshark Team: Acknowledged.

In the BurnRangeCall.perform function the position.amount0 and position.amount1 are decremented by the cache.amount0 and cache.amount1 after the RangePositions.remove call. The goal of this is simply to remove the burned fees amount from the position.

However the subtraction takes place after the RangePositions.remove call, where the cache.amount0 and cache.amount1 are incremented by the position liquidity removed by the burn.

For this reason the position.amount0 and position.amount1 are also increased by the removed liquidity amounts in the RangePositions.remove function.

However this unnecessary addition can be avoided by simply subtracting the cache.amount0 and cache.amount1 from the position amounts directly after the RangePositions.update call where the cache.amount0 and cache.amount1 represent exactly the burned fee amounts.

Move the position.amount0 and position.amount1 subtractions directly after the RangePositions.update call.

Poolshark Team: Acknowledged.