Haircut is never reached

[c4-bot-9]

Lines of code

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

Vulnerability details

Vulnerability details

The haircut method implemented in the BackingManager.sol contract is designed to manage the situation where the actual basket holdings of the contract do not meet the required basket composition(basketsNeeded). When the BackingManager detects that the baskets held by the contract are insufficient to meet the basketsNeeded (i.e., the contract is undercollateralized), it enters a "haircut" mode. But here haircut mode is never reached. So it caused protocol to a insolvent mode.

Proof of Concept

In the rebalance function its invoked prepareRecollateralizationTrade.

 function rebalance(TradeKind kind) external nonReentrant {
     ...
          (
            bool doTrade,
            TradeRequest memory req,
            TradePrices memory prices
        ) = RecollateralizationLibP1.prepareRecollateralizationTrade(ctx, reg);
     ...

In prepareRecollateralizationTrade function its checked doTrade as always true.

 function prepareRecollateralizationTrade(TradingContext memory ctx, Registry memory reg)
        external
        view
        returns (
            bool doTrade,
            TradeRequest memory req,
            TradePrices memory prices
        )
    {

    ...

    assert(doTrade);

    ...
    }

That's mean its always executing this if statement in rebalance function not reaching to this else part.

function rebalance(TradeKind kind) external nonReentrant {

    ...

          if (doTrade) {
                      IERC20 sellERC20 = req.sell.erc20();

                      // Seize RSR if needed
                      if (sellERC20 == rsr) {
                          uint256 bal = sellERC20.balanceOf(address(this));
                          if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);
                      }

                      // Execute Trade
                      ITrade trade = tryTrade(kind, req, prices);
                      tradeEnd[kind] = trade.endTime(); // {s}
                      tokensOut[sellERC20] = trade.sellAmount(); // {tok}
                  } else {
                      // Haircut time
                      compromiseBasketsNeeded(basketsHeld.bottom);
                  }

    }

Impact

This Haircut mechanism is crucial for maintaining the integrity and stability of the system, ensuring that protocol is overcollateralized. If its cannot call compromiseBasketsNeeded function eventually protocol is going to be insolvent state.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove this assert(doTrade) so that it can be invoked compromiseBasketsNeeded as needed.

Assessed type

Other

[Yasashari]

@thereksfour Here compromiseBasketsNeeded(basketsHeld.bottom) is never reached since doTrade is always true due to the assertion in prepareRecollateralizationTrade function. So it caused protocol to a insolvent mode.

[thereksfour]

https://github.com/code-423n4/2024-07-reserve-findings/issues/69#issuecomment-2303349346

The early return in RecollateralizationLib.prepareRecollateralizationTrade ensures the haircut occurs when the only remaining trade is below the minTradeVolume.

function prepareRecollateralizationTrade(TradingContext memory ctx, Registry memory reg)
external
view
returns (
bool doTrade,
TradeRequest memory req,
TradePrices memory prices
)
{
// Compute a target basket range for trading -  {BU}
// The basket range is the full range of projected outcomes for the rebalancing process
BasketRange memory range = basketRange(ctx, reg);

    // Select a pair to trade next, if one exists
    TradeInfo memory trade = nextTradePair(ctx, reg, range);

    // Don't trade if no pair is selected
    if (address(trade.sell) == address(0) || address(trade.buy) == address(0)) {
        return (false, req, prices);
    }