accrued liquidity can be lose and never get recovered for a given tick period.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

accrued liquidity can be lose and never get recovered for a given tick period due to not incrementing tickTrackingIndex , which sets the tick end timestamp to nextweek and will start accruing liquidity for next tick period.

Proof of Concept

LiquidityMining.accrueConcentratedPositionTimeWeightedLiquidity sets tickActiveEnd = nextWeek and not increment tickTrackingIndex which makes current tick period to end by setting tickActiveEnd = nextWeek an d move to next tick period without incrementing tickTrackingIndex , when Tick is no longer active that is tickTracking.exitTimestamp < nextWeek it means tickTracking.exitTimestamp should be strictly less than the nextWeek to set tickActiveEnd = tickTracking.exitTimestamp and than calculate dt based on the tickActiveStart and tickActiveEnd values,

else {
                            // Tick is no longer active
                            if (tickTracking.exitTimestamp < nextWeek) {
                                // Exit was in this week, continue with next tick
                                tickActiveEnd = tickTracking.exitTimestamp;//1697068700
                                tickTrackingIndex++;
                                dt = tickActiveEnd - tickActiveStart;
                            } else {
                                // Exit was in next week, we need to consider the current tick there (i.e. not increase the index)
                                tickActiveEnd = nextWeek;
                            }

now, the issue here is with the if statement if (tickTracking.exitTimestamp < nextWeek) it calculate dt if exitTimestamp strictly lesser than the nextWeek , suppose their is a condition in which user exitTimestamp = nextWeek i.e; user exit concentrated liquidity position with some X poolIdx with the nextWeek timestamp means when the nextWeek about to end, user trigger exit and as soon as exit trigger this if statement will revert and tickTrackingIndex will not get incremented for that user or poolId tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] and therefore the accrued concentrated liquidity might be lose for a given tick period and will never get recovered due to setting tickActiveEnd = nextWeek in else statement, which will declare the tick end and move to the next tick period to accrued liquidity .

Tools Used

manual

Recommended Mitigation Steps

edit the if statement for exitTimestamp, which increment tickTrackingIndex when tickTracking.exitTimestamp == nextWeek,

 - if (tickTracking.exitTimestamp < nextWeek) {
            }
 +  if (tickTracking.exitTimestamp <= nextWeek) { 
            }

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-pre-sort]

141345 marked the issue as remove high or low quality report

[141345]

edge case when exitTimestamp == nextWeek, rewards could be lost

File: canto_ambient/contracts/mixins/LiquidityMining.sol
24:     function crossTicks() internal {

29:         uint256 numElementsExit = tickTracking_[poolIdx][exitTick].length;
30:         tickTracking_[poolIdx][exitTick][numElementsExit - 1]
31:             .exitTimestamp = uint32(block.timestamp);

Due to the low likelihood, high severity might not be approproate

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[OpenCoreCH]

Good point, requires quite a few conditions to actually cause damage (exactly aligned exitTimestamp and even then, the accrual has to happen a few weeks later with additional ticks that were in range since then for it to cause skipping ticks), but will definitely be fixed.

[c4-sponsor]

OpenCoreCH (sponsor) confirmed

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as selected for report

[twicek]

I believe that the original code was correct. I will try to explain why as clearly as possible.

Parameters:

• tickTracking.exitTimestamp = nextWeek
• I will only consider the case tickTracking.enterTimestamp < time for simplicity, therefore tickActiveStart = time.
• In this situation dt = nextWeek - time is the only option since the tick has been exited in the nextWeek.

In the first loop when we arrive to this point:

                            if (tickTracking.exitTimestamp < nextWeek) {
                                // Exit was in this week, continue with next tick
                                tickActiveEnd = tickTracking.exitTimestamp;
                                tickTrackingIndex++;
                                dt = tickActiveEnd - tickActiveStart;
                            } else {
                                // Exit was in next week, we need to consider the current tick there (i.e. not increase the index)
                                tickActiveEnd = nextWeek;
                            }

Here is what happens:

• tickActiveEnd = nextWeek is set.
• tickTrackingIndex is not incremented and later dt is added to time.
• The time weighted position is accrued correctly from time to nextWeek since tickActiveEnd - tickActiveStart = nextWeek - time.

At this point, will not incrementing tickTrackingIndex have a harmful side effect?

Let's see in the second loop, where this time tickTracking.exitTimestamp < nextWeek. Here is what happens:

• tickActiveEnd = tickTracking.exitTimestamp is set.
• tickTrackingIndex is incremented and later dt = tickActiveEnd - tickActiveStart = tickTracking.exitTimestamp - time = 0 is added to time.
• The time weighted position is accrued correctly since tickActiveEnd - tickActiveStart = 0 and no accrual happens.

In the third loop we will use the next tick with the correct last accrual time variable.

Please correct me if necessary, as the explanation in the report is not very clear so I could be wrong. @OpenCoreCH @dmvt

[dmvt]

This issue stands. Arguably it could be considered QA due to likelihood, but given that that sponsor saw enough value to fix it and there is an accounting error under vary narrow circumstances, medium makes sense to me. The only way I would have invalidated this due to another warden's objections is if that warden had proven their point with a test.