Canto Liquidity Mining Protocol - cookedcookee's results

[L-01] Claimable week elements are not forced to conform to the week standard used throughout

The uint32[] memory weeksToClaim input variable in both the claimConcentratedRewards() and claimAmbientRewards() functions do not enforce the week normalisation standard used throughout the contract. This exacerbates the unbounded input array issue highlighted already.

Github link

Recommended Mitigation

Consider requiring that every element in uint32[] memory weeksToClaim satisfies an additional require check in the respective for-loops as follows.

File: contracts/mixins/LiquidityMining.sol
175:             uint32 week = weeksToClaim[i];
176:             require(week % WEEK == 0, "Invalid week");

268:             uint32 week = weeksToClaim[i];
269:             require(week % WEEK == 0, "Invalid week");

[L-02] Governance can reset or remove liquidity rewards after or during the reward period

It is possible for governance to reset or remove the liquidity rewards for any given pool after, or during, the liquidity mining period. This could be used to exploit liquidity providers.

Github link

To see this, run the following patch, which demonstrates how governance can reset the liqudity rewards after the liquidity period has expired, but before users have claimed.

diff --git a/test_canto/TestLiquidityMiningOrig.js b/test_canto/TestLiquidityMining.js
index bd21a32..9656cfa 100644
--- a/test_canto/TestLiquidityMiningOrig.js
+++ b/test_canto/TestLiquidityMining.js
@@ -272,6 +272,24 @@ describe("Liquidity Mining Tests", function () {
 
                await time.increase(604800 * 5); // fast forward 1000 seconds so that rewards accrue
 
+               //////////////////////////////////////////////////
+               // OWNER RESETS LIQUIDITY REWARDS
+               //////////////////////////////////////////////////
+               
+               let setRewards0 = abi.encode(
+                       // [code, poolHash, startWeek, endWeek, rewardPerWeek]
+                       ["uint8", "bytes32", "uint32", "uint32", "uint64"],
+                       [
+                               117,
+                               ethers.utils.keccak256(poolHash),
+                               Math.floor(timestampBefore / 604800) * 604800,
+                               Math.floor(timestampBefore / 604800) * 604800 + 604800 * 2,
+                               BigNumber.from("0"), // YOU GET NOTHING! ZERO!
+                       ]
+               );
+               tx = await dex.protocolCmd(8, setRewards0, true);
+               await tx.wait();
+
                //////////////////////////////////////////////////
                // CLAIM REWARDS ACCRUED FROM CONCENTRATED REWARDS
                //////////////////////////////////////////////////
@@ -302,7 +320,7 @@ describe("Liquidity Mining Tests", function () {
 
                // expect dex to have 2 less CANTO since we claimed for 2 weeks worth of rewards
                expect(dexBalBefore.sub(dexBalAfter)).to.equal(
-                       BigNumber.from("2000000000000000000")
+                       BigNumber.from("0")
                );
        });
 });

Recommended Mitigation

Consider dis-allowing the rewards for a pool/week combination to be changed after they have been set once.

[N-01] Add clarifying comment to storage variable

The tickTrackingIndexAccruedUpTo_ storage variable lacks a clarifying comment, such as those found with all other equally complex storage variables.

Github link

Recommended Mitigation

Consider adding a clarifying comment to the tickTrackingIndexAccruedUpTo_ storage variable.

File: contracts/mixins/StorageLayout.sol
191:     // Pool -> Position -> Tick -> Index
192:     mapping(bytes32 => mapping(bytes32 => mapping(int32 => uint32))) tickTrackingIndexAccruedUpTo_;

[N-02] Remove commented out logic

No longer used logic remains as commented out code.

File: contracts/callpaths/LiquidityMiningPath.sol
66:         // require(msg.sender == governance_, "Only callable by governance");

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

Github link

Recommended Mitigation

Remove lines 66 and 75 from LiquidityMiningPath.sol.

Analysis

Overall both LiquidityMiningPath.sol and LiquidityMining.sol are well written, efficient, and logical contracts. The logic employed in the accrueConcentratedPositionTimeWeightedLiquidity() function, in particular, is clean and very thoroughly considered given the complexity involved. The way in which the novel contracts integrate into the pre-existing architecture was impressive and very well done also. This speaks to both the quality of the novel contracts and the pre-existing contracts.

With all of that being said, one dimension that the current design fails to properly consider is how to handle unclaimed and unclaimable liquidity mining rewards. Unclaimed liquidity mining rewards are locked in the DEX contract, and there are conditions under which some percentage of the liquidity mining rewards are unclaimable by anyone. More details regarding these issues can be found in the accompanying submitted reports. It is recognised that novel sidecar contracts and/or upgrades can be built to allow governance actors to retrieve the unclaimed and unclaimable funds, however, a simpler and more thorough approach is to build this functionality into the liquidity mining contracts initially and directly.

Additionally, many variables of the current design are hard-coded into the contracts. Examples include the WEEK constant, and the tick range of plus or minus 10 ticks in LiquidityMining,sol. While taking this approach has the clear benefit of saving gas and reducing code complexity, it also limits the utility of these contracts to be used in other circumstances. For instance, if a wider minimum range of in range liquidity is desired a fork changing the relevant values will need to be created, deployed, and integrated into the DEX. Depending on the intended scope and utility of these contracts, it may prove beneficial to add protocol command functions to allow governance to change some or all of these hard-coded variables.

Finally, implicit in much of this discussion, and the overall design of the Ambient DEX architecture, is a high degree of centralisation and trust in the governance structure. While this is acknowledged and mitigated by the governance team, it is worth emphasising that users must trust the designers and maintainers of this code. Governance can, for instance, reset or remove the liquidity rewards after the liquidity period has expired, but before the rewards have been claimed. This could be used to exploit liquidity providers, and it is important for users to be cognizant of this risk.

The auditing approach I employed involved a detailed manual review, followed by targeted hardhat testing of key functionality.

Thank you for your time!

Time spent:

15 hours