Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect calculations for owed premia may potentially disrupt protocols that rely on SFPM. #295

Open
c4-bot-5 opened this issue Dec 4, 2023 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-211 edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-5
Copy link
Contributor

c4-bot-5 commented Dec 4, 2023

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f4b61b57bdd539f827f3ef7c335c5bde2d5c62a2/contracts/SemiFungiblePositionManager.sol#L1275-L1306

Vulnerability details

Impact

There are two types of liquidity: one remains in Uniswap, and the other is transferred from Uniswap by users.
SFPM tracks owed fees for the liquidity moved from Uniswap as if it were still in Uniswap.
This is crucial because protocols utilizing SFPM can access these owed fees and request fees from users moving liquidity from Uniswap.
However, an inaccurate calculation of owed premia can disrupt the expectations of protocols relying on SFPM for precise values.
Some protocols may design their functionality based on these values, and if they are incorrect, it has the potential to disrupt the intended behavior of those protocols.
For instance, protocols may determine the fees received from the Uniswap pool by calculating the difference between gross and owed amounts, updating their strategy accordingly.
However, if this difference is inaccurate due to incorrect calculations, it can lead to the adoption of a flawed strategy.

The incorrect calculations occur consistently.
The process, in which removed liquidity fluctuates between values larger than 0 and 0, repeats
throughout the lifecycle.
Consequently, the calculated owed premia value becomes unreliable and should not be relied upon.

Proof of Concept

There are specific equations used to calculate the owed premia.

owed_feesCollectedX128 = feeGrowthX128 * R * (1 + spread)                      (Eqn 1)
spread = ν*R/N
feesCollected = feesGrowthX128 * (T-R)

Based on the above information, we derive the final equation for owed premia per liquidity as follows:

s_accountPremiumOwed += feeGrowthX128 * R * (1 + ν*R/N) / R
    += feeGrowthX128 * (T - R + ν*R)/N
    += feeGrowthX128 * T/N * (1 - R/T + ν*R/T)

I'd like to bring to your attention that the above equation is incorrect when R is 0.
In this scenario, owed_feesCollectedX128 would be 0 according to Equation 1.
However, when we divide this by R in subsequent steps, we end up with a non-zero value as the owed growth per liquidity.
In reality, it's not feasible to calculate 0 / 0, and this aspect needs to be addressed in the equation.

As a consequence, the obtained amount is incorrect in this scenario.

s_accountPremiumOwed += feeGrowthX128 * T/N * (1 - R/T + ν*R/T)
    += feeGrowthX128 * T / N * (1 - 0 + 0)
    += feeGrowthX128 * T / N
    += feeGrowthX128

It's essential to account for scenarios where the removed liquidity is 0.

function _getPremiaDeltas(
    uint256 currentLiquidity,
    int256 collectedAmounts
) private pure returns (uint256 deltaPremiumOwed, uint256 deltaPremiumGross) {
    premium0X64_base = Math
        .mulDiv(collected0, totalLiquidity * 2 ** 64, netLiquidity ** 2)
        .toUint128();
   
    uint256 numerator = netLiquidity + (removedLiquidity / 2 ** VEGOID);

    premium0X64_owed = Math
        .mulDiv(premium0X64_base, numerator, totalLiquidity)
        .toUint128();
}

The PoC for this is as below:

