BackingManager can seize more RSR than intended due to calculation error (`StRSRP1::seizeRSR()`)

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

The StRSRP1 contract contains a critical vulnerability in its seizeRSR() function that allows the BackingManager to seize more RSR than intended. This function is designed to allow the BackingManager to seize a specified amount of RSR from the contract, adjusting the stakeRSR and draftRSR balances accordingly. However, the current implementation uses the total RSR balance of the contract (rsrBalance) in its calculations, which can be manipulated to skew the results.

The core issue lies in the calculation of stakeRSRToTake and draftRSRToTake. These values are computed using the following formulas:

File: StRSR.sol
441:         uint256 stakeRSRToTake = (stakeRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
456:         uint256 draftRSRToTake = (draftRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;

The problem arises because rsrBalance includes the amount of RSR that is being seized (rsrAmount). This means that if an attacker were to transfer additional RSR to the contract immediately before calling seizeRSR(), they could artificially inflate rsrBalance, leading to an incorrect calculation of the amounts to be seized.

For example, if the contract initially has 1000 RSR, and the BackingManager attempts to seize 100 RSR, the calculation would be based on a total balance of 1000 RSR. However, if an attacker transfers an additional 9000 RSR to the contract just before the seizure, the calculation would now be based on a total balance of 10,000 RSR, significantly altering the proportion of RSR seized from each pool.

Impact

It allows the BackingManager to seize more RSR than intended, which can lead to significant financial loss and instability in the system. An attacker could manipulate the rsrBalance by transferring RSR to the contract just before calling seizeRSR(), leading to incorrect calculations and potentially allowing the BackingManager to seize more RSR than intended.

Proof of Concept

1. Initially, the contract has 1000 RSR total, with 500 in stakeRSR and 500 in draftRSR.
2. The BackingManager prepares to seize 100 RSR by calling seizeRSR(100).
3. Just before the transaction is processed, an attacker transfers 9000 RSR to the contract.
4. The seizeRSR() function calculates stakeRSRToTake as: (500 * 100 + 9999) / 10000 = 55 (rounded up)
5. Similarly, draftRSRToTake is calculated as 55.
6. The total seized amount is 110 RSR, which is more than the intended 100 RSR.
7. The remaining balances are skewed: stakeRSR = 445, draftRSR = 445, instead of the expected 450 each.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this vulnerability, the calculation of stakeRSRToTake and draftRSRToTake should be based on the proportion of rsrAmount to the total RSR in the contract before the seizure, rather than including rsrAmount in the denominator. Here's the recommended fix:

function seizeRSR(uint256 rsrAmount) external {
    // ... (existing checks)

    uint256 rsrBalance = rsr.balanceOf(address(this));
    require(rsrAmount <= rsrBalance, "seize exceeds balance");

    uint256 preSeizureBalance = rsrBalance - rsrAmount;

-   uint256 stakeRSRToTake = (stakeRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
+   uint256 stakeRSRToTake = (stakeRSR * rsrAmount) / preSeizureBalance;

    // ... (update stakeRSR and other variables)

-   uint256 draftRSRToTake = (draftRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
+   uint256 draftRSRToTake = (draftRSR * rsrAmount) / preSeizureBalance;

    // ... (update draftRSR and other variables)

-   seizedRSR += (rewards * rsrAmount + (rsrBalance - 1)) / rsrBalance;
+   seizedRSR += (rewards * rsrAmount) / preSeizureBalance;

    // ... (remaining function logic)
}

This change ensures that the calculation is based on the pre-seizure balance, preventing manipulation through last-minute RSR transfers to the contract.

Assessed type

Other

[tbrent]

To mitigate this vulnerability, the calculation of stakeRSRToTake and draftRSRToTake should be based on the proportion of rsrAmount to the total RSR in the contract before the seizure

This is how it's currently coded.

The provided mitigation code is the one that contains the bug of dividing by the post-seizure amount. The preSeizureBalance variable is definitely the balance after the seizure.

[tbrent]

I believe line 4 in the PoC is the incorrect one.

The seizeRSR() function calculates stakeRSRToTake as: (500 * 100 + 9999) / 10000 = 55 (rounded up)

No: (500 * 100 + 9999) / 10000 = 5.9999 = 5 (rounded down)

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid