Open c4-bot-6 opened 5 months ago
Off by one are QA per SC
GalloDaSballo marked the issue as insufficient quality report
trust1995 changed the severity to QA (Quality Assurance)
trust1995 marked the issue as grade-c
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 https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/18. As you can see, their recommendation includes the edge cases _answer
== minAnswer
and _answer
== maxAnswer
.
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!
This previously downgraded issue has been upgraded by trust1995
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.
@trust1995
Thanks for your judge!
But did you forget to remove the tags: unsatisfactory
, grade-c
, and insufficient quality report
?
@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
* @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
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
@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();
}
}
trust1995 marked the issue as satisfactory
trust1995 marked the issue as selected for report
For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.
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 theOracleHelper::_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-determinedminAnswer
instead of the actual price. In other words, the possible minimum value of the_answer
would beminAnswer
.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.Tools Used
Manual Review
Recommended Mitigation Steps
Add the cases
_answer
==minAnswer
and_answer
==maxAnswer
like the snippet below.Assessed type
Oracle