Sushi Trident contest phase 2 - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

The positionId is used to retrieve the incentive info instead of incentiveId.

Incentive storage / memory incentive = incentives[position.pool][positionId];

Recommended Mitigation Steps

Incentive storage / memory incentive = incentives[position.pool][incentiveId];

Handle

hickuphh3

Vulnerability details

Impact

reclaimIncentive() withdraws any unclaimed rewards to the incentive owner. While there is a check to prevent re-claiming of rewards

require(incentive.rewardsUnclaimed >= amount, "ALREADY_CLAIMED");

it is ineffective because incentive.rewardsUnclaimed is not decremented by the amount specified. This means an incentive owner can claim other incentives that are denominated in the same token.

Recommended Mitigation Steps

The critical line incentive.rewardsUnclaimed -= amount; should be added after the checks are performed, as part of the best practice of the CEI pattern.

Handle

hickuphh3

Vulnerability details

Impact

Swap fees are taken from the output. Hence, if swapping token0 for token1 (zeroForOne is true), then fees are taken in token1. We see this to be the case in the initialization of feeGrowthGlobal in the swap cache

feeGrowthGlobal = zeroForOne ? feeGrowthGlobal1 : feeGrowthGlobal0;

and in _updateFees().

However, looking at Ticks.cross(), the logic is the reverse, which causes wrong fee accounting.

if (zeroForOne) {
	...
	ticks[nextTickToCross].feeGrowthOutside0 = feeGrowthGlobal - ticks[nextTickToCross].feeGrowthOutside0;
} else {
	...
	ticks[nextTickToCross].feeGrowthOutside1 = feeGrowthGlobal - ticks[nextTickToCross].feeGrowthOutside1;
}

Recommended Mitigation Steps

Switch the 0 and 1 in Ticks.cross().

if (zeroForOne) {
	...
	// feeGrowthGlobal = feeGrowthGlobal1
	ticks[nextTickToCross].feeGrowthOutside1 = feeGrowthGlobal - ticks[nextTickToCross].feeGrowthOutside1;
} else {
	...
	// feeGrowthGlobal = feeGrowthGlobal0
	ticks[nextTickToCross].feeGrowthOutside0 = feeGrowthGlobal - ticks[nextTickToCross].feeGrowthOutside0;
}

Handle

hickuphh3

Vulnerability details

Impact

secondsPerLiquidity is updated as such: secondsPerLiquidity += uint160((diff << 128) / liquidity); where diff = timestamp - uint256(lastObservation). Hence, whenever liquidity changes, secondsPerLiquidity should be updated prior to the change.

In particular, this affects the mint() and burn() functions, in the case where liquidity changes when lowerTick <= currentTick < upperTick.

In fact, the latest secondsPerLiquidity value should be calculated and used in Ticks.insert(). For comparison, notice how UniswapV3 fetches the latest value by calling observations.observeSingle() in its _updatePosition() function.

Recommended Mitigation Steps

The secondsPerLiquidity increment logic should be applied prior to liquidity addition in mint() and removal in burn().

// insert logic before these lines in mint()
unchecked {
  if (priceLower < currentPrice && currentPrice < priceUpper) liquidity += uint128(_liquidity);
}

nearestTick = Ticks.insert(
            ticks,
            feeGrowthGlobal0,
            feeGrowthGlobal1,
            secondsPerLiquidity, // should calculate and use latest secondsPerLiquidity value
						...
);

// insert logic before before these lines in burn()
unchecked {
  if (priceLower < currentPrice && currentPrice < priceUpper) liquidity -= amount;
}

Handle

hickuphh3

Vulnerability details

Impact

The fee growth mechanism, and by extension, secondsPerLiquidity mechanism of Uniswap V3 has the ability to underflow. It is therefore a necessity for the math to (ironically) be unsafe / unchecked.

Proof of Concept

Assume the following scenario and initial conditions:

• Price at parity (nearestTick is 0)
• tickSpacing of 10
• Swaps only increase the price (nearestTick moves up only)
• feeGrowthGlobal initializes with 0, increases by 1 for every tick moved for simplicity
• Existing positions that provide enough liquidity and enable nearestTick to be set to values in the example
• Every tick initialized in the example is ≤ nearestTick, so that its feeGrowthOutside = feeGrowthGlobal

