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
Txn will always revert when its false
the check in it self contradict the logic in rebalance function and here is why
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 :
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.
Lines of code
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/RecollateralizationLib.sol#L75
Vulnerability details
Impact
compromiseBasketsNeeded
would never be called), and the RToken state would stay undercollateralizedProof of Concept
The problem arises in
BackingManagerP1::rebalance
in Line 153 we call
prepareRecollateralizationTrade
and returnbool doTrade
seeing the implementation of itin Line 47 we call
nextTradePair
but the problem is that we assume that it always returns a valid pair with enough Amounts, which is notisEnoughToSell
check functionThe
doTrade
bool returned by eitherprepareTradeSell
orprepareTradeToCoverDeficit
is basically if the amounts are dust or not.doTrade
so we need it to always to be true, this has two problemsIn
rebalance
we check in Line 155 if
doTrade
is true (ir. not dust) we initiate a TradeElse in Line 168 we call
compromiseBasketsNeeded
but in fact due to the assert in
prepareRecollateralizationTrade
we either do trade or revert the txnThe above facts combined RToken maybe
reweightable
== false and RSR stakes can never be withdrawn in uncollateralized sate, due to this check inwithdraw
:Combined with the fact that RTokens holers can yet
redeemCustom
their tokens, leading to less collateral held in the backingManager which will causeisEnoughToSell
to return false.Then this will leave the protocol in very bad state and RSR stakers to be in a bad situation
For example:
rebalance
will fail due to the above descriptionweightable
by setting new basket fitting the current stateTools Used
manual review
Recommended Mitigation Steps
remove the redundant assert in the library that causes the problem and overhead.
Assessed type
Invalid Validation