In function _payoutRewards, if the payout criteria are not met, payoutLastPaid should not be updated. However, the payoutLastPaid is actually updated even if the payout criteria are not met, which leads subsequent payouts to be calculated incorrectly.
Proof of Concept
In function _payoutRewards, the payout is done only if enough RSR (i.e. >= FIX_ONE) is staked. If not, the payout will not happen. The payoutLastPaid records the last time when rewards are paid out. Thus if the payout does not happen, payoutLastPaid should not be updated.
However, in the function _payoutRewards, the payoutLastPaid is always updated regardless of whether a payout happens or not. This is problematic if the totalStakes of the previous stakes are less than FIX_ONE. When the totalStakes after a new stake is greater than FIX_ONE, a payout will happen. At this time, the payoutLastPaid will be smaller than it should be. This leads to the payoutRatio and payout being smaller than they should be.
The payoutRatio is calculated as (1 - (1-rewardRatio)^numPeriods). As payoutLastPaid becomes larger, numPeriods becomes smaller, (1-rewardRatio)^numPeriods becomes larger, and therefore (1 - (1-rewardRatio)^numPeriods) becomes smaller.
// Function: StRSR.sol#_payoutRewards()
594: if (block.timestamp < payoutLastPaid + 1) return;
595:@> uint48 numPeriods = uint48(block.timestamp) - payoutLastPaid;
596:
597: uint192 initRate = exchangeRate();
598: uint256 payout;
599:
600: // Do an actual payout if and only if enough RSR is staked!
601: if (totalStakes >= FIX_ONE) {
602: // Paying out the ratio r, N times, equals paying out the ratio (1 - (1-r)^N) 1 time.
603: // Apply payout to RSR backing
604: // payoutRatio: D18 = FIX_ONE: D18 - FixLib.powu(): D18
605: // Both uses of uint192(-) are fine, as it's equivalent to FixLib.sub().
606:@> uint192 payoutRatio = FIX_ONE - FixLib.powu(FIX_ONE - rewardRatio, numPeriods);
607:
608: // payout: {qRSR} = D18{1} * {qRSR} / D18
609: payout = (payoutRatio * rsrRewardsAtLastPayout) / FIX_ONE;
610: stakeRSR += payout;
611: }
612:
613:@> payoutLastPaid += numPeriods;
614: rsrRewardsAtLastPayout = rsrRewards();
Lines of code
https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L594-L614
Vulnerability details
Impact
In function
_payoutRewards
, if the payout criteria are not met,payoutLastPaid
should not be updated. However, thepayoutLastPaid
is actually updated even if the payout criteria are not met, which leads subsequent payouts to be calculated incorrectly.Proof of Concept
In function
_payoutRewards
, the payout is done only if enough RSR (i.e. >= FIX_ONE) is staked. If not, the payout will not happen. ThepayoutLastPaid
records the last time when rewards are paid out. Thus if the payout does not happen,payoutLastPaid
should not be updated.However, in the function
_payoutRewards
, thepayoutLastPaid
is always updated regardless of whether a payout happens or not. This is problematic if thetotalStakes
of the previous stakes are less than FIX_ONE. When thetotalStakes
after a new stake is greater than FIX_ONE, a payout will happen. At this time, thepayoutLastPaid
will be smaller than it should be. This leads to thepayoutRatio
andpayout
being smaller than they should be.The
payoutRatio
is calculated as(1 - (1-rewardRatio)^numPeriods)
. AspayoutLastPaid
becomes larger,numPeriods
becomes smaller,(1-rewardRatio)^numPeriods
becomes larger, and therefore(1 - (1-rewardRatio)^numPeriods)
becomes smaller.https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L594-L614
Tools Used
VS Code
Recommended Mitigation Steps
Update
payoutLastPaid
only iftotalStakes >= FIX_ONE
.Assessed type
Math