code-423n4 / 2024-09-reserve-mitigation-findings

0 stars 0 forks source link

M-05 Unmitigated #16

Closed c4-bot-1 closed 1 month ago

c4-bot-1 commented 1 month ago

We found the finding M-05: Users can dodge losses due to StRSR era changes with instant operations only partially mitigated.

The root cause of the original finding is that:

Both these aspects are still present in the code. We acknowledge that allowing governance to reset both rates together decreases the likelihood of a single bucket resetting without the other. However, the reason why this finding was judged as Medium severity, that is, quoting the judge:

[...] users can frontrun resetStakes to avoid losses or get benefits. if this is correct, I would consider this issue to be low likelihood + high impact = medium severity

still applies in full. The likelihood is lowered, but the fix relies solely on a Governance action, which may not be able to come in time due to its organic voting period and subsequent timelock execution.

For the issue to be mitigated in full, we recommend changing the seizeRSR logic to reset both rates whenever one passes its safe zone: this way, even if Governance doesn't (or can't) act with a resetStakes call, the attack is never possible.

tbrent commented 1 month ago

This is intentional. The change to seizeRSR was considered and we decided against making the change in favor of the lighterweight governance solution, despite it not being sufficient in all cases.

More detail on our thinking:

Also, it's actually not very obvious how to safely change seizeRSR() to reset both rates together. The breakdown occurs due to the draftRSR == 0 case, which is currently used as an indicator of an extreme rate since the rate is technically undefined.

For example: if draftRSR is 1 wei before the seizure, then any seizure amount will wipe out the drafts, and if we let it, the stakes too.

        // update draftRate, possibly beginning a new draft era
        if (draftRSR != 0 && totalDrafts != 0) {
            // Downcast is safe: totalDrafts is 1e38 at most so expression maximum value is 1e56
            draftRate = uint192((FIX_ONE_256 * totalDrafts + (draftRSR - 1)) / draftRSR);
        }
        if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) {
            seizedRSR += draftRSR;
            beginDraftEra();
        }

I won't claim there doesn't exist some way to change seizeRSR() to deal with this, but it was at this point that we decided the change introduces more danger than it removes. Meanwhile, the resetStakes() function is most likely sufficient, as the cases where lots of value could be gained by a dodging attacker are also the cases where resetStakes() could be used, because the rate was already nearby the 1e9 bounds.

tbrent commented 1 month ago

In short, we think the mitigation we have applied changes the issue from being LOW likelihood and HIGH impact, to LOW(er) likelihood and MEDIUM impact.

Under an assumption of responsible governance, the amount of value the attacker can get becomes bounded at 1e-3 of their holdings, and the impact is mitigated.

The exception to this is a cascade of seizures that does not permit governance action due to time delays, but this is a lower overall likelihood than the likelihood of the original issue.

c4-judge commented 1 month ago

thereksfour marked the issue as nullified