blutorque - `AssetManager::deposit()` not handling the case where `remaining` still true, as a result the deposited token will be lost forever in manager contract

[sherlock-admin4]

blutorque

High

AssetManager::deposit() not handling the case where remaining still true, as a result the deposited token will be lost forever in manager contract

Summary

Vulnerability Detail

An user can deposit via UserManager::stake() function, the amount is transfer from user to the AssetManager contract which supply these funds to corresponding lending markets through adapters.

If the deposit call below returns true, the txn is considered successful, https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/user/UserManager.sol#L756

        if (!IAssetManager(assetManager).deposit(stakingToken, amount)) revert AssetManagerDepositFailed();

Issue

Aave3Adapter has an edge case, if the supply() call to lendingPool fails for any reason, it transferred back the token amount to the AssetManager, returning false,

File: AaveV3Adapter.sol

    function deposit(
        address tokenAddress
    ) external override onlyAssetManager checkTokenSupported(tokenAddress) returns (bool) {
        IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
        uint256 amount = token.balanceOf(address(this));
        try lendingPool.supply(tokenAddress, amount, address(this), 0) {
            return true;
        } catch {    // <@ trigger on failure
            token.safeTransfer(assetManager, amount);
            return false;
        }
    }

This edge case is not handled by the AssetManager deposit() function, it is expected to return true in case the user amount deposited successfully to the lendingPool. However, the issue is it always return true even when the deposit was unsuccessful.

The following check in UserManager::deposit() which reverts for unsuccessful deposit get bypass, as a consequences, user deposited funds lost forever in the AssetManager contract.

        if (!IAssetManager(assetManager).deposit(stakingToken, amount)) revert AssetManagerDepositFailed();

Impact

Deposited assets will be lost in the AssetManager

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/asset/AssetManager.sol#L325

Tool used

Manual Review

Recommendation

The AssetManager deposit function should return !remaining, instead of default true. https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/asset/AssetManager.sol#L325

-        return true;
+        return !remaining; 
    }

If the amount successfully deposited to the lendingPool, the remaining will be set false, means assets deposited. And if the amount transferred back to the AssetManager, the remaining still true, means assets didn't got deposited, the check will revert the txn.

[twicek]

Escalate This is not an issue because sending tokens to money markets is optional, only if they are available as described by this comment above the deposit function:

     *  @dev  Deposit tokens to AssetManager, and those tokens will be passed along to
     *        adapters to deposit to integrated asset protocols if any is available.

If sending tokens to a money market fails, tokens are sent back from the adapter to the asset manager and the admin can try to send them again later. The stake function should never revert because sending the funds to a money market failed, this is why the deposit function of the AssetManager returns true.

[sherlock-admin3]

Escalate This is not an issue because sending tokens to money markets is optional, only if they are available as described by this comment above the deposit function:

     *  @dev  Deposit tokens to AssetManager, and those tokens will be passed along to
     *        adapters to deposit to integrated asset protocols if any is available.

If sending tokens to a money market fails, tokens are sent back from the adapter to the asset manager and the admin can try to send them again later. The stake function should never revert because sending the funds to a money market failed, this is why the deposit function of the AssetManager returns true.

You've created a valid escalation!

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.

[olaoyesalem]

Escalate This is not an issue because sending tokens to money markets is optional, only if they are available as described by this comment above the deposit function:

     *  @dev  Deposit tokens to AssetManager, and those tokens will be passed along to
     *        adapters to deposit to integrated asset protocols if any is available.

If sending tokens to a money market fails, tokens are sent back from the adapter to the asset manager and the admin can try to send them again later. The stake function should never revert because sending the funds to a money market failed, this is why the deposit function of the AssetManager returns true.

If sending money to a money market fails. Users don't stand to gain from their funds as no yield eill be generated also the funds will just be stucked there.

[olaoyesalem]

Escalate This is not an issue because sending tokens to money markets is optional, only if they are available as described by this comment above the deposit function:

     *  @dev  Deposit tokens to AssetManager, and those tokens will be passed along to
     *        adapters to deposit to integrated asset protocols if any is available.

If sending tokens to a money market fails, tokens are sent back from the adapter to the asset manager and the admin can try to send them again later. The stake function should never revert because sending the funds to a money market failed, this is why the deposit function of the AssetManager returns true.

Sending tokens is optional, but this does not mean the vulnerability cannot occur under certain scenarios. Therefore, I consider this a significant issue.

[mystery0x]

This finding should remain valid given the reasonings of olaoyesalem.

[twicek]

@mystery0x Did you ask the protocol why they disputed? This code has been like that for 2 years, the protocol's team was well aware that only true could be returned when depositing to the asset manager, this is a low severity report at best. Link to the exact same line of code 2 years ago: https://github.com/sherlock-audit/2022-10-union-finance/blob/6aeeaeebdbb01712bcc38e4339e1f62f9f6193d3/union-v2-contracts/contracts/user/UserManager.sol#L680

To reiterate why this is not an issue:

If sending tokens to a money market fails, tokens are sent back from the adapter to the asset manager and the admin can try to send them again later.

[vsharma4394]

I don't think there is any issue if any adaptor fails to deposit assets because those assets will be sent to the asset manager and when withdraw is called you can see in the following line that initially the balance of asset manager is checked to see if there is enough amount that can be filled or not. So from that it is pretty clear that the contract logic covers the case when deposit fails in any adpator. https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/asset/AssetManager.sol#L345

Plus the balance of staker is already increased before calling the asset manager therefore no loss to the stakers as well so this is an invalid issue.

[olaoyesalem]

stakes.The main reason while it funds is sent to the adaptor is for users to gain yield from their deposits. If it fails and it gets sent to the asset manager. The won't be any yield therefore there won't be any gain for the stakers.

Therefore this is a valid issue .

[vsharma4394]

stakes.The main reason while it funds is sent to the adaptor is for users to gain yield from their deposits. If it fails and it gets sent to the asset manager. The won't be any yield therefore there won't be any gain for the stakers.

Therefore this is a valid issue .

Stakers gain are calculated in Comptroller and is not related to the assets deposited in the adaptors. Stakers gain is not affected if adaptor fails to deposit due to which there won't be any yield.

[olaoyesalem]

If the adapter fails, it will impact the gains of the stakers. The reason is that the funds won't be distributed to the money markets, which defeats the primary purpose of the 'deposit' function. Without the adapter, the 'deposit' function would merely allow users to send their tokens directly to the contract without achieving its intended functionality.

[olaoyesalem]

Also the function doesn't have to revert (necessarily) it can just return false. So that admin can be notified and admin can rescue.

[c-plus-plus-equals-c-plus-one]

Just a quick comment: admins can rescue anyways. @olaoyesalem

[olaoyesalem]

But admins won't know which tokens to rescue and they can only know if false is emitted. Else admin will think the txn went fine and was sent to money markets

[WangSecurity]

Admins can know which tokens to rescue just if they check the contract balance. Secondly, here's a code comment inside the withdraw function, which clearly says the protocol is aware there can be funds lying in the contract and they can be withdrawn. Hence, I agree that the funds can be rescued in case the deposit fails and this report is then low severity.

Planning to accept the escalation and invalidate the report.

[olaoyesalem]

While I understand the perspective that this issue may not seem critical, I believe it deserves a medium severity classification, if not high. The primary functionality of the deposit function is to allocate funds to money markets to generate yield for users. If the adapter fails and funds remain in the AssetManager, this primary functionality is compromised, directly impacting user returns. Although funds are not technically "lost," they fail to generate the expected yield, representing a significant issue for users. Furthermore, relying on manual rescue operations increases operational complexity and the risk of human error, while automated systems with clear status feedback (true/false) ensure robustness and align with best practices in smart contract design. Therefore, this issue should be classified as medium severity, as it directly affects the core value proposition of the protocol and user expectations.

[olaoyesalem]

Admins can know which tokens to rescue just if they check the contract balance. Secondly, here's a code comment inside the withdraw function, which clearly says the protocol is aware there can be funds lying in the contract and they can be withdrawn. Hence, I agree that the funds can be rescued in case the deposit fails and this report is then low severity.

