Suboptimal Asset Selection May Lead to Financial Losses (`RecollateralizationLibP1::nextTradePair`)

[c4-bot-5]

Lines of code

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

Vulnerability details

Description

The RecollateralizationLibP1 library is a crucial component of the protocol's rebalancing mechanism, implemented within the BackingManager contract. Its primary function, nextTradePair(), is responsible for selecting the optimal pair of assets to trade based on their surplus and deficit values. This selection process relies heavily on the isBetterSurplus() function, which compares assets based on their collateral status and surplus amount.

The current implementation of isBetterSurplus() contains a logical flaw that could lead to suboptimal asset selection. Specifically, when comparing assets with different collateral statuses, the function may incorrectly prioritize an IFFY asset over a SOUND asset if the IFFY asset has a higher surplus amount. This contradicts the intended risk management strategy, where SOUND collateral should always be preferred over IFFY collateral, regardless of the surplus amount.

The problematic section of the isBetterSurplus() function is as follows:

File: RecollateralizationLib.sol
381:     function isBetterSurplus(
382:         MaxSurplusDeficit memory curr,
383:         CollateralStatus other,
384:         uint192 surplusAmt
385:     ) private pure returns (bool) {
386:         // NOTE: If the CollateralStatus enum changes then this has to change!
387:         if (curr.surplusStatus == CollateralStatus.DISABLED) {
388:             return other == CollateralStatus.DISABLED && surplusAmt.gt(curr.surplus);
389:         } else if (curr.surplusStatus == CollateralStatus.SOUND) {
390:             return
391:                 other == CollateralStatus.DISABLED ||
392:                 (other == CollateralStatus.SOUND && surplusAmt.gt(curr.surplus));
393:         } else {
394:             // curr is IFFY
395:             return other != CollateralStatus.IFFY || surplusAmt.gt(curr.surplus);
396:         }
397:     }

This implementation allows an IFFY asset to be selected over a SOUND asset if it has a higher surplus amount, which could potentially expose the protocol to unnecessary risk.

Impact

The impact of this issue is significant as it may lead to suboptimal trading decisions that prioritize riskier assets (IFFY) over safer ones (SOUND). This misalignment with the protocol's risk management strategy could result in:

1. Increased exposure to potentially unstable or risky assets.
2. Reduced overall stability of the protocol's collateral portfolio.
3. Potential financial losses if IFFY assets deteriorate in value or status.

Proof of Concept

Consider the following scenario:

1. The nextTradePair() function is called to determine the best asset to sell.
2. The current best surplus asset is a SOUND collateral with a surplus amount of 100 units.
3. The function encounters an IFFY collateral with a surplus amount of 150 units.
4. The isBetterSurplus() function is called to compare these assets.
5. Due to the current implementation, the IFFY collateral is selected as the better surplus asset because it has a higher surplus amount.
6. The protocol proceeds to sell the IFFY collateral instead of the SOUND collateral, potentially exposing itself to unnecessary risk.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, the isBetterSurplus() function should be modified to always prioritize SOUND collateral over IFFY collateral, regardless of the surplus amount. Here's the recommended fix:

function isBetterSurplus(
    MaxSurplusDeficit memory curr,
    CollateralStatus other,
    uint192 surplusAmt
) private pure returns (bool) {
    if (curr.surplusStatus == CollateralStatus.DISABLED) {
        return other == CollateralStatus.DISABLED && surplusAmt.gt(curr.surplus);
    } else if (curr.surplusStatus == CollateralStatus.SOUND) {
-        return
-            other == CollateralStatus.DISABLED ||
-            (other == CollateralStatus.SOUND && surplusAmt.gt(curr.surplus));
+        return other != CollateralStatus.SOUND || surplusAmt.gt(curr.surplus);
    } else {
        // curr is IFFY
-        return other != CollateralStatus.IFFY || surplusAmt.gt(curr.surplus);
+        return other == CollateralStatus.DISABLED || 
+               (other == CollateralStatus.IFFY && surplusAmt.gt(curr.surplus));
    }
}

Assessed type

Other