Open c4-bot-10 opened 3 months ago
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~
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.
thereksfour marked the issue as duplicate of #89
thereksfour marked the issue as unsatisfactory: Invalid
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.
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.
- 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
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.
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:
1 wei
token, the available amount will be correctly updated to 100
.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.
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.
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 thehourlyLimit
function, which will be50
in both cases regardless of if1 wei
was minted or not. ThecurrentlyAvailable
function caps it atlimit
regardless of the value oflastAvailable
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.
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.
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
thereksfour removed the grade
thereksfour changed the severity to QA (Quality Assurance)
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 netissuance
orredemption
of anRToken
does not exceed certain limits per hour. Correctly updating these values is essential to maintain the intendedissuance
andredemption
processes. The currenttotal supply
is crucial in calculating the available amount forissuance
andredemption
. Therefore, thethrottle
should be updated properly before thetotal supply
changes, such as duringissuance
andredemption
.However, there is an exception when dissolving
RTokens
duringrecollateralization
in theBackingManager
or meltingRTokens
. In this case, thethrottle
is not updated before thetotal supply
decreases. As a result, thethrottle
value becomes lower than it should be, potentially affecting futureissuance
andredemption
.Proof of Concept
When
issuing
orredeeming
RTokens
, the calculation of currently available amounts is crucial (line 121, 122
).The current
total supply
plays a key role in this process. Therefore, it's important to update thethrottle
before changing thetotal supply
. However, thethrottle
is not updated in thedissolve
andmelt
functions.Also the
throttle
is not updated in themint
function.Let's look at an example to understand the impact: Suppose the current
total supply
is100
, thepctRate
is10%
, and theamtRate
is1
. The last update occurred an hour ago and thethrottle.lastAvailable
is0
. In this case, thehourlyLimit
function would return10
.And the
currentlyAvailable
function would also return10
.Now, if the
melt
ordissolve
function is called, reducing thetotal supply
to20
, thehourlyLimit
function would then return2
, andcurrentlyAvailable
would also return2
for the nextissuance
andredemption
. This happens because we didn't updatethrottle.lastAvailable
using the originaltotal supply
before it was changed.Tools Used
Recommended Mitigation Steps
Add the following lines to the
dissolve
andmelt
functions.If we consider
dissolving
andmelting
as types ofredemption
, theamount
value can reference those values.Assessed type
Error