Canto Liquidity Mining Protocol - adriro's results

Lines of code

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

Vulnerability details

Summary

The implementation of accrueConcentratedPositionTimeWeightedLiquidity() incurs in complex and unbounded computations that could lead to significant gast costs and a potential denial of service.

Impact

The liquidity mining program in the Ambient DEX will track each concentrated liquidity position using the accrueConcentratedPositionTimeWeightedLiquidity():

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

069:     function accrueConcentratedPositionTimeWeightedLiquidity(
070:         address payable owner,
071:         bytes32 poolIdx,
072:         int24 lowerTick,
073:         int24 upperTick
074:     ) internal {
075:         RangePosition72 storage pos = lookupPosition(
076:             owner,
077:             poolIdx,
078:             lowerTick,
079:             upperTick
080:         );
081:         bytes32 posKey = encodePosKey(owner, poolIdx, lowerTick, upperTick);
082:         uint32 lastAccrued = timeWeightedWeeklyPositionConcLiquidityLastSet_[
083:             poolIdx
084:         ][posKey];
085:         // Only set time on first call
086:         if (lastAccrued != 0) {
087:             uint256 liquidity = pos.liquidity_;
088:             for (int24 i = lowerTick + 10; i <= upperTick - 10; ++i) {
089:                 uint32 tickTrackingIndex = tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i];
090:                 uint32 origIndex = tickTrackingIndex;
091:                 uint32 numTickTracking = uint32(tickTracking_[poolIdx][i].length);
092:                 uint32 time = lastAccrued;
093:                 // Loop through all in-range time spans for the tick or up to the current time (if it is still in range)
094:                 while (time < block.timestamp && tickTrackingIndex < numTickTracking) {
095:                     TickTracking memory tickTracking = tickTracking_[poolIdx][i][tickTrackingIndex];
096:                     uint32 currWeek = uint32((time / WEEK) * WEEK);
097:                     uint32 nextWeek = uint32(((time + WEEK) / WEEK) * WEEK);
098:                     uint32 dt = uint32(
099:                         nextWeek < block.timestamp
100:                             ? nextWeek - time
101:                             : block.timestamp - time
102:                     );
103:                     uint32 tickActiveStart; // Timestamp to use for the liquidity addition
104:                     uint32 tickActiveEnd;
105:                     if (tickTracking.enterTimestamp < nextWeek) {
106:                         // Tick was active before next week, need to add the liquidity
107:                         if (tickTracking.enterTimestamp < time) {
108:                             // Tick was already active when last claim happened, only accrue from last claim timestamp
109:                             tickActiveStart = time;
110:                         } else {
111:                             // Tick has become active this week
112:                             tickActiveStart = tickTracking.enterTimestamp;
113:                         }
114:                         if (tickTracking.exitTimestamp == 0) {
115:                             // Tick still active, do not increase index because we need to continue from here
116:                             tickActiveEnd = uint32(nextWeek < block.timestamp ? nextWeek : block.timestamp);
117:                         } else {
118:                             // Tick is no longer active
119:                             if (tickTracking.exitTimestamp < nextWeek) {
120:                                 // Exit was in this week, continue with next tick
121:                                 tickActiveEnd = tickTracking.exitTimestamp;
122:                                 tickTrackingIndex++;
123:                                 dt = tickActiveEnd - tickActiveStart;
124:                             } else {
125:                                 // Exit was in next week, we need to consider the current tick there (i.e. not increase the index)
126:                                 tickActiveEnd = nextWeek;
127:                             }
128:                         }
129:                         timeWeightedWeeklyPositionInRangeConcLiquidity_[poolIdx][posKey][currWeek][i] +=
130:                             (tickActiveEnd - tickActiveStart) * liquidity;
131:                     }
132:                     time += dt;
133:                 }
134:                 if (tickTrackingIndex != origIndex) {
135:                     tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = tickTrackingIndex;
136:                 }
137:             }
138:         } else {
139:             for (int24 i = lowerTick + 10; i <= upperTick - 10; ++i) {
140:                 uint32 numTickTracking = uint32(tickTracking_[poolIdx][i].length);
141:                 if (numTickTracking > 0) {
142:                     if (tickTracking_[poolIdx][i][numTickTracking - 1].exitTimestamp == 0) {
143:                         // Tick currently active
144:                         tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = numTickTracking - 1;
145:                     } else {
146:                         tickTrackingIndexAccruedUpTo_[poolIdx][posKey][i] = numTickTracking;
147:                     }
148:                 }
149:             }
150:         }
151:         timeWeightedWeeklyPositionConcLiquidityLastSet_[poolIdx][
152:             posKey
153:         ] = uint32(block.timestamp);
154:     }

