Fee overflow problem

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/src/ILOPool.sol#L213-L225 https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/src/ILOPool.sol#L259-L261

Vulnerability details

In the claim function, during fee calculation, there is a potential for overflow due to the way fees are computed based on the user's position. Specifically, the calculation can result in amount0 + fees0 > amountCollected0, causing an overflow. This issue arises because during the collect process, all fees collected up to that point, along with the burned amount, are gathered. If an overflow occurs, all the fees might be transferred to the fee taker, leading to unintended fund distribution.

Impact

The overflow issue in the claim function can have several severe consequences:

1. Unauthorized Fund Transfer: If an overflow occurs, the fee taker might receive more fees than intended, including the entire amount collected so far.
2. Financial Loss: Other users could be deprived of their rightful fees, leading to significant financial losses.

3. System Exploit: Malicious users could exploit this vulnerability to drain funds from the contract, undermining the trust and stability of the system.

   Proof of Concept

   Fees are calculated using the following formula:

uint256 fees0 = FullMath.mulDiv(feeGrowthInside0LastX128 - position.feeGrowthInside0LastX128,positionLiquidity,FixedPoint128.Q128 );

After the calculation, it can happen that amount0 + fees0 > amountCollected0, leading to an overflow.

Simple example: A user initiates a claim and withdraws all fees and burned amount from the contract. If another user calls the claim function immediately after, the fee calculation could result in fees0 + amount0 being higher than amountCollected0. The reason for this is that position.feeGrowthInside0LastX128 (based on only one token) could have a very small value compared to the real feeGrowthInside0LastX128 of the contract position. Since the contract version is below 0.8.0, this overflow will not revert the transaction, and the overflowed value will be treated as valid, leading to the unintended transfer of excess fees to the user.

Recommended Mitigation Steps

1. Upgrade Solidity Version:

   Upgrade the contract to Solidity version 0.8.0 or above to benefit from built-in overflow checks.

2. Implement Overflow Checks:

   Explicitly check for potential overflows in the fee calculation and handle them appropriately.

require(amount0 + fees0 <= amountCollected0, "Overflow: fees exceed collected amount");

3. Separate Fee Calculation and Collection:

   Separate the fee calculation logic from the collection logic to ensure accurate tracking and prevent manipulation. This is the point that i suggest to separate the two.

Assessed type

Math

[PeterSRWeb3]

Hi @alex-ppg, thanks for you effort. I think the above issue is valid, because we have a talk with @Haupc during the contest and he agreed on that, this issue is valid. I think it's partially similar to https://github.com/code-423n4/2024-06-vultisig-findings/issues/43

[Haupc]

confirmed this is valid and seems like we have dups

[alex-ppg]

Hey @PeterSRWeb3, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

The quality of the submission is quite low and does not fit the criteria for a submission that you would see in a formal audit report. It has been created with the assistance of generative tooling, and it does not adequately describe the vulnerability as defined in the issue referenced. As a result, its invalidation ruling will be retained.

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.

[PeterSRWeb3]

Yeah, I agree with you that this issue is not described with the expected quality, I took my notes and I will create a better reports for the next contest. But it describes a real problem in the system and even the sponsor confirmed it, please take a look again! I can drop more details if it's needed @alex-ppg

[alex-ppg]

Hey @PeterSRWeb3, disregarding the rules of PJQA is grounds for the termination of SR access, so I strongly advise you to avoid such offenses in the near future.

As the Sponsor commented, there were submissions that better described the vulnerability than this one. If this was the sole submission of the vulnerability, it would have been accepted as satisfactory and best. Given that C4 is a contest, the comparative quality of a report is what matters when evaluating their eligibility for a reward.