C4 310 & 256 Mitigation

[rayeaster]

address https://github.com/code-423n4/2023-10-badger-findings/issues/310 & https://github.com/code-423n4/2023-10-badger-findings/issues/256 by adding extra checks around PriceFeed state machine if fallback oracle is empty (not set) and CL reports 50% deviation between rounds

• Additional checks in _bothOraclesLiveAndUnbrokenAndSimilarPrice() & _bothOraclesSimilarPrice() to distinguish the situations when fallback is set (broken/frozen) AND when fallback is not set at all
• Ensure only change to usingChainlinkFallbackUntrusted if CL reports pass the max deviation check in status bothOraclesUntrusted ( case 3 in the PriceFeed state machine)
• Ensure correct state and price returned if CL reports doesn't pass the max deviation check in status usingFallbackChainlinkFrozen ( case 4 in the PriceFeed state machine)
• Ensue above similarity comparison (_bothOraclesLiveAndUnbrokenAndSimilarPrice() or _bothOraclesSimilarPrice()) between primary & fallback oracle happens ONLY AFTER other single-source checks have passed (bad/frozen/max-deviation) in each status

[CodingNameKiki]

Minor changes are made to PriceFeed so that the system can maintain good functionality when the fallback isn't set:

1. If l understand correctly, the system should maintain a state between usingChainlinkFallbackUntrusted and bothOraclesUntrusted if the fallback isn't set and equals address(0), so when usingChainlinkFallbackUntrusted and chainlink is working the system keeps the same state and returns the latest answer from chainlink, otherwise it switches back to bothOraclesUntrusted, in there the system returns the lastGoodPrice or returns the chainlink latest answer and switches back to usingChainlinkFallbackUntrusted if chainlink is working again.

• Little refactoring in the state of usingFallbackChainlinkUntrusted, now the system first checks whether the fallback is broken or frozen and after that check if both oracles are live and have similar prices. In our case if the fallback isn't set and chainlink isn't trusted the system will switch to state of bothOraclesUntrusted

        // --- CASE 2: The system fetched last price from Fallback ---
        if (status == Status.usingFallbackChainlinkUntrusted) {
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

            /*
             * If Fallback is only frozen but otherwise returning valid data, just return the last good price.
             * Fallback may need to be tipped to return current data.
             */
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

            // If both Fallback and Chainlink are live, unbroken, and reporting similar prices, switch back to Chainlink
            if (
                _bothOraclesLiveAndUnbrokenAndSimilarPrice(
                    chainlinkResponse,
                    prevChainlinkResponse,
                    fallbackResponse
                )
            ) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise, use Fallback price
            return _storeFallbackPrice(fallbackResponse);
        }

• In case the state is bothOraclesUntrusted, and in our case when the fallback isn't set the system checks if Chainlink is no longer broken, frozen or the price changes above max and changes the state to usingChainlinkFallbackUntrusted) otherwise if Chainlink remains not useful and the fallback isn't set, the system return the last good price. On the other hand if the fallback is set, the system checks if both oracles are live and if so changes back to state of chainlinkWorking, otherwise both oracle are still untrusted and the last good price is returned.

        // --- 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;
                }
            }

            /*
             * If both oracles are now live, unbroken and similar price, we assume that they are reporting
             * accurately, and so we switch back to Chainlink.
             */
            if (
                _bothOraclesLiveAndUnbrokenAndSimilarPrice(
                    chainlinkResponse,
                    prevChainlinkResponse,
                    fallbackResponse
                )
            ) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise, return the last good price - both oracles are still untrusted (no status change)
            return lastGoodPrice;
        }

• In case we are in state usingChainlinkFallbackUntrusted, the system checks if Chainlink is broken, frozen or price change above max and later checks if both oracles are live and trusted, BUT only changes the state to chainlinkWorking when the fallback is set, otherwise the system remains its state of usingChainlinkFallbackUntrusted and returns the last answer from chainlink.

        // --- CASE 5: Using Chainlink, Fallback is untrusted ---
        if (status == Status.usingChainlinkFallbackUntrusted) {
            // If Chainlink breaks, now both oracles are untrusted
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

            // If Chainlink is frozen, return last good price (no status change)
            if (_chainlinkIsFrozen(chainlinkResponse)) {
                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)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

            // If Chainlink and Fallback are both live, unbroken and similar price, switch back to chainlinkWorking and return Chainlink price
            if (
                _bothOraclesLiveAndUnbrokenAndSimilarPrice(
                    chainlinkResponse,
                    prevChainlinkResponse,
                    fallbackResponse
                )
            ) {
                if (address(fallbackCaller) != address(0)) {
                    _changeStatus(Status.chainlinkWorking);
                }
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise if Chainlink is live and deviated <50% from it's previous price and Fallback is still untrusted,
            // return Chainlink price (no status change)
            return _storeChainlinkPrice(chainlinkResponse.answer);
        }

• Minor change in the function _bothOraclesLiveAndUnbrokenAndSimilarPrice is made which ensures that the if statement can be only triggered when the fallback is set, otherwise the function calls _bothOraclesSimilarPrice

    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))) ||
            _chainlinkIsBroken(_chainlinkResponse, _prevChainlinkResponse) ||
            _chainlinkIsFrozen(_chainlinkResponse)
        ) {
            return false;
        }

        return _bothOraclesSimilarPrice(_chainlinkResponse, _fallbackResponse);
    }

• The minor change in _bothOraclesSimilarPrice is that if the function is called and the fallback isn't set, it will return true.

    function _bothOraclesSimilarPrice(
        ChainlinkResponse memory _chainlinkResponse,
        FallbackResponse memory _fallbackResponse
    ) internal view 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);
        if (minPrice == 0) return false;
        uint256 maxPrice = EbtcMath._max(_fallbackResponse.answer, _chainlinkResponse.answer);
        uint256 percentPriceDifference = ((maxPrice - minPrice) * EbtcMath.DECIMAL_PRECISION) /
            minPrice;

        /*
         * Return true if the relative price difference is <= MAX_PRICE_DIFFERENCE_BETWEEN_ORACLES: if so, we assume both oracles are probably reporting
         * the honest market price, as it is unlikely that both have been broken/hacked and are still in-sync.
         */
        return percentPriceDifference <= MAX_PRICE_DIFFERENCE_BETWEEN_ORACLES;
    }

[CodingNameKiki]

Will make a research to confirm that there aren't any other occurring issues with the changes made.

• This miro board contains how the code looked before and after the changes. https://miro.com/app/board/uXjVNIzP0KA=/

[CodingNameKiki]

l am not sure if this is even possible when chainlink is frozen, but l would rather ask than keep it to myself.

• Take as example that the system is in state usingFallbackChainlinkFrozen, basically the system checks if chainlink is broken, frozen and if not it checks if the fallback is broken and changes the status to usingChainlinkFallbackUntrusted and uses the latest answer from chainlink. One thing l noticed is that along with checking if chainlink is broken or frozen, the system misses to check if the prev and current price of chainlink changes above the max. So in theory if this every happens instead of switching to bothOraclesUntrusted and using the lastGoodPrice, the system will use the chainlink answer and switch to usingChainlinkFallbackUntrusted.

        // --- CASE 4: Using Fallback, and Chainlink is frozen ---
        if (status == Status.usingFallbackChainlinkFrozen) {
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                // If both Oracles are broken, return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.bothOraclesUntrusted);
                    return lastGoodPrice;
                }

                // If Chainlink is broken, remember it and switch to using Fallback
                _changeStatus(Status.usingFallbackChainlinkUntrusted);

                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // If Fallback is working, return Fallback current price
                return _storeFallbackPrice(fallbackResponse);
            }

            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // if Chainlink is frozen and Fallback is broken, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

                // If both are frozen, just use lastGoodPrice
                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // if Chainlink is frozen and Fallback is working, keep using Fallback (no status change)
                return _storeFallbackPrice(fallbackResponse);
            }

            // if Chainlink is live and Fallback is broken, remember Fallback broke, and return Chainlink price
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // If Chainlink is live and Fallback is frozen, just use last good price (no status change) since we have no basis for comparison
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

            // If Chainlink is live and Fallback is working, compare prices. Switch to Chainlink
            // if prices are within 5%, and return Chainlink price.
            if (_bothOraclesSimilarPrice(chainlinkResponse, fallbackResponse)) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise if Chainlink is live but price not within 5% of Fallback, distrust Chainlink, and return Fallback price
            _changeStatus(Status.usingFallbackChainlinkUntrusted);
            return _storeFallbackPrice(fallbackResponse);
        }

