Sushi Trident contest phase 2 - cmichel's results

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPool.burn function performs an unsafe cast of a uint128 type to a signed integer.

(uint256 amount0fees, uint256 amount1fees) = _updatePosition(msg.sender, lower, upper, -int128(amount));

Note that amount is chosen by the caller and when choosing amount = 2**128 - 1, this is interpreted as 0xFFFFFFFFF... = -1 as a signed integer. Thus -(-1)=1 adds 1 liquidity unit to the position

This allows an attacker to not only mint LP tokens for free but as this is the burn function it also redeems token0/1 amounts according to the unmodified uint128 amount which is an extremely large value.

POC

I created this POC that implements a hardhat test and shows how to steal the pool tokens.

Choosing the correct amount of liquidity to burn and lower, upper ticks is not straight-forward because of two competing constraints:

1. the -int128(amount) must be less than MAX_TICK_LIQUIDITY (see _updatePosition). This drives the the amount up to its max value (as the max uint128 value is -1 => -(-1)=1 is very low)
2. The redeemed amount0, amount1 values must be less than the current pool balance as the transfers would otherwise fail. This drives the amount down. However, by choosing a smart lower and upper tick range we can redeem fewer tokens for the same liquidity.

This example shows how to steal 99% of the token0 pool reserves: https://gist.github.com/MrToph/1731dd6947073343cf6f942985d556a6

Impact

An attacker can steal the pool tokens.

Recommended Mitigation Steps

Even though Solidity 0.8.x is used, type casts do not throw an error. A SafeCast library must be used everywhere a typecast is done.

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPoolManager uses the positionId as an index for incentives[pool][positionId] when it should be incentiveId instead:

// @audit should be Incentive memory incentive = incentives[pool][incentiveId];
Incentive memory incentive = incentives[pool][positionId];

This issue occurs in subscribe, claimReward, getReward

Impact

The incentive reward mechanism does not work correctly and a single person owning the positionId of the actual incentiveId can claim all rewards.

Recommended Mitigation Steps

Use incentiveId to index incentives[pool][.]

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPoolManager keeps all tokens for all incentives in the same contract. The reclaimIncentive function does not reduce the incentive.rewardsUnclaimed field and thus one can reclaim tokens several times. This allows anyone to steal all tokens from all incentives by creating an incentive themself, and once it's expired, repeatedly claim the unclaimed rewards until the token balance is empty.

POC

• Attacker creates an incentive for a non-existent pool using a random address for pool (This is done such that no other user can claim rewards as we need a non-zero rewardsUnclaimed balance for expiry). They choose the incentive.token to be the token they want to steal from other incentives. (for example, WETH, USDC, or SUSHI) They choose the startTime, endTime, expiry such that the checks pass, i.e., starting and ending in a few seconds from now, expiring in 5 weeks. Then they choose a non-zero rewardsUnclaimed and transfer the incentive.token to the PoolManager.
• Attacker waits for 5 weeks until the incentive is expired
• Attacker can now call reclaimIncentive(pool, incentiveId, amount=incentive.rewardsUnclaimed, attacker, false) to withdraw incentive.rewardsUnclaimed of incentive.token from the pool manager.
• As the incentive.rewardsUnclaimed variable has not been decreased, they can keep calling reclaimIncentive until the pool is drained.

Impact

An attacker can steal all tokens in the PoolManager.

Recommended Mitigation Steps

In reclaimIncentive, reduce incentive.rewardsUnclaimed by the withdrawn amount.

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPool.mint/burn functions add/remove liquidity when (priceLower < currentPrice && currentPrice < priceUpper). Shouldn't it also be changed if priceLower == currentPrice?

Impact

Pools that mint/burn liquidity at a time where the currentPrice is right at the lower price range do not work correctly and will lead to wrong swap amounts.

Recommended Mitigation Steps

Change the inequalities to if (priceLower <= currentPrice && currentPrice < priceUpper).

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPool.burn function sends out amount0/amount1 tokens but only updates the reserves by decreasing it by the fees of these amounts.

unchecked {
    // @audit decreases by fees only, not by amount0/amount1
    reserve0 -= uint128(amount0fees);
    reserve1 -= uint128(amount1fees);
}

This leads to the pool having wrong reserves after any burn action. The pool's balance will be much lower than the reserve variables.

Impact

As the pool's actual balance will be much lower than the reserve variables, minting and swaping will not work correctly either. This is because of the amount0Actual + reserve0 <= _balance(token0) check in mint using a much higher reserve0 amount than the actual balance (already including the transferred assets from the user). An LP provider will have to make up for the missing reserve decrease from burn and pay more tokens.

The same holds true for swap which performs the same check in _updateReserves.

The pool essentially becomes unusable after a burn as LPs / traders need to pay more tokens.

Recommended Mitigation Steps

The reserve should be decreased by what is transferred out. In burn's case this is amount0 / amount1.

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPoolManager.addIncentive function can add an incentive that already has a non-zero incentive.secondsClaimed.

Impact

Rewards will be wrong.

Recommended Mitigation Steps

Add a check: require(incentive.secondsClaimed == 0, "!secondsClaimed").

Handle

cmichel

Vulnerability details

The ConcentratedLiquidityPoolManager.claimReward requires stake.initialized but it is never set. It also performs a strange computation as 128 - incentive.secondsClaimed which will almost always underflow and revert the transaction.

Impact

One cannot claim rewards.

Recommended Mitigation Steps

Rethink how claiming rewards should work.

Handle

cmichel

Vulnerability details

ConcentratedLiquidityPoolHelper.getTickState allocates only tickCount elements for the ticks array. But the while loop iterates over all ticks of the pool which can potentially become very large, much larger than any tickCount could iterate in a reasonable time and return with a reasonable data size.

Impact

The function reverts and cannot be used from off-chain.

Recommended Mitigation Steps

Add pagination by adding a startTick parameter and only iterate tickCount many ticks each call. Then return (ticks, nextTick) such that the caller can continue the pagination at nextTick.

Handle

cmichel

Vulnerability details

The TridentNFT.permit function ignores the recoveredAddress != 0 check if isApprovedForAll[owner][recoveredAddress] is true.

Impact

If a user accidentally set the zero address as the operator, tokens can be stolen by anyone as a wrong signature yield recoveredAddress == 0.

Recommended Mitigation Steps

Change the require logic to recoveredAddress != address(0) && (recoveredAddress == owner) || isApprovedForAll[owner][recoveredAddress]).