Seizure-triggered era reset will wipe out significant user holdings (`StRSRP1::seizeRSR`)

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L424-L479 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L671-L678 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L683-L690

Vulnerability details

Description

The StRSRP1 contract implements a staking mechanism for RSR tokens, allowing users to stake their tokens and earn rewards. The contract also manages a draft system for unstaking. A critical function in this contract is [seizeRSR()](https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L424-L479), which is designed to handle the seizure of RSR tokens in certain scenarios, such as when the system needs to recapitalize.

The seizeRSR() function adjusts the staking and drafting rates based on the amount of RSR seized. If these rates exceed predefined thresholds (MAX_STAKE_RATE and MAX_DRAFT_RATE), the function triggers a new era by calling beginEra() or beginDraftEra(), respectively. The intention behind this logic is to reset the system when there's a significant change in the staking or drafting dynamics.

However, this mechanism has a flaw. The decision to start a new era is based solely on the rate crossing a threshold, without considering the actual value held in stakes or drafts. This can lead to a scenario where a relatively small seizure pushes the rate just over the threshold, triggering a new era and wiping out all holdings, even if those holdings represent a significant value.

The root cause of this issue lies in the following code within the seizeRSR() function:

if (stakeRSR == 0 || stakeRate > MAX_STAKE_RATE) {
    seizedRSR += stakeRSR;
    beginEra();
}
// ... and similarly for drafts ...
if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) {
    seizedRSR += draftRSR;
    beginDraftEra();
}

This logic assumes that if the rate exceeds the threshold, there must not be much value left in stakes or drafts. However, this assumption doesn't hold in all scenarios, particularly when the rate is already close to the threshold before a seizure occurs.

Impact

If a new era is triggered when there's still substantial value held in stakes or drafts, all of these holdings will be wiped out. This not only causes direct financial harm to the users but also severely undermines trust in the protocol.

The likelihood of this occurring increases in volatile market conditions or when the protocol frequently needs to seize RSR.

Proof of Concept

Consider the following scenario:

1. The MAX_STAKE_RATE is set to 1e9 * FIX_ONE.
2. Due to previous market activities, the current stakeRate is at 9e8 * FIX_ONE, very close to the threshold.
3. Alice has staked 1,000,000 RSR tokens, which at the current rate represents a significant value.
4. The protocol needs to seize a relatively small amount of RSR, which increases the stakeRate to 1.01e9 * FIX_ONE.
5. This small increase triggers a new era in the seizeRSR() function:

if (stakeRate > MAX_STAKE_RATE) {
    seizedRSR += stakeRSR;
    beginEra();
}

6. The beginEra() function is called, which resets all stakes:

function beginEra() internal virtual {
    stakeRSR = 0;
    totalStakes = 0;
    stakeRate = FIX_ONE;
    era++;
    emit AllBalancesReset(era);
}

7. As a result, Alice loses her entire stake of 1,000,000 RSR tokens, despite the seizure being relatively small.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, you guys can implementing a more nuanced approach to triggering new eras. Instead of relying solely on rate thresholds, consider adding a value threshold alongside the rate threshold. This ensures that a new era is only triggered if both the rate exceeds the threshold AND the total value of stakes/drafts falls below a certain percentage of the total RSR.

Here's a potential code change to implement this suggestion ( just suggestion) :

diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol
index a1b2c3d..e4f5g6h 100644
--- a/contracts/p1/StRSR.sol
+++ b/contracts/p1/StRSR.sol
@@ -12,6 +12,9 @@ contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeable {
     uint192 private constant MAX_STAKE_RATE = 1e9 * FIX_ONE; // 1e9 D18{qStRSR/qRSR}
     uint192 private constant MAX_DRAFT_RATE = 1e9 * FIX_ONE; // 1e9 D18{qDrafts/qRSR}

+    // Add a new constant
+    uint256 private constant MIN_STAKE_VALUE_RATIO = 0.1e18; // 10% of total RSR
+
     function seizeRSR(uint256 rsrAmount) external {
         _requireNotTradingPausedOrFrozen();
         _notZero(rsrAmount);
@@ -23,13 +26,15 @@ contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeable {

         _payoutRewards();

+        uint256 totalRSR = rsr.balanceOf(address(this));
+
         // Remove RSR from stakeRSR
         uint256 stakeRSRToTake = (stakeRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
         stakeRSR -= stakeRSRToTake;
         seizedRSR = stakeRSRToTake;

         // update stakeRate, possibly beginning a new stake era
-        if (stakeRSR == 0 || stakeRate > MAX_STAKE_RATE) {
+        if (stakeRSR == 0 || (stakeRate > MAX_STAKE_RATE && stakeRSR < totalRSR * MIN_STAKE_VALUE_RATIO / FIX_ONE)) {
             seizedRSR += stakeRSR;
             beginEra();
         }
@@ -40,7 +45,7 @@ contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeable {
         seizedRSR += draftRSRToTake;

         // update draftRate, possibly beginning a new draft era
-        if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) {
+        if (draftRSR == 0 || (draftRate > MAX_DRAFT_RATE && draftRSR < totalRSR * MIN_STAKE_VALUE_RATIO / FIX_ONE)) {
             seizedRSR += draftRSR;
             beginDraftEra();
         }

This change ensures that a new era is only triggered if the stake rate exceeds the threshold AND the total staked RSR falls below 10% of the total RSR held by the contract. The exact percentage (MIN_STAKE_VALUE_RATIO) should be carefully chosen based on the Reserve's economics and risk tolerance (Up to you).

Assessed type

Other