[CodingNameKiki]

In our case lets compare the newly made changes, by going over every state with the condition of fallback being address(0).

1. state chainlinkWorking

• On short explanation when the state chainlinkWorking, the system first checks if chainlink is broke, after that if its frozen and finally check if the price of prev and current answer changes above the max allowed value. After performing all this statements, it checks if the fallback is broken and switches to usingChainlinkFallbackUntrusted and returns the latest chainlink answer. Given the scenario when our fallback is not set, if any of the chainlink statements are triggered the system first needs to check if the fallback is broken and switch to bothOraclesUntrusted and return the lastGoodPrice, this should be the case when fallback is address(0). This is already done in the current system which indicates that it's works correctly when the state is chainlinkWorking.

            // If Chainlink is broken, try Fallback
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                // If Fallback is broken then both oracles are untrusted, so return the last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.bothOraclesUntrusted);
                    return lastGoodPrice;
                }

            // If Chainlink is frozen, try Fallback
            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // If Fallback is broken too, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

            // If Chainlink price has changed by > 50% between two consecutive rounds, compare it to Fallback's price
            if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
                // 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;
                }

2. state usingFallbackChainlinkUntrusted

• Basically when in state usingFallbackChainlinkUntrusted, first if the fallback is broken the system returns the lastGoodPrice and switches back to bothOraclesUntrusted. However if the fallback is frozen, it only returns the lastGoodPrice and keeps the same state. Finally if the fallback is neither broken or frozen, the system checks if both oracles are live and return similar prices, which if true the state is changed to chainlinkWorking and the latest chainlink answer is returned. If neither of this conditions are satisfied the system stores the latest fallback price and keeps the same state. It's important to note that duo to the new changes made and in a case of fallback being address(0), the function _bothOraclesLiveAndUnbrokenAndSimilarPrice can return true so the wrong state and price will be changed and returned. However this shouldn't be an issue, as the first thing the system does Is to check if the fallback is broken and switches back to state bothOraclesUntrusted.

        if (status == Status.usingFallbackChainlinkUntrusted) {
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

3. state bothOraclesUntrusted

• This one of the important states when dealing with fallback being address(0), so when that happens the system specifically checks if the chainlink is back to working state and both changes the state to usingChainlinkFallbackUntrusted and returns the latest chainlink price. Otherwise if chainlink is non functional the system simply returns the lastGoodPrice and keeps the same state. When fallback is set the system checks if both oracles are live and return similar prices, if this statement is satisfied the state is changed to chainlinkWorking and the latest chainlink price is returned. If neither of the said conditions are satisfied the system returns the lastGoodPrice and keeps the same statement. Just to note that the concern of the function _bothOraclesLiveAndUnbrokenAndSimilarPrice returning true when the fallback is address(0) won't happen, as the first thing the system does in state bothOraclesUntrusted is to check if the fallback isn't set.

            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;
                }
            }

4. state usingFallbackChainlinkFrozen

