Canto Liquidity Mining Protocol - sces60107's results

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L205 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L215 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L235 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L245 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L277

Vulnerability details

Impact

The sponsor mentioned that they would like to ensure that the amount of incentives released is exactly as we specify in README. https://github.com/code-423n4/2023-10-canto/blob/main/README.md#scoping-details

This LM protocol will be used to incentivize pools on Canto. We would like to ensure that the amount of incentives released is exactly as we specify and the wallets who receive the incentives are the correct ones

But current logic of ambient reward calculation could make some rewards unable to be claimed by liquidity providers.

Proof of Concept

First, we need to know how to calculate the ambient rewards for liquidity providers.

In LiquidityMining.accrueAmbientGlobalTimeWeightedLiquidity, It calculates timeWeightedWeeklyGlobalAmbLiquidity_ which is the total weight of ambient liquidity. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L205 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L215

    function accrueAmbientGlobalTimeWeightedLiquidity(
        bytes32 poolIdx,
        CurveMath.CurveState memory curve
    ) internal {
        uint32 lastAccrued = timeWeightedWeeklyGlobalAmbLiquidityLastSet_[poolIdx];
        // Only set time on first call
        if (lastAccrued != 0) {
            uint256 liquidity = curve.ambientSeeds_;
            uint32 time = lastAccrued;
            while (time < block.timestamp) {
                uint32 currWeek = uint32((time / WEEK) * WEEK);
                uint32 nextWeek = uint32(((time + WEEK) / WEEK) * WEEK);
                uint32 dt = uint32(
                    nextWeek < block.timestamp
                        ? nextWeek - time
                        : block.timestamp - time
                );
                timeWeightedWeeklyGlobalAmbLiquidity_[poolIdx][currWeek] += dt * liquidity;
                time += dt;
            }
        }
        timeWeightedWeeklyGlobalAmbLiquidityLastSet_[poolIdx] = uint32(
            block.timestamp
        );
    }

Suppose curve.ambientSeeds_ is 100. And for simplicity, we suppose that it is not changed in Week-X. Thus, timeWeightedWeeklyGlobalAmbLiquidity_[poolIdx][X] is 100 * WEEK;

And the user shares are calculated in LiquidityMining.accrueAmbientPositionTimeWeightedLiquidity. It calculates timeWeightedWeeklyPositionAmbLiquidity_ https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L235 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L245

    function accrueAmbientPositionTimeWeightedLiquidity(
        address payable owner,
        bytes32 poolIdx
    ) internal {
        bytes32 posKey = encodePosKey(owner, poolIdx);
        uint32 lastAccrued = timeWeightedWeeklyPositionAmbLiquidityLastSet_[
            poolIdx
        ][posKey];
        // Only init time on first call
        if (lastAccrued != 0) {
            AmbientPosition storage pos = lookupPosition(owner, poolIdx);
            uint256 liquidity = pos.seeds_;
            uint32 time = lastAccrued;
            while (time < block.timestamp) {
                uint32 currWeek = uint32((time / WEEK) * WEEK);
                uint32 nextWeek = uint32(((time + WEEK) / WEEK) * WEEK);
                uint32 dt = uint32(
                    nextWeek < block.timestamp
                        ? nextWeek - time
                        : block.timestamp - time
                );
                timeWeightedWeeklyPositionAmbLiquidity_[poolIdx][posKey][
                    currWeek
                ] += dt * liquidity;
                time += dt;
            }
        }
        timeWeightedWeeklyPositionAmbLiquidityLastSet_[poolIdx][
            posKey
        ] = uint32(block.timestamp);
    }

Suppose pos.seeds_ of Alice is 50. And for simplicity, we suppose that it is not changed in Week-X. Thus, timeWeightedWeeklyPositionAmbLiquidity_[poolIdx][posKey][X] is 50 * WEEK;

And we suppose that Bob provides another 50 liquidity. Bob and Alice have the same weight.

