c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L126

Vulnerability details

Impact

In the function accrueConcentratedPositionTimeWeightedLiquidity, inside the while block, dt is initialised as:

    uint32 dt = uint32(
       nextWeek < block.timestamp
       ? nextWeek - time
       : block.timestamp - time
   );

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L98-L102

If tickTracking.exitTimestamp != 0 then the following else block is executed on line 117:

else {
   // Tick is no longer active
   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;
    }
 }

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

dt is now updated to tickActiveEnd - tickActiveStart; when tickTracking.exitTimestamp < nextWeek as seen in the if block of the outer else block above.

But, when tickTracking.exitTimestamp > nextWeek, in the inner else block, the value of dt is not updated:

 else {
     // Exit was in next week, we need to consider the current tick there (i.e. not increase the index)
     tickActiveEnd = nextWeek;
     //@audit - No update on dt value
    }

Inside this else block as well, dt must equal tickActiveEnd - tickActiveStart;, where tickActiveEnd = nextWeek;. Without this update if tickTracking.exitTimestamp > nextWeek, then dt = nextWeek - time or dt = block.timestamp - time as it was declared initially. For example, let's say that it was the former, that is, dt = nextWeek - time (according to the initial declaration). Assume that tickActiveStart = tickTracking.enterTimestamp (this is possible when tickTracking.enterTimestamp > time). Had the else block above (check @audit tag), updated dt, then dt = tickActiveEnd - tickActiveStart; => dt = nextWeek - tickTracking.enterTimestamp

So, on comparing again, initially -> dt = nextWeek - time and had there been an update on dt at the audit tag, dt would now be -> dt = nextWeek - tickTracking.enterTimestamp. Note that here, tickTracking.enterTimestamp > time (check the above para). So, nextWeek - tickTracking.enterTimestamp < nextWeek - time. That means dt would be a smaller value had it been equal to nextWeek - tickTracking.enterTimestamp. But, since its not updated, dt equals nextWeek - time, which is a bigger value.

dt is used to increase the value of time (used as an iterator in while loop). Since dt is a bigger value than required, the number of iterations of the while loop would be less than what it should be. This means that fewer iterations would be used to update timeWeightedWeeklyPositionInRangeConcLiquidity_ on line 129

timeWeightedWeeklyPositionInRangeConcLiquidity_[poolIdx][posKey][currWeek][i] +=
                            (tickActiveEnd - tickActiveStart) * liquidity;

timeWeightedWeeklyPositionInRangeConcLiquidity_ is used to calculate the rewardsToSend value in claimConcentratedRewards function. If timeWeightedWeeklyPositionInRangeConcLiquidity_ is incorrect, then the rewards to be sent to the user will be calculated incorrectly.

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L181-L188

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; //@audit - rewards to send will be less

Also, due to fewer number of iterations, tickTrackingIndexAccruedUpTo_ might not be updated correctly.

if (tickTrackingIndex != origIndex) {
  tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = tickTrackingIndex;
}

Proof of Concept

    /// @notice Accrues the in-range time-weighted concentrated liquidity for a position by going over the tick entry / exit history
    /// @dev Needs to be called whenever a position is modified
    function accrueConcentratedPositionTimeWeightedLiquidity(
        address payable owner,
        bytes32 poolIdx,
        int24 lowerTick,
        int24 upperTick
    ) internal {
        RangePosition72 storage pos = lookupPosition(
            owner,
            poolIdx,
            lowerTick,
            upperTick
        );
        bytes32 posKey = encodePosKey(owner, poolIdx, lowerTick, upperTick);
        uint32 lastAccrued = timeWeightedWeeklyPositionConcLiquidityLastSet_[
            poolIdx
        ][posKey];
        // Only set time on first call
        if (lastAccrued != 0) {
            uint256 liquidity = pos.liquidity_;
            for (int24 i = lowerTick + 10; i <= upperTick - 10; ++i) {
                uint32 tickTrackingIndex = tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i];
                uint32 origIndex = tickTrackingIndex;
                uint32 numTickTracking = uint32(tickTracking_[poolIdx][i].length);
                uint32 time = lastAccrued;
                // Loop through all in-range time spans for the tick or up to the current time (if it is still in range)
                while (time < block.timestamp && tickTrackingIndex < numTickTracking) {
                    TickTracking memory tickTracking = tickTracking_[poolIdx][i][tickTrackingIndex];
                    uint32 currWeek = uint32((time / WEEK) * WEEK);
                    uint32 nextWeek = uint32(((time + WEEK) / WEEK) * WEEK);
                    uint32 dt = uint32(
                        nextWeek < block.timestamp
                            ? nextWeek - time
                            : block.timestamp - time
                    );
                    uint32 tickActiveStart; // Timestamp to use for the liquidity addition
                    uint32 tickActiveEnd;
                    if (tickTracking.enterTimestamp < nextWeek) {
                        // Tick was active before next week, need to add the liquidity
                        if (tickTracking.enterTimestamp < time) {
                            // Tick was already active when last claim happened, only accrue from last claim timestamp
                            tickActiveStart = time;
                        } else {
                            // Tick has become active this week
                            tickActiveStart = tickTracking.enterTimestamp;
                        }
                        if (tickTracking.exitTimestamp == 0) {
                            // Tick still active, do not increase index because we need to continue from here
                            tickActiveEnd = uint32(nextWeek < block.timestamp ? nextWeek : block.timestamp);
                        } else {
                            // Tick is no longer active
                            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;
                            }
                        }
                        timeWeightedWeeklyPositionInRangeConcLiquidity_[poolIdx][posKey][currWeek][i] +=
                            (tickActiveEnd - tickActiveStart) * liquidity;
                    }
                    time += dt;
                }
                if (tickTrackingIndex != origIndex) {
                    tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = tickTrackingIndex;
                }
            }
        } else {
            for (int24 i = lowerTick + 10; i <= upperTick - 10; ++i) {
                uint32 numTickTracking = uint32(tickTracking_[poolIdx][i].length);
                if (numTickTracking > 0) {
                    if (tickTracking_[poolIdx][i][numTickTracking - 1].exitTimestamp == 0) {
                        // Tick currently active
                        tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = numTickTracking - 1;
                    } else {
                        tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = numTickTracking;
                    }
                }
            }
        }
        timeWeightedWeeklyPositionConcLiquidityLastSet_[poolIdx][
            posKey
        ] = uint32(block.timestamp);
    }