• This state may be a little longer than the others, but it still not so hard to understand. In this state the system first checks if chainlink is broken or frozen, after that it checks if the fallback is broken and changes the state to usingChainlinkFallbackUntrusted and returns the latest chainlink answer. After that the system makes a check to see if the fallback frozen and both returns the lastGoodPrice and keeps the same state. Lastly the function checks if both oracles have similar prices and switches back to chainlinkWorking and returns the latest chainlink answer. If finally non of this conditions are satisfied, it indicates that the chainlink is live but the price between the two oracles is off, so the system changes the state to usingFallbackChainlinkUntrusted and both stores and returns the latest fallback response. The concern here is that the function _bothOraclesSimilarPrice returns true if the fallback is address(0), so the system needs to check if the fallback is broken before performing the check for the similar prices. As the system does, it's safe to say that _bothOraclesSimilarPrice can't be reached when fallback is address(0). Also to note that in the beginning of the state when the system checks if chainlink is broken or frozen, if any of this statements are triggered the first thing the system does is to check if the fallback is broken, so wrong state and price can't be reached.

            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // If Chainlink is live and Fallback is frozen, just use last good price (no status change) since we have no basis for comparison
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

            // If Chainlink is live and Fallback is working, compare prices. Switch to Chainlink
            // if prices are within 5%, and return Chainlink price.
            if (_bothOraclesSimilarPrice(chainlinkResponse, fallbackResponse)) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

5. state usingChainlinkFallbackUntrusted

• Finally we arrived at the last state in PriceFeed which is state usingChainlinkFallbackUntrusted. In this state the system performs couple of checks, first it checks if chainlink is either broken, frozen or the price changes above max, finally after this statements it checks if both oracles are live and have similar prices. If non of this conditions are satisfied the system returns the latest chainlink answer and keeps the current state of usingChainlinkFallbackUntrusted. The little difference here is that if either chainlink is broken or chainlink price is above max, the system switches to state bothOraclesUntrusted and returns the lastGoodPrice, however if chainlink is frozen it only returns lastGoodPrice but keeps the same state. In this state when the fallback is address(0) and the state machine reaches _bothOraclesLiveAndUnbrokenAndSimilarPrice, the function will return true but the state will be changed to chainlinkWorking only if the fallback is set, so everything works as expected.

            if (
                _bothOraclesLiveAndUnbrokenAndSimilarPrice(
                    chainlinkResponse,
                    prevChainlinkResponse,
                    fallbackResponse
                )
            ) {
                if (address(fallbackCaller) != address(0)) {
                    _changeStatus(Status.chainlinkWorking);
                }
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

[rayeaster]

l am not sure if this is even possible when chainlink is frozen, but l would rather ask than keep it to myself.

• Take as example that the system is in state usingFallbackChainlinkFrozen, basically the system checks if chainlink is broken, frozen and if not it checks if the fallback is broken and changes the status to usingChainlinkFallbackUntrusted and uses the latest answer from chainlink. One thing l noticed is that along with checking if chainlink is broken or frozen, the system misses to check if the prev and current price of chainlink changes above the max. So in theory if this every happens instead of switching to bothOraclesUntrusted and using the lastGoodPrice, the system will use the chainlink answer and switch to usingChainlinkFallbackUntrusted.

        // --- CASE 4: Using Fallback, and Chainlink is frozen ---
        if (status == Status.usingFallbackChainlinkFrozen) {
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                // If both Oracles are broken, return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.bothOraclesUntrusted);
                    return lastGoodPrice;
                }

                // If Chainlink is broken, remember it and switch to using Fallback
                _changeStatus(Status.usingFallbackChainlinkUntrusted);

                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // If Fallback is working, return Fallback current price
                return _storeFallbackPrice(fallbackResponse);
            }

            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // if Chainlink is frozen and Fallback is broken, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

                // If both are frozen, just use lastGoodPrice
                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // if Chainlink is frozen and Fallback is working, keep using Fallback (no status change)
                return _storeFallbackPrice(fallbackResponse);
            }

            // if Chainlink is live and Fallback is broken, remember Fallback broke, and return Chainlink price
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // If Chainlink is live and Fallback is frozen, just use last good price (no status change) since we have no basis for comparison
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

            // If Chainlink is live and Fallback is working, compare prices. Switch to Chainlink
            // if prices are within 5%, and return Chainlink price.
            if (_bothOraclesSimilarPrice(chainlinkResponse, fallbackResponse)) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise if Chainlink is live but price not within 5% of Fallback, distrust Chainlink, and return Fallback price
            _changeStatus(Status.usingFallbackChainlinkUntrusted);
            return _storeFallbackPrice(fallbackResponse);
        }

