Sushi Trident contest phase 2 - WatchPug's results

Handle

WatchPug

Vulnerability details

The reclaimIncentive() function allows users who added incentives before to withdraw unclaimed rewards.

However, the current implementation did not manage the state correctly, incentive.rewardsUnclaimed is not updated after the token transfer, which allows the user to call reclaimIncentive() again and get incentive.token up to the amount of rewardsUnclaimed.

When the incentive.token is set to token0 or token1, this can be used for stealing yields from other users.

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L59-L60

/// @dev Withdraws any unclaimed incentive rewards.
function reclaimIncentive(
    IConcentratedLiquidityPool pool,
    uint256 incentiveId,
    uint256 amount,
    address receiver,
    bool unwrapBento
) public {
    Incentive storage incentive = incentives[pool][incentiveId];
    require(incentive.owner == msg.sender, "NOT_OWNER");
    require(incentive.expiry < block.timestamp, "EXPIRED");
    require(incentive.rewardsUnclaimed >= amount, "ALREADY_CLAIMED");
    _transfer(incentive.token, address(this), receiver, amount, unwrapBento);
    emit ReclaimIncentive(pool, incentiveId);
}

Recommended Mitigation Steps

Consider removing the amount parameter and always withdraw all the unclaimed rewards to the owner, and delete the incentive.

Handle

WatchPug

Vulnerability details

When a user calls ConcentratedLiquidityPosition.sol#collect() to collect their yield, it calcuates the yield based on position.pool.rangeFeeGrowth() and position.feeGrowthInside0, position.feeGrowthInside1:

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPosition.sol#L75-L101

When there are enough tokens in bento.balanceOf, it will not call position.pool.collect() to collect fees from the pool.

This makes the user who collect() their yield when there is enough balance to get double yield when they call burn() to remove liquidity. Because burn() will automatically collect fees on the pool contract.

Impact

The yield belongs to other users will be diluted.

Recommended Mitigation Steps

Consider making ConcentratedLiquidityPosition.sol#burn() call position.pool.collect() before position.pool.burn(). User will need to call ConcentratedLiquidityPosition.sol#collect() to collect unclaimed fees after burn().

Or ConcentratedLiquidityPosition.sol#collect() can be changed into a public method and ConcentratedLiquidityPosition.sol#burn() can call it after position.pool.burn().

Handle

WatchPug

Vulnerability details

When a user calls ConcentratedLiquidityPosition.sol#burn() to burn their liquidity, it calls ConcentratedLiquidityPool.sol#burn() -> _updatePosition():

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPool.sol#L525-L553

The _updatePosition() function will return amount0fees and amount1fees of the whole position with the lower and upper tick and send them to the recipient alongside the burned liquidity amounts.

Proof of Concept

1. Alice minted $10000 worth of liquidity with lower and upper tick set to 99 and 199;
2. Alice accumulated $1000 worth of fee in token0 and token1;
3. The attacker can mint a small amount ($1 worth) of liquidity using the same lower and upper tick;
4. The attacker calls ConcentratedLiquidityPosition.sol#burn() to steal all the unclaimed yield with the ticks of (99, 199) include the $1000 worth of yield from Alice.

Recommended Mitigation Steps

Consider making ConcentratedLiquidityPosition.sol#burn() always use address(this) as recipient in:

position.pool.burn(abi.encode(position.lower, position.upper, amount, recipient, unwrapBento));

and transfer proper amounts to the user.

Handle

WatchPug

Vulnerability details

The reserves should be updated once LP tokens are burned to match the actual total bento shares hold by the pool.

However, the current implementation only updated reserves with the fees subtracted.

Makes the reserve0 and reserve1 smaller than the current balance0 and balance1.

Impact

As a result, many essential features of the contract will malfunction, includes swap() and mint().

Recommended Mitigation Steps

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPool.sol#L263-L267

Change:

        unchecked {
            reserve0 -= uint128(amount0fees);
            reserve1 -= uint128(amount1fees);
        }

to:

        unchecked {
            reserve0 -= uint128(amount0);
            reserve1 -= uint128(amount1);
        }

Handle

WatchPug

Vulnerability details

ConcentratedLiquidityPoolManager.sol Wrong implementation of claimReward() and getReward()

Given incentive.secondsClaimed can usually be larger than 128, both claimReward() and getReward() will revert at:

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

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L93

Handle

WatchPug

Vulnerability details

In ConcentratedLiquidityPosition.collect(), balances of token0 and token1 in bento will be used to pay the fees.

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPosition.sol#L103-L116

    uint256 balance0 = bento.balanceOf(token0, address(this));
    uint256 balance1 = bento.balanceOf(token1, address(this));
    if (balance0 < token0amount || balance1 < token1amount) {
        (uint256 amount0fees, uint256 amount1fees) = position.pool.collect(position.lower, position.upper, address(this), false);

        uint256 newBalance0 = amount0fees + balance0;
        uint256 newBalance1 = amount1fees + balance1;

        /// @dev Rounding errors due to frequent claiming of other users in the same position may cost us some raw
        if (token0amount > newBalance0) token0amount = newBalance0;
        if (token1amount > newBalance1) token1amount = newBalance1;
    }
    _transfer(token0, address(this), recipient, token0amount, unwrapBento);
    _transfer(token1, address(this), recipient, token1amount, unwrapBento);

In the case of someone add an incentive with token0 or token1, the incentive in the balance of bento will be used to pay fees until the balance is completely consumed.

As a result, when a user calls claimReward(), the contract may not have enough balance to pay (it supposed to have it), cause the transaction to fail.

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L78-L100

function claimReward(
        uint256 positionId,
        uint256 incentiveId,
        address recipient,
        bool unwrapBento
    ) public {
        require(ownerOf[positionId] == msg.sender, "OWNER");
        Position memory position = positions[positionId];
        IConcentratedLiquidityPool pool = position.pool;
        Incentive storage incentive = incentives[position.pool][positionId];
        Stake storage stake = stakes[positionId][incentiveId];
        require(stake.initialized, "UNINITIALIZED");
        uint256 secondsPerLiquidityInside = pool.rangeSecondsInside(position.lower, position.upper) - stake.secondsInsideLast;
        uint256 secondsInside = secondsPerLiquidityInside * position.liquidity;
        uint256 maxTime = incentive.endTime < block.timestamp ? block.timestamp : incentive.endTime;
        uint256 secondsUnclaimed = (maxTime - incentive.startTime) << (128 - incentive.secondsClaimed);
        uint256 rewards = (incentive.rewardsUnclaimed * secondsInside) / secondsUnclaimed;
        incentive.rewardsUnclaimed -= rewards;
        incentive.secondsClaimed += uint160(secondsInside);
        stake.secondsInsideLast += uint160(secondsPerLiquidityInside);
        _transfer(incentive.token, address(this), recipient, rewards, unwrapBento);
        emit ClaimReward(positionId, incentiveId, recipient);
    }

The same issue applies to reclaimIncentive() as well.

https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L49-L62

function reclaimIncentive(
    IConcentratedLiquidityPool pool,
    uint256 incentiveId,
    uint256 amount,
    address receiver,
    bool unwrapBento
) public {
    Incentive storage incentive = incentives[pool][incentiveId];
    require(incentive.owner == msg.sender, "NOT_OWNER");
    require(incentive.expiry < block.timestamp, "EXPIRED");
    require(incentive.rewardsUnclaimed >= amount, "ALREADY_CLAIMED");
    _transfer(incentive.token, address(this), receiver, amount, unwrapBento);
    emit ReclaimIncentive(pool, incentiveId);
}

Recommendation

Consider making adding token0 or token1 as incentives disallowed, or keep a record of total remaining incentive amounts for the incentive tokens and avoid consuming these revered balances when collect().