code-423n4 / 2023-10-canto-findings

0 stars 1 forks source link

Unfair Ambient Reward Distribution: A Gap in Fair Distribution Due to Mid-Week Changes #284

Open c4-submissions opened 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L65-L72 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L74-L81 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L156-L196 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L256-L289

Vulnerability details

Impact

  1. The system allows for unfair reward distribution based on timing and governance decisions. Specifically, users who claim their rewards early (before governance makes a change to rewards) may receive fewer rewards than those who claim later (after the change). This can lead to significant discrepancies in rewards among users based on the timing of claims, undermining the equitable intent of a rewards system.
  2. After the values for weekly Concentrated and Ambient Rewards are changed, users who claimed before the update will not receive any rewards afterward.

Vulnerability Details

Let's first analyse the current protocol logic

  1. The setConcRewards() and setAmbRewards() functions are used to set the weekly reward rate for the liquidity mining sidecar. Reward rates are set by determining a total amount that will be disbursed per week. Governance can choose how many weeks that the reward rate will be set for.
    function setConcRewards(bytes32 poolIdx, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) public payable {
        // require(msg.sender == governance_, "Only callable by governance");
        require(weekFrom % WEEK == 0 && weekTo % WEEK == 0, "Invalid weeks");
        while (weekFrom <= weekTo) {
            concRewardPerWeek_[poolIdx][weekFrom] = weeklyReward;
            weekFrom += uint32(WEEK);
        }
    }

    function setAmbRewards(bytes32 poolIdx, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) public payable {
        // require(msg.sender == governance_, "Only callable by governance");
        require(weekFrom % WEEK == 0 && weekTo % WEEK == 0, "Invalid weeks");
        while (weekFrom <= weekTo) {
            ambRewardPerWeek_[poolIdx][weekFrom] = weeklyReward;
            weekFrom += uint32(WEEK);
        }
    }
  1. LiquidityMining.sol contains all of the logic for accruing and claiming rewards. This is the most important part of the codebase and should be the main focus for wardens. Via claimAmbientRewards() and claimConcentratedRewards() (functions in LiquidityMiningPath.sol) functions users can claim their accrued rewards.
    function claimConcentratedRewards(
        address payable owner,
        bytes32 poolIdx,
        int24 lowerTick,
        int24 upperTick,
        uint32[] memory weeksToClaim
    ) internal {
        accrueConcentratedPositionTimeWeightedLiquidity(
            owner,
            poolIdx,
            lowerTick,
            upperTick
        );
        CurveMath.CurveState memory curve = curves_[poolIdx];
        // Need to do a global accrual in case the current tick was already in range for a long time without any modifications that triggered an accrual
        accrueConcentratedGlobalTimeWeightedLiquidity(poolIdx, curve);
        bytes32 posKey = encodePosKey(owner, poolIdx, lowerTick, upperTick);
        uint256 rewardsToSend;
        for (uint256 i; i < weeksToClaim.length; ++i) {
            uint32 week = weeksToClaim[i];
            require(week + WEEK < block.timestamp, "Week not over yet");
            require(
                !concLiquidityRewardsClaimed_[poolIdx][posKey][week],
                "Already claimed"
            );
            uint256 overallInRangeLiquidity = timeWeightedWeeklyGlobalConcLiquidity_[poolIdx][week];
            if (overallInRangeLiquidity > 0) {
                uint256 inRangeLiquidityOfPosition;
                for (int24 j = lowerTick + 10; j <= upperTick - 10; ++j) {
                    inRangeLiquidityOfPosition += timeWeightedWeeklyPositionInRangeConcLiquidity_[poolIdx][posKey][week][j];
                }
                // Percentage of this weeks overall in range liquidity that was provided by the user times the overall weekly rewards
                rewardsToSend += inRangeLiquidityOfPosition * concRewardPerWeek_[poolIdx][week] / overallInRangeLiquidity;
            }
            concLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
        }
        if (rewardsToSend > 0) {
            (bool sent, ) = owner.call{value: rewardsToSend}("");
            require(sent, "Sending rewards failed");
        }
    }

    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");
        }
    }
  1. When a user claims their Ambient Rewards for a specific week or set of weeks, they cannot reclaim for the same period due to the ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true; line in claimAmbientRewards(). The same restriction applies when claiming Concentrated Rewards via claimConcentratedRewards().
           // claimAmbientRewards()
            ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
           // claimConcentratedRewards()
            concLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;

In essence, users cannot claim their rewards multiple times within the same week.

Proof of Concept

  1. Consider a week where user A and user B both participate and accrue Ambient rewards for example.
  2. User A decides to claim his Ambient rewards for week 10 to week 15.
  3. Later, governance observes heightened activity from week 10 to week 15 and opts to increase ambRewardPerWeek_ for that weeks to further incentivize participants.
  4. User B claims rewards after this increase.
  5. User A try to get his new rewards but actually can't. (the function revert)

Result: User A receives rewards based on the original rate, while user B gets rewards based on the increased rate. This leads to User B receiving a significantly larger reward for the same set of weeks, even if both users had similar activity.

Tools Used

Recommended Mitigation Steps

  1. Immutable Week Rewards: Once rewards for a week are set, they should be made immutable. This prevents the governance or admin from making changes that can impact users who've already claimed their rewards.

  2. Reward Adjustments for Future Weeks Only: If governance wishes to increase the reward amount due to increased activity, this change should only apply to subsequent weeks. This ensures all users within a particular week receive rewards based on the same rate.

  3. Reclaim Mechanism: Implement a mechanism where, if rewards for a week are adjusted upwards after some users have already claimed, those users can reclaim the difference. However, this adds complexity and could be gas-inefficient.

Assessed type

Other

c4-pre-sort commented 11 months ago

141345 marked the issue as duplicate of #81

c4-judge commented 11 months ago

dmvt changed the severity to QA (Quality Assurance)

c4-judge commented 11 months ago

dmvt marked the issue as grade-a