according to Liquity doc, it seems ok https://docs.google.com/spreadsheets/d/18fdtTUoqgmsK3Mb6LBO-6na0oK-Y9LWBqnPCJRp5Hsg/edit#gid=316701608

[CodingNameKiki]

Based on the statements in the comment, do we actually need to fix this issue?

https://github.com/code-423n4/2023-10-badger-findings/issues/256#issuecomment-1842572212

[GalloDaSballo]

What is 256 saying that was worth fixing? Basically the same issue?

[CodingNameKiki]

Yes basically the same core issue can occur when the system is in usingFallbackChainlinkFrozen, so we fix both places.

[GalloDaSballo]

@wtj2021 it could be good to write the following (fuzz or invariant)

fetchPrice can never return a different price when called twice or three times in a row

Basically adapt EchidnaPriceFeedTester to have that check, can work on this tomorrow if you don't have the bandwith

NOTE: In reality this invariant will break, but we want to assert that without a change of CL / Fallback, the feed must always return the same value if called more than once, whereas the C4 issues highlight that that's not the case

[CodingNameKiki]

When both oracles were in frozen state but are back alive and there is 5% difference in the prices, the system naturally selects the response of the fallback oracle as more accurate even tho chainlink is the primary oracle.

We can see that when the system is in status chainlinkWorking and chainlink is frozen and the fallback is not broken but frozen. The system returns the lastGoodPrice and switches to status usingFallbackChainlinkFrozen even tho the fallback is frozen as well.

            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // If Fallback is broken too, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

                // If Fallback is frozen or working, remember Chainlink froze, and switch to Fallback
                _changeStatus(Status.usingFallbackChainlinkFrozen);

                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // If Fallback is working, use it
                return _storeFallbackPrice(fallbackResponse);
            }

When the system is in usingFallbackChainlinkFrozen and both the chainlink and fallback oracles are frozen, the system will return the lastGoodPrice and keep the status.

But an interesting flow happens when both of the oracles are live and back to work but have a 5% price difference.

• What happens is that even tho both chainlink and the fallback were frozen before, and now that they are live and chainlink have 5% difference than the fallback response. The system will naturally select the fallback response over the chainlink one even tho chainlink is the primary oracle as a result the system can potentially return an incorrect price and swtich to status usingFallbackChainlinkUntrusted.

            // If Chainlink is live and Fallback is working, compare prices. Switch to Chainlink
            // if prices are within 5%, and return Chainlink price.
            if (_bothOraclesSimilarPrice(chainlinkResponse, fallbackResponse)) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise if Chainlink is live but price not within 5% of Fallback, distrust Chainlink, and return Fallback price
            _changeStatus(Status.usingFallbackChainlinkUntrusted);
            return _storeFallbackPrice(fallbackResponse);