The algorithm is quite dense and difficult to read at first, but it can be simplified (at least for the current context) as:

• For each tick between lowTick + 10 and upperTick - 10:
  • For each segment of time between last accrued and now:
    • Add liquidity to the current week proportional to the span of time determined by the segment

Here we refer to segment of time as any period that is bounded between the start of the week, the end of the week, the tick becoming active or the tick becoming inactive. For example, a segment could be defined by the start of week (in a tick that became active before the start of the week) and the exit timestamp of the tick (because the tick exited before the end of the week).

Note, then, that the implemented logic executes two nested loops:

1. The outer loop ranges for each tick in the position (minus 20, technically, to account for the +-10 padding). A position that provides liquidity in a wide range will need to iterate each value of a potential big list.
2. The inner loop goes through segmentations of week periods and/or tick history. For an active pool, with lots of tick crossing, this can have a devastating effect in size of the tick history. Imagine a very active pool being heavily arbitraged, that jumps between close (but different) ticks. Each jump will create a new history entry in the corresponding key of the tickTracking_ mapping. Every entry in this list will need to be iterated and accounted for any potential liquidity (these jumps between near ticks will likely be included in the same position).

A position that has a wide tick range, or belongs to a pool that is subject to lots of trading activity and a long list of tick tracking history, or simply isn't updated frequently and accumulates pending changes (remember the accrual happens when the position is minted, burned or the user claims the rewards) may result in significant computational requirements, leading to unbounded gas costs and a potential denial of service. The owner of the position may not be able to exit it or claim the associated rewards, leading to loss of funds.

Recommendation

It is difficult to provide a recommendation with the given architecture. Clearly, the data structures chosen to implement the solution require an algorithm that goes through ticks and segmentations of time.

A potential solution, that could require substantial design changes, would be to mimic the traditional staking rewards algorithm used in different protocols. The following article introduces the algorithm for Uniswap V3 (same style as concentrated liquidity in Ambient). An implementation is provided in the following repository.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L26

Vulnerability details

Summary

Rewards are set up using protocol commands, but it's entrypoint is not payable.

Impact

Rewards can be set up by protocol authorities using the functions setConcRewards() and setAmbRewards() present in the LiquidityMiningPath contracts. These two are part of the "command" pattern used in the protocol: the main entrypoint receives commands which are then routed to the proper place using codes.

The protocol command flow starts at the CrocSwapDex contract:

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/CrocSwapDex.sol#L103-L106

103:     function protocolCmd (uint16 callpath, bytes calldata cmd, bool sudo)
104:         protocolOnly(sudo) public payable override {
105:         callProtocolCmd(callpath, cmd);
106:     }

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/ProxyCaller.sol#L22-L28

22:     function callProtocolCmd (uint16 proxyIdx, bytes calldata input) internal
23:         returns (bytes memory) {
24:         assertProxy(proxyIdx);
25:         (bool success, bytes memory output) = proxyPaths_[proxyIdx].delegatecall(
26:             abi.encodeWithSignature("protocolCmd(bytes)", input));
27:         return verifyCallResult(success, output);
28:     }

The protocolCmd() function checks the call is properly authorized and calls callProtocolCmd(), which then executes a delegatecall to the corresponding implementation, in this case the LiquidityMiningPath contract:

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L26-L37

26:     function protocolCmd(bytes calldata cmd) public virtual {
27:         (uint8 code, bytes32 poolHash, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) =
28:             abi.decode(cmd, (uint8, bytes32, uint32, uint32, uint64));
29: 
30:         if (code == ProtocolCmd.SET_CONC_REWARDS_CODE) {
31:             setConcRewards(poolHash, weekFrom, weekTo, weeklyReward);
32:         } else if (code == ProtocolCmd.SET_AMB_REWARDS_CODE) {
33:             setAmbRewards(poolHash, weekFrom, weekTo, weeklyReward);
34:         } else {
35:             revert("Invalid protocol command");
36:         }
37:     }

