c4-bot-2 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L132-L137

Vulnerability details

Impact

Return of Lower or Cheaper Price Amount in a Situation Price Feed Amount is suppose to be of more average value in the PriceAggregator.sol Contract

Proof of Concept

function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
        {
        uint256 numNonZero;

        if (price1 > 0)
            numNonZero++;

        if (price2 > 0)
            numNonZero++;

        if (price3 > 0)
            numNonZero++;

        // If less than two price sources then return zero to indicate failure
        if ( numNonZero < 2 )
            return 0;

        uint256 diff12 = _absoluteDifference(price1, price2);
        uint256 diff13 = _absoluteDifference(price1, price3);
        uint256 diff23 = _absoluteDifference(price2, price3);

        uint256 priceA;
        uint256 priceB;

        if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
            (priceA, priceB) = (price1, price2);
        else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
            (priceA, priceB) = (price1, price3);
        else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
            (priceA, priceB) = (price2, price3);

        uint256 averagePrice = ( priceA + priceB ) / 2;

        // If price sources are too far apart then return zero to indicate failure
        if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
            return 0;

        return averagePrice;
        }

The function above shows how _aggregatePrices is implemented, it can be noted from the function that minimum of two price feeds must be active before the price feeds with closer range would be used, the implementation is not properly done as an inactive price feed can end up being used over an active price fee

Scenario

Let us consider a scenario:

Only Two Price Fee is Active e.g PriceB and PriceC
The price between this two is wide such that PriceA to PriceB difference is lower than PriceB to PriceC

Therefore PriceA and PriceB will be used even though PriceA is zero and inactive This will be totally wrong as the average price should be calculated only between two active Price Feeds under any circumtances

Tools Used

Manual Review

Recommended Mitigation Steps

The code should be adjusted as provided below to ensure only active prices are used in a situation one of the price feed is inactive

function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
    {
    uint256 numNonZero;

    if (price1 > 0)
        numNonZero++;

    if (price2 > 0)
        numNonZero++;

    if (price3 > 0)
        numNonZero++;

    // If less than two price sources then return zero to indicate failure
    if ( numNonZero < 2 )
        return 0;

    uint256 diff12 = _absoluteDifference(price1, price2);
    uint256 diff13 = _absoluteDifference(price1, price3);
    uint256 diff23 = _absoluteDifference(price2, price3);

    uint256 priceA;
    uint256 priceB;
+++          if ( price1 != 0 && price2 != 0 && price3 != 0 ) {
    if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
        (priceA, priceB) = (price1, price2);
    else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
        (priceA, priceB) = (price1, price3);
    else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
        (priceA, priceB) = (price2, price3);
+++             }
+++          else{
+++             if ( price1 == 0 )
+++         (priceA, priceB) = (price2, price3);
+++     else if ( price2 == 0 )
+++         (priceA, priceB) = (price1, price3);
+++     else if ( price3 == 0 )
+++         (priceA, priceB) = (price1, price2);
+++          }

    uint256 averagePrice = ( priceA + priceB ) / 2;

    // If price sources are too far apart then return zero to indicate failure
    if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
        return 0;

    return averagePrice;
    }

Assessed type

Oracle

c4-judge commented 9 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 9 months ago

othernet-global (sponsor) confirmed

c4-sponsor commented 8 months ago

othernet-global (sponsor) disputed

othernet-global commented 8 months ago

It is not possible for the zero price to ever be included in the price average.

This is because there is a maximum allowable price difference between the two valid prices.

Given prices A=0, B < C

In order for zero to be closer to B than B is to C then (C-B) needs to be greater than B. If so, then the price difference of B and C is greater than 100% - which means a reverted price as expected.

c4-judge commented 8 months ago

Picodes marked the issue as unsatisfactory: Invalid

Topmark1 commented 8 months ago

I would please implore Judge and Sponsor to have a look at this issue The Argument of the Sponsor on this issue is a situation of "using two wrongs to make a right", the price difference can indeed be wide due to situation of price volatility which the protocol failed to put into consideration, this can be proven by the validity of issue 809 https://github.com/code-423n4/2024-01-salty-findings/issues/809 , The validity of issue 809 means the issue in this report becomes a problem. But my point is that the wrong in the vulnerability of this issue should be treated individually, and not because another wrong implementation temporarily stops the issue, Inactive Price Feed should not be allowed under any Circumstances, and necessary validation should be done to stop it.

Picodes commented 8 months ago

@Topmark1 has explained above, assuming one of the price is 0 (let's say price 1):

either the Oracle is natively considering only price 2 and price 3 so the mitigation suggested here is useless
either it's considering price 1 and another one, say price 2, in which case the absolute difference is |price 2| and it reverts because of the check.

So in both cases this report is invalid.