Planning to accept the escalation and invalidate the report.

Also this is my main argument

https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/9#issuecomment-2244735976

[dmitriia]

As far as I understand, the funds are moved back and forth between money market adapter and asset manager contracts until deposited, there is no issue with that.

If the funds remain undeposited after all the adapters are tried then the funds are left with the asset manager contract. The user deposit duties are deemed satisfied and their internal deposit is credited. So there is no loss for the user in this situation.

As for the asset manager it uses the balance for withdrawals, so the funds are not lost. It looks like there is no need to rescue anything manually.

Then, asset manager also uses the balance for rebalancing, so there is no substantial loss of interest revenue either, i.e. the team is supposed to monitor the balances and run the rebalance operation whenever any substantial idle funds are observed.

This way, while the suggestion can be implemented (there can be the requirement for successfull deposit to money market), but it doesn't solve any issues I'm aware of, and also it will forbid void money market setup, i.e. running Union without money market depositing. This way the issue by itself doesn't look valid.

[olaoyesalem]

Thank you for your response. While I understand the funds are moved back and forth between the money market adapter and asset manager contracts, I respectfully disagree with the assessment that there is no issue. Here are my key points:

Substantial Loss of Interest Revenue: You mentioned that the asset manager uses the balance for rebalancing, and the team monitors balances to run rebalance operations whenever substantial idle funds are observed. However, as you said yourself, the loss of interest revenue is indeed substantial. The timing of the rebalance operations is critical; as every block passes without the funds being deposited into money markets, users lose potential gains.

User Expectation and Core Functionality: The primary purpose of the deposit function is to allocate funds to money markets to generate yield. If the funds remain in the AssetManager due to failed deposits, this primary functionality is compromised, directly impacting user returns. Users expect their deposits to be actively generating yield, and any delay or failure in achieving this results in a loss of expected benefits.

Operational Complexity and Risk: Relying on manual monitoring and rebalancing introduces operational complexity and the risk of human error. An automated system that immediately indicates failed deposits and returns a clear status (true/false) ensures robustness and aligns with best practices in smart contract design. This proactive approach minimizes the time funds remain idle and ensures optimal yield generation for users.

Rebalancing Limitation: Even if the rebalance function is called, it relies on the availability of money markets. If no money markets are available, the issue of idle funds persists. The rebalance function attempts to redistribute the supply of a token according to specified percentages, but if the underlying money markets are not available or fail to accept the deposits, the funds will remain in the AssetManager, continuing the problem of lost user gains.

In summary, while the system has mechanisms to eventually correct idle funds, the interim period where funds are not generating yield represents a loss to the user. Therefore, this issue is valid as it directly affects the core value proposition of the protocol and user expectations.

[dmitriia]

Hi!

I kindly note that I said there is no substantial loss of revenue.

Also, protocol did not stated the invariant of being always invested, while Vault level manual rebalancing is a common practice, so nothing looks to be broken in this regard.

[olaoyesalem]

There can be substantial loss if it takes time for admin to call the rebalance function. After the deposit. And only when there's availability of money markets.

The main purpose of the deposit function is to transfer users funds to the money market which means investing. Else users can just directly send their token to the contract address. Or the deposit function won't contain addition of users token to the money markets.

I'll await the judge @mystery0x judgement. 😊.

[WangSecurity]

You mentioned that the asset manager uses the balance for rebalancing, and the team monitors balances to run rebalance operations whenever substantial idle funds are observed. However, as you said yourself, the loss of interest revenue is indeed substantial. The timing of the rebalance operations is critical; as every block passes without the funds being deposited into money markets, users lose potential gains.

I don't believe the "loss" of funds in such scenario is a considerable amount and doesn't qualify for Medium severity "exceeding small, finite amounts".

The primary purpose of the deposit function is to allocate funds to money markets to generate yield. If the funds remain in the AssetManager due to failed deposits, this primary functionality is compromised, directly impacting user returns. Users expect their deposits to be actively generating yield, and any delay or failure in achieving this results in a loss of expected benefits.

Answered in the above paragraph.

Relying on manual monitoring and rebalancing introduces operational complexity and the risk of human error. An automated system that immediately indicates failed deposits and returns a clear status (true/false) ensures robustness and aligns with best practices in smart contract design. This proactive approach minimizes the time funds remain idle and ensures optimal yield generation for users.

This is design recommendation rather than a security issue.

Even if the rebalance function is called, it relies on the availability of money markets. If no money markets are available, the issue of idle funds persists. The rebalance function attempts to redistribute the supply of a token according to specified percentages, but if the underlying money markets are not available or fail to accept the deposits, the funds will remain in the AssetManager, continuing the problem of lost user gains.

If Money markets cannot receive funds, then the funds shouldn't be deposited into money markets and remain in the Asset Manager to cover withdrawals. This is the design of the protocol.

The decision remains the same, accept the escalation and invalidate the issue.

[olaoyesalem]

Saying the loss of funds is not considerable is theoretical. Practically as stated above the loss of funds(gain) increases with every block.

Basically, all I'm saying is that users send funds through the deposit function just for them for yield gains by being in the money market and if that purpose is defeated as shown above. Then this is a vulnerability as the core functionality of the deposit function is Practically defeated .

If the deposit function didn't want the funds to go to the money market. Then the logic won't be in the deposit function. Which means that the deposit function will just take in funds from the user and transfer them to the AssetManager contract

Due to this I assume this is a valid issue, which can be a medium.

@WangSecurity

[olaoyesalem]

Scenario: A user deposits 100,000 USDC tokens The AssetManager receives these tokens and attempts to deposit them into money markets. Due to an issue with the money markets, the deposit fails, and the tokens are transferred back to the AssetManager. The AssetManager function returns true despite the failed deposit, bypassing the check . Impact calculation: Assume a conservative APY of 3% for USDC in Aave. Daily yield: (100,000 0.03) / 365 = 8.22 USDC per day If the rebalancing doesn't occur for a week: Lost yield: 8.22 7 = 57.54 USDC If the issue persists for a month: Lost yield: 8.22 * 30 = 246.6 USDC This scenario demonstrates that: The loss is not just theoretical but quantifiable. The loss increases with time until the issue is detected and resolved. The amount lost can be significant, especially for larger deposits or if the issue affects multiple users. The core functionality of the deposit function (generating yield for users) is indeed compromised.

This issue meets the criteria for a medium severity vulnerability because: It causes a loss of funds (in the form of unrealized yield). The loss is constrained (limited to the potential yield, not the principal). The amount lost can exceed small, finite amounts, especially for larger deposits or over longer periods. It requires specific conditions (failure in the money market deposit) to occur.

[WangSecurity]

Daily yield: (100,000 * 0.03) / 365 = 8.22 USDC per day

For one day the loss would equal only 0.008%.

Additionally, the funds that lay inside the AssetManager are not lying and doing nothing, they're used for withdrawals. So the protocol is ready for situations when funds are not in the money market. Also, the admin can rebalance the funds if it's needed.

Hence, I still believe it's low severity, planning to accept the escalation and invalidate the issue.

[olaoyesalem]

I agree with your judgment. Although the loss will be on the user's end. But you only talked about the loss for a day , what about the loss week or even month which is well feasible. Since the ADMINS won't know that the money market is filled up and will think the txn was successful. because it will always return true, Also note that in case of multiple users not depositing their money to money markets. The overall loss will be much. I also agree that the funds can be used for withdrawals but note that the funds deposited Through the deposit function is not mainly for that. (The funds are meant to generate yield in the money market) and if that is defeated that it should be of great concern. It will be great if you can review this. Thanks!

            // assumption: less liquid markets provide more yield
            // iterate markets in reverse to optimize for yield
            // do this only if floors are filled i.e. min liquidity satisfied
            // deposit in the market where ceiling is not being exceeded

   // assumption: markets are arranged in order of decreasing liquidity
            // iterate markets till floors are filled
            // floors define minimum amount to maintain confidence in liquidity

These comments are pulled out from the deposit function. Which shows that the main reason of depositing funds to through the deposit function is to gain yield.

If there is no gains to be yielded. Users can just send tokens directly to the contract through another function.

secondly

In the withdraw function this is an excerpt

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

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

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

In this excerpt , It iterates through all the market to check and retrieve the gains. And If eventually the user or users token was not put in the money market on time due to the money market being filled. The user will not have any or not enough gain to withdraw.

 if (moneyMarket.withdraw(token, account, withdrawAmount)) {
                    remaining -= withdrawAmount;
                }

Lastly Note that even when you claim that it can be rebalanced by the admin. Yes the token addresses can be rebalanced but not on behalf of the user

    /**
     * @dev Take all the supply of `tokenAddress` and redistribute it according to `percentages`.
     *
     * Rejects if the token is not supported.
     *
     * @param tokenAddress Address of the token that is going to be rebalanced
     * @param percentages A list of percentages, expressed as units in 10000, indicating how to deposit the tokens in
     * each underlying money market. The length of this array is one less than the amount of money markets: the last
     * money market will receive the remaining tokens. For example, if there are 3 money markets, and you want to
     * rebalance so that the first one has 10.5% of the tokens, the second one 55%, and the third one 34.5%, this param
     * will be [1050, 5500].
     */
    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));

        for (uint256 i = 0; i < percentagesLength; i++) {
            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];
        if (remainingTokens > 0) {
            token.safeTransfer(address(lastMoneyMarket), remainingTokens);
            lastMoneyMarket.deposit(tokenAddress);
        }

        emit LogRebalance(tokenAddress, percentages);
    }

As we can see in this code above there is no part where it was updated on behalf of the user. Which means that once the money markets are filled. New Users will not get any yelid on the token they deposit since it cannot be rebalance on their behalf

With All this I have proven that it is valid. It's up to the judge to choose the severity. @WangSecurity @mystery0x

[olaoyesalem]

Daily yield: (100,000 * 0.03) / 365 = 8.22 USDC per day

For one day the loss would equal only 0.008%.

Additionally, the funds that lay inside the AssetManager are not lying and doing nothing, they're used for withdrawals. So the protocol is ready for situations when funds are not in the money market. Also, the admin can rebalance the funds if it's needed.

Hence, I still believe it's low severity, planning to reject the escalation and leave the issue as it is.

Do you mean accepting the issue is valid?

[c-plus-plus-equals-c-plus-one]

@WangSecurity the escalation by @twicek indeed states that the issue is invalid or is of low severity. Shouldn't the escalation be accepted then? I think it should be either downgraded or rejected for sure.

[olaoyesalem]

Just a quick comment: admins can rescue anyways. @olaoyesalem

ADMIN cannot rescue!

[WangSecurity]

Yep, excuse me, I made a typo in the previous comment.

But you only talked about the loss for a day , what about the loss week or even month which is well feasible. Since the ADMINS won't know that the money market is filled up and will think the txn was successful. because it will always return true,

I believe 1 day is the most optimal we can assume the admins won't call rebalance. Not even because some deposits were unsuccessful but just to make sure the protocol is balanced appropriately.

Also note that in case of multiple users not depositing their money to money markets. The overall loss will be much. I also agree that the funds can be used for withdrawals but note that the funds deposited

Still, it will be a 0.008% loss for every individual user. Additionally, we're talking about deposits failing suddenly, i.e. "if the supply() call to lendingPool fails for any reason". So I don't think we should assume that the deposits will fail suddenly for a set of users.

Through the deposit function is not mainly for that. (The funds are meant to generate yield in the money market) and if that is defeated that it should be of great concern. It will be great if you can review this. Thanks

It doesn't change the fact that the funds that remain inside AssetManager are not lost (as claimed in this report). Hence, this report is factually incorrect and funds will not be lost.

In this excerpt , It iterates through all the market to check and retrieve the gains. And If eventually the user or users token was not put in the money market on time due to the money market being filled

Please stop using examples of the money market being filled. If the market is filled, you cannot put more funds in the market, I believe that's the definition of the word "filled". In the case of filled money markets funds must stay inside the Asset Manager, it's the design of the protocol.

As we can see in this code above there is no part where it was updated on behalf of the user. Which means that once the money markets are filled. New Users will not get any yelid on the token they deposit since it cannot be rebalance on their behalf

Again money market scenario is not viable, you cannot add anything when it's filled. And the funds are not allocated to the user because it's already allocated to the user in the deposit function here I believe, but of course correct me if it doesn't happen here.

The decision remains the same, planning to accept the escalation and invalidate the report.

[olaoyesalem]

Understood! that the funds are not lost .My point is users won't gain yield for for a day and the function shouldn't revert but just return false so the admins can know. But

If you say 0.008% (which is just a scenario and could be worst cases)loss of users funds is not enough to make it medium issue. We'll, it's your call. Thanks!

[WangSecurity]

Yep, I believe 0.008% doesn't exceed small and finite amounts as per Medium severity description.

Planning to accept the escalation and invalidate the issue.

[olaoyesalem]

Yep, I believe 0.008% doesn't exceed small and finite amounts as per Medium severity description.

Planning to accept the escalation and invalidate the issue.

It's 0.008% provided the apy is 3%..(as just an example).when the apy is more than 3% the loss will also be more than 0.08%

Scenario A user deposits 100,000 USDC tokens The AssetManager receives these tokens and attempts to deposit them into money markets. Due to an issue with the money markets, the deposit fails, and the tokens are transferred back to the AssetManager. The AssetManager function returns true despite the failed deposit, bypassing the check . Impact calculation: Assume a conservative APY of 5% for USDC in Aave. Daily yield: (100,000 * 0.05) / 365 = 13.70 USDC per day which is 0.013% loss This scenario demonstrates that: The loss is not just theoretical but quantifiable.

The amount lost can be significant, especially for larger deposits The core functionality of the deposit function (generating yield for users) is indeed compromised.

This issue meets the criteria for a medium severity vulnerability because: It causes a loss of funds (in the form of unrealized yield). The loss is constrained (limited to the potential yield, not the principal). The amount lost can exceed small, finite amounts, especially for larger de

I think this is a valid medium issue

[WangSecurity]

As you've seen, there are other arguments for making this issue low severity. The loss being larger than 0.01% depends on many constraints and it's not even consistent. The admin can call rebalance earlier, the APY can be lower, the tokens can be immediately used for the withdrawal, so they're not just lying in the contract. Moreover, if the contracts are not deposited in the market they have a purpose and are used for the deposit.

Hence, with all of these arguments altogether, I believe the issue is indeed low severity. The decision remains the same, planning to accept the escalation and invalidate the issue.

[olaoyesalem]

Understood that the money in the contract can also be used for withdrawals, which can extend the time before a rebalance is needed. Additionally, the admin can call rebalance if the money markets are not filled again (which means they've withdrawn money from the money market). However, we're essentially saying the same thing: the loss is inevitable once the money market is filled.

On Thu, Aug 1, 2024, 8:21 PM WangSecurity @.***> wrote:

As you've seen, there are other arguments for making this issue low severity. The loss being larger than 0.01% depends on many constraints and it's not even consistent. The admin can call rebalance earlier, the APY can be lower, the tokens can be immediately used for the withdrawal, so they're not just lying in the contract. Moreover, if the contracts are not deposited in the market they have a purpose and are used for the deposit.

Hence, with all of these arguments altogether, I believe the issue is indeed low severity. The decision remains the same, planning to accept the escalation and invalidate the issue.

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/9#issuecomment-2263801837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVLICIQJYDHCM52L2SMCCMDZPKDFHAVCNFSM6AAAAABK2LDUGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTHAYDCOBTG4 . You are receiving this because you were mentioned.Message ID: <sherlock-audit/2024-06-union-finance-update-2-judging/issues/9/2263801837 @github.com>

[WangSecurity]

My decision remains the same as explained in the previous comments. Planning to accept the escalation and invalidate the report.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• twicek: accepted