Lack of claimRewards when manageToken in RevenueTrader

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L83-L104

Vulnerability details

There is a dev comment in the Assert.sol:

DEPRECATED: claimRewards() will be removed from all assets and collateral plugins

The claimRewards is moved to the TradingP1.claimRewards/claimRewardsSingle.

But when the RevenueTraderP1 trade and distribute revenues by manageToken, it only calls the refresh function of the asserts:

if (erc20 != IERC20(address(rToken)) && tokenToBuy != IERC20(address(rToken))) {
    IAsset sell_ = assetRegistry.toAsset(erc20);
    IAsset buy_ = assetRegistry.toAsset(tokenToBuy);
    if (sell_.lastSave() != uint48(block.timestamp)) sell_.refresh();
    if (buy_.lastSave() != uint48(block.timestamp)) buy_.refresh();
}

The claimRewards is left out.

Impact

Loss a part of rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Add claimRewardsSingle when refresh assert in the manageToken.

Assessed type

Context

[tbrent]

This is similar to an (unmitigated) issue from an earlier contest: https://github.com/code-423n4/2023-02-reserve-mitigation-contest-findings/issues/22

However in this case it has to do with RevenueTraderP1.manageToken(), as opposed to BackingManagerP1.manageTokens().

I think that difference matters, because the loss of the rewards for this auction does not have serious long-term consequences. This is not like the BackingManager where it's important that all capital always be available else an unnecessarily large haircut could occur. Instead, the worst that can happen is for the revenue auction to complete at high slippage, and for a second reward token revenue auction to complete afterwards at high slippage yet again, when it could have been a single revenue auction with less slippage.

[tbrent]

The recommended mitigation would not succeed, because recall, we may be selling token X but any number of additional assets could have token X as a reward token. We would need to call claimRewards(), which is simply too gas-costly to do everytime for revenue auctions.

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[0xean]

Instead, the worst that can happen is for the revenue auction to complete at high slippage, and for a second reward token revenue auction to complete afterwards at high slippage yet again, when it could have been a single revenue auction with less slippage.

The impact sounds like a "leak of value" and therefore I think M is the correct severity per the c4 docs. (cc @tbrent - open to additional comment here)

[c4-judge]

0xean marked the issue as satisfactory