Cover

Proof of concept: PoC

The PoolState memory pool variable is loaded into memory before the pool0 or pool1 storage variables are updated by syncLatest. This can yield an outdated pool.price when the syncLatest call would have updated the relevant pool price.

When this outdated pool.price is used to compute the maxDx or maxDy, it can result in an unchecked underflow allowing virtually any amount of tokens to be swapped in a single auction. Therefore the entirety of the pool’s liquidity may be used at the current tick, regardless of the ranges each LPer wanted their position to be active over.

Load the memory pool variable into memory after the syncLatest function is called.

Poolshark Team: The recommendation was implemented in commit 8414f63.

Proof of concept: PoC

When users attempt to burn their position at a valid claimTick, the transaction reverts due to an underflow as the cache.finalDeltas.amountOutDeltaMax is greater than the amountOutDeltaMax stored on the position’s end tick.

When burning the cache.finalDeltas from the updateTick, the cache.finalDeltas.amountOutDeltaMax should never be greater than the updateTick.deltas.amountOutDeltaMax.

Ensure that the cache.finalDeltas.amountOutDeltaMax is never greater than the updateTick.deltas.amountOutDeltaMax when burning the cache.finalDeltas from the updateTick.deltas.

Additionally, add logic in Deltas.burn to protect against underflow in the event that rounding would result in an underflow revert and prevent users from burning.

Poolshark Team: The root cause of the underflow was fixed in 79e2bb6 and the Deltas.burn underflow protection was implemented in 74e646b.

Proof of concept: PoC

In Deltas.to the fromDeltas.amountOutDeltaMax is added to the toTick.deltas.amountOutDelta. Since an amountOutDeltaMax value is treated as an amountOutDelta value, more amountOut is errantly attributed to the toTick.

This can prevent users from burning as more funds may be attempted to be transferred than are in the contract. Furthermore, in many cases, assets that belong to other users’ positions will be transferred out, potentially causing catastrophic loss.

Replace the toTick.amountOutDelta += fromDeltas.amountOutDeltaMax with toTick.deltas.amountOutDelta += fromDeltas.amountOutDelta.

Poolshark Team: The recommendation was implemented in commit b924d61.

Proof of concept: PoC

When the price from the TWAP reverses directions, the active liquidity is not “stashed” on the stopTick.

This can lead to the CoverPool being bricked in the following scenario:

• The TWAP price begins at tick 20.
• Alice creates a position from tick 0 to tick -60 in pool0.
• The TWAP price goes down to tick -20, entering Alice’s position.
• Because tick 0 is a cross tick while syncing, the liquidityDelta on it will be cleared.
• Now the TWAP price goes back up to tick 0, and the liquidity for pool0 is zeroed out without being stashed.
• As the TWAP price goes down and crosses tick -60, in the _cross function the currentLiquidity will be 0 but the liquidityDelta on the lower tick of her position will still exist. As a result, the end tick liquidityDelta subtracted from zero will underflow for uint.

The end result is that the CoverPool is bricked once the TWAP tick goes below Alice’s lower tick.

When reversing directions and zeroing out the pool.liquidity, “stash” this remaining liquidity onto the stopTick so that it may be reactivated when the TWAP price continues in that direction.

Poolshark Team: The recommended fix was implemented in 79e2bb6.

Proof of concept: PoC1

In cases where an auction is fully filled at the time of claiming, the position ought to be shrunk so that the user is not able to claim again for this auction as a past auction.

However, since the position continues to include the previously filled auction tick, users who claim from a currently filled auction are credited with more tokens then they should be, effectively stealing from others in the pool.

When a user is claiming from a fully filled auction, shrink the position so that the user is not errantly credited with more tokens than they should be.

Poolshark Team: The recommendation was implemented in commit f4c6cf1.

Proof of concept: PoC

When syncs jump multiple ticks at a time and the direct nextTickToAccum does not exist in the TickMap, it is possible for users to experience significant loss of assets when the amountOutDelta calculations in _rollover are restricted to the range between the pool.price and the crossPrice.

This is because the range restriction in _rollover leaves out potentially several ticks that should be accounted for in the amountDelta calculations.

Consider initializing the nextTickToAccum0 and nextTickToAccum1 ticks if they don’t exist in the TickMap when creating the cache in syncLatest.

Alternatively, when TWAP updates span more than the tickSpread, include an amountOutDelta calculation from the auction starting price to the accumPrice in _rollover.

Poolshark Team: The recommendation was implemented in commit b87f161.

During syncLatest, pool1 is utilizing pool0 ticks when updating the EpochMap. In many cases this results in users being locked into their positions and unable to exit due to their end tick being unclaimable and a previous claimTick yielding an underflow.

if (cache.nextTickToAccum0 > cache.stopTick0
                 && ticks0[cache.nextTickToAccum0].liquidityDeltaMinus > 0) {
                EpochMap.set(tickMap, cache.nextTickToAccum0, state.accumEpoch);
}

Use pool1 ticks to update the ticks for pool1:

if (cache.nextTickToAccum1 < cache.stopTick1
                 && ticks1[cache.nextTickToAccum1].liquidityDeltaMinus > 0) {
                EpochMap.set(tickMap, cache.nextTickToAccum1, state.accumEpoch);
}

Poolshark Team: The recommendation was implemented in commit a77bd18.

Proof of concept: PoC

When the state.lastBlock - state.auctionStart is not an exact multiple of the auctionLength, the resulting maxLatestTickMove is not a multiple of the tickSpread.

This results in positions being created on invalid ticks and swaps receiving more than the available liquidity for the active auction.

Adjust the maxLatestTickMove so that it is a valid multiple of the tickSpread.

Poolshark Team: The recommendation was implemented in commit f0d52ad.

Proof of concept: PoC

When instantiating the cache using Claims.getDeltas in Positions.update the amountOutDeltaMaxStashed is unstashed onto the cache.deltas.amountOutDeltaMax.

The cache.deltas.amountOutDeltaMax is ultimately removed from the position’s end tick.

However, in section2 this same value is removed from the position’s end tick a second time, therefore perturbing the position’s accounting and locking the position in the pool.

Do not remove the same amountOutDeltaMax value from the position’s end tick in section2.

E.g. remove the following line from section2:

params.zeroForOne ? ticks[params.lower].deltas.amountOutDeltaMax -= amountOutUnfilledMax
                  : ticks[params.upper].deltas.amountOutDeltaMax -= amountOutUnfilledMax;

Poolshark Team: The recommendation was implemented in commit c9a3a42.

Proof of concept: PoC

It is possible for the pool to have active liquidity after the end of a position because the liquidityDeltaMinus double counts a portion of the current active liquidity stashed on the stopTick.

Consider the following scenario in pool0:

1. Alice creates a position from tick -20 to tick -60.
2. The TWAP goes down to tick -40, entering Alice’s position.
3. The TWAP goes up to tick -20. During the stash, the pool’s active liquidity is added to the liquidityDelta on the stopTick, -60. Additionally, Alice’s liquidityDeltaMinus is added to the stopTick, double counting her active liquidity.
4. The TWAP goes down to tick -60. Although this is the end of Alice’s position, pool0.liquidity is now non-zero as her liquidity was stashed onto this tick twice, negating the negative liquidityDelta at the end of her position.

When crossing the end of a position, account for the extra liquidityDelta by subtracting stashTick.liquidityDeltaMinus. Otherwise, remove the liquidityDeltaMinus altogether.

Poolshark Team: The recommendation was implemented in commit b8d5c44.

Proof of concept: PoC

A user is able to steal a portion of another user’s tokens due to their amountOutDeltaMax being considered during a claim even if they did not contribute to the current amountOutDelta on the claimTick. As a result, the percentOutDelta calculation will attribute tokens for a user when they should not be.

Consider the following scenario:

1. Price is at tick 0
2. Bob mints a position for 100 tokens from 20 to 60
3. Price goes past Bob’s claim tick and then back down to tick 20
4. Alice mints a position for 100 tokens from 40 to 60
5. amountOutDeltaMax on tick 60 is 200 and amountOutDelta is 100
6. Bob burns his entire liquidity with claim tick 60 but only receives 50% of his tokens
7. Alice burns her entire liquidity afterwards and receives 150 tokens, stealing 50 tokens from Bob.

Create another parameter to ignore some amountOutDeltaMax if a user has not contributed to a tick’s amountOutDelta, similar to the paradigm between liquidityDelta and liquidityDeltaMinus.

Poolshark Team: The recommendation was implemented in commit 72f87d3.

Proof of concept: PoC

Due to rounding in section3, in some cases users will not be able to burn all of their liquidity since amountOutRemoved is 1 wei greater than the amountOutDeltaMax stored on the position’s end tick. This will lead to the user's tx reverting with underflow if the user burns for most of their liquidity.

Consider performing an explicit check to see whether amountOutRemoved is greater than the tick’s amountOutDeltaMax. If so, set the amountOutDeltaMax to 0.

Poolshark Team: The recommendation was implemented in commit ba281fa.

Users may still create positions that begin inside of the safetyWindow due to the following check:

if (params.zeroForOne) {
    if (params.lower > cache.requiredStart) revert PositionInsideSafetyWindow();
} else {
    if (params.upper < cache.requiredStart) revert PositionInsideSafetyWindow();
}