function test_getAccountPremium_Owed(
        uint256 x,
        uint256 widthSeed,
        int256 strikeSeed,
        uint256 positionSizeSeed,
        uint256 swapSize
    ) public {
        _initPool(x);
        (int24 width, int24 strike) = PositionUtils.getInRangeSW(
            widthSeed,
            strikeSeed,
            uint24(tickSpacing),
            currentTick
        );
        populatePositionData(width, strike, positionSizeSeed);

        uint256 tokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 1, 0, strike, width);
        sfpm.mintTokenizedPosition(tokenId, uint128(positionSize), TickMath.MIN_TICK, TickMath.MAX_TICK);

        changePrank(Bob);

        swapSize = bound(swapSize, 10 ** 15, 10 ** 19);
        router.exactInputSingle(
            ISwapRouter.ExactInputSingleParams(
                isWETH == 0 ? token0 : token1,
                isWETH == 1 ? token0 : token1,
                fee,
                Bob,
                block.timestamp,
                swapSize,
                0,
                0
            )
        );
        router.exactOutputSingle(
            ISwapRouter.ExactOutputSingleParams(
                isWETH == 1 ? token0 : token1,
                isWETH == 0 ? token0 : token1,
                fee,
                Bob,
                block.timestamp,
                swapSize - (swapSize * fee) / 1_000_000,
                type(uint256).max,
                0
            )
        );

        (, currentTick, , , , , ) = pool.slot0();

        changePrank(address(sfpm));
        pool.burn(tickLower, tickUpper, 0);
        changePrank(Alice);

        {
            (uint128 premiumToken0, uint128 premiumtoken1) = sfpm.getAccountPremium(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper,
                currentTick,
                // we want to calculate owed premia
                1
            );
            // Alice get owed premia larger than 0, even though there is no removed liquidity (i.e., no liquidity moved from Uniswap).
            assertGt(premiumToken0, 0);
            assertGt(premiumtoken1, 0);
        }
}

Tools Used

Manual

Recommended Mitigation Steps

Please modify _getPremiaDeltas function as below:

function _getPremiaDeltas(
    uint256 currentLiquidity,
    int256 collectedAmounts
) private pure returns (uint256 deltaPremiumOwed, uint256 deltaPremiumGross) {
    premium0X64_base = Math
        .mulDiv(collected0, totalLiquidity * 2 ** 64, netLiquidity ** 2)
        .toUint128();
   
    - uint256 numerator = netLiquidity + (removedLiquidity / 2 ** VEGOID);
    + uint256 numerator = totalLiquidity == netLiquidity ? 0 : netLiquidity + (removedLiquidity / 2 ** VEGOID);

    premium0X64_owed = Math
        .mulDiv(premium0X64_base, numerator, totalLiquidity)
        .toUint128();
}

Assessed type

Math

@c4-bot-5 c4-bot-5 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 4, 2023
c4-bot-6 added a commit that referenced this issue Dec 4, 2023
@c4-bot-5 c4-bot-5 removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Dec 4, 2023
@code4rena-admin code4rena-admin added 3 (High Risk) Assets can be stolen/lost/compromised directly edited-by-warden labels Dec 4, 2023
@c4-bot-8 c4-bot-8 changed the title Incorrect calculations for owed premia may result in the protocol erroneously requesting more fees from its users. Incorrect calculations for owed premia may potentially disrupt protocols that rely on SFPM. Dec 4, 2023
@dyedm1
Copy link
Member

dyedm1 commented Dec 18, 2023

dup #211,

It's important to note that this account premium calculation is not actually used anywhere within the SFPM, and is an opinionated calculation exposed for optional use by protocols such as Panoptic to price options.

With that being said, part of the opinionated definition of that calculation includes a multiplier on the premium for option buyers based on the utilization on a given chunk. As is illustrated by this graph of the relationship between the utilization and the premium multiplier, there is a vertical asymptote at 100% utilization. Thus, it is expected that protocols using this feature will cap utilization at a certain percentage (for example, Panoptic allows a maximum utilization of 90%).

The long premium calculations at a utilization of 100% (where netLiquidity=0) cannot be performed because the multiplier approaches infinity and would be undefined at that utilization. In that situation, it makes the most sense to forego the premium calculation. Protocols who use this calculation should understand this behavior and either implement a utilization cap or accept that premium will be 0 at utilization=100%

@c4-sponsor
Copy link

dyedm1 (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Dec 18, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #211

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Dec 26, 2023
@c4-judge
Copy link
Contributor

Picodes changed the severity to QA (Quality Assurance)

@C4-Staff C4-Staff reopened this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-211 edited-by-warden grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

6 participants