search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

`CoreChainlinkFeed` uses BTC/USD Chainlink oracle to price WBTC which can lead to DoS and undercollateralized borrowing if WBTC depegs #646

Closed c4-bot-5 closed 9 months ago

c4-bot-5 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol#L15

Vulnerability details

Summary

The CoreChainlinkFeed contract utilizes a BTC/USD Chainlink oracle to determine the price of WBTC. However, this approach can lead to potential issues if WBTC were to depeg from BTC. In such a scenario, WBTC would no longer maintain an equivalent value to BTC. This can result in significant problems, including borrowing against a devalued asset and the accumulation of bad debt. Given that one of the three protocol's feeds continues to value WBTC based on BTC/USD, the issuance of bad USDS loans could persist, exacerbating the overall level of bad debt.

Vulnerability Details

Note: there is no WBTC/USD price feed on Ethereum mainnet, so it's not just a wrong feed address issue. It will require requesting both BTC/USD and WBTC/BTC prices. The only available WBTC feed is WBTC/BTC.

The vulnerability lies in the reliance on a BTC/USD Chainlink oracle to obtain the price of WBTC. If the bridge connecting WBTC to BTC becomes compromised and WBTC depegs, WBTC may depeg from BTC. Consequently, WBTC's value would no longer be equivalent to BTC, potentially rendering it worthless. https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreChainlinkFeed.sol

I. Let's say WBTC suddenly depegged to 1 USD a) CoreChainlinkFeed feed returns 40_000 (BTC/USD price) b) CoreUniswapFeed (TWAP) feed slowly changes the price from 40_000 to 1 (WBTC/USDC price) for 30 minutes c) CoreSaltyFeed returns 1 (WBTC/USDS price)

It will lead to DoS for at least 30 minutes, until CoreUniswapFeed price == CoreSaltyFeed price (see PoC): 1) PriceAggregator::_aggregatePrices returns 0

        if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
            return 0;

2) PriceAggregator::getPriceBTC reverts

price = _aggregatePrices(price1, price2, price3);
require (price != 0, "Invalid BTC price" );

3) CollateralAndLiquidity::borrowUSDS calls CollateralAndLiquidity::maxBorrowableUSDS calls CollateralAndLiquidity::userCollateralValueInUSD calls CollateralAndLiquidity::underlyingTokenValueInUSD
calls PriceAggregator::getPriceBTC will revert 4) CollateralAndLiquidity::liquidateUser calls canUserBeLiquidated calls CollateralAndLiquidity::userCollateralValueInUSD calls CollateralAndLiquidity::underlyingTokenValueInUSD
calls PriceAggregator::getPriceBTC will revert

`liquidateUser` also 
calls [`CollateralAndLiquidity::underlyingTokenValueInUSD`](https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L163)  
calls [`PriceAggregator::getPriceBTC`](https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L198) 
will revert

5) withdrawCollateralAndClaim calls maxWithdrawableCollateral calls userCollateralValueInUSD calls PriceAggregator::getPriceBTC
will revert

Note: DoS will probably affect USDS price, it can depeg and lead to different prices in CoreSaltyFeed and CoreUniswapFeed even after 30 minutes because USDS != USDC => WBTC/USDC != WBTC/USDS. Which will lead to a permanent DoS.

II. In case the depeg is slow, PriceAggregator will return a higher price: a) CoreChainlinkFeed feed returns 40_000 (BTC/USD price) b) CoreUniswapFeed (TWAP) feed slowly changes the price from 40_000 to a lower price (WBTC/USDC price) c) CoreSaltyFeed returns a lower price immediately At first, while _absoluteDifference < PriceAggregator::maximumPriceFeedPercentDifferenceTimes1000 (default 3%) it will report higher than the spot price, average from CoreChainlinkFeed and CoreUniswapFeed, which will allow to borrowUSDS without appropriate collateral.

Note that it will lead to a drop in USDS price. Attackers/bots would use the opportunity to mint a lot of USDS backed by WBTC (that dropped in price but Salty.io still values it almost as BTC), sell it, buy more WBTC, and repeat. Because of that CoreSaltyFeed will return a value different from CoreUniswapFeed

Then we will have DoS as described in "Let's say WBTC suddenly depegged to 1 USD".

Impact

  1. DoS => no liquidations => bad debt => loss of collateral
  2. Uncontrollable growth of the supply of USDS => significant depeg of USDS => loss to USDS holders

    Proof of Concept

    • Most of the PriceAggregator behavior is already described in TestPriceAggregator::testAggregatePrices.
    • Uniswap TWAP behavior can be checked using a Geometric Mean Calculator (e.g., this one). We need 150 values because 30*60=1800 seconds TWAP / 12 seconds per ETH block = 150. You can use JavaScript '40000 '.repeat(149) + '1' to print it, then paste it into the calculator.
    • You can place this file in src/stable/tests/M1.t.sol and run COVERAGE="yes" forge test -f wss://ethereum-sepolia.publicnode.com -vvv --mt testOne to see DoS proof in case all 3 prices are significantly different. 
      
// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "./CollateralAndLiquidity.t.sol"; import "../../price_feed/tests/ForcedPriceFeed.sol";

contract TestPriceAggregator is TestCollateral { // Does not matter for this example uint constant ETH_PRICE = 3000 ether;

    function testOne() external {
        /* Based on `testLiquidatePosition` */
        uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
        uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();
        vm.startPrank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare( 
            depositedWBTC, depositedWETH, 0, block.timestamp, false 
        );
        vm.warp( block.timestamp + 1 hours );
        collateralAndLiquidity.depositCollateralAndIncreaseShare( 
            depositedWBTC, depositedWETH, 0, block.timestamp, false 
        );
        // Account for both deposits
        depositedWBTC = depositedWBTC * 2;
        depositedWETH = depositedWETH * 2;
        vm.stopPrank();

    // Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
    vm.prank(DEPLOYER);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(1 * 10**8, 1 ether, 0, block.timestamp, false);
    vm.warp(block.timestamp + 1 hours);

    vm.startPrank(alice);
    uint maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS(maxUSDS);
    vm.stopPrank();

    /* Reverts with Invalid BTC price */

    ForcedPriceFeed feed1 = new ForcedPriceFeed(40_000 ether, ETH_PRICE);
    ForcedPriceFeed feed2 = new ForcedPriceFeed(20_000 ether, ETH_PRICE);
    ForcedPriceFeed feed3 = new ForcedPriceFeed(1 ether, ETH_PRICE);

    vm.startPrank(PriceAggregator(address(priceAggregator)).owner());

    priceAggregator.setPriceFeed(1, IPriceFeed(address(feed1)));
    vm.warp(block.timestamp + priceAggregator.priceFeedModificationCooldown());
    priceAggregator.setPriceFeed(2, IPriceFeed(address(feed2)));
    vm.warp(block.timestamp + priceAggregator.priceFeedModificationCooldown());
    priceAggregator.setPriceFeed(3, IPriceFeed(address(feed3)));

    vm.stopPrank();

    vm.expectRevert("Invalid BTC price");
    vm.prank(bob);
    collateralAndLiquidity.liquidateUser(alice);
}

}


## Tools Used
Manual review
## Recommended Mitigation Steps
Rewrite to use WBTC/USD price, which will require reading two feeds, BTC/USD and WBTC/BTC.

## Assessed type

Oracle
c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #632

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory