Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Off-by-one bug prevents the _compareMinMax() from detecting Chainlink aggregators' circuit-breaking events #251

Open
c4-bot-6 opened this issue Mar 11, 2024 · 13 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality M-06 🤖_04_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseOracleHub/OracleHelper.sol#L97

Vulnerability details

Impact

The Wise Lending protocol implements the OracleHelper::_compareMinMax() to detect the circuit-breaking events of Chainlink aggregators when an asset price goes outside of pre-determined min/max values. For instance, in case of a significant price drop (e.g., LUNA crash), the asset price reported by the Chainlink price feed will continue to be at the pre-determined minAnswer instead of the actual price.

The _compareMinMax()'s objective is to prevent such a crash event that would allow a user to borrow other assets with the wrongly reported asset price. For more, refer to the case of Venus Protocol and Blizz Finance in the crash of LUNA.

However, the current implementation of the _compareMinMax() got an off-by-one bug that would prevent the function from detecting the mentioned Chainlink aggregators' circuit-breaking events. In other words, the function will not revert the transaction if the flash crash event occurs as expected.

Proof of Concept

In the flash crash event, the Chainlink price feed will continue to return the _answer at the pre-determined minAnswer instead of the actual price. In other words, the possible minimum value of the _answer would be minAnswer.

Since the _compareMinMax() does not include the case of _answer == minAnswer (also, _answer == maxAnswer), the function could not detect whether or not the crash event happens.

    function _compareMinMax(
        IAggregator _tokenAggregator,
        int192 _answer
    )
        internal
        view
    {
        int192 maxAnswer = _tokenAggregator.maxAnswer();
        int192 minAnswer = _tokenAggregator.minAnswer();

@>      if (_answer > maxAnswer || _answer < minAnswer) {
            revert OracleIsDead();
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the cases _answer == minAnswer and _answer == maxAnswer like the snippet below.

    function _compareMinMax(
        IAggregator _tokenAggregator,
        int192 _answer
    )
        internal
        view
    {
        int192 maxAnswer = _tokenAggregator.maxAnswer();
        int192 minAnswer = _tokenAggregator.minAnswer();

-       if (_answer > maxAnswer || _answer < minAnswer) {
+       if (_answer >= maxAnswer || _answer <= minAnswer) {
            revert OracleIsDead();
        }
    }

Assessed type

Oracle

@c4-bot-6 c4-bot-6 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 11, 2024
c4-bot-9 added a commit that referenced this issue Mar 11, 2024
@c4-bot-12 c4-bot-12 added the 🤖_04_group AI based duplicate group recommendation label Mar 12, 2024
@GalloDaSballo
Copy link

Off by one are QA per SC

@c4-pre-sort
Copy link

GalloDaSballo marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Mar 16, 2024
@c4-judge
Copy link
Contributor

trust1995 changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Mar 26, 2024
@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Mar 26, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as grade-c

@serial-coder
Copy link
Member

Hi @trust1995,

Thanks for judging the contest, sir.

This is a valid medium issue as the _compareMinMax() was implemented incorrectly.

Specifically, the _compareMinMax() does not include the edge cases _answer == minAnswer and _answer == maxAnswer. So, the function cannot detect the minAnswer or maxAnswer as expected.

To elaborate, for example, the minAnswer the aggregator can report for the LINK (one of the tokens in scope) is 100000000000000 (10 ** 14). Suppose, in the event of a flash crash, the price of the LINK token drops significantly below the minAnswer (below 10 ** 14).

However, the price feed will continue to report the pre-determined minAnswer (not the actual price). Since the _compareMinMax() does not include the case _answer == minAnswer, it cannot detect this flash crash event.

In other words, the least reported price (i.e., _answer) will be the pre-determined minAnswer, not the actual price. Thus, the _compareMinMax() will never enter the " if " case and revert the transaction as expected because the _answer will never be less than the aggregator's minAnswer.

    function _compareMinMax(
        IAggregator _tokenAggregator,
        int192 _answer
    )
        internal
        view
    {
        int192 maxAnswer = _tokenAggregator.maxAnswer();
        int192 minAnswer = _tokenAggregator.minAnswer();

@>      if (_answer > maxAnswer || _answer < minAnswer) { //@audit -- The least reported price (i.e., `_answer`) will be the `minAnswer`. So, the function will never enter the " if " case
            revert OracleIsDead();
        }
    }

An example reference is sherlock-audit/2023-02-blueberry-judging#18. As you can see, their recommendation includes the edge cases _answer == minAnswer and _answer == maxAnswer.

M-03-02

Further on the TWAP oracle:

Someone may argue that the protocol has the TWAP.

I want to point out that the TWAP setup for each price feed is only optional. If the TWAP is not set, the price deviation comparison mechanism between the TWAP's price and Chainlink's price will not be triggered.

For this reason, this issue deserves a Medium severity.

Thanks for your time, sir!

@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by trust1995

@c4-judge c4-judge reopened this Mar 27, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Mar 27, 2024
@trust1995
Copy link

The impact demonstrated is an incorrect validation check, which in the case maxAnswer/minAnswer are used by the feed, will make detecting black swans fail. As the team assumes those protections are in place, Med severity is appropriate.

@serial-coder
Copy link
Member

@trust1995

Thanks for your judge!

But did you forget to remove the tags: unsatisfactory, grade-c, and insufficient quality report?

@00xSEV
Copy link

00xSEV commented Mar 28, 2024

@trust1995
Could you double-check that it's a valid issue?
If we follow the link from the Chainlink docs and check the code https://docs.chain.link/data-feeds#monitoring-data-feeds

https://github.com/smartcontractkit/libocr/blob/9e4afd8896f365b964bdf769ca28f373a3fb0300/contract/OffchainAggregator.sol#L68-L69

   * @param _minAnswer lowest answer the median of a report is allowed to be
   * @param _maxAnswer highest answer the median of a report is allowed to be

https://github.com/smartcontractkit/libocr/blob/9e4afd8896f365b964bdf769ca28f373a3fb0300/contract/OffchainAggregator.sol#L640

require(minAnswer <= median && median <= maxAnswer, "median is out of min-max range");

It means that if median == minAnswer or median == maxAnswer it still a valid value and we don't have to revert

@serial-coder
Copy link
Member

serial-coder commented Mar 28, 2024

@00xSEV

The least reported price will be the pre-determined minAnswer. If you do not include the case ==, the _compareMinMax() will never enter the " if " case and revert the transaction. You cannot detect the black swan events without it.

The reported price will never be less than theminAnswer. (Without the ==, the if condition will always be false)

    function _compareMinMax(
        IAggregator _tokenAggregator,
        int192 _answer
    )
        internal
        view
    {
        int192 maxAnswer = _tokenAggregator.maxAnswer();
        int192 minAnswer = _tokenAggregator.minAnswer();

@>      if (_answer > maxAnswer || _answer < minAnswer) { //@audit -- The least reported price (i.e., `_answer`) will be the `minAnswer`. So, the function will never enter the " if " case
            revert OracleIsDead();
        }
    }

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Mar 28, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@c4-judge
Copy link
Contributor

trust1995 marked the issue as selected for report

@thebrittfactor
Copy link

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality M-06 🤖_04_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

10 participants