Sushi Trident contest phase 2 - broccoli's results

Handle

broccoli

Vulnerability details

Impact

In the burn function of ConcentratedLiquidityPool, when calling _updatePosition, the amount of liquidity to burn is explicitly converted from uint128 to int128, which could result in a positive integer if amount is larger than (1 << 127) and less than (1 << 128) (i.e., the MSB of amount is set). As a result, the pool's liquidity decreases, but the attacker's liquidity at a specific location increases. The incorrect liquidity could affect all trades in the pool afterward.

Proof of Concept

1. Suppose that the current price is at the tick 500000. Consider an attacker calling the burn function with lower = upper = 50000, and amount = (1 << 128) - 100000.
2. Since the current price, the lower price, and the upper price are all the same (there's no check to ensure that the upper price should be greater than the lower price), the amount0 and amount1 to be withdrawn from the pool will both be 0. The pool's liquidity is decreased by amount (and could underflow to a large number).
3. However, when the _updatePosition function is called, the liquidity to update is calculated by -int128(amount) = -int128((1 << 128) - 100000) = -(-100000) = 100000, which causes the attacker's liquidity at positions[attacker][lower][lower] to be increased by 100000.
4. In short, the attacker does not have to prepare any liquidity at any position to control (pump or decrease) the pool's liquidity. Due to the incorrect liquidity value, all swaps afterward are affected.
5. Moreover, the attacker could potentially exploit the controlled liquidity value to profit from the pool.

Referenced code: ConcentratedLiquidityPool.sol#L252

Recommended Mitigation Steps

In the burn function, check the amount exceeds type(uint128).max or not. If so, then revert the transaction. Or consider using the SafeCast library from OpenZeppelin instead.

Handle

broccoli

Vulnerability details

Impact

In the subscribe function of ConcentratedLiquidityPoolManager, the incentive to subscribed is determined as follows:

Incentive memory incentive = incentives[pool][positionId];

However, positionId should be incentiveId, a counter that increases by one whenever a new incentive is added to the pool. The usage of positionId could cause the wrong incentive to be used, and in general, the incentive is not found, and the transaction reverts (the condition block.timestamp < incentive.endTime is not met). The getReward and claimReward functions have the bug of misusing positionId as the index of incentives.

Proof of Concept

Referenced code: ConcentratedLiquidityPoolManager.sol#L68 ConcentratedLiquidityPoolManager.sol#L87 ConcentratedLiquidityPoolManager.sol#L105

Recommended Mitigation Steps

Change positionId to incentiveId in the referenced lines of code.

Handle

broccoli

Vulnerability details

Impact

The reclaimIncentive function of ConcentratedLiquidityPoolManager does not update the rewardsUnclaimed variable after some rewards are reclaimed. Thus, an attacker could add an incentive with a corresponding token, such as DAI, and reclaim the incentive multiple times to drain all the DAI within the manager contract.

Proof of Concept

Referenced code: ConcentratedLiquidityPoolManager.sol#L59-L60

Recommended Mitigation Steps

Subtract incentive.rewardsUnclaimed by amount before the _transfer (which prevents reentrancy attacks).

Handle

broccoli

Vulnerability details

Impact

Similar to a previous finding in the IndexPool contract, the mint function of ConcentratedLiquidityPool allows integer overflows when checking the balance is larger or equal to the received amount of token plus the reserve. As a result, the attacker could get a large amount of liquidity but only provide a small number of tokens to the pool, effectively stealing other LPs' funds when burning his liquidity.

Notice that this bug is independent of another bug of incorrect casting uint256 type to uint128 in the _getAmountsForLiquidity function. Even if the previously mentioned bug does not exist, the attacker could still steal the funds in the pool by exploiting this bug.

Proof of Concept

1. Suppose that the current price is at the tick 500000, an attacker calls the mint function with the following parameters:

mintParams.lower = 100000
mintParams.upper = 500000
mintParams.amount1Desired = (1 << 128) - 47541305835 # a carefully chosen number
mintParams.amount0Desired = 0

2. Since the current price is equal to the upper price, we have

_liquidity = mintParams.amount1Desired * (1 << 96) // (priceUpper - priceLower)
           = 4731732988155153573010127839

3. The amounts of token0 and token1 that the attacker has to pay is

amount0Actual = 0
amount1Actual = uint128(DyDxMath.getDy(_liquidity, priceLower, priceUpper, true))
              = uint128(_liquidity * (priceUpper - priceLower) // (1 << 96)) # round up
              = uint128(340282366920938463463374607384226905622)
              = 340282366920938463463374607384226905622
              = (1 << 128) - 47541305834

4. As long as reserve1 is greater than 47541305834, the addition amount1Actual + reserve1 overflows to a small number, causing the attacker to pass the balance check.

Referenced code: ConcentratedLiquidityPool.sol#L204 ConcentratedLiquidityPool.sol#L209

Recommended Mitigation Steps

Consider removing the unchecked statement to check for integer overflow or casting both amount1Actual and reserve1 to type uint256 before adding them and comparing to the _balance(token).

Handle

broccoli

Vulnerability details

Impact

The _getAmountsForLiquidity function of ConcentratedLiquidityPool explicitly converts the result of DyDxMath.getDy and DyDxMath.getDx from type uint256 to type uint128. The explicit casting without checking whether the integer exceeds the maximum number (i.e., type(uint128).max) could cause incorrect results being used. Specifically, an attacker could exploit this bug to mint a large amount of liquidity but only pay a little of token0 or token1 to the pool and effectively steal other's funds when burning his liquidity.

Proof of Concept

1. Suppose that the current price is at the tick 500000, an attacker calls the mint function with the following parameters:

mintParams.lower = 100000
mintParams.upper = 500000
mintParams.amount1Desired = (1 << 128) + 71914955423 # a carefully chosen number
mintParams.amount0Desired = 0

2. Since the current price is equal to the upper price, we have

_liquidity = mintParams.amount1Desired * (1 << 96) // (priceUpper - priceLower)
           = 4731732988155153573010127840

3. The amounts of token0 and token1 that the attacker has to pay is

amount0Actual = 0
amount1Actual = uint128(DyDxMath.getDy(_liquidity, priceLower, priceUpper, true))
              = uint128(_liquidity * (priceUpper - priceLower) // (1 << 96)) # round up
              = uint128(340282366920938463463374607456141861046)             # exceed the max
              = 24373649590                                                  # truncated

4. The attacker only pays 24373649590 of token1 to get 4731732988155153573010127840 of the liquidity, which he could burn to get more token1. As a result, the attacker is stealing the funds from the pool and could potentially drain it.

Referenced code: ConcentratedLiquidityPool.sol#L480 concentratedPool/DyDxMath.sol#L15 concentratedPool/DyDxMath.sol#L30

Recommended Mitigation Steps

Check whether the result of DyDxMath.getDy or DyDxMath.getDx exceeds type(uint128).max or not. If so, then revert the transaction. Or consider using the SafeCast library from OpenZeppelin instead.

Handle

broccoli

Vulnerability details

range fee growth underflow

Impact

The function RangeFeeGrowth ConcentratedLiquidityPool.sol#L601-L633 would revert the transaction in some cases.

When a pool cross a tick, it only updates either feeGrowthOutside0 or feeGrowthOutside1. Ticks.sol#L23-L53

RangeFeeGrowth calculates the fee as follow:

            feeGrowthInside0 = _feeGrowthGlobal0 - feeGrowthBelow0 - feeGrowthAbove0;
            feeGrowthInside1 = _feeGrowthGlobal1 - feeGrowthBelow1 - feeGrowthAbove1;

feeGrowthBelow + feeGrowthAbove is not necessary smaller than _feeGrowthGlobal. Please see POC.

Users can not provide liquidity or burn liquidity. Fund will get stocked in the contract. I consider this is a high-risk issue.

Proof of Concept

    # This is the wrapper.
    # def add_liquidity(pool, amount, lower, upper)
    # def swap(pool, buy, amount)

    add_liquidity(pool, deposit_amount, -800, 500)
    add_liquidity(pool, deposit_amount, 400, 700)
    # We cross the tick here to trigger the bug.

    swap(pool, False, deposit_amount)
    # Only tick 700's feeGrowthOutside1 is updated

    swap(pool, True, deposit_amount)
    # Only tick 500's feeGrowthOutside0 is updated
    
    # current tick at -800

    # this would revert
    # feeGrowthBelow1 = feeGrowthGlobal1
    # feeGrowthGlobal1 - feeGrowthBelow1 - feeGrowthAbove1 would revert
    # user would not be able to mint/withdraw/cross this tick. The pool is broken
    add_liquidity(pool, deposit_amount, 400, 700)

Tools Used

Hardhat

Recommended Mitigation Steps

It's either modify the tick's algo or RangeFeeGrowth. The quick-fix I come up with is to deal with the fee in RangeFeeGrowth. However, I recommend the team to go through tick's logic again.

Handle

broccoli

Vulnerability details

burn do not burn reserve

Impact

ConcentratedLiquidityPool.sol#L263-L266

The dev mistakenly write:

            reserve0 -= uint128(amount0fees);
            reserve1 -= uint128(amount1fees);

It should be

            reserve0 -= uint128(amount0fees);
            reserve1 -= uint128(amount1fees);

Other users can't mint lp unless they first send the extra tokens to match the reserve. I consider this is a high-risk issue.

Proof of Concept

None

Tools Used

None

Recommended Mitigation Steps

I believe it's a mistake.

            reserve0 -= uint128(amount0fees);
            reserve1 -= uint128(amount1fees);

Handle

broccoli

Vulnerability details

Impact

The claimReward function of ConcentratedLiquidityPoolManager calculates the secondsUnclaimed variable using a formula with an unclear intention:

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

This formula causes an integer underflow error when incentive.secondsClaimed is greater than 128, which generally happens in most cases since incentive.secondsClaimed is scaled by a factor of 1 << 128. The integer underflow reverts the transaction and prevents anyone from claiming the reward.

Proof of Concept

Referenced code: ConcentratedLiquidityPoolManager.sol#L93 ConcentratedLiquidityPoolManager.sol#L110

Recommended Mitigation Steps

Fix the calculation of secondsUnclaimed.

Handle

broccoli

Vulnerability details

Impact

There could be an integer underflow error when the reward of an incentive is claimed, forcing users to wait for a sufficient period or reduce their liquidity to claim the rewards.

Proof of Concept

The unclaimed reward that a user could claim is proportional to the secondsInside, which is, in fact, proportional to the position's liquidity. It is possible that the liquidity is too large and causes secondsInside to be larger than secondsUnclaimed. As a result, the rewards that the user wants to claim exceed the incentive.rewardsUnclaimed and causes an integer underflow error, which prevents him from getting the rewards.

Referenced code: ConcentratedLiquidityPoolManager.sol#L94-L95

Recommended Mitigation Steps

Check whether the rewards exceeds the incentive.rewardsUnclaimed. If so, then send only incentive.rewardsUnclaimed amount of rewards to the user.