The validation occurs on the position’s end tick rather than the start tick. Therefore users can create positions that begin before the cache.requiredStart. In the event that the position’s start tick is at or before the state.latestTick, it will be adjusted to the state.latestTick +/- state.tickSpread. This adjustment will still lie within the safetyWindow. However, if the safetyWindow validation above is corrected, this adjustment logic can be removed as users should never be able to create positions where the beginning of the position is before or equal to the state.latestTick.

Correct the above validation to compare against the params.upper in the pool0 case and the params.lower in the pool1 case.

Alternatively correct the shrinking logic to appropriately shrink the beginning of the position to outside of the safetyWindow by adjusting it to the requiredStart if it is before the requiredStart.

Poolshark Team: The recommendation was implemented in commit a02cb7c and 113a0e0.

In the Positions.validate function, minAmountPerAuction has already been adjusted to token1 precision when line 146 is reached, yet minAmountPerAuction is adjusted once again to token1 precision. As a result, minAmountPerAuction will be significantly smaller than intended (often 0), and the validation will be rendered useless.

Consider the following scenario:

• minAmountPerAuction = 1e18
• token1Decimals = 6
• minAmountPerAuction = 1e18 / 1e12 / 1e12 = 0

Remove the second adjustment.

Poolshark Team: The recommendation was implemented in commit 113a0e0.

Proof of concept: PoC

It is possible for a user to be credited with filled amounts that do not belong to them from a previous auction. This is because a stashed tick contains the deltaMax values for all positions — regardless of if they had already claimed from the stashed auction result.

Consider the following scenario:

1. Alice mints a position from tick 20 to tick 80 for 100 tokens.
2. Bob mints a position from tick 20 to tick 60 for 100 tokens.
3. The TWAP moves up to tick 20, 83 tokens are swapped.
4. The TWAP moves to tick 40, Bob burns half his liquidity receiving 50 tokenIn and 25 tokenOut.
5. The TWAP moves to tick 60, Bob burns his remaining liquidity. Bob receives 9 more tokenIn although his entire share of the auction was already claimed.
6. The TWAP moves to tick 80, Alice closes her position. Alice receives 24 tokenIn although her share should have been 33 tokenIn. Bob took 9 of Alice’s tokenIn.

Account for users having already claimed from past auctions or creating positions and being errantly credited with filled amounts from past auctions. Otherwise document this behavior and implement a “safety window” so that this mechanism cannot be harnessed to vamp filled amounts from LPers.

Poolshark Team: The suggested “safety window” was implemented in commit 36cb7a4.

Proof of concept: PoC

The cache.nextTickToAccum0 is set in the EpochMap even when the cache.nextTickToAccum0 is not crossed e.g. it is past the stopTick0.

Therefore users are unable to claim at the right tick and are able to claim at a tick that has not yet been accumulated to, perturbing the pool accounting.

Only set the cache.nextTickToAccum0 in the EpochMap if it is being crossed into.

Poolshark Team: The recommendation was implemented in commit 74e646b.

Proof of concept: PoC

In section5, an extra wei may be added to the position’s amountOut due to rounding up. Therefore, in some cases more funds are attempted to be transferred than are in the contract. Otherwise, 1 wei is taken from another user’s position.

Consider switching to Deltas.max or perform explicit handling.

Poolshark Team: The recommendation was implemented in commit e308d31.

state.liquidityGlobal is never set, so the liquidity overflow validation only catches amounts greater than type(int128).max.

Set the state.liquidityGlobal.

Poolshark Team: state.liquidityGlobal is now updated in commit d15bb47.

The TWAP tick of the reference pool is always rounded down. It may be beneficial to round up in certain cases to achieve a more accurate price point, and increase the speed of liquidity unlocking.

Consider rounding the TWAP tick of the reference pool to the nearest valid tick rather than always rounding down to the lower valid tick.

Poolshark Team: The TWAP tick now shifts by quartiles in commit db9e57e.

Throughout the codebase, block.number is used to perform syncs and determine auctionDepth.

However, block.number is synced with the mainnet block number every minute. Therefore less syncs will occur and the auctionDepth can be out of date on the order of ~4 blocks at the maximum.

Consider using ArbSys(100).arbBlockNumber() to rely on Arbitrum block numbers that are available in real-time.

Poolshark Team: block.timestamp was adopted to replace block.number in commit 116830b.

In the mint, burn and swap functions, the globalState storage variable is only updated after a token has been transferred to the recipient.

If the token is an ERC777 token and the receiver implements the tokensReceived hook, a potential read-only reentrancy arises in the quote function because protocol parameters such as latestPrice, auctionStart, and others are out of date.

Utilize the Check-Effects-Interactions pattern

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

