Incorrect baskets can be created when swapping registered assets

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L268 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L99 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/BasketLib.sol#L303 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/mixins/BasketLib.sol#L253

Vulnerability details

Impact

In the AssetRegistry, it is possible to swap a registered asset for another. If the asset being swapped is part of the current basket, we need to switch to a new basket. While these two assets share the same erc20, they may have different target names, which can result in the creation of an incorrect basket.

Although we can immediately switch to a correct basket afterward, the presence of the wrong basket can negatively impact future custom redemptions.

Proof of Concept

Suppose asset A is currently in the basket, and we want to swap it with a new asset B. Both assets share the same erc20, but they have different target names—Ta for A and Tb for B. (This is possible) Since A is being removed from the basket, we need to switch to a new basket. However, because B has a different target name, the prime basket for the next basket should be different. We cannot set the prime basket for the new basket before swapping A with B, as A's target name is still in use (line 268).

function _setPrimeBasket(
    IERC20[] calldata erc20s,
    uint192[] memory targetAmts,
    bool disableTargetAmountCheck
) internal {
    requireGovernanceOnly();
    require(erc20s.length != 0 && erc20s.length == targetAmts.length, "invalid lengths");
    requireValidCollArray(erc20s);

    for (uint256 i = 0; i < erc20s.length; ++i) {
        config.erc20s.push(erc20s[i]);
        config.targetAmts[erc20s[i]] = targetAmts[i];
268:        names[i] = assetRegistry.toColl(erc20s[i]).targetName();
        config.targetNames[erc20s[i]] = names[i];
    }
}

The correct steps are:

1. Swap A with B using the swapRegistered function.

   function swapRegistered(IAsset asset) external governance returns (bool swapped) {
       require(_erc20s.contains(address(asset.erc20())), "no ERC20 collision");
       try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
           if (quantity != 0) basketHandler.disableBasket(); // not an interaction
       } catch {
           basketHandler.disableBasket();
       }
       swapped = _registerIgnoringCollisions(asset);
   }

2. Set the new prime basket . At this point, the target name Tb for asset B will be registered.
3. Switch to the new basket.

This sequence ensures that everything functions correctly.

However, if someone calls the refreshBasket function before step 2, the old prime basket will be used for the new basket, incorrectly retaining A'starget name, Ta. The asset A is not a good collateral because asset B is now registered for this erc20 in the AssetRegistry, it returns Tb as the target name(line 303).

function goodCollateral(
    bytes32 targetName,
    IERC20 erc20,
    IAssetRegistry assetRegistry
) private view returns (bool) {
    try assetRegistry.toColl(erc20) returns (ICollateral coll) {
        return
303:            targetName == coll.targetName() &&  // Ta != Tb
            coll.status() == CollateralStatus.SOUND &&
            coll.refPerTok() != 0 &&
            coll.targetPerRef() != 0;
    } catch {
        return false;
    }
}

This mismatch means that other tokens with target name Ta will be used to back asset A, creating an invalid basket (line 253).

function nextBasket(
    Basket storage newBasket,
    EnumerableSet.Bytes32Set storage targetNames,
    BasketConfig storage config,
    IAssetRegistry assetRegistry
) external returns (bool) {
    for (uint256 i = 0; i < targetsLength; ++i) {
        if (totalWeights[i].lte(goodWeights[i])) continue; // Don't need any backup weight
        bytes32 _targetName = targetNames.at(i);

        uint256 size = 0; // backup basket size
        BackupConfig storage backup = config.backups[_targetName];

        for (uint256 j = 0; j < backup.erc20s.length && size < backup.max; ++j) {
253:            if (goodCollateral(_targetName, backup.erc20s[j], assetRegistry)) size++;
        }
    }
}

Although the owner can later complete steps 2 and 3 to switch to a new, correct basket with the proper target name Tb, any old baskets in a system where reweightable is false will permanently affect custom redemptions, as we cannot increase lastCollateralized(line 187).

function trackStatus() public {     
    // Invalidate old nonces if fully collateralized
187:    if (reweightable && nonce > lastCollateralized && fullyCollateralized()) {
            emit LastCollateralizedChanged(lastCollateralized, nonce);
            lastCollateralized = nonce;
    }
}

Therefore, it is crucial to prevent the creation of incorrect baskets by mistake. (Clearly, no one wants to create this temporary basket.)

Tools Used

Recommended Mitigation Steps

At this point, the refreshBasket function can be called at any time by the owner. Alternatively, anyone can call this function when the basket is disabled. However, there is no need to allow anyone to call this function. The owner should also be able to call it when the basket is disabled. In summary, restrict the ability to call this function to only the owner.

Assessed type

Access Control

[thereksfour]

requires same erc20s with different target names, this relies on governance misconfiguration, invalid

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[etherSky111]

Hi @thereksfour , @tbrent , @akshatmittal , Thanks for your review.

This is not a governance misconfiguration.

There could be a case where a switch is made to another collateral that shares the same erc20 token but has a different targetName. This means that the two collaterals are based on the same erc20 token, but one tracks USD while the other tracks a different currency.

When this happens, an incorrect basket will be created, as I described.

I would appreciate it if you could reconsider this issue.

[tbrent]

My answer to your question was on a technical basis, purely.

If it causes issues, that is a governance misconfiguration.

It is not always an issue. It's actually necessary in at least one case: when adding management fee to an ERC20. In this case the plugin is changed so that the targetName encodes (i) the base unit, such as USD; (ii) the interest rate, such as 1%; and (iii) t_0, the time the fee should be applied from. Then the prime basket is changed (must be reweightable RToken) to target this unit.

(we call that concept demurrage collateral, internally, there is some historical discussion in the repo though we are not pushing it currently)

[etherSky111]

agree! thanks