'compromiseBasketsNeeded' function in the 'rebalance' function is never called

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BackingManager.sol#L155-L171

Vulnerability details

Impact

The rebalance function plays a crucial role in restoring balance when the system is undercollateralized. This function updates the asset registry, checks the collateral status, and if necessary, initiates trades to replenish the deficient collateral. If the trades are not sufficient to fully restore collateralization, the compromiseBasketsNeeded function is called to adjust the required collateral to match the actual collateral held.

The compromiseBasketsNeeded function ensures the system's safety in an undercollateralized state by taking necessary measures. If this function is not called, the system remains undercollateralized, increasing the associated risks.

Proof of Concept

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BackingManager.sol#L148-L153

    (TradingContext memory ctx, Registry memory reg) = tradingContext(basketsHeld);
    (
        bool doTrade,
        TradeRequest memory req,
        TradePrices memory prices
    ) = RecollateralizationLibP1.prepareRecollateralizationTrade(ctx, reg);

The value of doTrade, which is a Boolean type data, is set by the result value of the RecollateralizationLibP1.prepareRecollateralizationTrade function.

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

        assert(doTrade);

        return (doTrade, req, trade.prices);

The prepareRecollateralizationTrade function in the RecollateralizationLib.sol contract contains the 'assert(doTrade)' statement, which ensures that doTrade always returns true.

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BackingManager.sol#L155-L171

        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);
        }

Since the prepareRecollateralizationTrade function described above always returns the value of doTrade as true, the else clause in the if-else statement where the value of doTrade is used will never be reached.

Tools Used

VScode

Recommended Mitigation Steps

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/mixins/RecollateralizationLib.sol#L75-L77 I recommend removing the assert statement.

        // assert(doTrade);

        return (doTrade, req, trade.prices);

Assessed type

Context