Shouldn't sell reward rtokens when basket is undercollateralized

[code423n4]

Lines of code

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

Vulnerability details

RevenueTraderP1.manageToken will sell the erc20 tokens for tokenToBuy. In the main.rsrTrader, the token flow is selling RToken, buying RSR.

But when the basket is undercollateralized, the RTokenAsset.price() will give a lower price for the basketRange.bottom, which is lower than the pirce of BU.

function tryPrice() external view virtual returns (uint192 low, uint192 high) {
    ...
    BasketRange memory range = basketRange(); // {BU}

    // {UoA/tok} = {BU} * {UoA/BU} / {tok}
    low = range.bottom.mulDiv(lowBUPrice, supply, FLOOR);
    high = range.top.mulDiv(highBUPrice, supply, CEIL);

RTokenAsset.basketRange

function basketRange() private view returns (BasketRange memory range) {
    ...
    if (basketsHeld.bottom >= basketsNeeded) {
        range.bottom = basketsNeeded;
        range.top = basketsNeeded;
    } else {
        // here is undercollateralized
        range = RecollateralizationLibP1.basketRange(ctx, reg);
    }

However when the trader sold the rtoken at a lower price and sent the purchased RSR to the stRSR, the RToken price will increase because of the overcollateralization effects of the new RSR tokens.

It very inefficient and results in lost of exchange rate.

Impact

RToken has to take more haircut.

Proof of Concept

Assumption:

1. There is a simple basket with single collateral, aUSDC.
2. 90 Rtoken hold by users and 100 aUSDC in BasketManager
3. 10 Rtoken sent to rsrTrader to sold for rsr
4. 1 rsr = 1 USDC
5. stake 20 rsr

And at present, aUSDC is cut down to 0.5 USDC.

The low price of RToken now is about (50+20)/(90+10) = 0.7 USDC.

The trader sold 10 RToken at this price will get 10*0.7= 7 RSR back.

Now the low price of RToken is (50+27)/(90+10) = 0.77. The basketsNeeded will be haircut to 77 BU.

But if the 10 Rtokens are sent back to the BasketManager and burn for rebalance, the low price of RToken will be (50+20)/90 = 0.77777777. The basketsNeeded only needs to be haircut to 77.77 BU. One percent increases.

Tools Used

Manual review

Recommended Mitigation Steps

Assessed type

Context

[0xean]

This issue reads more like a design suggestion than a vulnerability. Will leave open for sponsor review, but probably should be QA.

[tbrent]

This is not related to RToken haircut, since the issue being pointed out occurs in the rsrTrader. This means these token balances are already revenue.

However, it is true that the rsrTrader does not take into account RToken status or collateralization, and this could cause it to receive fewer RSR vs if it had just waited. It depends on how the broader market prices the RTokens in that instant.

[tbrent]

Ah, I see I misunderstood this. Will continue to consider.

[tbrent]

Isn't the falling price auction of DutchTrade or the batch auction of GnosisTrade sufficient to find the right clearing price in this case? It would be an issue if the price of the buy token were being underestimated, but the auction mechanisms are built to handle cases where the sell token price is underestimated. I think this is QA.

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[c4-judge]

0xean changed the severity to QA (Quality Assurance)