LiquidityMining.claimAmbientRewards computes rewardsForWeek. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L277

    function claimAmbientRewards(
        address owner,
        bytes32 poolIdx,
        uint32[] memory weeksToClaim
    ) internal {
        CurveMath.CurveState memory curve = curves_[poolIdx];
        accrueAmbientPositionTimeWeightedLiquidity(payable(owner), poolIdx);
        accrueAmbientGlobalTimeWeightedLiquidity(poolIdx, curve);
        bytes32 posKey = encodePosKey(owner, poolIdx);
        uint256 rewardsToSend;
        for (uint256 i; i < weeksToClaim.length; ++i) {
            uint32 week = weeksToClaim[i];
            require(week + WEEK < block.timestamp, "Week not over yet");
            require(
                !ambLiquidityRewardsClaimed_[poolIdx][posKey][week],
                "Already claimed"
            );
            uint256 overallTimeWeightedLiquidity = timeWeightedWeeklyGlobalAmbLiquidity_[
                    poolIdx
                ][week];
            if (overallTimeWeightedLiquidity > 0) {
                uint256 rewardsForWeek = (timeWeightedWeeklyPositionAmbLiquidity_[
                    poolIdx
                ][posKey][week] * ambRewardPerWeek_[poolIdx][week]) /
                    overallTimeWeightedLiquidity;
                rewardsToSend += rewardsForWeek;
            }
            ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
        }
        if (rewardsToSend > 0) {
            (bool sent, ) = owner.call{value: rewardsToSend}("");
            require(sent, "Sending rewards failed");
        }
    }

rewardsForWeek is (timeWeightedWeeklyPositionAmbLiquidity_[poolIdx][posKey][week] * ambRewardPerWeek_[poolIdx][week]) / overallTimeWeightedLiquidity. Suppose the ambRewardPerWeek_[poolIdx][X] is 2000. Alice can get 1000:

(timeWeightedWeeklyPositionAmbLiquidity_[poolIdx][posKey][week] * ambRewardPerWeek_[poolIdx][week]) / overallTimeWeightedLiquidity
(50 * WEEK * 2000) / (100 * WEEK) = 1000

Because Bob owns the same amount of ambient liquidity. Bob can also get 1000. All the rewards are sent.

However, it is possible that some rewards are not able to be claimed by liquidity providers. If curve.ambientSeeds_ is greater than the sum of all pos.seeds_. Suppose the total ambient liquidity is 200 instead of 100. Alice can only get 500:

(timeWeightedWeeklyPositionAmbLiquidity_[poolIdx][posKey][week] * ambRewardPerWeek_[poolIdx][week]) / overallTimeWeightedLiquidity
(50 * WEEK * 2000) / (200 * WEEK) = 500

It seems to be impossible that the total ambient liquidity is more than the sum of all liquidity provided by users. But there is an example that shows the possibility. TradeMatcher.lockAmbient calls liquidityReceivable to increase curve.ambientSeeds_. Then the total ambient liquidity is more than the sum of all liquidity provided by users. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/TradeMatcher.sol#L79

    function lockAmbient (CurveMath.CurveState memory curve, uint128 liqAdded)
        internal pure returns (int128, int128) {
        (uint128 base, uint128 quote) = liquidityReceivable(curve, liqAdded);
        return signMintFlow(base, quote);        
    }

    function liquidityReceivable (CurveMath.CurveState memory curve, uint128 seeds) 
        internal pure returns (uint128, uint128) {
        (uint128 base, uint128 quote) = liquidityFlows(curve, seeds);
        bumpAmbient(curve, seeds);
        return chargeConservative(base, quote, true);
    }

    function bumpAmbient (CurveMath.CurveState memory curve, uint128 seedDelta)
        private pure {
        bumpAmbient(curve, seedDelta.toInt128Sign());
    }

    function bumpAmbient (CurveMath.CurveState memory curve, int128 seedDelta)
        private pure {
        curve.ambientSeeds_ = curve.ambientSeeds_.addDelta(seedDelta);
    }

In conclusion, if the total ambient liquidity is more than the sum of all liquidity provided by users. Liquidity providers cannot get the full rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

