The throttle may be incorrectly updated when dissolving or melting rTokens

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L121-L122
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L387-L391
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/libraries/Throttle.sol#L80-L90
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/libraries/Throttle.sol#L69-L77

Vulnerability details

Impact

The throttle is designed to ensure that the net issuance or redemption of an RToken does not exceed certain limits per hour.
Correctly updating these values is essential to maintain the intended issuance and redemption processes.
The current total supply is crucial in calculating the available amount for issuance and redemption.
Therefore, the throttle should be updated properly before the total supply changes, such as during issuance and redemption.

However, there is an exception when dissolving RTokens during recollateralization in the BackingManager or melting RTokens.
In this case, the throttle is not updated before the total supply decreases.
As a result, the throttle value becomes lower than it should be, potentially affecting future issuance and redemption.

Proof of Concept

When issuing or redeeming RTokens, the calculation of currently available amounts is crucial (line 121, 122).

function issueTo(address recipient, uint256 amount) public notIssuancePausedOrFrozen {
    require(amount != 0, "Cannot issue zero");
    assetRegistry.refresh();
    address issuer = _msgSender(); // OK to save: it can't be changed in reentrant runs
    require(basketHandler.isReady(), "basket not ready");
    uint256 supply = totalSupply();

121:    issuanceThrottle.useAvailable(supply, int256(amount)); // reverts on over-issuance
122:    redemptionThrottle.useAvailable(supply, -int256(amount)); // shouldn't revert
}

The current total supply plays a key role in this process.
Therefore, it's important to update the throttle before changing the total supply.
However, the throttle is not updated in the dissolve and melt functions.

function dissolve(uint256 amount) external {
    address caller = _msgSender();
    require(caller == address(backingManager), "not backing manager");
    _scaleDown(caller, amount);
}

Also the throttle is not updated in the mint function.

Let's look at an example to understand the impact:
Suppose the current total supply is 100, the pctRate is 10%, and the amtRate is 1.
The last update occurred an hour ago and the throttle.lastAvailable is 0.
In this case, the hourlyLimit function would return 10.

function hourlyLimit(Throttle storage throttle, uint256 supply)
    internal
    view
    returns (uint256 limit)
{
    Params storage params = throttle.params;

    limit = (supply * params.pctRate) / FIX_ONE_256; // 100 * 0.1 = 10
    if (params.amtRate > limit) limit = params.amtRate; // 10 = max(10, 1)

}

And the currentlyAvailable function would also return 10.

function currentlyAvailable(Throttle storage throttle, uint256 limit)
    internal
    view
    returns (uint256 available)
{
    uint48 delta = uint48(block.timestamp) - throttle.lastTimestamp; // 1 hour
    available = throttle.lastAvailable + (limit * delta) / ONE_HOUR; // 0 + 10 = 10
    if (available > limit) available = limit; // 10
}

Now, if the melt or dissolve function is called, reducing the total supply to 20, the hourlyLimit function would then return 2, and currentlyAvailable would also return 2 for the next issuance and redemption.
This happens because we didn't update throttle.lastAvailable using the original total supply before it was changed.

Tools Used

Recommended Mitigation Steps

Add the following lines to the dissolve and melt functions.

uint256 supply = totalSupply();
uint256 amount = 0;
issuanceThrottle.useAvailable(supply, -int256(amount));
redemptionThrottle.useAvailable(supply, int256(amount));

If we consider dissolving and melting as types of redemption, the amount value can reference those values.

Assessed type

Error

[tbrent]

dupe with #89

Think the correct mitigation is the one in 89, not here; melting/dissolving should not consume the throttle itself, it should just update it based on issuance/redemption

[akshatmittal]

Going back at this.

The throttle is supposed to apply to external interactions not internal ones, and the throttle works correctly in all of those cases.

You can obviously see people making arguments in both directions on when should and should not the throttle apply, which bring it down to the goals of the throttle in the first place.

[c4-judge]

thereksfour marked the issue as duplicate of #89

[c4-judge]

thereksfour marked the issue as unsatisfactory:
Invalid

[etherSky111]

Hi @thereksfour , @tbrent , @akshatmittal ,
Thanks for your review.

The following code does not mean to use the throttle for internal operations like minting, melting, dissolving, etc.:

issuanceThrottle.useAvailable(totalSupply(), 0);

This line simply updates the available amount at this point using the current total supply.

The available amount is directly related to the total supply.

function hourlyLimit(Throttle storage throttle, uint256 supply)
    internal
    view
    returns (uint256 limit)
{
    Params storage params = throttle.params;

    // Calculate hourly limit as: max(params.amtRate, supply.mul(params.pctRate))
    limit = (supply * params.pctRate) / FIX_ONE_256; // {qRTok}
    if (params.amtRate > limit) limit = params.amtRate;
}

For example, if the current total supply is 1,000 and the pctRate is 10%, and if more than one hour has passed since the last update, the available amount should be 100.
If the melt or dissolve function reduces the total supply to 500, then the available amount for the next issuance would be 50.

It is normal to update the current available amount before changing the total supply, as the available amount strictly depends on it.
We are not suggesting using the throttle in these functions, only updating the current available amount at this point.

Especially, I suggested below.

uint256 supply = totalSupply();
uint256 amount = 0;
issuanceThrottle.useAvailable(supply, -int256(amount));
redemptionThrottle.useAvailable(supply, int256(amount));

If we consider dissolving and melting as types of redemption, the amount value can reference those values.

A non-zero amount would mean using the throttle for these operations, but using 0 is just for updating the current available amount at this point using current total supply.

I would appreciate it if you could reconsider this issue.

[thereksfour]

Warden described that the available amount is incorrect because useAvailable() is not called to update lastAvailable before the RToken totalSupply is changed (before calling dissolve/melt/mint, etc.).
Another example is in #89, the focus is on step 7. Because lastAvailable is not updated, the protocol mistakenly uses the current totalSupply to calculate the available amount of the previous hour, even though the totalSupply for most of the previous hour was smaller.

7. At t = 3600, A user can calls issueTo to issue RToken with amount = 200.
   Since totalSupply = 1000 from t = 0 to t = 3599 and totalSupply = 2000 from t = 3599 to t = 3600, the hourly limit of issuance should be (1000 * 10% * 3599 + 2000 * 10% * 1) / 3600 = 100.
   However, the user issued 200 tokens in 1 hour which exceeds 100.

@akshatmittal and @tbrent, Please reconsider it, I think it's valid M

[akshatmittal]

This is still incorrect @thereksfour. The throttle limits what you can do in the future, not in any sliding window or the past. The behaviour is correct at the moment.

[etherSky111]

Thank you for your comments.

I understand that you believe this behavior is intentional.
However, as a warden, I still see it as an issue.
Let me expand on my previous example:

Suppose the current total supply is 1,000, the pctRate is 10%, and more than one hour has passed since the last update.
The available amount should be 100.
Now, let’s say the total supply is reduced to 500 by calling the melt or dissolve functions.
Here’s the problem:

• If any user deposits or redeems just 1 wei token, the available amount will be correctly updated to 100.
• However, if no update occurs before the total supply changes, the last available amount would be 50, which could negatively affect future users.

The throttle states are changing a lot even with minor actions like depositing just 1 wei.
While you might consider this acceptable as part of the design, we wardens still see it as an issue.

If this behavior had been listed as a known issue, we would accept it.

[akshatmittal]

Are you simply talking about the lastAvailable variable that we store? It's irrelevant here since the limit is defined by the hourlyLimit function, which will be 50 in both cases regardless of if 1 wei was minted or not. The currentlyAvailable function caps it at limit regardless of the value of lastAvailable variable.

[etherSky111]

I am not sure that PJQA has ended or not.
However, I think the answer is needed for above question.

Are you simply talking about the lastAvailable variable that we store? It's irrelevant here since the limit is defined by the hourlyLimit function, which will be 50 in both cases regardless of if 1 wei was minted or not. The currentlyAvailable function caps it at limit regardless of the value of lastAvailable variable.

No, I am not simply talking about the lastAvailable variable.
I hope this will be my last comment.
So far, I just showed that the hourly limit is strictly based on current total supply, but at this time I think that I need to provide more specific example.

Suppose the current total supply is 1000 and pctRate is 10% and amtRate is 50 and 30 mins have passed after last update.
- The first scenario
someone mints 1 wei before chaning the total supply to 500.
The hourlyLimit function will return 100. (max(1000 * 10%, 50))
The currentlyAvailable function will return 50. (30 mins * 100 / 1 hour) and this value will be stored in lastAvailable value.
The total supply changes to 500.
At this point, the available amount is still 50 as this is less than current hourlyLimt 50. (max(50, 500 * 10%))

- The second scenario
The total supply changes to 500 without minting 1 wei (i.e. no snapshot total supply)
The hourlyLimit function returns 50. (max(50, 500 * 10%))
The currentlyAvailable returns 25. (50 * 30 mins / 1 hour)
As a result, the available amount is 25 for the new depositors

This is just simple example to show that the total supply affects the throttle directly.

[akshatmittal]

Thanks for the comment @etherSky111! I clearly misunderstood your comment before, thanks for clarifying. Will come back to you once I'm able to think about this more and validate it.

[thereksfour]

After discussions with sponsors, we think this finding is valid.
But considering the mint()/melt()/dissolve() events would be very small sizes and have very little impact, so this finding will be considered as L.

However, all the examples (mine; warden's) involve absurd assumptions about the size of mint()/melt()/dissolve() events. In practice the differences between the two approaches are negligible in the expected case

[c4-judge]

thereksfour removed the grade

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)