After that the system will be in status usingFallbackChainlinkUntrusted and return only the fallback response as the correct one until the fallback gets back out of the potentially staled phase.

        if (status == Status.usingFallbackChainlinkUntrusted) {
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

            /*
             * If Fallback is only frozen but otherwise returning valid data, just return the last good price.
             * Fallback may need to be tipped to return current data.
             */
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

            // If both Fallback and Chainlink are live, unbroken, and reporting similar prices, switch back to Chainlink
            if (
                _bothOraclesLiveAndUnbrokenAndSimilarPrice(
                    chainlinkResponse,
                    prevChainlinkResponse,
                    fallbackResponse
                )
            ) {
                _changeStatus(Status.chainlinkWorking);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

            // Otherwise, use Fallback price
            return _storeFallbackPrice(fallbackResponse);
        }

• Duo to the fact that chainlink is selected as primary oracle, it was decided that the chance of chainlink returning more accurate price is bigger than chance of the fallback oracle. But in this particular flow the system selects the fallback response over the chainlink one even tho both of the oracles were frozen.

Recommendation

Let the system keep the same status when the fallback oracle is frozen as well.

            // If Chainlink is frozen, try Fallback
            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // If Fallback is broken too, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

                // If Fallback is frozen or working, remember Chainlink froze, and switch to Fallback
-               _changeStatus(Status.usingFallbackChainlinkFrozen);

                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

+               _changeStatus(Status.usingFallbackChainlinkFrozen);

                // If Fallback is working, use it
                return _storeFallbackPrice(fallbackResponse);
            }

• Just to note when both of the oracles are frozen and in status usingFallbackChainlinkFrozen the system does the same thing as in status chainlinkWorking which is to return the lastGoodPrice, so there is not any difference here apart from allowing the mentioned issue to occur.

            if (_chainlinkIsFrozen(chainlinkResponse)) {
                // if Chainlink is frozen and Fallback is broken, remember Fallback broke, and return last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return lastGoodPrice;
                }

                // If both are frozen, just use lastGoodPrice
                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

                // if Chainlink is frozen and Fallback is working, keep using Fallback (no status change)
                return _storeFallbackPrice(fallbackResponse);
            }

[rayeaster]

@CodingNameKiki I think the issue you mentioned around both primary and fallback are frozen and the code decide to choose fallback over primary is valid if we do prefer primary. However, what I am not quite sure about is whether we should change the behavior.

Considering usingFallbackChainlinkFrozen is the only proper status indicating primary (CL) is frozen (but not broken), and Liquity also clearly states in both the code comment and its design doc that "If Fallback is frozen or working, remember Chainlink froze, and switch to Fallback", it might be acceptable to keep it untouched.

[CodingNameKiki]

Hey @rayeaster, l think It's good for the system to enforce the right behaviour in this situation, e.g if we choose the primary oracle to have more safe and accurate response the system shouldn't pick the fallback oracle over the chainlink one.

Just to note that the previous issue we fixed related to 256 was also acceptable and as design based on the Liquity doc.

So l think this is good decision to make - whether we want the best for our system or to follow strictly the Liquity's design.

l also did a research and this recommendation doesn't change any other behaviour of the price feed:

• Both chainlink and the fallback oracles are frozen - the system keeps its status as chainlinkWorking and returns the lastGoodPrice. Later the fallback is no more frozen, the system safely changes to status usingFallbackChainlinkFrozen and continues its work as design.
• If chainlink is no more frozen and in status chainlinkWorking, the system will do all the needed checks which are also done in usingFallbackChainlinkFrozen, so everything works as expected.

                // If Fallback is frozen or working, remember Chainlink froze, and switch to Fallback
-               _changeStatus(Status.usingFallbackChainlinkFrozen);

                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

+               _changeStatus(Status.usingFallbackChainlinkFrozen);

Lets say we decide to keep it as it is now:

• Both oracles are frozen, the system enters usingFallbackChainlinkFrozen and only returns the lastGoodPrice like in the recommendation, so there is no significant change apart from allowing this issue to occur.

                // If both are frozen, just use lastGoodPrice
                if (_fallbackIsFrozen(fallbackResponse)) {
                    return lastGoodPrice;
                }

[rayeaster]

would love to have @dapp-whisperer @GalloDaSballo @wtj2021 insights on this topic

[dapp-whisperer]

When both oracles were in frozen state but are back alive and there is 5% difference in the prices, the system naturally selects the response of the fallback oracle as more accurate even tho chainlink is the primary oracle.

I think this is acceptable due to the threshold. I agree with Kiki that it's not desired behavior - a great find. I'm inclined towards NOFIX due to the status of the project and the fact that we're not using a fallback oracle for the foreseeable future.