If the goal is to ensure that liquidity providers can get full rewards. I suggest not using curve.ambientSeeds in LiquidityMining.accrueAmbientGlobalTimeWeightedLiquidity since curve.ambientSeeds could be more than the sum of all liquidity provided by users. Maintain another state variable like curve.totalUserSeeds to record the sum of liquidity provided by users and update curve.totalUserSeeds when pos.seeds_ is changed. Then, use curve.totalUserSeeds instead of curve.ambientSeeds in LiquidityMining.accrueAmbientGlobalTimeWeightedLiquidity.

Assessed type

Context

Summary

I list 2 low-critical finding2 and 1 non-critical finding:

• (Low) LiquidityMining.crossTicks should ensure numElementsExit != 0
• (Low) Changing the weekly rewards leads to unfairness.
• (Non) LiquidityMiningPath.userCmd is not compatible with userCmdRouter and userCmdRelayer

(Low) LiquidityMining.crossTicks should ensure numElementsExit != 0

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L30

Impact

LiquidityMining.crossTicks updates tickTracking_[poolIdx][exitTick][numElementsExit - 1].exitTimeStamp. But it should check whether numElementsExit is zero. Or LiquidityMining.crossTicks could revert.

Proof of Concept

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L30

    function crossTicks(
        bytes32 poolIdx,
        int24 exitTick,
        int24 entryTick
    ) internal {
        uint256 numElementsExit = tickTracking_[poolIdx][exitTick].length;
        tickTracking_[poolIdx][exitTick][numElementsExit - 1]
            .exitTimestamp = uint32(block.timestamp);
        StorageLayout.TickTracking memory tickTrackingData = StorageLayout
            .TickTracking(uint32(block.timestamp), 0);
        tickTracking_[poolIdx][entryTick].push(tickTrackingData);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

If numElementsExit is zero. no exitTimeStamp should be updated.

(Low) Changing the weekly rewards leads to unfairness.

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L69 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L78

Impact

Once the weekly rewards are set, the governor cannot change the value of weekly rewards. Because if one user has already claimed the rewards, changing weekly rewards leads to unfairness.

Proof of Concept

The governor can set weekly rewards. And the functions don’t stop them from changing the value of weekly rewards. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L69 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L78

However, if the rewards are changed. It only leads to unfairness. There are two examples.

Suppose Alice and Bob have the same amount of liquidity. The original reward is 1000. Alice claims the reward and gets 500.

1. If the governor updates the reward to 2000. Bob can get 1000, but Alice cannot claim the reward again. Alice only gets 500
2. If the governor updates the reward to 500. Bob can only get 250. Alice gets more reward than she should claim.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in setConcRewards and setAmbRewards to stop changing the rewards.

(Non) LiquidityMiningPath.userCmd is not compatible with userCmdRouter and userCmdRelayer

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L58 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L62

Impact

userCmdRouter and userCmdRelayer in CrocSwapDex.sol are used to call userCmd on behalf of another user. But LiquidityMiningPath.claimConcentratedRewards and LiquidityMiningPath.claimAmbientRewards always use msg.sender.

Proof of Concept

LiquidityMiningPath.claimConcentratedRewards and LiquidityMiningPath.claimAmbientRewards always use msg.sender. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L58 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L62

    function claimConcentratedRewards(bytes32 poolIdx, int24 lowerTick, int24 upperTick, uint32[] memory weeksToClaim)
        public
        payable
    {
        claimConcentratedRewards(payable(msg.sender), poolIdx, lowerTick, upperTick, weeksToClaim);
    }

    function claimAmbientRewards(bytes32 poolIdx, uint32[] memory weeksToClaim) public payable {
        claimAmbientRewards(payable(msg.sender), poolIdx, weeksToClaim);
    }

But userCmdRouter and userCmdRelayer in CrocSwapDex.sol are used to call userCmd on behalf of another user. https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/CrocSwapDex.sol#L136 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/CrocSwapDex.sol#L155

Tools Used

Manual Review

Recommended Mitigation Steps

It seems to be a design choice, but the protocol provides userCmdRouter and userCmdRelayer in CrocSwapDex. It is better to be compatible with these two functions. Or the users may misuse these two functions.