Protocol unintentionally implements an asymmetric method of calculating the price difference

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-beanstalk-2/blob/27ff8c87c9164c1fbff054be5f22e56f86cdf127/protocol/contracts/libraries/Oracle/LibOracleHelpers.sol#L25-L32

https://github.com/Cyfrin/2024-04-beanstalk-2/blob/27ff8c87c9164c1fbff054be5f22e56f86cdf127/protocol/contracts/libraries/Oracle/LibWstethEthOracle.sol#L68-L98

Summary

The protocol exhibits a disparity in the calculation of price differences due to the current implementation in LibOracleHelpers::getPercentDifference(). This affects LibWstethEthOracle::getWstethEthPrice(), since the present implementation causes unintended returns of 0 for price queries. The discrepancy arises from the flawed calculation logic as explained in the report, causing the protocol to incorrectly assume price manipulation in certain valid scenarios. This flaw undermines the reliability of price queries and cwould result in denial-of-service unintended implementation

Vulnerability Details

First take a look at https://github.com/Cyfrin/2024-04-beanstalk-2/blob/27ff8c87c9164c1fbff054be5f22e56f86cdf127/protocol/contracts/libraries/Oracle/LibOracleHelpers.sol#L25-L32

    function getPercentDifference(
        uint x,
        uint y
    ) internal pure returns (uint256 percentDifference) {
        percentDifference = x.mul(ONE).div(y);
        percentDifference = x > y ? percentDifference - ONE : ONE - percentDifference; // SafeMath unnecessary due to conditional check
    }
}

This function is used to get the percent difference between two values, keep in mind that "values" in this context are "prices" provided by different oracle sources, protocol hardcodes the numerator and denominator of getting the percentage as x and y respectively x.mul(ONE).div(y), but evidently we can see that the percentage of the price difference is finalized depending on if x > y or not, causing the equation to be flawed, cause percentDifference - ONE & ONE - percentDifference would yield different results in the context for two same x and y price.

To deduce the differences in the formular, we can see that

• If x > y then the percentDifference is gotten as ( ( (1e18 * X ) / Y ) - 1e18), however
• If x < y then the percentDifference is gotten as (1e18 - ( ( 1e18 * X ) / Y ) )

Using generic values of 5 and 6... and assuming 1e18 as 1000 just for POC sake, i.e instances of 1e18 * X is now 1000X.

For the first case (X = 5, Y = 6), we get a percentDifference of 166,67 ~ 16,7% For the second case (X = 6, Y = 5), we get a percentDifference of 200 = 20%

Now, take a look at the only instance where this check is implemented in scope, https://github.com/Cyfrin/2024-04-beanstalk-2/blob/27ff8c87c9164c1fbff054be5f22e56f86cdf127/protocol/contracts/libraries/Oracle/LibWstethEthOracle.sol#L68-L98

    function getWstethEthPrice(uint256 lookback) internal view returns (uint256 wstethEthPrice) {

        uint256 chainlinkPrice = lookback == 0 ?
            LibChainlinkOracle.getPrice(WSTETH_ETH_CHAINLINK_PRICE_AGGREGATOR, LibChainlinkOracle.FOUR_DAY_TIMEOUT) :
            LibChainlinkOracle.getTwap(WSTETH_ETH_CHAINLINK_PRICE_AGGREGATOR, LibChainlinkOracle.FOUR_DAY_TIMEOUT, lookback);

        // Check if the chainlink price is broken or frozen.
        if (chainlinkPrice == 0) return 0;

        uint256 stethPerWsteth = IWsteth(C.WSTETH).stEthPerToken();

        chainlinkPrice = chainlinkPrice.mul(stethPerWsteth).div(CHAINLINK_DENOMINATOR);


        // Uniswap V3 only supports a uint32 lookback.
        if (lookback > type(uint32).max) return 0;
        uint256 uniswapPrice = LibUniswapOracle.getTwap(
            lookback == 0 ? LibUniswapOracle.FIFTEEN_MINUTES :
            uint32(lookback),
            WSTETH_ETH_UNIV3_01_POOL, C.WSTETH, C.WETH, ONE
        );

        // Check if the uniswapPrice oracle fails.
        if (uniswapPrice == 0) return 0;
        //@audit `getPercentDifference` is queried with `MAX_DIFFERENCE`
        if (LibOracleHelpers.getPercentDifference(chainlinkPrice, uniswapPrice) < MAX_DIFFERENCE) {
            wstethEthPrice = chainlinkPrice.add(uniswapPrice).div(AVERAGE_DENOMINATOR);
            if (wstethEthPrice > stethPerWsteth) wstethEthPrice = stethPerWsteth;
            wstethEthPrice = wstethEthPrice.div(PRECISION_DENOMINATOR);
        }
    }