1. When nearestTick is at 40, Alice creates a position for uninitialised ticks [-20, 30]. The ticks are initialized, resulting in their feeGrowthOutside values to be set to 40.
2. nearestTick moves to 50. Bob creates a position with ticks [20, 30] (tick 20 is uninitialised, 30 was initialized from Alice's mint). tick 20 will therefore have a feeGrowthOutside of 50.
3. Let us calculate rangeFeeGrowth(20,30).

   • lowerTick = 20, upperTick = 30

   • feeGrowthBelow = 50 (lowerTick's feeGrowthOutside) since lowerTick < currentTick

   • feeGrowthAbove = 50 - 40 = 10 (feeGrowthGlobal - upperTick's feeGrowthOutside) since upperTick < currentTick

   • feeGrowthInside

     = feeGrowthGlobal - feeGrowthBelow - feeGrowthAbove

     = 50 - 50 - 10

     = -10

We therefore have negative feeGrowthInside.

This behaviour is actually acceptable, because the important thing about this mechanism is the relative values to each other, not the absolute values themselves.

Recommended Mitigation Steps

rangeFeeGrowth() and rangeSecondsInside() has to be unchecked. In addition, the subtraction of feeGrowthInside values should also be unchecked in _updatePosition() and ConcentratedLiquidityPosition#collect().

The same also applies for the subtraction of pool.rangeSecondsInside and stake.secondsInsideLast in claimReward() and getReward() of the ConcentratedLiquidityPoolManager contract.

Handle

hickuphh3

Vulnerability details

Impact

No check is performed for the initial price. This means that it can be set to be below the MIN_SQRT_RATIO or above MAX_SQRT_RATIO (Eg. zero value), which will prevent the usability of all other functions (minting, swapping, burning).

For example, Ticks.insert() would fail when attempting to calculate actualNearestTick = TickMath.getTickAtSqrtRatio(currentPrice);, which means no one will be able to mint positions.

Recommended Mitigation Steps

Check the initialPrice is within the acceptable range, ie. MIN_SQRT_RATIO <= initialPrice <= MAX_SQRT_RATIO

Handle

hickuphh3

Vulnerability details

Introduction

Uniswap V3's whitepaper describes the fee growth mechanism, but the intuition behind it is not explained well (IMO). I've not been able to find any material that tries to describe it, so allow me the luxury of doing so. It is crucial to understand how it works, so that other issues regarding the fee growth variables (and by extension, secondsPerLiquidity) raised by fellow wardens / auditors are better understood by readers.

Objective

We want a way to accurately track the fees accumulated by a position. Fees should only be given to the position it is active (the current tick / price is within the lower and upper ticks of the position).

feeGrowthGlobal

Defined as the total amount of fees that would have been earned by 1 unit of unbounded liquidity that was deposited when the contract was first initialized. For simplicity, we can take this to be the range between MIN_TICK and MAX_TICK. We represent it visually like this:

// <-------------------------------------------------------------------------->
// MIN_TICK                                                               MAX_TICK

feeGrowthOutside

The fee growth per unit of liquidity on the other side of this tick (relative to the current tick). What does this mean?

As defined, it is the fee growth relative to the current tick. Based on the convention, we define 2 cases:

• Case 1: initialized tick ≤ pool tick
• Case 2: Initialized tick > pool tick

Visually, the feeGrowthOutside will look like this:

// CASE 1
// <--------------------|--------------------|
// MIN_TICK         INIT_TICK            POOL_TICK
// <-----------------------------------------|
// MIN_TICK                        INIT_TICK = POOL_TICK

// CASE 2
//                                           |--------------------|---------------->
//                                       POOL_TICK           INIT_TICK          MAX_TICK

Hence, regardless of whether the tick to initialize is either a lower or upper tick of a position, the feeGrowthOutside value that it is referring to is relatve to the pool tick.

In other words, if initialized tick ≤ pool tick, then its feeGrowthOutside is towards MIN_TICK. Otherwise, its feeGrowthOutside is towards MAX_TICK.

Initialization

By convention, when a tick is initialized, all fee growth is assumed to happen below it. Hence, the feeGrowthOutside is initialized to the following values:

• Case 1: tick's feeGrowthOutside = feeGrowthGlobal
• Case 2: tick's feeGrowthOtuside = 0

Implications

One should now understand why the feeGrowthOutside value is being flipped when crossing a tick, ie. tick.feeGrowthOutside = feeGrowthGlobal - tick.feeGrowthOutside in Tick.cross(), because it needs to follow the definition. (Case 1 becomes case 2 and vice versa).

It should hopefully become clear why using nearestTick as the reference point for fee growth calculations instead of the pool tick might not a wise choice. (Case 1 and 2 becomes rather ambiguous).

Range fee growth / feeGrowthInside

Going back to our objective of calculating the fee growth accumulated for a position, we can break it down into 3 cases (take caution with the boundary cases), and understand how their values are calculated. In general, we take it to be feeGrowthGlobal - fee growth below lower tick - fee growth above upper tick (see illustrations), although it can be simplified further.

1. pool tick < lower tick

   // ---------------------|---------------------|-----------------|-----------------
   //                  POOL_TICK            LOWER_TICK          UPPER_TICK
   // <---------------------------- feeGrowthGlobal -------------------------------->
   //       LOWER_TICK.feeGrowthOutside (CASE 2) |---------------------------------->
   //                         UPPER_TICK.feeGrowthOutside (CASE 2) |---------------->
   
   // we want the range between LOWER_TICK and UPPER_TICK
   // = LOWER_TICK.feeGrowthOutside - UPPER_TICK.feeGrowthOutside
   
   // alternatively, following the general formula, it is
   // = feeGrowthGLobal - fee growth below LOWER_TICK - fee growth above UPPER_TICK
   // = feeGrowthGlobal - (feeGrowthGlobal - LOWER_TICK.feeGrowthOutside) - UPPER_TICK.feeGrowthOtuside
   // = LOWER_TICK.feeGrowthOutside - UPPER_TICK.feeGrowthOutside

2. lower tick ≤ pool tick < upper tick

   // ---------------------|---------------------|-----------------|-----------------
   //                  LOWER_TICK            POOL_TICK        UPPER_TICK
   // <---------------------------- feeGrowthGlobal -------------------------------->
   // <--------------------| LOWER_TICK's feeGrowthOutside (CASE 1)
   //                       UPPER_TICK's feeGrowthOutside (CASE 2) |----------------> 
   
   // we want the range between LOWER_TICK and UPPER_TICK
   // = feeGrowthGLobal - fee growth below LOWER_TICK - fee growth above UPPER_TICK
   // = feeGrowthGLobal - LOWER_TICK.feeGrowthOutside - UPPER_TICK.feeGrowthOutside

3. upper tick ≤ pool tick

   // ---------------------|---------------------|-----------------|-----------------
   //                  LOWER_TICK            POOL_TICK        UPPER_TICK
   // <---------------------------- feeGrowthGlobal -------------------------------->
   // <--------------------| LOWER_TICK's feeGrowthOutside (CASE 1)
   // <------------------------------------------------------------| UPPER_TICK's feeGrowthOutside (CASE 1)
   
   // we want the range between LOWER_TICK and UPPER_TICK
   // = UPPER_TICK.feeGrowthOutside - LOWER_TICK.feeGrowthOutside
   
   // alternatively, following the general formula, it is
   // = feeGrowthGLobal - fee growth below LOWER_TICK - fee growth above UPPER_TICK
   // = feeGrowthGLobal - LOWER_TICK.feeGrowthOutside - (feeGrowthGlobal - UPPER_TICK.feeGrowthOutside)
   // = UPPER_TICK.feeGrowthOutside - LOWER_TICK.feeGrowthOutside

Handling The Boundary Case

An under appreciated, but very critical line of Uniswap V3's pool contract is the following:

state.tick = zeroForOne ? step.tickNext - 1 : step.tickNext;

It serves a dual purpose:

1. Because of how Tick Bitmap works, the tick needs to be manually decremented by 1 so that the next tick to be found is in the next word.
2. More importantly, it handles the boundary case, where zeroForOne is true (pool tick goes down). In this scenario, case 1 becomes case 2 when the tick is crossed. However, should the poolTick after the swap be equal to step.tickNext, then when calculating fee growth inside a position that so happens to have step.tickNext as one of its ticks, it will be treated as case 1 (poolTick = lowerTick / upperTick) when it is required to be treated as case 2.

Impact

Hopefully, this writeup helps readers understand the fee growth mechanism and its workings. More importantly, I hope it helps the team to understand why using nearestTick as the reference point for fee growth mechanism is unsuitable. Specifically, we have 2 high severity issues:

• Wrong initialization value of feeGrowthOutside in the case either the lower or upper tick becomes the nearestTick upon insertion of a new tick.
  • You are (in a sense) crossing the old nearestTick, so its secondsPerLiquidityOutside has to be flipped
  • The lower / upper tick's feeGrowthOutside is incorrectly initialized to be 0 when it should be feeGrowthOutside
• Case 1 and 2 becomes ambiguous. When a position is modified with either tick being nearestTick, it is treated to be case 1 when in fact there are times it should be treated as case 2.

Recommended Mitigation Steps

Having a pool tick counter that closely matches the current pool price is rather critical for fee growth and seconds per liquidity initializations / calculations.

Where relevant, the nearestTick should be replaced by poolTick.

Handle

hickuphh3

Vulnerability details

Impact

The subtraction of secondsClaimed should be performed after the left shifting of bits in

uint256 secondsUnclaimed = (maxTime - incentive.startTime) << (128 - incentive.secondsClaimed);

Recommended Mitigation Steps

uint256 secondsUnclaimed = ((maxTime - incentive.startTime) << 128) - incentive.secondsClaimed);

Handle

hickuphh3

Vulnerability details

Impact

getTickState() attempts to fetch the state of all inserted ticks (including MIN_TICK and MAX_TICK) of a pool. Depending on the tick spacing, this function may run out of gas.

Recommended Mitigation Steps

Have a starting index parameter to start the iteration from. Also, tickCount can be made use of more meaningfully to limit the number of iterations performed.

function getTickState(
	IConcentratedLiquidityPool pool,
	int24 startIndex,
	uint24 tickCount
) external view returns (SimpleTick[] memory) {
  SimpleTick[] memory ticks = new SimpleTick[](tickCount);

  IConcentratedLiquidityPool.Tick memory tick;
	int24 current = startIndex;

	for (uint24 i; i < tickCount; i++) {
		tick = pool.ticks(current);
		ticks[i] = SimpleTick({index: current, liquidity: tick.liquidity});
		// reached end of linked list, exit loop
		if (current == TickMath.MAX_TICK) break;
		// else, continue with next iteration
		current = tick.nextTick;
	}

  return ticks;
}