ArmedGoose - After delisting the market users will not be able to redeem their supplied tokens (even after relisting)

[sherlock-admin]

ArmedGoose

medium

After delisting the market users will not be able to redeem their supplied tokens (even after relisting)

Summary

When a market is delisted, users are not able to reedem their tokens they supplied to that market. If it is listed back, it is also not possible, because the supply amount is not updated from "previous" listing period.

Vulnerability Detail

Markets can be delisted (harddelist) by the protocol. Any suppliers to that market may be not aware of the decision and may have already some ibTokens they received when supplying to that market. However the Redeem function checks in line 412 if the market is listed, otherwise reverts. This is the first point, where users may be left with unrealized ibTokens.

Now, if the protocol wishes to list that market again, even with the same ibTokens that were left previously, users will not be able to realize them, because the supply amount will be not enough (since they did not supply after the recent listing)

Impact

Users may not be able to retrieve their funds, which is high impact, but it may happen only in some edge cases for some group of users, which is low likelihood, so the overall rating is medium.

Code Snippet

Run forge test -vv --match-test testRedeem_redeem_after_delisting. Observe that test passes, while two expected reverts happens - vm.expectRevert("not listed"); after delisting and then vm.expectRevert("insufficient balance"); after re-listing. The test:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "./Common.t.sol";
import "forge-std/console.sol";

contract RedeemTest is Test, Common {
    int256 internal constant market1Price = 1500e8;
    int256 internal constant market2Price = 1000e8;
    uint16 internal constant reserveFactor = 1000; // 10%
    uint16 internal constant collateralFactor = 8000; // 80%

    IronBank ib;
    MarketConfigurator configurator;
    CreditLimitManager creditLimitManager;
    FeedRegistry registry;
    PriceOracle oracle;
    TripleSlopeRateModel irm;

    ERC20Market market1;
    ERC20Market market2;
    IBToken ibToken1;
    IBToken ibToken2;

    address admin = address(64);
    address user1 = address(128);
    address user2 = address(256);

    function setUp() public {
        ib = createIronBank(admin);

        configurator = createMarketConfigurator(admin, ib);

        vm.prank(admin);
        ib.setMarketConfigurator(address(configurator));

        creditLimitManager = createCreditLimitManager(admin, ib);

        vm.prank(admin);
        ib.setCreditLimitManager(address(creditLimitManager));

        irm = createDefaultIRM();

        (market1, ibToken1,) = createAndListERC20Market(18, admin, ib, configurator, irm, reserveFactor);
        (market2, ibToken2,) = createAndListERC20Market(18, admin, ib, configurator, irm, reserveFactor);

        registry = createRegistry();
        oracle = createPriceOracle(admin, address(registry));

        vm.prank(admin);
        ib.setPriceOracle(address(oracle));

        setPriceForMarket(oracle, registry, admin, address(market1), address(market1), Denominations.USD, market1Price);
        setPriceForMarket(oracle, registry, admin, address(market2), address(market2), Denominations.USD, market2Price);

        configureMarketAsCollateral(admin, configurator, address(market1), collateralFactor);
        configureMarketAsCollateral(admin, configurator, address(market2), collateralFactor);

        deal(address(market1), user1, 10000e18);
    }

    function testRedeem_redeem_after_delisting() public { 
        uint256 supplyAmount = 100e18;
        uint256 redeemAmount = 50e18;
        vm.startPrank(user1);
        market1.approve(address(ib), supplyAmount);
        ib.supply(user1, user1, address(market1), supplyAmount);
        fastForwardTime(86400);
        vm.stopPrank();

        vm.startPrank(admin);
        configurator.adjustMarketCollateralFactor(address(market1), uint16(0));//
        configurator.adjustMarketLiquidationThreshold(address(market1), uint16(0));//
        configurator.softDelistMarket(address(market1));//
        configurator.hardDelistMarket(address(market1));//
        vm.stopPrank();

        //Cannot redeem delisted market
        vm.expectRevert("not listed");
        vm.prank(user1);
        ib.redeem(user1, user1, address(market1), redeemAmount); 

        vm.prank(admin);
        (market1, ibToken1,) = createAndListERC20Market(18, admin, ib, configurator, irm, reserveFactor);

        //Cannot redeem because user did not supply these tokens after re-listing
        vm.expectRevert("insufficient balance");
        vm.prank(user1);
        ib.redeem(user1, user1, address(market1), redeemAmount); 

    }

}

Tool used

Foundry Manual Review

Recommendation

Delisting market should do more than just delete the structure. There might be also other (as another issue with Liquidations I submitted) financial dependencies of a market. Before it can be called not used, it should be made sure that there no debts that will remain after closing such market, and that all users that the market is due to, will have possibility to redeem their funds later on. There is no straightforward solution to it, but it seems reasonable to at least account the remainders into some variables instead of delete the market completely, so the market may stay in a "hidden" more and operations such as redeem could be still performed. It's similar to common pause logic - even if protocol is paused, user should be able to exit the protocol, but not enter it anymore, I believe the same should be applied to a delisted market, instead of banning it completely.

[ArmedGoose]

Escalate for 10 USDC Any explanation why this is marked as excluded? The poc is very extensive and clearly shows how an user, who fails to redeem tokens on time before delisting, will not be able to redeem them anymore, even of protocol reslists the market. I believe this is a valid issue.

[sherlock-admin]

Escalate for 10 USDC Any explanation why this is marked as excluded? The poc is very extensive and clearly shows how an user, who fails to redeem tokens on time before delisting, will not be able to redeem them anymore, even of protocol reslists the market. I believe this is a valid issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xffff11]

Duplicate of #426 . Low

[hrishibhat]

Result: Low Unique Considering this issue a low based on Sponsor comment https://github.com/sherlock-audit/2023-05-ironbank-judging/issues/426#issuecomment-1620755589

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ArmedGoose: rejected