Currently, there is no minimum bound on the twapLength, however, an insufficiently large twapLength will lead to viable oracle manipulation attacks.

Additionally, a twapLength of 0 will break the protocol, causing a panic revert when computing the averageTick in _calculateAverageTick.

Implement a minimum twapLength.

Poolshark Team: The recommendation was implemented in commit 2e9d570.

Users are able to update arbitrary positions as the UpdateParams in the add function utilize params.to rather than the msg.sender as the owner.

This way users can manipulate the fill percentage of others by forcing them to update their position when the pool fill percentage is unfavorable.

Use the msg.sender as the owner for the Positions.update call.

Poolshark Team: The recommendation was implemented in commit c703e95.

When a user mints to add to their existing position that was already crossed into, they end up with two positions: the previous position that was shrunk upon the Positions.update, and a newly minted positions that spans the original range.

This may lead to confusion as users may have expected to end up with a single position with added liquidity, rather than two separate positions.

Consider if the mint function should add liquidity to the newly shrunk position, otherwise ensure the existing behavior is well documented.

Poolshark Team: This is the expected behavior and it will be well documented.

In the documentation for the swap function, it is stated that “The router must prefund this contract…”, however the swap function transfers in the amountIn with a call to _transferIn.

Update the docs for the swap function.

Poolshark Team: The outdated docs were removed in commit 1ef04c0.

Although currently only ERC20 tokens are supported, the protocol would be incompatible with other contracts, arbitrageurs, and multisig functions if it sent Ether due to the use of transfer in SafeTransfers._transferOut.

transfer forwards only 2300 gas to protect from reentrancy, however, this hard-coded gas limit should be avoided as other protocols and contracts building on top of Poolshark may consume more than 2300 gas in their fallback/receive function.

Note that there are some multi-sig wallets that use more than 2300 gas in the fallback function.

Additionally, gas prices for certain opcodes may change in the future which would force fallback/receive functions that currently consume <2300 gas to consume >2300 gas and therefore become incompatible.

In the event that a contract cannot receive Ether due to this gas limitation, it may result in loss of funds.

Consider using call with a configurable gas limit that can be set sufficiently high and adding a lock modifier everywhere these transfer functions are used and can potentially reenter.

Poolshark Team: Native token transfers are not used in the system at the moment, so no code change will be made at this time.

In the Ticks.remove function, when removeUpper and removeLower are false, there are unnecessary storage manipulations that result in no net changes to the ticks mapping.

Only do these storage reads and writes inside of the conditionals where the tickLower and tickUpper are modified.

Poolshark Team: The recommendation was implemented in commit 9de5d31.

In the _collect function the CollectParams.to value is not used, and rather the claimed amount is always sent to the msg.sender.

Use the CollectParams.to address when transferring claimed amounts to the user.

Poolshark Team: The recommendation was implemented in commit e18ee82.

In the _rollover function, there is underflow protection for the maxes for pool0, but not for pool1.

Add underflow protection for pool1.

Poolshark Team: The underflow protection for pool1 was implemented in commit 6d12f2d.

Rather than returning a default state and allowing execution to continue, the tx should revert in the case that the priceLimit is unsatisfiable.

Consider reverting rather than returning default values, or choose to revert in the swap function when the amountOut is 0.

Poolshark Team: This behavior allows users to still trigger a syncLatest in the event that their priceLimit is unsatisfied.

The nextTickPrice variable is assigned to the state.latestPrice and then immediately the nextPrice is assigned to the nextTickPrice. The nextTickPrice is then never referenced again.

Remove the nextTickPrice variable and assign the nextPrice to the state.latestPrice directly.

Poolshark Team: The recommendation was implemented in commit 3a0d67a.

The Q128 constant is never referenced after assignment in the Ticks Epochs and Positions libraries.

Remove the Q128 constant.

Poolshark Team: The recommendation was implemented in commit b97c088.

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.

Pending Fix

Variables such as tickSpread and auctionLength in the global state can be made immutable.

Declare these variables immutable.

Poolshark Team: The recommendation was implemented in commit 4346660.

Proof of concept: PoC

The syncLatest function can be simplified in the following ways:

1. The additional _cross function call for the stopTick can be deduplicated if the check inside the while loop is modified from cache.nextTickToAccum0 > cache.stopTick0 to cache.nextTickToAccum0 >= cache.stopTick0.
2. The two cases where the stopTick is set in the TickMap if newLatestTick > state.latestTick or newLatestTick < state.latestTick can be consolidated into one newLatestTick != state.latestTick condition.
3. The increment and decrement of stopTick.liquidityDelta by liquidityDeltaMinus can be removed as there is no net effect.

Implement the suggested simplifications.

Poolshark Team: The suggested simplifications were implemented in 8edd1ce.