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

0 stars 1 forks source link

Front-Running Vulnerability: Exploiting Reward Updates for Maximized Payouts #292

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L65-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

Malicious users claim rewards at a higher rate than what was intended by front-running governance actions meant to reduce rewards. This allows them to claim rewards at a higher rate than what was intended, undermining the protocol's intended economic incentives and potentially causing undue inflation.

Proof of Concept

Attack Scenario:

  1. Monitoring for Reward Update Transactions: An malicious user sets up a bot or script to monitor pending transactions sent from the governance address or any transaction calling setConcRewards or setAmbRewards. This can be done using tools like Etherscan's API or Web3 libraries to listen to mempool transactions.

  2. Identification of the Reduction: Once the user's script identifies a transaction intending to reduce the rewards, it triggers the next steps automatically.

  3. Swift Claiming: The malicious user immediately sends a transaction calling claimAmbientRewards or claimConcentratedRewards. To ensure their transaction gets processed first, they set a gas price significantly higher than the current average or the gas price of the governance's transaction.

  4. Transaction Confirmation: Given the higher gas price, miners prioritize the malicious user transaction over the governance's reward adjustment transaction. As a result, the malicious user claim transaction gets confirmed first, allowing them to receive rewards at the old, higher rate.

  5. Result: By the time the governance transaction gets confirmed and the reward rate is reduced, the malicious user has already claimed a significant portion of the rewards at the higher rate. Over time and repeated instances, this can lead to substantial losses for the protocol.

Evidence:

Code Snippet

  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);
        }
    }

    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");
        }
    }

Tools Used

Manual Inspection

Recommended Mitigation Steps

Time-Locked Updates: Introduce a delay on governance actions that adjust rewards, providing users with a clear window of upcoming changes.

Assessed type

Other

141345 commented 1 year ago

seems invalid

past rewards are not expected to chagne, front run can not profit

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-pre-sort commented 1 year ago

141345 marked the issue as low quality report

c4-sponsor commented 1 year ago

OpenCoreCH (sponsor) disputed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

radeveth commented 1 year ago

I strongly believe that this should be considered a valid low-severity issue because the root cause of this problem is the same as issue #284 (changing rewards ratios before the week is over and altering past rewards). Additionally, I have provided a detailed attack scenario.


@dmvt @141345

dmvt commented 1 year ago

@radeveth changing it from invalid medium to valid low risk won't change anything about the scoring or cause it to be included in the report.

I don't think this is valid the way described, but for future contests, a functioning test may have been able to convince me otherwise. Your fix is also one of our classic QA suggestions so overinflated severity could also be applied here. Finally, the fix doesn't change anything as users are already aware of governance actions via voting. A delay would not provide for a higher level of warning than simply following the results of the vote. Once again though, changing this to a grade-a QA report wouldn't change your score, payout, or cause it to be included in the report.