hansfriese - AssetManager.removeAdapter() doesn't update withdrawSeq after removing an adapter.

[sherlock-admin]

hansfriese

high

AssetManager.removeAdapter() doesn't update withdrawSeq after removing an adapter.

Summary

AssetManager.removeAdapter() doesn't update withdrawSeq after removing an adapter.

Vulnerability Detail

In the AssetManager.sol, it stores all market adapters using moneyMarkets and priority sequence of money market indices using withdrawSeq.

As we can see from addAdapter(), it adds the index to withdrawSeq.

    function addAdapter(address adapterAddress) external override onlyAdmin {
        bool isExist = false;
        uint256 moneyMarketsLength = moneyMarkets.length;

        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            if (adapterAddress == address(moneyMarkets[i])) isExist = true;
        }

        if (!isExist) {
            moneyMarkets.push(IMoneyMarketAdapter(adapterAddress));
            withdrawSeq.push(moneyMarkets.length - 1);
        }

        approveAllTokensMax(adapterAddress);
    }

But in the removeAdapter(), it doesn't update withdrawSeq properly.

    function removeAdapter(address adapterAddress) external override onlyAdmin {
        bool isExist = false;
        uint256 index;
        uint256 moneyMarketsLength = moneyMarkets.length;

        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            if (adapterAddress == address(moneyMarkets[i])) {
                isExist = true;
                index = i;
                break;
            }
        }

        if (isExist) {
            moneyMarkets[index] = moneyMarkets[moneyMarketsLength - 1];
            moneyMarkets.pop();
        }
    }

So the below scenario would be possible.

• At the first time, the admin added 2 markets. moneyMarkets = [adapter1, adapter2], withdrawSeq = [0, 1]
• After that, he removed adapter2 using removeAdapter().
• Then moneyMarkets = [adapter1], withdrawSeq = [0, 1]
• When users are trying to withdraw using withdraw(), it will revert when i == 1 because the length of moneyMarkets is 1 here.

    uint256 withdrawSeqLength = withdrawSeq.length;
    // iterate markets according to defined sequence and withdraw
    for (uint256 i = 0; i < withdrawSeqLength && remaining > 0; i++) {
        IMoneyMarketAdapter moneyMarket = moneyMarkets[withdrawSeq[i]];
        if (!moneyMarket.supportsToken(token)) continue;

        uint256 supply = moneyMarket.getSupply(token);
        if (supply == 0) continue;

        uint256 withdrawAmount = supply < remaining ? supply : remaining;
        remaining -= withdrawAmount;
        moneyMarket.withdraw(token, account, withdrawAmount);
    }

• Furthermore, the admin can't change to withdrawSeq = [0] using setWithdrawSequence() because the lengths should be same.

    function setWithdrawSequence(uint256[] calldata newSeq) external override onlyAdmin {
        if (newSeq.length != withdrawSeq.length) revert NotParity();
        withdrawSeq = newSeq;
    }

Impact

AssetManager.withdraw() will revert after some adapters are removed from moneyMarkets.

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L440-L457

Tool used

Manual Review

Recommendation

We should remove the last index from withdrawSeq in removeAdapter() like below.

    function removeAdapter(address adapterAddress) external override onlyAdmin {
        bool isExist = false;
        uint256 index;
        uint256 moneyMarketsLength = moneyMarkets.length;

        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            if (adapterAddress == address(moneyMarkets[i])) {
                isExist = true;
                index = i;
                break;
            }
        }

        if (isExist) {
            moneyMarkets[index] = moneyMarkets[moneyMarketsLength - 1];
            moneyMarkets.pop();

            // remove moneyMarketsLength-1 from withdrawSeq
            for (uint256 i = 0; i < moneyMarketsLength; i++) {
                if (withdrawSeq[i] == moneyMarketsLength - 1) {
                    withdrawSeq[i] = withdrawSeq[moneyMarketsLength - 1];
                    withdrawSeq.pop();
                    break;
                }
            }
        }
    }

The above approach doesn't keep the original order after removing an adapter and we might try another method for that.

Duplicate of #76