howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

Users who stand to gain more distribution after the change could prevent distribution so they gain an unfair distribution.

Proof of Concept

Distributor.sol#L61-L71

function setDistribution(address dest, RevenueShare calldata share) external governance {
    // solhint-disable-next-line no-empty-blocks
    try main.rsrTrader().distributeTokenToBuy() {} catch {}
    // solhint-disable-next-line no-empty-blocks
    try main.rTokenTrader().distributeTokenToBuy() {} catch {}

    _setDistribution(dest, share);

    RevenueTotals memory revTotals = totals();
    _ensureSufficientTotal(revTotals.rTokenTotal, revTotals.rsrTotal);
}

When setting/updating distribution shares, the distributor attempts to distribute all pending tokens before making any changes. This ensures that tokens are distributed fairly according to the pre-update distributions.

When using try-catch, the default gas sent is 63/64 of the transaction gas. Although distribution is set by governance proposal, anyone can carry out the final execution. The result is that an interested party could provide just little enough gas to cause the distribution to trigger a OOG error but still have enough gas to finish updating the revenue shares. After the new distribution is in place they can distribute the token and benefit unfairly.

Tools Used

Manual review

Recommended Mitigation Steps

The try-catch pattern in setDistribution and setDistributions should be updated with the pattern that reverts on OOG errors.

Assessed type

Other

akshatmittal commented 3 months ago

Let's talk through the assumption for a second. There are two try-catch calls here, which means in order for both of them to revert yet the transaction to succeed, the first one will need to consume 63/64 and revert with oog and then second to consume 63/64 of the remaining and revert with oog. That seems like a lot to me, but I'm going to ignore it here for reasons mentioned below.
These functions are only a helper here. If we wanted to make sure the distribution happened before setting the distribution, we'd just use the same flow as settleTrade which disallows OOG reverts.
Extending the previous point, the function distributeTokenToBuy is itself is just a helper. The trades distribute automatically and this function only exists for when for some reason that didn't happen or more tokens were directly transferred to them.
Generally speaking, distributions in the protocol are treated as continous actions instead of one-shot. You'd see that the protocol's revenue accumulation is independent of its distribution and that's maintained throughout the flows in order to make sure several parts of the protocol can operate independently.
While we often do recommend Governance of RTokens to distribute before setting distributions, it's often just not necessary, required or wanted in the first place. Even for RTokens as big as eUSD, see this proposal for example.

thereksfour commented 3 months ago

And distributeTokenToBuy and distributeTokenToBuy do not depend on setDistribution being called, they can be called individually, which makes it difficult to accumulate a large number of undistributed tokens. will downgrade it to QA.

c4-judge commented 3 months ago

thereksfour changed the severity to QA (Quality Assurance)

c4-judge commented 2 months ago

thereksfour marked the issue as grade-a