hyh - Market adapter removal corrupts withdraw sequence

[github-actions[bot]]

hyh

medium

Market adapter removal corrupts withdraw sequence

Summary

Withdraw sequence entry is being deleted in AssetManager's removeAdapter() using incorrect index, so the sequence array becomes corrupted and withdrawals can end up unavailable.

Vulnerability Detail

index used for adapter removal is determined for moneyMarkets array, but applied to withdrawSeq as well, while in there indices do differ.

Impact

As withdrawals logic are based on withdraw sequence array, the withdrawals can end up unavailable. For example, if 95% of the funds are held in a market, whose entry corresponded to the index removed, withdrawals will be frozen for an arbitrary time for the whole protocol until withdrawSeq be manually restored.

Code Snippet

removeAdapter() determines the index for moneyMarkets array:

https://github.com/sherlock-audit/2023-02-union/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L435-L463

    /**
     *  @dev Remove a adapter for the underlying lending protocol
     *  @param adapterAddress adapter address
     */
    function removeAdapter(address adapterAddress) external override onlyAdmin {
        bool isExist = false;
        uint256 index = 0;
        uint256 moneyMarketsLength = moneyMarkets.length;
        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            if (adapterAddress == address(moneyMarkets[i])) {
                isExist = true;
                index = i;
                break;
            }
        }

        if (isExist) {
            for (uint256 i = 0; i < supportedTokensList.length; i++) {
                if (moneyMarkets[index].getSupply(supportedTokensList[i]) >= 10000) revert RemainingFunds(); //ignore the dust
            }
            moneyMarkets[index] = moneyMarkets[moneyMarketsLength - 1];
            moneyMarkets.pop();

            withdrawSeq[index] = withdrawSeq[withdrawSeq.length - 1];
            withdrawSeq.pop();

            _removeTokenApprovals(adapterAddress);
        }
    }

But withdrawSeq is a permutation of moneyMarkets, so their indices are independent and do not correspond to each other:

https://github.com/sherlock-audit/2023-02-union/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L136-L143

    /**
     *  @dev Set withdraw sequence
     *  @param newSeq priority sequence of money market indices to be used while withdrawing
     */
    function setWithdrawSequence(uint256[] calldata newSeq) external override onlyAdmin {
        if (newSeq.length != moneyMarkets.length) revert NotParity();
        withdrawSeq = newSeq;
    }

So index in withdrawSeq being deleted do not generally correspond to the adapter, i.e. some other market end up being removed:

https://github.com/unioncredit/union-v2-contracts/blob/49d1a7261a7be20fe77b91a8a73e3cba8bc5bda5/contracts/asset/AssetManager.sol#L464-L465

            withdrawSeq[index] = withdrawSeq[withdrawSeq.length - 1];
            withdrawSeq.pop();

Tool used

Manual Review

Recommendation

Consider repeating the entry finding logic for withdrawSeq, for example:

https://github.com/sherlock-audit/2023-02-union/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L435-L463

    /**
     *  @dev Remove a adapter for the underlying lending protocol
     *  @param adapterAddress adapter address
     */
    function removeAdapter(address adapterAddress) external override onlyAdmin {
        bool isExist = false;
        uint256 index = 0;
+       uint256 indexSeq = 0;
        uint256 moneyMarketsLength = moneyMarkets.length;
        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            if (adapterAddress == address(moneyMarkets[i])) {
                isExist = true;
                index = i;
                break;
            }
        }

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

            for (uint256 i = 0; i < supportedTokensList.length; i++) {
                if (moneyMarkets[index].getSupply(supportedTokensList[i]) >= 10000) revert RemainingFunds(); //ignore the dust
            }
            moneyMarkets[index] = moneyMarkets[moneyMarketsLength - 1];
            moneyMarkets.pop();

-           withdrawSeq[index] = withdrawSeq[withdrawSeq.length - 1];
+           withdrawSeq[indexSeq] = withdrawSeq[withdrawSeq.length - 1];
            withdrawSeq.pop();

            _removeTokenApprovals(adapterAddress);
        }
    }

[kingjacob]

24 -https://github.com/unioncredit/union-v2-contracts/pull/108

[dmitriia]

Looks ok given that setWithdrawSequence() is always set duplicate-free withdrawSeq