In case Distributor.setDistribution use, revenue from rToken RevenueTrader and rsr token RevenueTrader should be distributed

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Distributor.sol#L61-L65

Vulnerability details

Impact

In case Distributor.setDistribution use, revenue from rToken RevenueTrader and rsr token RevenueTrader should be distributed. Otherwise wrong distribution will be used.

Proof of Concept

BackingManager.forwardRevenue function sends revenue amount to the rsrTrader and rTokenTrader contracts, according to the distribution inside Distributor contract. For example it can 50%/50%. In case if we have 2 destinations in Distributor: strsr and furnace, that means that half of revenue will be received by strsr stakers as rewards.

This distribution can be changed at any time. The job of RevenueTrader is to sell provided token for a tokenToBuy and then distribute it using Distributor.distribute function. There are 2 ways of auction that are used: dutch and gnosis. Dutch auction will call RevenueTrader.settleTrade, which will initiate distribution. But Gnosis trade will not do that and user should call distributeTokenToBuy manually, after auction is settled.

The problem that i want to discuss is next. Suppose, that governance at the beginning set distribution as 50/50 between 2 destinations: strsr and furnace. And then later forwardRevenue sent some tokens to the rsrTrader and rTokenTrader. Then, when trade was active to exchange some token to rsr token, Distributor.setDistribution was set in order to make strsr share to 0, so now everything goes to Furnace only. As result, when trade will be finished in the rsrTrader and Distributor.distribute will be called, then those tokens will not be sent to the strsr contract, because their share is 0 now. They will be stucked inside rsrTrader.

Another problem here is that strsr holders should receive all revenue from the time, where they distribution were created. What i mean is if in time 0, rsr share was 50% and in time 10 rsr share is 10%, then BackingManager.forwardRevenue should be called for all tokens that has surplus, because if that will be done after changing to 10%, then strsr stakers will receive less revenue.

Tools Used

VsCode

Recommended Mitigation Steps

You need to think how to guarantee fair distribution to the strsr stakers, when distribution params are changed.

Assessed type

Error

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[tbrent]

This is a good find. The mitigation we have in mind is adding a new function to the RevenueTrader that allows anyone to transfer a registered ERC20 back to the BackingManager, as long as the current distribution for that tokenToBuy is 0%.

[c4-judge]

0xean marked the issue as satisfactory