Incorrect auction trade execution due to `issuancePremium` being applied in the `RecollateralizationLib.basketRange` computation

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/mixins/RecollateralizationLib.sol#L116 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L429 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/assets/RTokenAsset.sol#L66 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L446

Vulnerability details

Impact

The BasketHanlder.price(bool) function is used to return the price of a Basket Unit. The passed in boolean input parameter indicates whether to apply the issuance premium to the high price. The BasketHandler.price(bool) function is called in the RecollateralizationLib.basketRange function to determine the BasketRange.top and BasketRange.bottom values for a given TradingContext and AssetRegistry.

        (uint192 buPriceLow, uint192 buPriceHigh) = ctx.bh.price(false); 

As you can see when calculating the basket unit price the issuance premium is not applied (false is passsed in). But there is a scenario where the issuance premium is applied as explained below:

1. The Baket can include a different RTokenCollateral (Not the RToken which the current basket maps to) as one of its collaterals. This is because when the primary basket is set only collaterals which are not allowed are rsr, rToken and stRSR as verified in the BasketHandler.requireValidCollArray function.
2. During the BackingManager.rebalance function the RecollateralizationLib.prepareRecollateralizationTrade is called which inturn calls the BasketHandler.basketRange function. Here the ctx.bh.price(false) is called.
3. Now to determine the low and high price of the Basket Unit, the BasketHandler.price(bool) function iterates through all the collaterals in the basket and calculates the low and high price of each of the collaterals as shown below:

                (uint192 lowP, uint192 highP) = coll.price();

4. Since RTokenCollateral is also an active collateral of the Basket the RTokenAsset.price() is called which inturn calls the RTokenAsset.tryPrice function. In the tryPrice function the basketHandler.price(true) call is made which applies the issuance premium to the price of all of the collaterals which back the RTokenCollateral, as shown below:

        (uint192 lowBUPrice, uint192 highBUPrice) = basketHandler.price(true); 

5. As a result the RTokenCollateral price returned by the RTokenAsset.price() accounts for the issuancePremium if there is a depeg between the (tok/ref) of the respective collateral tokens of the RTokenCollateral. And this price which has the issuancePremium accounted for, is returned to the BasketHandler.price(bool) function.

6. Eventhough the quantity of the RTokenCollateral is not applied the issuancePremium (since it bool is false) its price is applied the issuancePremium as explained above. As a result when the price of the RTokenCollateral in the basket is calculated it includes the issuancePremium as shown below:

                    high256 += qty.safeMul(highP, CEIL);

7. Hence now high256 which is used to calculate the high value of the basket unit is now applied the issuancePremium for the RTokenCollateral which is not the intended behaviour since the basket unit price calculation didn't intend to include the issuancePremium for any of its collaterals since the bool false was passed in when calling the ctx.bh.price(false) via the RecollateralizationLib.basketRange function.

8. Since the issuancePremium is included in the above basket unit price calculation any oracle error (2%-3% either way) which occurs in savedPegPrice could result in errorneous issuancePremium calculation thus provding wrong (Higher or Lower than the accurate price) RTokenCollateral price. This will further extend to providing a wrong Basket Unit Price thus making the BasketRange.top and BasketRange.bottom values errorneous. As a result the trade.sellAmount and trade.buyAmount values of the TradeInfo struct will be inaccurate. As a result the triggerred auction trades in the BackingManager.rebalance function will have inaccurate trade parameters. This could lead to unintended behaviour such as trade auction not being initiated, basket not being properly rebalanced, trade being disadvantegeous to either the rsr staker or the RToken holder etc ...

9. Above issue can be aggravated if multiple RTokenCollaterals are used as active collaterals of a Basket, since issuancePremium could be applied multiple times for the respective RTokenCollaterals.

Proof of Concept

        (uint192 buPriceLow, uint192 buPriceHigh) = ctx.bh.price(false); // {UoA/BU}

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/mixins/RecollateralizationLib.sol#L116

                (uint192 lowP, uint192 highP) = coll.price();

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L429

        (uint192 lowBUPrice, uint192 highBUPrice) = basketHandler.price(true); // {UoA/BU}

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/assets/RTokenAsset.sol#L66

                    high256 += qty.safeMul(highP, CEIL);

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L446

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to ensure the uniformity of not including the issuancePremium when calculating the basket unit price when computing the basketRange. Hence it is recommended to ensure that RTokenAsset.tryPrice function calls the basketHandler.price function with bool false when the call is sent (msg.sender) from the BasketHandler contract. This will ensure that RTokenCollateral will not include the issuancePremium in its price calculation and as a result the basket unit price calculated in the BasketHandler.price() function will provide an accurate price value. This will in turn provide the expected BasketRange value without unexpected behavior.

Assessed type

Other