While CrocSwapDex::protocolCmd() is payable, its counterpart in LiquidityMiningPath is not. This means that rewards cannot be transferred while setting them up, rejecting any command that has positive callvalue.

Recommendation

Make LiquidityMiningPath::protocolCmd() payable.

-   function protocolCmd(bytes calldata cmd) public virtual {
+   function protocolCmd(bytes calldata cmd) public virtual payable {
        (uint8 code, bytes32 poolHash, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) =
            abi.decode(cmd, (uint8, bytes32, uint32, uint32, uint64));

        if (code == ProtocolCmd.SET_CONC_REWARDS_CODE) {
            setConcRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else if (code == ProtocolCmd.SET_AMB_REWARDS_CODE) {
            setAmbRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else {
            revert("Invalid protocol command");
        }
    }

Assessed type

Payable

Report

Summary

Low Issues

Total of 5 issues:

Non Critical Issues

Total of 1 issues:

Low Issues

<a name="L-1"></a>[L-1] claimConcentratedRewards() and claimAmbientRewards() should be internal function

• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L54
• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L61

Both of these functions present in the LiquidityMiningPath contract are intended to be called via user commands whose entrypoint is the userCmd() function.

Consider changing the visibility of claimConcentratedRewards() and claimAmbientRewards() to internal or private.

<a name="L-2"></a>[L-2] Governance check is commented out

• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L66
• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L75

Both setConcRewards() and setAmbRewards() have a privilege check that is commented out and currently ignored:

// require(msg.sender == governance_, "Only callable by governance");

These functions are called as protocol commands, which undergo a privilege check in the entrypoint (see https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/CrocSwapDex.sol#L104), hence the low severity of this issue.

However, it is not clear if there's a missing additional check to ensure these are called by the governance. Consider either removing the lines or uncommenting the check.

<a name="L-3"></a>[L-3] Rewards can be unintentionally overridden

• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L65
• https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L74

Lidiquity mining rewards are set up using setConcRewards() or setAmbRewards(). In both cases, the rewards are overridden instead of accumulated. Taking setConcRewards() as the example:

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L65-L72

65:     function setConcRewards(bytes32 poolIdx, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) public payable {
66:         // require(msg.sender == governance_, "Only callable by governance");
67:         require(weekFrom % WEEK == 0 && weekTo % WEEK == 0, "Invalid weeks");
68:         while (weekFrom <= weekTo) {
69:             concRewardPerWeek_[poolIdx][weekFrom] = weeklyReward;
70:             weekFrom += uint32(WEEK);
71:         }
72:     }

Line 69 assigns the new value weeklyReward to the storage mapping. If rewards were already set for this pool, i.e. concRewardPerWeek_[poolIdx][weekFrom] != 0, then the new assignment will override the existing value.

<a name="L-4"></a>[L-4] Unused rewards cannot be claimed back

Although highly unlikely, there is no provided mechanism to claim back unused rewards assigned to periods of time of inactivity in the pool.

<a name="L-5"></a>[L-5] Missing week validation while claiming rewards

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

In claimConcentratedRewards() and claimAmbientRewards(), the implementation takes an array of the weeks which are expected to be claimed. This is user input, and lacks any validation over the provided argument values.

Using claimConcentratedRewards() as the example, we can see each element in weeksToClaim is not checked to be an actual week, i.e. that week % WEEK == 0.

174:         for (uint256 i; i < weeksToClaim.length; ++i) {
175:             uint32 week = weeksToClaim[i];
176:             require(week + WEEK < block.timestamp, "Week not over yet");
177:             require(
178:                 !concLiquidityRewardsClaimed_[poolIdx][posKey][week],
179:                 "Already claimed"
180:             );
181:             uint256 overallInRangeLiquidity = timeWeightedWeeklyGlobalConcLiquidity_[poolIdx][week];

Consider adding an explicit check to ensure the elements in weeksToClaim are valid weeks.

    uint32 week = weeksToClaim[i];
    require(week + WEEK < block.timestamp, "Week not over yet");
+   require(week % WEEK == 0, "Invalid week");

Non Critical Issues

<a name="NC-1"></a>[NC-1] Use Solidity time units

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

Instead of defining spans of time manually using seconds, consider using Solidity built-in time units.