In case a gauge weight reduction is performed via `GaugeController::change_gauge_weight`, it is possible that all functions will stop functioning permanently for that gauge

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/d6e6127e6763b93c23ee95cdf7622fe950d9ed30/src/GaugeController.sol#L189

Vulnerability details

Impact

If the weight of a given gauge has been manually changed using the GaugeController::change_gauge_weight function, all functions related to that gauge might become permanently DoSed

Proof of Concept

This issue has been reported in this and this report from the previous Code4rena contest of the codebase. However, by the looks of it, no fix has been applied in order to mitigate it, so it is still present.

Tools Used

Manual Review

Recommended Mitigation Steps

Whenever pt.slope is less than d.slope, set the value of pt.slope to 0:

        if (pt.bias > d_bias) {
            pt.bias -= d_bias;
            uint256 d_slope = changes_weight[_gauge_addr][t];
-           pt.slope -= d_slope;
+           if(pt.slope >= d.slope) {
+               pt.slope -= d_slope;
+           } else {
+               pt.slope = 0;
+           }
        } else {
            pt.bias = 0;
            pt.slope = 0;
        }

Assessed type

Under/Overflow

[c4-judge]

MarioPoneder marked the issue as primary issue

[zjesko]

mitigation PR:

https://github.com/mkt-market/canto-neofinance-coordinator/pull/17

[c4-sponsor]

zjesko (sponsor) confirmed

[MarioPoneder]

Assets not at direct risk, but the function of the protocol or its availability could be impacted.
change_gauge_weight is only callable by the governance. But no admin mistake is required for this to happen.

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[Arabadzhiew]

First of all, my apologies for not making this report showcase the full impact of the issue at question. I literary found the issue 30 minutes before the end of the contest, so I had to assemble the report in a matter of a few minutes.

This issue will actually lead to an absolutely certain loss of funds in the event where a gauge weight reduction is performed via GaugeController::change_gauge_weight. This is because inside of the LendingLedger::update_market function ,which is called inside of both LendingLedger::claim and LendingLedger::sync_ledger there is a call to GaugeController::gauge_relative_weight_write:

    market.accCantoPerShare += uint128(
        (blockDelta *
            cantoPerBlock[epoch] *
            gaugeController.gauge_relative_weight_write(_market, epochTime)) / marketSupply
    );

What this means is that since GaugeController::gauge_relative_weight_write calls GaugeController::_get_weight internally, whenever a gauge weight reduction is performed, all of those functions will become permanently DoSed, making it impossible for users to claim their rewards for a given market/gauge from the LendingLedger. Also, if the gauge that the weight reduction is performed on uses a LiquidityGauge, the users that have deposited into that liquidity gauge won't be able to withdraw their underlying assets from it, since in the LiquidityGauge::_afterTokenTransfer hook there are calls to LendingLedger::sync_ledger.

Because of that, I believe that this issue deserves to be judged as one of a high severity, rather than a medium.

[MarioPoneder]

Hey and thank you for your comment! I can definitely relate to your view from a personal perspective.

From a judging perspective my reasoning is as follows:
The original report, which is already quite minimalist (I acknowledge that you were under time pressure), states:

... , all functions related to that gauge might become permanently DoSed

Furthermore, we have a verdict about additional warden output during PJQA:

No new information should be introduced and considered in PJQA. Elaborations of the already introduced information can be considered (e.g. tweaking a POC), from either the Judge or the Warden, but they will only count towards the validity of the issue, not its quality score.

Although the underlying issue might qualify for High severity in case more elaboration of impacts and/or a PoC was provided in the original report, it seems to be fair and most in line with our present ruling to maintain Medium severity.

Thank you for your understanding!

[Arabadzhiew]

Thank you for the detailed response.

I have one final note to make. In this report from the previous audit of the codebase that I have linked to in my report, the following is stated in its impact section:

... the impact is that the entire gauge is useless, voting powers are permanently locked there and its weight is impossible to change, so the impact is high.

Given that this is actually a part of the original report and that issue #18 was judged as a high under pretty much the same reasoning, doesn't this issue also fall within the high severity category because of that?

[MarioPoneder]

Thanks for the follow-up, I can definitely relate to your point here!

About the reference to the High severity report, I treat it as a supporting reference but not as a "replacement" for your report.
Although the PoC and reasoning there might fully apply to this audit's code in scope, it's reasonable & fair to expect the present report to include impact-based reasoning for High severity and/or a coded PoC which undoubtedly applies to the current audit / code in scope.

In contrast, #18 clearly argues and proves in the original report that voting power will be lost/locked.

I understand if you have reservations about my judgement as this one is definitely a narrow path to walk. Nevertheless, your input is legit and appreciated.