Inflated rsrTotal Due to Unchecked DAO Fee Calculation in totals() Function

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Vulnerability Details

The totals() function is responsible for calculating the total shares for RSR and RToken distribution. It's a critical component for ensuring proper revenue distribution and maintaining system invariants.

The issue is in the DAO fee calculation part of the function. The current implementation can lead to an inflation of the rsrTotal, potentially breaking system invariants and causing unexpected behavior in revenue distribution.

Details

1. The function calculates base totals correctly.
2. When a DAO fee is present, it adds an additional amount to rsrTotal.
3. This addition can cause the total distribution to exceed 100% (or MAX_DISTRIBUTION).
4. The function doesn't adjust other distributions to accommodate the DAO fee, leading to an inflated total.

Code Snippet

if (feeRecipient != address(0) && feeNumerator != 0) {
    revTotals.rsrTotal += uint24(
        (feeNumerator * uint256(revTotals.rTokenTotal + revTotals.rsrTotal)) /
        (feeDenominator - feeNumerator)
    );
}

Impact

1. Violation of system invariants: The total distribution may exceed 100%.
2. Incorrect revenue distribution: Other recipients may receive less than their intended share.
3. Potential for system instability or unexpected behavior in other components that rely on these totals.

Scenario

Suppose the initial distribution is:

• RToken: 50%
• RSR: 50%
• DAO Fee: 10%

The totals() function will return:

• RToken: 50%
• RSR: 55.55% (50% + 5.55% DAO fee)

Total distribution: 105.55%, which exceeds 100%.

Fix

To address this issue, the function should be modified to incorporate the DAO fee without inflating the total distribution. Here's a potential fix:

1. Calculate the DAO fee separately.
2. Adjust the RSR and RToken totals proportionally to accommodate the DAO fee.
3. Ensure the final total (including the DAO fee) equals MAX_DISTRIBUTION.

Example implementation:

function totals() public view returns (RevenueTotals memory revTotals) {
    // Calculate base totals as before
    ...

    DAOFeeRegistry daoFeeRegistry = main.daoFeeRegistry();
    if (address(daoFeeRegistry) != address(0)) {
        (address feeRecipient, uint256 feeNumerator, uint256 feeDenominator) = main.daoFeeRegistry().getFeeDetails(address(rToken));

        if (feeRecipient != address(0) && feeNumerator != 0) {
            uint256 totalBeforeFee = revTotals.rTokenTotal + revTotals.rsrTotal;
            uint256 daoFee = (totalBeforeFee * feeNumerator) / feeDenominator;

            // Adjust RSR and RToken totals proportionally
            revTotals.rsrTotal = uint24((uint256(revTotals.rsrTotal) * (totalBeforeFee - daoFee)) / totalBeforeFee);
            revTotals.rTokenTotal = uint24((uint256(revTotals.rTokenTotal) * (totalBeforeFee - daoFee)) / totalBeforeFee);

            // Add DAO fee to RSR total
            revTotals.rsrTotal += uint24(daoFee);
        }
    }

    require(revTotals.rsrTotal + revTotals.rTokenTotal == MAX_DISTRIBUTION, "Invalid total distribution");
}

This fix ensures that the total distribution always equals MAX_DISTRIBUTION, maintaining system invariants and providing a more accurate representation of revenue shares.

Assessed type

Context

[tbrent]

require(revTotals.rsrTotal + revTotals.rTokenTotal == MAX_DISTRIBUTION, "Invalid total distribution");

This is not an invariant of the protocol.

The Distributor table does not need to sum to 10000 (100%). This is documented here as the invariant

sum across destinations must be at least MAX_DISTRIBUTION

[akshatmittal]

Explicitly saying, they are ratios but need to be a minimum of MAX_DISTRIBUTION, they do not need to sum to it!

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid