The basket can mistakenly be disabled when unregistering a token that is not in the basket

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L397 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L112 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L121

Vulnerability details

Impact

Assets are registered in the AssetRegistry, and some of them may be part of the current basket. When we unregister a token that is in the basket, the current basket should be disabled, and switch to a new basket. After switching, there is a warm-up period during which certain critical operations, like recollateralization in the BackingManager, cannot be performed. Therefore, it’s important to disable the basket carefully. However, we mistakenly disable the basket when unregistering a token that is not part of it and the new basket will be identical to the current one, potentially causing unnecessary disruptions. Clearly, there's no need to disable the basket when unregistering a token that is not in the current basket.

Proof of Concept

At this point, we determine whether a token is in the current basket by checking its quantity within the basket. If the token is in the basket, the refAmts for it will not be 0 so the quantity will also be 0 (line 400). It's necessary to disable the basket if the quantity is non-zero.

However, the _quantity function can also return a non-zero value (FIX_MAX) when the refPerTok is 0(line 397).

function _quantity(
    IERC20 erc20,
    ICollateral coll,
    RoundingMode rounding
) internal view returns (uint192) {
    uint192 refPerTok = coll.refPerTok();
397:    if (refPerTok == 0) return FIX_MAX;

400:    return basket.refAmts[erc20].div(refPerTok, rounding);
}

This means that even if the token is not in the current basket, the quantity function might return a non-zero value, causing us to mistakenly disable the basket (line 113).

function unregister(IAsset asset) external governance {
    require(_erc20s.contains(address(asset.erc20())), "no asset to unregister");
    require(assets[asset.erc20()] == asset, "asset not found");

    try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
113:        if (quantity != 0) basketHandler.disableBasket(); // not an interaction
    } catch {
        basketHandler.disableBasket();
    }
}

While it's possible to switch to a new basket immediately, this step is unnecessary because the new basket will be identical to the current one. Moreover, important operations like recollateralization and forwardRevenue distribution cannot be performed during the warm-up period that follows a basket switch (line121).

function rebalance(TradeKind kind) external nonReentrant {
    requireNotTradingPausedOrFrozen();

    assetRegistry.refresh();
    require(
        _msgSender() == address(this) || tradeEnd[kind] < block.timestamp,
        "already rebalancing"
    );

    require(tradesOpen == 0, "trade open");
121:    require(basketHandler.isReady(), "basket not ready");
}

Please add below test to the test/Main.test.ts.

describe('Unregister disabled token', () => {
  it('Should not disable the basket when the token being unregistered is not part of it', async () => {
    await basketHandler.connect(owner).setPrimeBasket([token0.address], [fp('1')])
    await basketHandler.connect(owner).refreshBasket()

    const backing = await facade.basketTokens(rToken.address)
    // token 2 is not included in the basket.
    expect(backing.length).to.equal(1)
    expect(backing[0]).to.equal(token0.address)

    // the quantity of token 2 is 0 because it is not in the basket
    expect(await basketHandler.quantity(token2.address)).to.equal(0)
    
    await token2.setExchangeRate(fp('0'))
    await collateral2.refresh()

    // the quantity of token 2 is set to FIX_MAX because its refPerTok is 0
    expect(await basketHandler.quantity(token2.address)).to.equal(MAX_UINT192)

    // current status is SOUND
    expect(await basketHandler.status()).to.equal(CollateralStatus.SOUND);

    assetRegistry.connect(owner).unregister(collateral2.address)

    // the basket was mistakenly disabled when token 2 was unregistered.
    expect(await basketHandler.status()).to.equal(CollateralStatus.DISABLED)
  })
})

Tools Used

Recommended Mitigation Steps

The basket should not be disabled when unregistering a token with a quantity of FIX_MAX if it is not in the current basket.

Assessed type

Invalid Validation

[akshatmittal]

The refPerTok value can not be 0. See: https://github.com/reserve-protocol/protocol/blob/master/docs/solidity-style.md#rates

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[etherSky111]

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

First, I asked about this in Discord, and tbrent | Reserve confirmed that refPerTok can indeed be 0. The link provided above (https://github.com/reserve-protocol/protocol/blob/master/docs/solidity-style.md#rates) is related to tokens that are in a SOUND status.

However, my concern is with tokens that have been disabled for various reasons. Once tokens are disabled, the owner may decide to unregister them. Until they are unregistered, these tokens should not impact the protocol, but they still can. Additionally, unregistering these tokens can accidentally disable the basket, which is critical to the protocol's functionality. Recollateralization and basket disabling are among the most crucial aspects of this protocol.

I would appreciate it if you could reconsider this issue.

[akshatmittal]

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

[etherSky111]

thanks