This function is used to get the wstEth price, but the MAX_DIFFERENCE check is applied here and in a case where the price discrepancy is more than MAX_DIFFERENCE this function returns 0.

The above explained case means that the current implementation of getPercentDifference would cause protocol assuming the price has been manipulated for the same difference just dependent on if the chainlink price is higher/lower than the uniswap returned price, causing 0 to be returned in some valid cases and considering the fact that the MAX_DIFFERENCE has been set very low, this would frequently occur, to use a real life case a sa POC, as at the time of writing this report, the value of wstEth in comparison to eth is 1,16323, so assuming the queried price X from chainlink returns 1,17490, and uniswap price Y returns 1,16323, the getPercentDifference() check fails, since the difference in percentage is = 1,0032409755594 * 1e18 i.e > MAX_DIFFERENCE, however if we swap which provider provides which, i.e now chainlink X returns 1,16323 and uniswap price Y returns 1,17490, the getPercentDifference() check passes since the difference in percentage would be = 0.99327602349136 * 1e18 i.e < MAX_DIFFERENCE proving the disparity and this calculation wstethEthPrice = chainlinkPrice.add(uniswapPrice).div(AVERAGE_DENOMINATOR); would be the same for the two cases irrespective of (x > y)? i.e in our highlighted case above for both instances the calculation would equal ((1,17490, + 1,16323 ) / 2) = 1,169065 and should be considered valid in both cases.

Impact

Desynchronization of price queries and this directly affects all instances in protocol where LibWstethEthOracle.sol's getWstethEthPrice() is called, i.e in two same cases where the provided prices are the same, the attempt to query the price correctly works with one where as it doesn't with the other, leading to unintended return of 0 for price essentially a DOS to LibWstethEthOracle.sol's getWstethEthPrice() since protocol would now assume that the price has been manipulated, keep in mind that in real sense this calculation wstethEthPrice = chainlinkPrice.add(uniswapPrice).div(AVERAGE_DENOMINATOR); would always return the same value for both cases and as such the price should be valid and accepted in both cases.

Tool Used

Manual review

Recommended Mitigation Steps.

Make the check to always be in sync, this can be by ensuring that the divisor is always the lower/greater price and then maybe reimplementing the value of MAX_DIFFERENCE to take this into consideration, the former alone is sufficient, i.e reimplement https://github.com/Cyfrin/2024-04-beanstalk-2/blob/27ff8c87c9164c1fbff054be5f22e56f86cdf127/protocol/contracts/libraries/Oracle/LibOracleHelpers.sol#L25-L32 to:

        function getPercentDifference(
            uint x,
            uint y
        ) internal pure returns (uint256 percentDifference) {
            if (x == y) {
                percentDifference = 0;
            } else if (x < y) {
                percentDifference = x.mul(ONE).div(y);
                percentDifference = ONE - percentDifference;
            } else {
                percentDifference = y.mul(ONE).div(x);
                percentDifference = ONE - percentDifference;
            }
            return percentDifference;
        }

Here we are always using the bigger price as the denominator, thereby making sure that in whichever of the two cases explained in report, i.e if x > y or not a fixed percentDifference is provided and this can then be accurately checked against protocol's set MAX_DIFFERENCE value.