Badger eBTC Audit + Certora Formal Verification Competition - ether_sky's results

bytes032 marked the issue as insufficient quality report

OOS

bytes032 marked the issue as duplicate of #141

jhsagd76 marked the issue as not a duplicate

jhsagd76 marked the issue as duplicate of #310

jhsagd76 marked the issue as satisfactory

The report does not identify the key issues and provide the incorrect mitigation solutions. I downgrade it to 25%.

jhsagd76 removed the grade

jhsagd76 marked the issue as partial-25

Hi @jhsagd76 ,

Thank you for reviewing the reports. Could you kindly review my comments?

The main issue here is the inconsistency in our use of validation checks across different cases. In the 'chainlinkWorking' and 'usingChainlinkFallbackUntrusted' cases, we ensure ChainLink validity by verifying three conditions:

• ChainLink is not broken.
• ChainLink is not frozen.
• The current price value has not deviated by more than 50% from the previous price. If the third condition is not satisfied, we use the 'lastGoodPrice' instead of the Chainlink value.

However, in 'bothOraclesUntrusted', we only check the first two conditions. This discrepancy is the reason why we can obtain different prices within the same transaction.

Let me clarify my concern. Suppose the fallback is not set. In Status.usingChainlinkFallbackUntrusted, we check the third condition even when the fallback is 0, and we use lastGoodPrice and convert it to Status.bothOraclesUntrusted status.

if (status == Status.usingChainlinkFallbackUntrusted) {
    if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
        _changeStatus(Status.bothOraclesUntrusted);
        return lastGoodPrice;
    }
}

However, in Status.bothOraclesUntrusted status, we don't check the third condition and use the Chainlink response.

if (status == Status.bothOraclesUntrusted) {
    if (address(fallbackCaller) == address(0)) {
        // If CL has resumed working
        if (
            !_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse) &&
            !_chainlinkIsFrozen(chainlinkResponse)
        ) {
            _changeStatus(Status.usingChainlinkFallbackUntrusted);
            return _storeChainlinkPrice(chainlinkResponse.answer);
        }
    }
}

I believe there are two possible solutions based on how we consider the third condition when there is no fallback. The first approach is to consider Chainlink as reliable only when all three conditions are satisfied, regardless of whether the fallback is set or not. In this case, my suggested approach is correct. Because we have already used the lastGoodPrice before transitioning to the bothOraclesUntrusted status, and if the fallback is not set and the Chainlink is neither broken nor frozen, we can use lastGoodPrice once more and transition to the usingChainlinkFallbackUntrusted status. In this status, we evaluate the third condition irrespective of whether the fallback is set or not. If the third condition is met, we can confidently use the Chainlink value obtained at this step without any inconsistency. If the third condition is not met, we can still use lastGoodPrice and transition back to the bothOraclesUntrusted status. So if we consider Chainlink as reliable only when all three conditions are met, this approach ensures that there is no risk to use untrusted price values.

The second approach is to consider Chainlink as reliable when the first two conditions are satisfied and the fallback is not set or all three conditions are satisfied with the fallback set. In this case, the suggested approach in 310 is correct.

In short, both solutions can prevent incorrect price values, and the choice depends on how we want to handle the reliability of Chainlink in the absence of a fallback.

Thanks.

I don't think the mitigation in the original report is valid, and it also didn't point out the main impact.

But I found that it seems like a diff edge case wasn't mentioned in the primary report. It can't result in return different prices in the same transaction. This impact is not that severe, but also worth considering.

Codelines: https://github.com/code-423n4/2023-10-badger/blob/2bacb215bf3257320e4f6dd7dcb039f1f06f3214/packages/contracts/contracts/PriceFeed.sol#L291-L294

scenario:

1. The fallback oracle is set.
2. The current init status is usingFallbackChainlinkFrozen, case 4.
3. The current chainlink price feed is alive and not broken. But it returns a 50% diff price cmp with the pre round.
4. The fallback oracle is broken now.

When calls the fetchPrice function: Call I.:

                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);

State is changed to usingChainlinkFallbackUntrusted, and return the current chainlink print directly.

Call II.: Jump to state usingChainlinkFallbackUntrusted, case 5. Chainlink print is not broken, not Frozen, not Similar, but ChangeAboveMax. So the lastGoodPrice is returned, which is equal to the chainlinkResponse.answer in the Call I. And the state -> bothOraclesUntrusted.

            if (_chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

So finally, after the Call I, an unreliable value has been stored as the lastGoodPrice. kindly ping the sponsor @CodingNameKiki to ensure they won't miss anything.

Given this new edge case mentioned in the original report, I am increasing the reward to 50%.

jhsagd76 marked the issue as partial-50

Hey @jhsagd76, l have previously discussed this concern when l was doing a research on the mitigations. here.

It seems that Liquity decided to keep the functionality that way given the fact it's very unlikely there will be 50% Deviation between two rounds. But l agree that this issue shows edge cases in the original report and should be awarded as duplicate because the core issue achieved is the same - 50% Deviation between two rounds in both reports. here.

Based on the below statements the core outcome is the same.

Original issue:

• When the state machine is in status bothOraclesUntrusted and the fallback is address(0) the if statement will be triggered, the system will further check if chainlink is neither broken or frozen and change the status to usingChainlinkFallbackUntrusted, return the chainlink answer and store it as the lastGoodPrice.

            if (address(fallbackCaller) == address(0)) {
                // If CL has resumed working
                if (
                    !_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse) &&
                    !_chainlinkIsFrozen(chainlinkResponse)
                ) {
                    _changeStatus(Status.usingChainlinkFallbackUntrusted);
                    return _storeChainlinkPrice(chainlinkResponse.answer);
                }
            }

Duplicate issue:

• The same situation happens when the state machine is in status usingFallbackChainlinkFrozen, the system checks if chainlink is neither broken or frozen after that the next if statement is triggered which indicates the fallback is broken, so the system will change the state to usingChainlinkFallbackUntrusted return the chainlink answer and store is as the lastGoodPrice.

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

Originally ebtc seems to be designed based on Liquity, so based on Liquity if the duplicated issue is okay scenario for use. Perhaps l think the issue in the original report should be okay as well or perhaps they should be fixed on both places? Thanks for the ping will forward to the team.

^ We decided to fix the concern on both places.

Again, thanks for the mention.

Hi @jhsagd76 , could you please check CodingNameKiki's comment?

Thanks, @jhsagd76 , @CodingNameKiki

Based on the above discussion, and 75% tag are currently unavailable and according to the rules related to "Penalty / Award Standardization - Duplicate Report PoC Thoroughness", remove partial 50%

jhsagd76 marked the issue as full credit

jhsagd76 marked the issue as satisfactory

bytes032 marked the issue as insufficient quality report

No proof of impact

GalloDaSballo (sponsor) disputed

We have shared broken invariants and this one also doesn't demonstrate a valid impact, this is not weaponized

Just mark it as a dup of #199 , although they are a little different. Pls check the comment I left in the #199 and prove it should be a MED.

jhsagd76 marked the issue as duplicate of #199

jhsagd76 changed the severity to QA (Quality Assurance)

jhsagd76 marked the issue as grade-c

This previously downgraded issue has been upgraded by jhsagd76

jhsagd76 marked the issue as selected for report

jhsagd76 marked the issue as satisfactory

jhsagd76 marked the issue as not selected for report

jhsagd76 marked issue #199 as primary and marked this issue as a duplicate of 199

make sense, still maintain a 100% reward for this issue.

bytes032 marked the issue as insufficient quality report

1L+2S = 7