code-423n4 / 2024-07-reserve-findings

5 stars 4 forks source link

Missing check of `rsrIndex` reverts recollateralization. #14

Closed c4-bot-3 closed 3 months ago

c4-bot-3 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

It reverts recollateralization instead of haircut, which makes it impossible to recollateralize.

Proof of Concept

When the protocol goes under-collateralized, it chooses tokens to buy and sell based on deficiency and surplus.

If there is no surplus token, it tries to sell RSR token to buy deficient tokens.

// Use RSR if needed
if (address(trade.sell) == address(0) && address(trade.buy) != address(0)) {
    (uint192 low, uint192 high) = reg.assets[rsrIndex].price(); // {UoA/RSR}

    // if rsr does not have a registered asset the below array accesses will revert
    if (
        high != 0 &&
        TradeLib.isEnoughToSell(
            reg.assets[rsrIndex],
            ctx.bals[rsrIndex],
            low,
            ctx.minTradeVolume
        )
    ) {
        trade.sell = reg.assets[rsrIndex];
        trade.sellAmount = ctx.bals[rsrIndex];
        trade.prices.sellLow = low;
        trade.prices.sellHigh = high;
    }
}

However, it does not check if RSR has its registered asset, thus recollateralization reverts when RSR does not exist, which is not correct. The correct behavior should be haircut. If it reverts, there's no way to resolve under-collateralization.

Tools Used

Manual Review

Recommended Mitigation Steps

It should return when RSR does not have registered asset.

// Use RSR if needed
if (address(trade.sell) == address(0) && address(trade.buy) != address(0)) {

+   if (rsrIndex == reg.erc20s.length) return trade;

    (uint192 low, uint192 high) = reg.assets[rsrIndex].price(); // {UoA/RSR}

    // if rsr does not have a registered asset the below array accesses will revert
    ...
}

Assessed type

Context

thereksfour commented 2 months ago

rsr will make sure to be registered, unregistering rsr will be malicious governance behavior

        IAsset[] memory assets = new IAsset[](2);
        assets[0] = new RTokenAsset(components.rToken, params.rTokenMaxTradeVolume);
        assets[1] = rsrAsset;

        // Init Asset Registry
        main.assetRegistry().init(main, assets);
c4-judge commented 2 months ago

thereksfour marked the issue as unsatisfactory: Invalid