Canto Liquidity Mining Protocol - GKBG's results

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L256-L290 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L256-L271 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L277-L280 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L74-L79 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L277-L280 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L283 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L269-L271 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L156-L196

Vulnerability details

Impact

In the event in which a LP has been providing liquidity for a week , the AmbRewards and ConcRewards were not set and LP claims his rewards he won't have anything to claim. The impact of this issue is high because it can lead to loss of funds for a LP , however the likelihood is low , hence the overall severity is medium.

Vulnerability Details

First we have the LiquidityMining.claimAmbientRewards function :

 function claimAmbientRewards(address owner,bytes32 poolIdx,uint32[] memory weeksToClaim) internal 

In the beginning of the function the pool is provided , the liquidity for the particular position and the global AmbientGlobal liquidity are accrued for. Iterating through the weeksToClaim array we make sure that the whole week is finished . The rewards are checked if they are not already claimed.

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

Then rewardsToSend are calculated. This calculation happens on lines L277-L280

  uint256 rewardsForWeek = (timeWeightedWeeklyPositionAmbLiquidity_[
                    poolIdx
                ][posKey][week] * ambRewardPerWeek_[poolIdx][week]) /
                    overallTimeWeightedLiquidity;
                rewardsToSend += rewardsForWeek;

The multiplicator in this equation is ambRewardPerWeek_[poolIdx][week]. This value is set by governance in LiquidityMiningPath.sol in the function setAmbRewards() (L74-L79).

   function setAmbRewards(bytes32 poolIdx, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) public payable {
        // require(msg.sender == governance_, "Only callable by governance"); @audit //  commented out code
        require(weekFrom % WEEK == 0 && weekTo % WEEK == 0, "Invalid weeks");
        while (weekFrom <= weekTo) {
            ambRewardPerWeek_[poolIdx][weekFrom] = weeklyReward; //(pool => week => reward)
            weekFrom += uint32(WEEK);
        }
    }

The ambRewardPerWeek_ mapping is of type pool => week => reward. In the event in which rewards are not set yet and a LP has provided liquidity already for at least a week , the ambRewardPerWeek_ are 0. Then the equation on lines L277-L280 of LiquidityMining.claimAmbientRewards() becomes equal to :

 uint256 rewardsForWeek = (timeWeightedWeeklyPositionAmbLiquidity_[ 
                    poolIdx
                ][posKey][week] * 0 ) /
                    overallTimeWeightedLiquidity; 
                rewardsToSend += rewardsForWeek;

rewardsToSend become 0. The issue lies here. On L283 the ambLiquidityRewardsClaimed_ for the particular LP and week is set to true

     }
            ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
        }

After that there is an if statement which checks rewardsToSend to be greather than 0 and only then rewards are sent

     if (rewardsToSend > 0) {
            (bool sent, ) = owner.call{value: rewardsToSend}("");
            require(sent, "Sending rewards failed");
        }
    }
}

In our case the rewardsToSend are 0 and this if won't be entered but the ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true; is set to true. So rewards for that week are "claimed" although AmbRewards were not set. LP looses the chance to earn rewards for this particular week although he has been providing liquidity during that period.

A LP can't attempt to reclaim , first because there were no rewards set and the check on L269-L271 will fail because rewards(although non-existent) are already claimed for that week

require(
                !ambLiquidityRewardsClaimed_[poolIdx][posKey][week],
                "Already claimed"
            );

The function LiquidityMining.claimConcentratedRewards is using the same logic with the difference that the liquidity is concentrated and provided in a range of 21 ticks. The concLiquidityRewardsClaimed_[poolIdx][posKey][week] = true; is set to true before the if (rewardsToSend > 0) and the same scenario can happen here. If concRewardPerWeek_ are not set and are equal to 0 then rewardsToSend are 0 and the if is not entered , but that week is already claimed.

  function claimConcentratedRewards(
        address payable owner,
        bytes32 poolIdx,
        int24 lowerTick,
        int24 upperTick,
        uint32[] memory weeksToClaim
    ) internal {
      
      
      (........)


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

My recommendation is the same as for LiquidityMining.claimAmbientRewards.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Move the update of the ambLiquidityRewardsClaimed_[poolIdx][posKey][week] mapping to be inside the if (rewardsToSend > 0) so the function will look like this

- ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;

if (rewardsToSend > 0) {
+ ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
(bool sent, ) = owner.call{value: rewardsToSend}("");
require(sent, "Sending rewards failed");
        }

2. Move the update of the LiquidityMining.claimConcentratedRewards() mapping to be inside the if (rewardsToSend > 0) so the function will look like this

// 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) {
+concLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
(bool sent, ) = owner.call{value: rewardsToSend}("");
require(sent, "Sending rewards failed");
        }
    }

Assessed type

Timing