ast3ros - Incorrect deviation calculation in isDeviatingWithBpsCheck function

[sherlock-admin]

ast3ros

medium

Incorrect deviation calculation in isDeviatingWithBpsCheck function

Summary

The current implementation of the isDeviatingWithBpsCheck function in the codebase leads to inaccurate deviation calculations, potentially allowing deviations beyond the specified limits.

Vulnerability Detail

The function isDeviatingWithBpsCheck checks if the deviation between two values exceeds a defined threshold. This function incorrectly calculates the deviation, considering only the deviation from the larger value to the smaller one, instead of the deviation from the mean (or TWAP).

    function isDeviatingWithBpsCheck(
        uint256 value0_,
        uint256 value1_,
        uint256 deviationBps_,
        uint256 deviationMax_
    ) internal pure returns (bool) {
        if (deviationBps_ > deviationMax_)
            revert Deviation_InvalidDeviationBps(deviationBps_, deviationMax_);

        return isDeviating(value0_, value1_, deviationBps_, deviationMax_);
    }

    function isDeviating(
        uint256 value0_,
        uint256 value1_,
        uint256 deviationBps_,
        uint256 deviationMax_
    ) internal pure returns (bool) {
        return
            (value0_ < value1_)
                ? _isDeviating(value1_, value0_, deviationBps_, deviationMax_)
                : _isDeviating(value0_, value1_, deviationBps_, deviationMax_);
    }

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/libraries/Deviation.sol#L23-L52

The function then call _isDeviating to calculate how much the smaller value is deviated from the bigger value.

    function _isDeviating(
        uint256 value0_,
        uint256 value1_,
        uint256 deviationBps_,
        uint256 deviationMax_
    ) internal pure returns (bool) {
        return ((value0_ - value1_) * deviationMax_) / value0_ > deviationBps_;
    }

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/libraries/Deviation.sol#L63-L70

The function isDeviatingWithBpsCheck is usually used to check how much the current value is deviated from the TWAP value to make sure that the value is not manipulated. Such as spot price and twap price in UniswapV3.

    if (
        // `isDeviatingWithBpsCheck()` will revert if `deviationBps` is invalid.
        Deviation.isDeviatingWithBpsCheck(
            baseInQuotePrice,
            baseInQuoteTWAP,
            params.maxDeviationBps,
            DEVIATION_BASE
        )
    ) {
        revert UniswapV3_PriceMismatch(address(params.pool), baseInQuoteTWAP, baseInQuotePrice);
    }

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/UniswapV3Price.sol#L225-L235

The issue is isDeviatingWithBpsCheck is not check the deviation of current value to the TWAP but deviation from the bigger value to the smaller value. This leads to an incorrect allowance range for the price, permitting deviations that exceed the acceptable threshold.

Example:

TWAP price: 1000 Allow deviation: 10%.

The correct deviation calculation will use deviation from the mean. The allow price will be from 900 to 1100 since:

• |1100 - 1000| / 1000 = 10%
• |900 - 1000| / 1000 = 10%

However the current calculation will allow the price from 900 to 1111

• (1111 - 1000) / 1111 = 10%
• (1000 - 900) / 1000 = 10%

Even though the actual deviation of 1111 to 1000 is |1111 - 1000| / 1000 = 11.11% > 10%

Impact

This miscalculation allows for greater deviations than intended, increasing the vulnerability to price manipulation and inaccuracies in Oracle price reporting.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/libraries/Deviation.sol#L63-L70

Tool used

Manual review

Recommendation

To accurately measure deviation, the isDeviating function should be revised to calculate the deviation based on the mean value: | spot value - twap value | / twap value.

[0xrusowsky]

https://github.com/OlympusDAO/bophades/pull/245

[IAm0x52]

Escalate

This is purely a design choice. Nothing here is wrong with the implementation. The deviation is purely subjective and is measured objectively the same in both directions. This should be a low severity issue in my opinion and I strongly believe it should be. At the maximum this should be a medium severity issues as impact is not large at all for any reasonable variation and only subjectively incorrect

[sherlock-admin2]

Escalate

This is purely a design choice. Nothing here is wrong with the implementation. The deviation is purely subjective and is measured objectively the same in both directions. This should be a low severity issue in my opinion and I strongly believe it should be. At the maximum this should be a medium severity issues as impact is not large at all for any reasonable variation and only subjectively incorrect

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]

@IAm0x52 I'm pretty sure sponsor acknowledging this with a fix indicates this is not a design choice. Let me know if there are any publicly available information at time of contests that points to that that I am missing.

Since price values are CORE components of price modules, I labelled it as high as the returned price should never be allowed to have too significant of a deviation if not every use case of this prices will be impacted. I think #3 highlights the possible impact of this issues well, and as such this issues should have a minimum of medium severity if not high.

[IAm0x52]

This is only used in the BUNNI library which is full range liquidity. This simply used to ensure that reserves have not been manipulated and is not the price being used. Using the example provided at a 10% deviation. Reserves can be ~1% different between methodologies.

Let's do a small bit of math to figure this. Assume current invariant is 10000 and there should be 100 of each token (100 100 = 10000). If each token is worth $1 then the true value of the pool is 200 (1 100 + 1 100) Assume price has been manipulated up 10% so now the pool has 110 and 90.9 (10000 / 110) so the value of the pool is now 200.9 (110 1 + 90.9 1). Lets move it 1.111% more to 11.111% this means there is 111.1111 and 90 (10000 / 111.111) so the value of the pool is now 201.111 (111.111 1 + 90 * 1). This results in a difference of 0.211 on a value of 200.9 or 0.1%. This is entirely negligible and hence why I say the deviation check order is a design choice and either way is negligible.

[nevillehuang]

@IAm0x52 Agree with your analysis, but on context that core contract functionality of deviation check is broken, suggest to keep medium severity.

[IAm0x52]

Fix looks good. Benchmark is now always the middle for comparison

[Czar102]

I agree that calculating deviation in log is a valid design choice. Nevertheless, I think it was clear from the comments in code that the deviation was supposed to be calculated symmetrically and linearly, I acknowledge the limitations of this bug as well.

Hence, planning to consider this a medium severity issue.

[Czar102]

Result: Medium Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: accepted