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

0 stars 0 forks source link

`BasketHandler.quantityUnsafe` function does not verify if the `erc20` is registered in the `AssetRegistry` thus leading to erroneous auction trade execution #230

Closed c4-bot-2 closed 1 month ago

c4-bot-2 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/AssetRegistry.sol#L134-L138 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L359-L362

Vulnerability details

Impact

In the BasketHandler.quantityUnsafe function it is assumed that the asset is correct which means the passed in erc20 is correctly mapped to the asset. But there is no check in place to ensure that the erc20 is registered in the AssetRegistry.

Prior to calling the BasketHandler._quantity function by respective transactions, it is verified that the erc20 is registered in the AssetRegistry.toColl as shown below:

    require(_erc20s.contains(address(erc20)), "erc20 unregistered");

But it is not verified that the erc20 is registered in the AssetRegistry while calling the BasketHandler.quantityUnsafe function as shown below:

        if (!asset.isCollateral()) return FIX_ZERO;
        return _quantity(erc20, ICollateral(address(asset)), CEIL);

Hence an unregistered erc20 token could be used in the BackingManager.

The BackingManager.tradingContext function calls the BasketHandler.quantityUnsafe function to calculate the token quantities. Due to the above missing check an unregistered erc20 asset could be used in the tradingContext which will be used in the BackingManager.rebalance function while calling the RecollateralizationLib.prepareRecollateralizationTrade function thus resulting in errorneous auction trade parameters. This will lead to erroneous auction trade execution which will be disadvantageous and loss of funds to both the rsrStakers and the RToken holders.

Proof of Concept

    function toColl(IERC20 erc20) external view returns (ICollateral) {
        require(_erc20s.contains(address(erc20)), "erc20 unregistered");
        require(assets[erc20].isCollateral(), "erc20 is not collateral");
        return ICollateral(address(assets[erc20]));
    }

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/AssetRegistry.sol#L134-L138

    function quantityUnsafe(IERC20 erc20, IAsset asset) public view returns (uint192) {
        if (!asset.isCollateral()) return FIX_ZERO;
        return _quantity(erc20, ICollateral(address(asset)), CEIL);
    }

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L359-L362

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to verify that the erc20 is registered in the AssetRegistry while calling the BasketHandler.quantityUnsafe function. This can be done by verifying the following logic _erc20s.contains(address(erc20)) on the AssetRegistry contract.

Assessed type

Invalid Validation