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#L24-L35 https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L122

Vulnerability details

Impact

A malicious user can brick minting, burning and harvesting of liquidity for almost all liquidity providers.

Important NOTE: This is a different vector from another gas issue, which is iterating over too many ticks in (int24 i = lowerTick + 10; i <= upperTick - 10; ++i). That issue affects wide liquidity positions, while this attack vector affects even liquidity positions with a relatively small number of ticks.

Proof of Concept

When accrueConcentratedPositionTimeWeightedLiquidity is called, under most conditions, for every potentially eligible tick, it will iterate over every tickTrackingData in tickTracking:

while (time < block.timestamp && tickTrackingIndex < numTickTracking)

tickTracking is iterated by tickTrackingIndex++;

The array mapped by tickTracking_ is increased by 1 for a tick every time a trade through the liquidity pool changes the price from a different tick to this tick. This is implemented in the crossTicks function:

    function crossTicks(
        bytes32 poolIdx,
        int24 exitTick,
        int24 entryTick
    ) internal {
        uint256 numElementsExit = tickTracking_[poolIdx][exitTick].length;
        tickTracking_[poolIdx][exitTick][numElementsExit - 1]
            .exitTimestamp = uint32(block.timestamp);
        StorageLayout.TickTracking memory tickTrackingData = StorageLayout
            .TickTracking(uint32(block.timestamp), 0);
        tickTracking_[poolIdx][entryTick].push(tickTrackingData);
    }

A user could purposely increase the length of the tickTracking_ array and hence cause the gas limit to be reached whenever the array is looped over.

The price impact required to cross a tick is from 0 to 1 bps, as 1 bps as the tick width. This is already extremely small, but the attacker could have the swap amount be a very small fraction of a bps if they first swap to make the price end very close to a tick boundary, and then execute multiple extremely small swaps which bounce the price back and forth over the tick boundary.

Note that the CANTO liquidity rewards are targeted to stable pools. An attacker can be quite confident, for example, that a USDC/USDT pool will trade at around \$1, and the ticks closest to \$1 will always be eligible for rewards and therefore be looped over by all rewardable positions when accrueConcentratedPositionTimeWeightedLiquidity is called. Therefore the attack can be targeted to just one or two ticks to affect almost every user.

accrueConcentratedPositionTimeWeightedLiquidity is called during minting, burning and harvesting liquidity positions. Therefore this gas griefing attack will make all these functions revert, for almost every user. This would basically break the functionality of concentrated liquidity pools on Ambient.

Contrast the effect to the cost to the attacker: using the aforementioned attack vector the main cost to the attacker will be the gas costs of performing the swaps. This is far lower than the damage that is done to the protocol/users

One additional factor which makes this attack easy to execute that crossing ticks even if the entry and exit is within the same block.timestamp adds to the array length. Tracking this is unnecessary, because the tick was active for 0 blocks, and therefore the time delta and hence allocated rewards is zero.

Tools Used

Manual Review

Recommended Mitigation Steps

One immediate step would to pop() tickTrackingData as soon as the exitTimestamp == entryTimestamp. This happens to the last element of the array when crossTicks is called. Tracking this is unnecessary, because the tick was active for 0 blocks, and therefore the time delta and hence allocated rewards is zero.

The documentation stated that CANTO rewards are meant to be distributed for stable pools for this codebase. The term "stable" could have different interpretations, but this reccomendation assumes that this refers to stablecoin-like or pegged asset pairs such as stETH/WETH, USDT/USDC etc.

Instead of iterating through every tick, one could assume a range where the stable assets could lie and then reward all positions that lie within the specified range - in this case +- 10 ticks of the price tick.

This makes an assumption that these "stable assets" will actually stay pegged to each other. However, the current accounting architecture has multiple problems:

Given the high number of loops required by the current accounting mechanism, there are multiple reasons that gas could run out. This includes iterating through too many ticks or having too many tick entries/exits
The current mechanism increases the gas costs of all minting, burning and harvesting
DOS attacks like the one described in this issue are possible.

Assuming a stable price has the downside of misallocating rewards if the stable assets depeg from each other. However, this may be a reasonable tradeoff to prevent this DOS attack.

Assessed type

DoS

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-pre-sort commented 1 year ago

141345 marked the issue as sufficient quality report

c4-sponsor commented 1 year ago

OpenCoreCH (sponsor) confirmed

c4-judge commented 1 year ago

dmvt marked the issue as selected for report

piken commented 1 year ago

I believe that above findings should be grouped two High Risk problems instead of one:

malicious tickTracking_ manipulation #114, #68, #221, #253, #276, #61, #11
tick iteration flaw #201, #215, #235, #82 The root causes of two grouped problems come from different attack vectors:
for malicious tickTracking manipulation, malicious user can quickly increase the length of the tickTracking array to make almost every accrueConcentratedPositionTimeWeightedLiquidity() fail due to out of gas caused by `tickTracking` iteration.
On the contrary, tick iteration flaw can be produced without any attack from malicious user. As soon as user operates their position in wide tick range, the problem will be produced. I think Banditx0x has described these two problems clearly since he found both two problems: #114 for malicious tickTracking_ manipulation and #82 for tick iteration flaw.

@dmvt Could you take a look at it?

dmvt commented 1 year ago

Thanks for the detail provided in your reasoning. To my eye, these are close enough in terms of reason, impact, and resolution to be in effect two different reasons that the same issue (gas limit exceeded in that specific loop) occurs. Many reported issues in every contest I've judged would be split if I allowed for "this can happen accidentally" and "a malicious user can cause this to happen" to be justification for doing so. In my view, for an issue to be distinct with the same code, the attack vector and resolution have to be meaningfully different. That the resolution could be different is not enough. Ruling stands.

Banditx0x commented 11 months ago

@dmvt I do not have backstage as my KYC is not going through so I couldn't escalate this issue. Despite this, someone else escalated without my prompting as these are indeed two seperate issues. My report was the selected submission which demonstrates that I did put alot of thought into these submissions.

Here's a simple explanation to demonstrate just how incredibly different these two scenarios are

Difference in Reason:

This submission works by increasing the number of swaps in a liquidity tick which is in the reward range.

Submission #82 is based on the number of ticks for the reward range.

Difference in Impact:

This submission can be a purposeful DOS which applies to a reward range with any number of ticks.

Submission #82 only applies to situations with a wide range of liquidity. It is not possible to create an out-of-gas error with low-number-of-tick liquidity positions with that issue.

Difference in Resolution:

The solution here is to pop some of the tickTracking data needs to be iterated through. Note that tickTracking is NOT the same as ticks. tickTracking is related to number of SWAPS not number of TICKS.

The solution to #82 concerns limiting the number of TICKS that are iterated through.

Different parts of code:

This concerns iterating through the tickTracking. The other submission concerns iterating through ticks.

I did mention these differences in both submissions.

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

Array Length of `tickTracking_ ` Can be Purposely Increased to Brick Minting and Burning of Most Users' Liquidity Positions #114