KupiaSec - Invalid price calculation for BunniTokens leads to price manipulation

[sherlock-admin]

KupiaSec

high

Invalid price calculation for BunniTokens leads to price manipulation

Summary

BunniToken price is calculated using price of reserved tokens in the pool, leads to easy price manipulation as Warp Finance has been attacked.
REF: Pricing LP tokens

Vulnerability Detail

BunniPrice submodule only works with BunniTokens with full-range positions.
It's not validated directly but it's guaranteed by checking deviations.

function _validateReserves(
    BunniKey memory key_,
    BunniLens lens_,
    uint16 twapMaxDeviationBps_,
    uint32 twapObservationWindow_
) internal view {
    uint256 reservesTokenRatio = BunniHelper.getReservesRatio(key_, lens_);
    uint256 twapTokenRatio = UniswapV3OracleHelper.getTWAPRatio(
        address(key_.pool),
        twapObservationWindow_
    );

    // Revert if the relative deviation is greater than the maximum.
    if (
        // `isDeviatingWithBpsCheck()` will revert if `deviationBps` is invalid.
        Deviation.isDeviatingWithBpsCheck(
            reservesTokenRatio,
            twapTokenRatio,
            twapMaxDeviationBps_,
            TWAP_MAX_DEVIATION_BASE
        )
    ) {
        revert BunniPrice_PriceMismatch(address(key_.pool), twapTokenRatio, reservesTokenRatio);
    }
}

For non full-range positions, reservesTokenRatio is not even similar to twapTokenRatio.
Thus, these full-range positions on UniswapV3 works as same as UniswapV2 pools.

However, in calculating BunniToken price(aka UniswapV3 LP price), it sums up the price of token reserves in the pool:

function _getTotalValue(
    BunniToken token_,
    BunniLens lens_,
    uint8 outputDecimals_
) internal view returns (uint256) {
    (address token0, uint256 reserve0, address token1, uint256 reserve1) = _getBunniReserves(
        token_,
        lens_,
        outputDecimals_
    );
    uint256 outputScale = 10 ** outputDecimals_;

    // Determine the value of each reserve token in USD
    uint256 totalValue;
    totalValue += _PRICE().getPrice(token0).mulDiv(reserve0, outputScale);
    totalValue += _PRICE().getPrice(token1).mulDiv(reserve1, outputScale);

    return totalValue;
}

Calculating the BunniToken price using this formula includes the vulnerability which is described in the reference link above(Warp Finance hack with price manipulation).

Impact

BunniToken price can be manipulated by an attacker to generate profits from the vulnerability.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BunniPrice.sol#L162-L165
https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BunniPrice.sol#L215-L233

Tool used

Manual Review

Recommendation

As in the above reference link, fair LP price calculation is introduced as follows: $$ p(LP) = \frac{2 * \sqrt{p0 * p1 * k}}{L}, k=x * y $$
For UniswapV3, $L = \sqrt{x * y}$, so we can rewrite the above formula like $$ p(LP) = \frac{2 * \sqrt{p0 * p1 * L^2}}{L} = 2 * \sqrt{p0 * p1} $$

[nevillehuang]

Invalid, known issue

The PRICE submodules that use an on-chain method to access the reserves of a liquidity pool or positions are susceptible to sandwich attacks and multi-block manipulation - Assets in PRICEv2 can be configured to track the moving average of an asset price in order to mitigate this risk - Assets in PRICEv2 can be configured with multiple price feeds and a reconciliation strategy (e.g. average, median, average if a deviation is present) in order to mitigate this risk - Where possible, PRICE submodules will check for re-entrancy in the source (e.g. liquidity pool). This has been implemented in the BalancerPoolTokenPrice and Uniswap submodule.

The SPPLY submodules that use an on-chain method to access the reserves of a liquidity pool or positions are susceptible to sandwich attacks and multi-block manipulation - Where possible, downstream consumers of the data need to conduct sanity-checks. - Where possible, SPPLY submodules will check for re-entrancy in the source (e.g. liquidity pool). This has been implemented in the AuraBalancerSupply submodule. - To guard against multi-block manipulation, where possible, SPPLY submodules will compare the implied price from reserves against the price from TWAP. This has been implemented in the BunniSupply submodule.

[KupiaSecAdmin]

Escalate

Hey @nevillehuang - I think there's a bit of misunderstanding here. The point of the issue is that the logic to calculate LP price of BunniTokens is incorrect. As shown in the codebase, it calculates the price of BunniTokens as what Warp Finance did(value of assets divided by number of LPs), which is incorrect. I've added a link Pricing LP tokens for better understanding.

[sherlock-admin2]

Escalate

Hey @nevillehuang - I think there's a bit of misunderstanding here. The point of the issue is that the logic to calculate LP price of BunniTokens is incorrect. As shown in the codebase, it calculates the price of BunniTokens as what Warp Finance did(value of assets divided by number of LPs), which is incorrect. I've added a link Pricing LP tokens for better understanding.

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.

[nevillehuang]

@KupiaSecAdmin there is no misunderstanding, the issue you highlighted is dependent on manipulating reserves of uniswap pools, which is a known issue as I highlighted. This is allowed because there are many mitigations in place to counter this issue, as known by this comments here by sponsor.

[KupiaSecAdmin]

@nevillehuang - This is not about manipulating uniswap reserves, what I meant is the logic itself to calculate BunniToken price is wrong.

[nevillehuang]

@0xJem you might want to take a look at this, however I am not convinced and believe this is expected behavior:

In the blog post under the section "Warp Finance Hack", the third step has shown it directly involves manipulating the reserves. As I mentioned above, olympus has a range of factors in place to mitigate this, thats why it is considered an accepted risk.

3. Swap the remainder of the flash loan in the same DAI <> WETH pool. This changes the LP reserves and leads to Warp Finance using a wrong LP price.

[0xJem]

I don't believe this is valid.

We acknowledge the potential for incorrect pricing when using the reserves of a full-range position to calculate the LP position price. For this reason, there are mitigations in place:

• Re-entrancy check on the Uniswap V3 pool
• Compare the token1/token0 ratio from TWAP to the token1/token0 ratio from reserves (in the flash loan example, there would be a deviation here and it would revert)

[KupiaSecAdmin]

Reconsidered, it seems like the deviation check will not be passed.
Thanks for the clarification guys! @0xJem @nevillehuang 👍

[Czar102]

Since the submitter agreed with invalidation, planning to execute on it.

[Czar102]

Result:
Invalid
Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: rejected