016.md

kutugu

medium

delistMarket not deletes userBorrows and userSupplies mapping

Summary

delistMarket deletes markets[market], but not deletes userBorrows and userSupplies sub mapping in market struct.
When the market is listed again, the legacy parameter can cause the user to obtain additional funds, breaking the internal accounting system.

Vulnerability Detail

    function hardDelistMarket(address market) external onlyOwner {
        DataTypes.MarketConfig memory config = getMarketConfiguration(market);
        // @audit not check totalSupply, totalCash... 
        require(config.isListed, "not listed");
        require(config.isSupplyPaused() && config.isBorrowPaused(), "not paused");
        require(config.reserveFactor == MAX_RESERVE_FACTOR, "reserve factor not max");
        require(
            config.collateralFactor == 0 && config.liquidationThreshold == 0,
            "collateral factor or liquidation threshold not zero"
        );

        if (config.isPToken) {
            address underlying = PTokenInterface(market).getUnderlying();
            DataTypes.MarketConfig memory underlyingConfig = getMarketConfiguration(underlying);
            // It's possible that the underlying is not listed.
            if (underlyingConfig.isListed && underlyingConfig.pTokenAddress != address(0)) {
                underlyingConfig.pTokenAddress = address(0);
                ironBank.setMarketConfiguration(underlying, underlyingConfig);

                emit MarketPTokenSet(underlying, address(0));
            }
        }

        // @audit entry here
        ironBank.delistMarket(market);

        emit MarketDelisted(market);
    }

    function delistMarket(address market) external onlyMarketConfigurator {
        DataTypes.Market storage m = markets[market];
        require(m.config.isListed, "not listed");

        // @audit not delete userBorrows, userSupplies
        delete markets[market];
        allMarkets.deleteElement(market);

        emit MarketDelisted(market);
    }

    struct Market {
        MarketConfig config;
        uint40 lastUpdateTimestamp;
        uint256 totalCash;
        uint256 totalBorrow;
        uint256 totalSupply;
        uint256 totalReserves;
        uint256 borrowIndex;
        mapping(address => UserBorrow) userBorrows;
        mapping(address => uint256) userSupplies;
    }

delistMarket deletes account data in Market struct, such as totalCash, totalBorrow, totalSupply, totalReserves, but not deletes userBorrows and userSupplies.
There are two cases:

1. ironBank team notifies in advance without compensation
2. ironBank team will compensate the user if the market is delist and the user funds will be frozen

For case 1, due to totalCash, totalBorrow, totalSupply, totalReserves are 0, but userBorrows, userSupplies are not 0. The internal accounting system is broken. A simple description of this situation:

1. MarketA is firstly list. Alice supply x market token, mint x * initialExchangeRate ibToken
2. IronBank team wants to delist marketA and notifies in advance, but Alice doesn't see
3. After marketA is delist, Alice's token are frozen
4. MarketA is list again. Bob supply x market token, mint x * initialExchangeRate ibToken
5. Alice can withdraw money now, but Bob's token are frozen again
6. Bad debts continue, like a Ponzi scheme, eventually drive away users.

For case 2, when the market delist user receive an offer of compensation, when it came time to list the market again, users will use these legacy parameters for profit like case 1. So user can receive two profits.

Impact

Medium. Internal accounting system is broken.

Code Snippet

• https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/pool/IronBank.sol#L603
• https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/libraries/DataTypes.sol#L38-L39

Tool used

Manual Review

Recommendation

For case 1, if ironBank team won't compensate the user, they can add a variable to mark that the market has been delist. Do not change the accounting system of the market so that when the market is listed again, the accounting system can remain normal.
For case 2, if ironBank team will compensate the user, they should delete userBorrows and userSupplies before delistMarket.