Tools Used

Manual review

Recommended Mitigation Steps

add the line - dt = tickActiveEnd - tickActiveStart in the place of the audit tag as shown above.

Assessed type

Other

c4-pre-sort commented 1 year ago

141345 marked the issue as sufficient quality report

OpenCoreCH commented 1 year ago

Note that if the mentioned else branch is reached, dt is necessarily set to nextWeek - time, it cannot be block.timestamp - time (if the tick was exited in the next week, the next week cannot be in the future and greater than the block timestamp). So the only possible difference is regarding tickActiveStart, namely when tickActiveStart = tickTracking.enterTimestamp (in the other case, when tickActiveStart = time, we have dt = nextWeek - time = tickActiveEnd - tickActiveStart).

I agree with the warden that in this particular case, the logic for updating dt is different in the if and else branch, which should not be the case. However, I think the logic in the if branch is wrong, not in the else: We want to set dt such that the next time value is equal to tickActiveEnd (because we accrued up to tickActiveEnd). To achieve this in all cases, we need to set dt = tickActiveEnd - time in the if block. Otherwise, when tickTracking.enterTimestamp > time we only add the time where the tick was in range to time (but not the time before that were the tick was out of range). Because we are also increasing the tick tracking index, this should not cause any problems in practice (the next enter timestamp will be greater than time by definition and we will only start accruing from there on again), but I think it should still be changed.

c4-sponsor commented 1 year ago

OpenCoreCH (sponsor) confirmed

c4-judge commented 1 year ago

dmvt marked the issue as selected for report

twicek commented 1 year ago

These two sentences are contradictory in the sponsor's comment:

I agree with the warden that in this particular case, the logic for updating dt is different in the if and else branch, which should not be the case.

and

However, I think the logic in the if branch is wrong, not in the else: We want to set dt such that the next time value is equal to tickActiveEnd (because we accrued up to tickActiveEnd).

dt should indeed not be updated in the else statement.

If the tick has been exited after nextWeek here are all the possibilities that I can see:

Parameters:

time is always the last accrual time.
tickActiveStart can be time or tickTracking.enterTimestamp.
tickActiveEnd can only be nextWeek.
dt can only be nextWeek - time because block.timestamp >= tickTracking.exitTimestamp > nextWeek.

tickActiveStart == time and dt = nextWeek - time: The enter timestamp was before the last accrual and the exit timestamp is after nextWeek. Therefore in this case dt = tickActiveEnd - tickActiveStart. We accrue from the last accrual time to nextWeek and we advance to the next week as intended (time += dt).
tickActiveStart == tickTracking.enterTimestamp and dt = nextWeek - time: The enter timestamp was after the last accrual and the exit timestamp is after nextWeek. Therefore in this case dt != tickActiveEnd - tickActiveStart, which is fine since accrual to the time weighted position should only be done from when the tick was entered but the accrual time should be set to nextWeek since the tick has not yet been exited in the current week.

At no point there is the need to update dt to tickActiveEnd - tickActiveStart in the else statement. If my reasoning is correct, this report is invalid.

Maybe we could get additional input from the sponsor? @OpenCoreCH

dmvt commented 1 year ago

Differences in accounting may not look like a problem now, but may lead to a big problem in the future. This is a valid medium. Ruling stands.

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

If `dt` is not updated accurately then `timeWeightedWeeklyPositionInRangeConcLiquidity_` might be updated incorrectly. #290