in BackingManagerP1::rebalance Wrong assertion will lead to panic reverts

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

• unintended behavior (compromiseBasketsNeeded would never be called), and the RToken state would stay undercollateralized
• left in state of under collateralization leading permeant freeze of RSR if the RToken is unweighable

Proof of Concept

The problem arises in BackingManagerP1::rebalance

• which is a function used to rebalance the collaterals and open trades on them in un collateralized state

File: BackingManager.sol
148:         (TradingContext memory ctx, Registry memory reg) = tradingContext(basketsHeld);
149:         (
150:             bool doTrade,
151:             TradeRequest memory req,
152:             TradePrices memory prices
153:         ) = RecollateralizationLibP1.prepareRecollateralizationTrade(ctx, reg);
154: 
155:         if (doTrade) {
156:             IERC20 sellERC20 = req.sell.erc20();
157: 
158:             // Seize RSR if needed
159:             if (sellERC20 == rsr) {
160:                 uint256 bal = sellERC20.balanceOf(address(this));
161:                 if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);
162:             }
163: 
164:             // Execute Trade
165:             ITrade trade = tryTrade(kind, req, prices);
166:             tradeEnd[kind] = trade.endTime(); // {s}
167:             tokensOut[sellERC20] = trade.sellAmount(); // {tok}
168:         } else {
169:             // Haircut time
170:             compromiseBasketsNeeded(basketsHeld.bottom);
171:         }

in Line 153 we call prepareRecollateralizationTrade and return bool doTrade seeing the implementation of it

File: RecollateralizationLib.sol
33:     function prepareRecollateralizationTrade(TradingContext memory ctx, Registry memory reg)
34:         external
35:         view
36:         returns (
37:             bool doTrade,
38:             TradeRequest memory req,
39:             TradePrices memory prices
40:         )
41:     {
42:         // Compute a target basket range for trading -  {BU}
43:         // The basket range is the full range of projected outcomes for the rebalancing process
44:         BasketRange memory range = basketRange(ctx, reg);
45: 
46:         // Select a pair to trade next, if one exists
47:         TradeInfo memory trade = nextTradePair(ctx, reg, range);
48: 
49:         // Don't trade if no pair is selected
50:         if (address(trade.sell) == address(0) || address(trade.buy) == address(0)) {
51:             return (false, req, prices);
52:         }
53: 
54:         // If we are selling a fully unpriced asset or UNSOUND collateral, do not cover deficit
55:         // untestable:
56:         //     sellLow will not be zero, those assets are skipped in nextTradePair
57:         if (
58:             trade.prices.sellLow == 0 ||
59:             (trade.sell.isCollateral() &&
60:                 ICollateral(address(trade.sell)).status() != CollateralStatus.SOUND)
61:         ) {
62:             // Emergency case
63:             // Set minBuyAmount as a function of sellAmount
64:             (doTrade, req) = trade.prepareTradeSell(ctx.minTradeVolume, ctx.maxTradeSlippage);
65:         } else {
66:             // Normal case
67:             // Set sellAmount as a function of minBuyAmount
68:             (doTrade, req) = trade.prepareTradeToCoverDeficit(
69:                 ctx.minTradeVolume,
70:                 ctx.maxTradeSlippage
71:             );
72:         }
73: 
74:         // At this point doTrade _must_ be true, otherwise nextTradePair assumptions are broken
75:         assert(doTrade);
76: 
77:         return (doTrade, req, trade.prices);
78:     }

in Line 47 we call nextTradePair but the problem is that we assume that it always returns a valid pair with enough Amounts, which is not

• cause some times collaterals may be small and doesn't fulfil the isEnoughToSell check function

The doTrade bool returned by either prepareTradeSell or prepareTradeToCoverDeficit is basically if the amounts are dust or not.

• The problem is that in Line 75 we assert doTrade so we need it to always to be true, this has two problems
  1. Txn will always revert when its false
  2. the check in it self contradict the logic in rebalance function and here is why

In rebalance

File: BackingManager.sol
155:         if (doTrade) {
156:             IERC20 sellERC20 = req.sell.erc20();
157: 
158:             // Seize RSR if needed
159:             if (sellERC20 == rsr) {
160:                 uint256 bal = sellERC20.balanceOf(address(this));
161:                 if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);
162:             }
163: 
164:             // Execute Trade
165:             ITrade trade = tryTrade(kind, req, prices);
166:             tradeEnd[kind] = trade.endTime(); // {s}
167:             tokensOut[sellERC20] = trade.sellAmount(); // {tok}
168:         } else {
169:             // Haircut time
170:             compromiseBasketsNeeded(basketsHeld.bottom);
171:         }

we check in Line 155 if doTrade is true (ir. not dust) we initiate a Trade

Else in Line 168 we call compromiseBasketsNeeded

but in fact due to the assert in prepareRecollateralizationTrade we either do trade or revert the txn

The above facts combined RToken maybe reweightable == false and RSR stakes can never be withdrawn in uncollateralized sate, due to this check in withdraw :

File: StRSR.sol
340:         // == Checks ==
341:         require(basketHandler.isReady() && basketHandler.fullyCollateralized(), "RToken readying");

Combined with the fact that RTokens holers can yet redeemCustom their tokens, leading to less collateral held in the backingManager which will cause isEnoughToSell to return false.

Then this will leave the protocol in very bad state and RSR stakers to be in a bad situation

For example:

• RToken is backed by 2 Collaterals
• the protocol becomes undercollateralized
• RToken holders redeemCustom
• now rebalance will fail due to the above description
• RSR will not be withdrawn
• Owner can't help them if the RToken is non weightable by setting new basket fitting the current state

Tools Used

manual review

Recommended Mitigation Steps

remove the redundant assert in the library that causes the problem and overhead.

Assessed type

Invalid Validation