hyh - Remaining funds check can block token and adapter removal

[github-actions[bot]]

hyh

medium

Remaining funds check can block token and adapter removal

Summary

Funds checks removeToken() and removeAdapter() perform don't ensure that token is supported by the market adapter called.

If moneyMarket.getSupply(token) reverts when the token isn’t supported by the market, the token and market removal can be permanently blocked.

As the implementation of IMoneyMarketAdapter is generally unknown, it can be reverting on an unknown token.

Vulnerability Detail

removeToken() and removeAdapter() call moneyMarkets[i].getSupply(tokenAddress) for all the markets available.

One of the approaches to react on a getSupply() call for an unsupported token is reverting in order to distinguish the situation from 0 balance of a supported token (that's quite common and architecturally viable behavior).

In this case an existence of unsupported token, i.e. any situation when not all the markets support all the tokens, will have token/adapter removal operation unavailable.

Impact

Token and adapter removal can be effectively disabled, which will lead to bloating of adapters/tokens list, that leads to raising gas costs for all the users, also to raising the overall operation risk with the corresponding growth of user fund loss probability.

For example, a market that was found to be malicious or contain bugs in its implementation, cannot be removed from the protocol.

Code Snippet

removeToken() and removeAdapter() don’t check token support and perform the full cycle over moneyMarkets:

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

    function removeToken(address tokenAddress) external override onlyAdmin {
        ...

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

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

    function removeAdapter(address adapterAddress) external override onlyAdmin {
        ...

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

As an example, checkTokenSupported() modifier of AaveV3Adapter revert the operations when it's not supported (AaveV3Adapter's _getSupply() doesn't have it now, but that's a design choice):

https://github.com/sherlock-audit/2023-02-union/blob/main/union-v2-contracts/contracts/asset/AaveV3Adapter.sol#L90-L96

    /**
     * @dev Check supplied token address is supported
     */
    modifier checkTokenSupported(address tokenAddress) {
        if (!_supportsToken(tokenAddress)) revert TokenNotSupported();
        _;
    }

Tool used

Manual Review

Recommendation

Consider checking the support in both removeToken() and removeAdapter(), for example:

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

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

[hrishibhat]

there is no direct loss of funds as the ceiling can be set to zero in the case above. Considering this issue a low severity

[dmitriia]

re PR 107 in union-v2 repo:

removeAdapter() is ok, but removeToken() is still to be fixed:

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

    function removeToken(address tokenAddress) external override onlyAdmin {
        ...

        if (isExist) {
            for (uint256 i = 0; i < moneyMarkets.length; i++) {
-                if (moneyMarkets[i].getSupply(tokenAddress) >= 10000) revert RemainingFunds(); //ignore the dust
+                if (moneyMarkets[i].supportsToken(tokenAddress) && moneyMarkets[i].getSupply(tokenAddress) >= 10000)
+                   revert RemainingFunds(); //ignore the dust
            }