Reward accounting functions still vulnerable to bypassing, leading to unintended reward distribution

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L594 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/Furnace.sol#L66 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/Distributor.sol#L194-L198

Vulnerability details

This issue is related to the TRST-M-4 finding from the Trust Security audit of v3.1.0 of the protocol. While a mitigation for the original issue was introduced, the current implementation still leads to reward accounting functions being bypassed under certain conditions.

The original TRST-M-4 finding highlighted that reward accounting functions were not called after sending reward tokens in BackingManager.forwardRevenue() and Distributor.distribute(). This could lead to unintended reward distribution due to outdated pending reward balances.

The team implemented a fix by calling the missing reward accounting functions. However, the current implementation of the reward distribution functions in the StRSRP1 and FurnaceP1 contracts lead to the issue only being mitigated under some conditions.

In the StRSRP1 contract, the _payoutRewards() function contains this check:

if (block.timestamp < payoutLastPaid + 1) return;

Similarly, in the FurnaceP1 contract, the melt() function has:

if (uint48(block.timestamp) < uint64(lastPayout + 1)) return;

These checks, while intended to prevent multiple reward calculations within the same block, can lead to bypassing of reward accounting if certain functions are called within the same block (technically, in the same second, as the block time on Arbitrum is 250 ms).

For instance, if stRSR.payoutRewards(), stake(), unstake(), or cancelUnstake(), or seizeRSR(), which all call _payoutRewards() internally, are executed in the same block, subsequent calls will not update the reward accounting variables. This will result in inaccurate reward calculations and distributions, to the same extent as the original TRST-M-4 issue. It will take until the next reward distribution for the additional rewards to be registered.

Impact

Inaccurate reward calculations and distributions for stakers and RToken holders. The impact of rewards not being accounted was shown in great detail in the original finding:

Let's consider two periods with meltRatio = 0.1

Scenario 1:
At time 0, lastPayoutBal = 100.
At time 0 + PERIOD, 100 RToken is distributed to Furnace and no action is taken.
At time 0 + PERIOD * 2, the number of RToken melted is (1 - (1 - 0.1) ** 2) * 100 = 19.
The total RToken melted are 19.

Scenario 2:
At time 0, lastPayoutBal = 100.
At time 0 + PERIOD, 100 RToken is distributed to Furnace, melt() is called, the number of RToken melted is (1 - (1 - 0.1) ** 1 ) * 100 = 10, and lastPayoutBal = 100 + 100 - 10 = 190.
At time 0 + PERIOD * 2, the number of RToken melted is (1 - (1 - 0.1) ** 1) * 190 = 19.
The total RToken melted are 29.

Where PERIOD is now effectively 1s. In this case, we have:

Scenario 3:
At time 0, lastPayoutBal = 100.
At time 0 + PERIOD, 100 RToken is distributed to Furnace but melt() is called before this happens. The number of RToken melted is (1 - (1 - 0.1) ** 1 ) * 100 = 10, and lastPayoutBal = 100 - 10.
At time 0 + PERIOD * 2, the number of RToken melted is (1 - (1 - 0.1) ** 1) * 90 = 9, and lastPayoutBal = 90 + 100 - 9 = 181.
The total RToken melted are 19.

Proof of Concept

1. A user calls stRSR.stake()
2. In the same block, Distributor.distribute() is called, sending a large amount of rewards to StRSR and calling stRSR.payoutRewards()
3. The second transaction will not update the reward accounting variables, leading to an incorrect distribution of rewards

Tools Used

Manual review

Recommended Mitigation Steps

To fully address the issue, consider updating the relevant accounting variables even when multiple calls occur within the same block. For the StRSRP1 contract:

function _payoutRewards() internal {
    if (block.timestamp < payoutLastPaid + 1) {
        rsrRewardsAtLastPayout = rsrRewards();
        return;
    }
    // ... rest of the function
}

For the FurnaceP1 contract:

function melt() public {
    if (uint48(block.timestamp) < uint64(lastPayout + 1)) {
        lastPayoutBal = rToken.balanceOf(address(this));
        return;
    }
    // ... rest of the function
}

Assessed type

Other

[akshatmittal]

This is intentional, as in the rewards are supposed to start at the next accounting period and should not account for new rewards in the current period. Essentially, the rewards start from when the accounting begins, and we ensure that rewards are paid out in distinct periods. (which is also why payoutRewards returns and not reverts)

It will take until the next reward distribution for the additional rewards to be registered.

This is exactly the outcome we intended with it, and also why the first reward distribution begins after the first unstake/stake action post reward accumulation.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid