c4-bot-4 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L183 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L142-L143

Vulnerability details

Impact

During price volatitily, aggregated price feed might DOS the liquidation process when the protocol needs liquidiation the most to keep USDS overcollateralized.

Proof of Concept

PriceAggregator.sol compares three price feeds and will pick two feeds that are closest and report the average price of the two.

In _aggregatePrices(), before reporting the average price, a sanity check is performed to compare the difference bewteen the two price (_absoluteDifference(priceA, priceB)). And if the difference is greater than a pre-set tolerance in percentage, it reports 0, which reverts getPriceBTC() and getPriceETH().

This mechanism is fine in normal circumstances, but when the asset price is volatile (e.g. WETH or WBTC price dropping), a higher-than-usual price discrepancy may occur, and the price difference between any two price feeds is likely higher than a preset maximumPriceFeedPercentDifferenceTimes1000.

//src/price_feed/PriceAggregator.sol
    function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
        {
...
        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;
}

    function getPriceBTC() external view returns (uint256 price)
...
        price = _aggregatePrices(price1, price2, price3);
|>      require (price != 0, "Invalid BTC price" );

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L182-L183)

The above suggests whenever WETH or WBTC prices become volatile, the risk increases that getPriceBTC() and getPriceETH() will revert. And this will DOS key protocol liquidation process liquidateUser(). Protocol bad debts cannot be liquidated and USDS can be undercollateralized when the protocol needs liquidation the most.

//src/stable/CollateralAndLiquidity.sol
    function liquidateUser(address wallet) external nonReentrant {
... 
          //@audit canUserBeliquidated(wallet) ->userCollateralValueInUSD()->underlyingTokenValueInUsd(userWBTC,userWETH) -> getPriceBTC() flow reverts during price volatility
|>        require(canUserBeLiquidated(wallet), "User cannot be liquidated");
...
}

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L145)

liquidation shouldn't be at risk of prolonged reverts during price volatility, this puts USDS pegging and protocol health at risk during critical times.

In addition, PriceAggregator.sol can never be replaced by DAO or protocol during emergencies. This leaves the protocol has no means to handle the above situation.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding a protocol-controlled funciton to replace PriceAggregator with a backup oracle during emergencies.

Assessed type

Other

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #809