AssetManager.balance() is in favor of the last money market, and might pass even when the sum of percentages is greater than 100%!

Summary

AssetManager.balance() is in favor of the last money market, and might pass even when the sum of percentages is greater than 100%! This happens because the function does not check whether deposit for a market succeed or not and will give all the remaining (including the failed deposit amount of previous markets) to the last market.

Moreover, it does not check whether the sum of all the percentages is no more than 100%. What's worse, the function might still succeed in this case, leaving this mistake undetected.

Vulnerability Detail

First, AssetManager.balance() does not check whether all the percentages will add together to be no more than 100%. This might get undetected since some market deposits might fail and the failure will get undetected since the return value of the following function is not checked:

 moneyMarket.deposit(tokenAddress);

As a result, their portions of the pie will be available for subsequent money market. For example, suppose we have three markets and the percentage is 70%, 40%. Although 70%+40% > 100%, an unreasonable distribution percentage. The execution might still go through: the first market deposit might fail and get undetected, and then the second market gets 40%, and the last market will get 60%. A result totally different from the intention of the user.

The more serious problem is, even when all the percentages sum is no more than 100%. The result will in favor of the last market since all previous deposit failure will contribute their portions to the last market.

Suppose we have three markets, A, B, and C. The percentages are 80%, 10%. That means, A should get 80%, B should get 10%, and C should get 10%. However, if deposit on A fails and deposit on B succeeds, then the result will be: A gets 0%, B gets 10%, and C will get 90%. Totally in favor of C, the last market.

Impact

AssetManager.balance() does not check whether all percentages add up to be less than 100%, and this might even get undetected.

Moreover, it is mostly in favor of the last market - it might get more shares than the intent of the user.

In both cases, the result might be different from what the user wants and become a surprise to the user.

Code Snippet

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

Tool used

Remix

Manual Review

Recommendation

We should calculate the percentage for the last market explicitly and also check the sum of percentages are no more than 100%.

function rebalance(
        address tokenAddress,
        uint256[] calldata percentages
    ) external override onlyAdmin checkMarketSupported(tokenAddress) {
        IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
        uint256 moneyMarketsLength = moneyMarkets.length;
        uint256 percentagesLength = percentages.length;

        IMoneyMarketAdapter[] memory supportedMoneyMarkets = new IMoneyMarketAdapter[](moneyMarketsLength);
        uint256 supportedMoneyMarketsSize;

        // Loop through each money market and withdraw all the tokens
        for (uint256 i = 0; i < moneyMarketsLength; i++) {
            IMoneyMarketAdapter moneyMarket = moneyMarkets[i];
            if (!moneyMarket.supportsToken(tokenAddress)) continue;
            supportedMoneyMarkets[supportedMoneyMarketsSize] = moneyMarket;
            supportedMoneyMarketsSize++;
            moneyMarket.withdrawAll(tokenAddress, address(this));
        }

        if (percentagesLength + 1 != supportedMoneyMarketsSize) revert NotParity();

        uint256 tokenSupply = token.balanceOf(address(this));

+        uint256 accumulativePercentage;

        for (uint256 i = 0; i < percentagesLength; i++) {
+           accumulativePercentage += percentages[i];
+           if(accumulativePercentage > 10000) revert PercentageExceedLimit();

            IMoneyMarketAdapter moneyMarket = supportedMoneyMarkets[i];
            uint256 amountToDeposit = (tokenSupply * percentages[i]) / 10000;
            if (amountToDeposit == 0) continue;
            token.safeTransfer(address(moneyMarket), amountToDeposit);
            moneyMarket.deposit(tokenAddress);
        }

-        uint256 remainingTokens = token.balanceOf(address(this));

        IMoneyMarketAdapter lastMoneyMarket = supportedMoneyMarkets[supportedMoneyMarketsSize - 1];
+     uint256 amountToDeposit = (tokenSupply * (10000-accumulativePercentage) / 10000;
-        if (remainingTokens > 0) {
-            token.safeTransfer(address(lastMoneyMarket), remainingTokens);
-            lastMoneyMarket.deposit(tokenAddress);
-        }
+      if(amountToDeposit > 0) {
+             token.safeTransfer(address(lastMoneyMarket), amountToDeposit);
+             lastMoneyMarket.deposit(tokenAddress);
+      }

        emit LogRebalance(tokenAddress, percentages);
    }

kingjacob commented 1 year ago

This is as designed. In a rebalance situation if this function is able to withdraw but then the deposit fails its better for "failed" deposits to end up in the last market, which is pure token rather than revert and leave those funds in the money market.

This is also an admin function, no one is calling this who doesnt know how it works.

hrishibhat commented 1 year ago

Agree with Sponsor comment. Not a valid medium/high

sherlock-audit / 2023-02-union-judging

chaduke - AssetManager.balance() is in favor of the last money market, and might pass even when the sum of percentages is greater than 100%! #2