Improper Handling of Price Divergence

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/main/src/mcag-contracts/MCAGAggregator.sol#L53-L55

Vulnerability details

Impact

The transmit function of MCAGAggregator enforces a _maxAnswer value limitation on the input answer, reverting if this limitation is not met. This type of behaviour is ill-advised as a significant inflationary event can cause the central bank rates to fluctuate significantly. The contract is meant to operate on multiple markets and we have observed a significant inflationary event as recent as the Venezuelan Bolivar's 2021 collapse.

The system is presently ill-equipped to deal with such an event, containing no fall-back mechanism if the price ceiling is met and instead reverting the transaction. In turn, this will cause the contract to serve an outdated rate and thus significantly compromise the economical model of the Kuma Protocol.

An example of such behaviour can be observed with the LUNA collapse whereby Chainlink stopped reporting prices to its oracles and in turn, caused an 11 million USD loss to Venus Protocol due to the latter using an outdated price rather than halting.

Proof of Concept

The highlighted lines of code sufficiently illustrate the issue.

Tools Used

Manual review.

Recommended Mitigation Steps

We advise the code to instead assign the maximum value permitted to the rate served by the aggregator. In turn, all system modules integrating with the aggregator should treat an answer equivalent to maxAnswer as a flag indicating that the system should automatically pause itself. A preemptive pause mechanism is much more desirable than inexistent handling of abnormal market events which in a project's lifetime can and have historically occured.

[GalloDaSballo]

https://github.com/code-423n4/2023-02-kuma/blob/22fd56b3f0df71714cb71f1ce2585f1c4dd21d64/src/mcag-contracts/MCAGAggregator.sol#L69-L72

    function setMaxAnswer(int256 newMaxAnswer) external onlyRole(Roles.MCAG_MANAGER_ROLE) {
        emit MaxAnswerSet(_maxAnswer, newMaxAnswer);
        _maxAnswer = newMaxAnswer;
    }

Technically this can be configured when necessary

I believe this to be valid, but due to multiple external requirements, as well as possibility to remedy am thinking of QA Severity

[c4-sponsor]

m19 marked the issue as disagree with severity

[m19]

We think this submission is not wrong per se but we feel it's of low risk to us as we're not dealing with LUNA nor the Venuzalian Bolivar. But fair enough, this is not enforced in code and is fully up to the DAO on which bonds to support. There is indeed a max answer in the aggregator itself but the systems using the aggregator treat a max answer the same as any answer, we will consider making changes here.

[GalloDaSballo]

Per the discussion above, I believe the finding to be valid, but because there are ways to directly deal with that issue, I think Low Severity to be most appropriate

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-a

[c4-judge]

GalloDaSballo marked the issue as selected for report

[c4-judge]

GalloDaSballo marked the issue as not selected for report