`ClaimConcentratedRewards` and `claimAmbientRewards` don't update liquidity, enabling double rewards claims. Update liquidity after claims.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L75-L80 https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L87 https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L234-L235

Vulnerability details

Impact

The claimConcentratedRewards and claimAmbientRewards functions do not update the liquidity amount after withdrawing rewards. This could allow a user to withdraw rewards multiple times for the same liquidity.

Proof of Concept

The liquidity amount is not updated after claiming rewards in the claimConcentratedRewards and claimAmbientRewards functions.

Specifically, the liquidity amount that should be updated is:

• For concentrated rewards: pos.liquidity_ in the RangePosition72 struct
• For ambient rewards: pos.seeds_ in the AmbientPosition struct

These liquidity amounts are read from the respective position structs: Line75-80, Line80, Line234-Line235

        RangePosition72 storage pos = lookupPosition(
            owner,
            poolIdx,
            lowerTick,
            upperTick
        );
//...
uint256 liquidity = pos.liquidity_;

AmbientPosition storage pos = lookupPosition(owner, poolIdx);  
//...
uint256 liquidity = pos.seeds_;

But they are never updated after transferring rewards.

Here is how claimConcentratedRewards

1. It first calls accrueConcentratedPositionTimeWeightedLiquidity to calculate the time-weighted liquidity the user provided in the position's tick range for each week. This updates the timeWeightedWeeklyPositionInRangeConcLiquidity mapping.

2. It also calls accrueConcentratedGlobalTimeWeightedLiquidity to calculate the total time-weighted liquidity provided globally in the pool's current tick range for each week. This updates timeWeightedWeeklyGlobalConcLiquidity.

3. It reads the user's position liquidity amount from pos.liquidity_: Line75-Line80, Line87

        RangePosition72 storage pos = lookupPosition(
            owner,
            poolIdx,
            lowerTick,
            upperTick
        );
...
uint256 liquidity = pos.liquidity_; 

4. For each week to claim, it calculates the rewards as:

   (User's time-weighted liquidity for week / Total time-weighted liquidity for week) * Total concentrated rewards for week

5. It sends the accumulated rewards to the user.

The issue is that pos.liquidity_ is never updated after withdrawing the rewards!

This means the user can repeatedly call claimConcentratedRewards for the same weeks and will continue getting rewards based on their original, unchanged liquidity amount.

claimAmbientRewards has the same flaw - it reads the pos.seeds_ liquidity amount but does not update it after sending rewards.

Here is some example showing how a user could exploit the lack of liquidity update in claimConcentratedRewards to double claim rewards:

// User has a range position with 100 ETH liquidity 
RangePosition72 pos = lookupPosition(user, poolId, tickLower, tickUpper);
pos.liquidity_ = 100 ether;

// User provided 50% of pool's liquidity in week 1  
timeWeightedWeeklyPositionInRangeConcLiquidity[week1][posKey][tick] = 50 ether
timeWeightedWeeklyGlobalConcLiquidity[week1] = 100 ether 

// Total rewards in week 1 is 10 ETH
concRewardPerWeek[week1] = 10 ether

// User claims week 1 rewards 
claimConcentratedRewards(user, poolId, tickLower, tickUpper, [week1])

// User gets 50% of 10 ETH = 5 ETH rewards 

// pos.liquidity_ is STILL 100 ETH!

// User claims week 1 again
claimConcentratedRewards(user, poolId, tickLower, tickUpper, [week1])  

// User gets 50% of 10 ETH = 5 ETH rewards again!

// For a total of 10 ETH instead of the 5 ETH they should get

This works because the initial pos.liquidity_ of 100 ETH is not updated after the first claim.

The same exploit would work for claimAmbientRewards by leaving pos.seeds_ unchanged after claims.

Adding the missing liquidity update would fix it:

// Claim concentrated rewards

...

pos.liquidity_ -= rewardsToSend

// Claim ambient rewards

...

pos.seeds_ -= rewardsToSend 

Tools Used

Vs

Recommended Mitigation Steps

The liquidity amount should be updated after each reward claim.

So the fix would be to add a line updating the liquidity amount after sending the rewards in both functions:

// claimConcentratedRewards

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

// Update liquidity 
pos.liquidity_ -= rewardsToSend;  </

// claimAmbientRewards

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

// Update liquidity
pos.seeds_ -= rewardsToSend;  </

This ensures the liquidity amount stays in sync after withdrawing rewards. Let me know if this helps identify the specific issue!

Assessed type

Governance

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient quality