tank - fee calculations

[sherlock-admin]

tank

medium

fee calculations

Summary

refer to this LOCs: https://github.com/sherlock-audit/2023-06-real-wagmi/blob/main/concentrator/contracts/Multipool.sol#L722-L746, these LOCs don't allow underflow/overflow, but refering to Uniswap/v3-core#573 which allows underflow/overflow. This will lead to some transactions will revert.

Vulnerability Detail

Impact

some user transactions will revert.

Code Snippet

            if (liquidity > 0) {
                (
                    uint256 feeGrowthInside0X128Pending,
                    uint256 feeGrowthInside1X128Pending
                ) = _getFeeGrowthInside(
                        IUniswapV3Pool(position.poolAddress),
                        slots[i].tick,
                        position.lowerTick,
                        position.upperTick
                    );
                pendingFee0 += uint128(
                    FullMath.mulDiv(
                        feeGrowthInside0X128Pending - feeGrowthInside0LastX128,
                        liquidity,
                        FixedPoint128.Q128
                    )
                );
                pendingFee1 += uint128(
                    FullMath.mulDiv(
                        feeGrowthInside1X128Pending - feeGrowthInside1LastX128,
                        liquidity,
                        FixedPoint128.Q128
                    )
                );
            }

Tool used

Manual Review

Recommendation

add unchecked {} when calculate the pending fee

[huuducsc]

Escalate

This issue is not a duplicate of #88, and it is an invalid issue. The above calculation is similar to UniswapV3's implementation for accruing fees, so it is correct. UniswapV3 doesn't allow overflowing/underflowing while updating the position, refer to this code snippet https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Position.sol#L61-L76
Additionally, the impact of this issue is low.

[sherlock-admin]

Escalate

This issue is not a duplicate of #88, and it is an invalid issue. The above calculation is similar to UniswapV3's implementation for accruing fees, so it is correct. UniswapV3 doesn't allow overflowing/underflowing while updating the position, refer to this code snippet https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Position.sol#L61-L76
Additionally, the impact of this issue is low.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[n1punp]

@huuducsc This is a valid issue (also dupe of #1 ) .

Your analysis is incorrect. UniswapV3 does allow overflowing / underflowing. The code snippet you're posting is for Solidity version < 0.8.0, which has not safe math by default

[JeffCX]

@huuducsc This is a valid issue (also dupe of #1 ) .

Your analysis is incorrect. UniswapV3 does allow overflowing / underflowing. The code snippet you're posting is for Solidity version < 0.8.0, which has not safe math by default

Agree,

not a duplicate of #88, seperate valid medium based on the comments;

Uniswap/v3-core#573

the Uniswap code use compiler version before 0.8.0 so the overflow / underflow is implicitly allowed

but in the current real wagmi codebase use compiler version 0.8.18 and does not allow overflow / underflow

based on the issue 573 discussion, the implicit overflow is needed to correctly calculated the fee.

so a valid issue

as for whether the severity is medium or low

Uniswap/v3-core#573 (comment)

I think the situation above is not a uncommon case, so recommend severity is medium

unless there are further comments from both side!

[hrishibhat]

@huuducsc Do you agree with the above comments?

[hrishibhat]

@n1punp
Jeiwan/uniswapv3-book#45 (comment)
Do you have any comments on this?

[n1punp]

It's as Jeiwan's commented -- the subtraction should expect underflow (so this will be an issue for solidity >= 0.8.x), so this is a valid issue.

@n1punp Jeiwan/uniswapv3-book#45 (comment) Do you have any comments on this?

[hrishibhat]

@n1punp Thanks for clarifying. Yes, that makes sense.
Sorry if I'm missing something.
I don't see reverting during a deposit as a major issue.
but the revert during withdrawal can be a problem.
Do you have a valid example with numbers for the possibility?

Or is this example applicable in this case?
Uniswap/v3-core#573 (comment)

[n1punp]

@hrishibhat Not sure about the specific case where user can deposit but cannot withdraw. It may require very precise calculations to find it out, which could not be worth the time (since allowing underflow should be the answer anyways), but having the core functionality unavailable should give this issue a valid Medium at least. I'm fine with it being valid Medium (as in #1 ).

(if anyone can somehow prove that it can deposit but cannot withdraw, then it could be a valid High).

[hrishibhat]

Result:
Medium
Has duplicates
Considering this a valid medium based on the above comments

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• huuducsc: accepted