xiaoming90 - Hardcoded swap path

[github-actions[bot]]

xiaoming90

high

Hardcoded swap path

Summary

Hardcoded swap path within the reward collector contract would lead to a loss of assets when swapping the reward tokens for debt tokens.

Vulnerability Detail

Per Linr 61 below, the reward collector will always attempt to swap the entire reward balance (OP tokens) on its contract to the debt assets (alETH or alUSD) via Velodrome on the Optimism chain.

The reward tokens (OP) can potentially be sent to the reward collector by Reward Router or transferred directly into the contract by anyone.

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L61

File: OptimismRewardCollector.sol
57:     function claimAndDonateRewards(address token, uint256 minimumAmountOut) external returns (uint256) {
58:         require(msg.sender == rewardRouter, "Must be Reward Router");
59: 
60:         // Amount of reward token claimed plus any sent to this contract from grants.
61:         uint256 amountRewardToken = IERC20(rewardToken).balanceOf(address(this));
..SNIP..

Per Line 67 and Line 74 below, it was found that the routes for the swap are hardcoded, which will lead to a number of issues:

Issue 1 - Stuck reward tokens due to pools with insufficient liquidity

Assume there is a large amount of OP tokens (e.g., 1M OP) residing on the reward collector contract. If one of the pools in the routing path does not have sufficient liquidity to perform the swap for the "in-transit" tokens, then the swap will always fail and revert.

Note that there is no point in waiting for liquidity to be replenished in a pool, as there is no guarantee that there will always be LPs available to provide liquidity to a pool.

The OP tokens will be stuck in the reward collector contract with no way to extract them as there is no sweeping function for the owner to recover the OP tokens.

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L61

File: OptimismRewardCollector.sol
57:     function claimAndDonateRewards(address token, uint256 minimumAmountOut) external returns (uint256) {
58:         require(msg.sender == rewardRouter, "Must be Reward Router");
59: 
60:         // Amount of reward token claimed plus any sent to this contract from grants.
61:         uint256 amountRewardToken = IERC20(rewardToken).balanceOf(address(this));
62: 
63:         if (amountRewardToken == 0) return 0;
64: 
65:         if (debtToken == 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A) {
66:             // Velodrome Swap Routes: OP -> USDC -> alUSD
67:             IVelodromeSwapRouter.Route[] memory routes = new IVelodromeSwapRouter.Route[](2);
68:             routes[0] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000042, 0x7F5c764cBc14f9669B88837ca1490cCa17c31607, false, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
69:             routes[1] = IVelodromeSwapRouter.Route(0x7F5c764cBc14f9669B88837ca1490cCa17c31607, 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A, true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
70:             TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken);
71:             IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp);
72:         } else if (debtToken == 0x3E29D3A9316dAB217754d13b28646B76607c5f04) {
73:             // Velodrome Swap Routes: OP -> alETH
74:             IVelodromeSwapRouter.Route[] memory routes = new IVelodromeSwapRouter.Route[](2);
75:             routes[0] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000042, 0x4200000000000000000000000000000000000006, false, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
76:             routes[1] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000006, 0x3E29D3A9316dAB217754d13b28646B76607c5f04, true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
77:             TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken);
78:             IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp);
79:         } else {
80:             revert IllegalState("Reward collector `debtToken` is not supported");
81:         }

Issue 2 - Swap with bad slippage

When a pool has insufficient liquidity or imbalanced liquidity (e.g., the pool's reserve_1 is much larger than the pool's reserve_2), the slippage will be very bad. Since the swap's routing path is hardcoded, the reward collector contract is forced to relax its slippage control and perform a swap against these pools, which incurs large slippage, leading to a loss for the protocol/users.

Impact

Loss of reward tokens.

Code Snippet

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L61

Tool used

Manual Review

Recommendation

Consider allowing the Alchemix DAO to update the swap's routing path so that it can be adjusted according to market conditions. Alternatively, allow the unused reward tokens to be recovered from the contract.

[Hesnicewithit]

There is a slippage check. If the hardcoded path failed the entire transaction would revert, which would NOT mean that tokens are lost. Just means issue needs to be resolved and can be re executed

[SteveHarrington0]

@Hash01011122 @xiaoming9090 according to @Hesnicewithit comment transaction will revert . Yes but here is the vulnerability is that the reward tokens will be stuck inside the contract and as @xiaoming9090 mentioned there is no sweeping method to take back the reward tokens. And there will be no point in waiting for liquidity to increase in the pool. So I request @Hash01011122 or @xiaoming9090 to escalate this issue.

[xiaoming9090]

Escalate

There is a slippage check. If the hardcoded path failed the entire transaction would revert, which would NOT mean that tokens are lost. Just means issue needs to be resolved and can be re executed

Whether or not having a slippage check is not relevant to this issue. The point is that the slippage will be bad when a pool has insufficient liquidity or imbalanced liquidity. From here, there are only two options to resolve the issue:

1. Relax the slippage parameter to perform a swap against these Velodrome pools and incur large slippage, but at least the reward tokens (OP) will be swapped to some debt assets (alETH or alUSD).
2. Have the reward tokens (OP) remain stuck in the contract. Since the swap path is hardcoded and there is no function for the admin/DAO to sweep the reward tokens out, the reward tokens will remain stuck. As mentioned in the report, one might consider waiting for liquidity to be replenished in a pool. However, there is no guarantee that LPs will always be available to provide liquidity to the Velodrome pools.

For both of the above options, the users/protocols will incur some losses, or reward tokens will be stuck in the contract.

If the swap path can be reconfigured, the protocol could reroute the swap to other pools with sufficient liquidity to minimize the slippage loss. If the admin/DAO can sweep the reward tokens, the protocol could recover them and re-deploy a new OptimismRewardCollector contract with an updated swap path. Unfortunately, neither of these options is available.

[sherlock-admin3]

Escalate

There is a slippage check. If the hardcoded path failed the entire transaction would revert, which would NOT mean that tokens are lost. Just means issue needs to be resolved and can be re executed

Whether or not having a slippage check is not relevant to this issue. The point is that the slippage will be bad when a pool has insufficient liquidity or imbalanced liquidity. From here, there are only two options to resolve the issue:

1. Relax the slippage parameter to perform a swap against these Velodrome pools and incur large slippage, but at least the reward tokens (OP) will be swapped to some debt assets (alETH or alUSD).
2. Have the reward tokens (OP) remain stuck in the contract. Since the swap path is hardcoded and there is no function for the admin/DAO to sweep the reward tokens out, the reward tokens will remain stuck. As mentioned in the report, one might consider waiting for liquidity to be replenished in a pool. However, there is no guarantee that LPs will always be available to provide liquidity to the Velodrome pools.

For both of the above options, the users/protocols will incur some losses, or reward tokens will be stuck in the contract.

If the swap path can be reconfigured, the protocol could reroute the swap to other pools with sufficient liquidity to minimize the slippage loss. If the admin/DAO can sweep the reward tokens, the protocol could recover them and re-deploy a new OptimismRewardCollector contract with an updated swap path. Unfortunately, neither of these options is available.

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.

[WangSecurity]

Agree with the escalation. For this to happen, there should be large amount of tokens in the reward collector contract and lack of liquidity in the velodrome pools.

There is no guarantee for either the first or the second to happen, hence, I believe there is no guarantee this bug will occur. Therefore, medium severity is appropriate here.

Planning to accept the escalation and assign medium severity.

[Evert0x]

Result: Medium Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[gstoyanovbg]

@WangSecurity I'm not a Velodrome expert but can't the sponsor just add the necessary liquidity in order to release the stuck tokens ? If so, there won't be any stuck tokens or losses from excessive slippage.

Also, the accumulation of an excessively large sum in the collector doesn't seem very likely. The idea of the reward router is to distribute a certain amount of tokens over a specific period of time, as described in this comment from the sponsor. Additionally, there is off-chain functionality that performs the corresponding reward distribution regularly. In this line of thought, it doesn't seem realistic to have a large sum in the collector, on the order of 1M OP, which would lead to the scenario described in the report. In my opinion, the only way this could happen is if such a large sum is deposited into the collector through a direct transfer and then claimAndDonateRewards is invoked. But this scenario strongly suggests a mistake by the trusted admin. Additionally, this use case contradicts the way sponsors plan to use the contract as described in the public Discord.

For reference TVL of the corresponding Velodrome pools on OP:

OP/USDC pool - 5M TVL USDC/alUSD pool - 3.3M TVL WETH/OP - 8M TVL alETH/WETH - 5.3M TVL

[WangSecurity]

I agree with your analysis, but there is still no guarantee that there will always be liquidity or rewards will not stack up to 1M OP or more. In the edge case the funds can actually be lost. I agree there is low probability of this happening, but the impact can be very high (even not considering the admin mistake of sending too many tokens at once), or am I missing something?

[gstoyanovbg]

For now, I set aside the scenario of a direct transfer that is too large, which could qualify as an admin mistake.

Let's first consider the sponsor's example from the comment on Discord - 125k OP for a period of 6 months. That's 694 OP tokens per day. From the README file, we know there is a gelato keeper that will regularly execute the reward distribution function. I think we can assume that once a day is a realistic frequency. Therefore, the maximum amount of OP tokens that may need to be exchanged at once in this case is 694. Even if we assume that due to a problem with the keeper, a certain number of iterations are skipped, there still wouldn't be enough accumulated to cause a problem. Furthermore the users of the protocol have incentive to execute the distribute rewards function which is permissionless frequently because they receive the donated tokens. Following this logic, for 1M OP tokens to need to be exchanged at once, we would either need a huge amount of OP tokens to distribute or a very small period. If we maintain the same period as in the example - 6 months - this would mean that sponsors would need to have 1M 6 30 = 180M OP tokens to distribute, which is a truly huge amount. Currently, there are around 1 billion OP tokens in circulation, and it doesn't seem realistic at all for sponsors to have almost 20% of them for distribution as rewards.

If we assume that due to some error, the scenario still occurs where 1M OP tokens need to be exchanged in one iteration and there isn't enough liquidity, then the sponsor can add liquidity as other LPs do, to ensure that the exchange is successful and these OP tokens do not remain locked forever. If the sponsor does not have the necessary capital for this purpose, they can take some form of loan to add the necessary liquidity to the pool and release the tokens.

From what has been said so far, it turns out that this report describes an event that is extremely unlikely to occur, and if it does happen, it is almost certainly administrators mistake. But even if it does happen, there is a way for the tokens to be released. In other words, we do not have a loss of funds here, and the impact is Low

[WangSecurity]

To sum up, these are the reasons leading to accumulation of high rewards:

1. Large reward (sent by the owner, correct?).
2. Gelato keeper not working (for a period of time and not one day, correct?).
3. Users not distributing rewards (also for a period of time, correct?).

Moreover, if we trigger large slippage it will just revert, the only impact is that the tokens are stuck in the reward router, but can be distributed once there is enough liquidity?

Also tagging @xiaoming9090 for counter arguments.

[gstoyanovbg]

1. Large reward and too short period for distribution, because if the period is large enough the large reward wouldn't be an issue. As far as i understand from the sponsor's comment they should send the rewards.
2. correct, for long enough period of time depending on the distribution rate.
3. correct, for long enough period of time depending on the distribution rate.

Moreover, if we trigger large slippage it will just revert, the only impact is that the tokens are stuck in the reward router, but can be distributed once there is enough liquidity?

I didn't explain this part well enough. Tokens cannot remain stuck in the reward router contract because there we have the function 'sweepTokens,' through which the owner can withdraw the available tokens whenever they wish. It is true that if there is high slippage, there will be a transaction revert in the collector. The only way for tokens to remain stuck is if they are deposited in the collector via a direct transfer but can be released if the sponsors add the required liquidity.

[WangSecurity]

So even if there is not much liquidity in the Velodrome pools, the owner can withdraw tokens via and manually distribute/swap them on other exchanges (or withdraw, make new contract with new route and distribute tokens via it)?

[gstoyanovbg]

Correct, if the tokens are inside the reward router contract. Here you can see the sweepTokens function.

But if a large amount of tokens is sent to the collector contract through a direct transfer, then the only way to release it is to provide the necessary liquidity.

[SteveHarrington0]

Correct, if the tokens are inside the reward router contract. Here you can see the sweepTokens function.

But if a large amount of tokens is sent to the collector contract through a direct transfer, then the only way to release it is to provide the necessary liquidity.

The comment describes the mitigation of the issue. There could be various reasons for reduced liquidity, such as liquidity providers (LPs) not receiving the expected rewards. While this scenario is impractical, it is still possible and the impact can be significant. The vulnerability lies in the hardcoded swap path. There is a distinction between an administrative mistake and the administration being unaware of the issue. The admin will have no idea if this issue not raised.

[SteveHarrington0]

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

These are the limitations for a valid medium. I believe this issue satisfies both rules: it causes a loss of funds when tokens get stuck in the collector, and it requires external conditions such as reduced liquidity in pools.

[WangSecurity]

To distribute rewards, the token (for example, OP) is directly trnasfered (via transfer) into the collector. Then somehow rewards have stacked up (a large amount) cause no one (not gelato keeper, not users, not even admin) called claimAndDonateRewards OR liquidity on Velodrome has significantly reduced, so now every time we call claimAndDonateRewards it reverts and the reward is inside the collector.

The admin can call sweepTokens and they will get back that OP rewards?

And another question, what is the intended by protocol design flow to distribute rewards? They're sent to the Reward Router, then we call distributeRewards, the rewards are sent to the collector. Then we call claimAndDonateRewards and distribute rewards? And if the rewards are directly transferred into collector that's not intended and is either a user or an admin mistke (whoever sends the tokens). Correct?

And in that case, the admin can call sweepTokens and get all the rewards out from the collector?

[gstoyanovbg]

To distribute rewards, the token (for example, OP) is directly trnasfered (via transfer) into the collector. Then somehow rewards have stacked up (a large amount) cause no one (not gelato keeper, not users, not even admin) called claimAndDonateRewards OR liquidity on Velodrome has significantly reduced, so now every time we call claimAndDonateRewards it reverts and the reward is inside the collector. The admin can call sweepTokens and they will get back that OP rewards?

The sweepTokens function can only be used for the router because it's implemented there. It cannot be used to withdraw already sent funds to the collector. In case an excessively large sum somehow ends up in the collector, the only way to release it is by temporarily adding the necessary liquidity to the respective pools. So, even in this unlikely scenario (including an admin mistake), there is a way to rescue the tokens, and there's no loss of funds.

And another question, what is the intended by protocol design flow to distribute rewards? They're sent to the Reward Router, then we call distributeRewards, the rewards are sent to the collector. Then we call claimAndDonateRewards and distribute rewards? And if the rewards are directly transferred into collector that's not intended and is either a user or an admin mistke (whoever sends the tokens). Correct?

Correct. It is not known to me anywhere to be described that direct sending of tokens to the collector is an intended design choice. But regardless of whether it is or isn't an intended design choice, in my opinion, it's the responsibility of the trusted admin to assess whether there is sufficient liquidity in the respective pools to avoid issues with the swap operation. If we assume that indeed, under some currently unknown circumstances, such action is necessary, the admin can simply divide the token quantity into parts and send them one by one over a certain period of time to avoid issues. So, it's entirely within the control of the admin.

And in that case, the admin can call sweepTokens and get all the rewards out from the collector?

This is not correct. Explained above why we can't get tokens out from the collector using this function.

[WangSecurity]

Oh, wait, claimAndDonateRewards is called at the very end of distributeRewards, therefore, if the swap in the hardcoded path reverts, then the tokens are back into Reward Router (not Collector). Therefore, the rewards can be either withdrawn from the RewardRouter or another collector is used.

Hence, the only way that rewards get stuck in the Collector is if they're directly transferred into Collector (yes, it was said above, just getting all my thoughts in the correct order). But as I understand from the code, the intended design is to sent rewards into Reward Router, from there we transfer them into Collector and donate (in the same function), and if it reverts, the funds are back into Reward Router.

Therefore, it's indeed a mistake if the tokens are sent directly into Collector (bypassing Reward Router). Tagging both @gstoyanovbg and @SteveHarrington0 so you can say if it's wrong or correct.

[gstoyanovbg]

Correct.

[WangSecurity]

Then if the rewards are sent out in the way they should be, then the tokens will not be stuck in the collector. They will remain in the Reward Router, hence, can either be withdrawn by the admin or distributed through another collector.

Planning to invalidate the report.

[xiaoming9090]

Then if the rewards are sent out in the way they should be, then the tokens will not be stuck in the collector. They will remain in the Reward Router, hence, can either be withdrawn by the admin or distributed through another collector.

Planning to invalidate the report.

@WangSecurity Per the source code's comment, there are two (2) scenarios where the tokens/rewards can be sent to the collector:

1) Via the Reward Router during claiming, which you mentioned above. 2) Tokens sent directly to the collector via grants

https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L60

    function claimAndDonateRewards(address token, uint256 minimumAmountOut) external returns (uint256) {
        require(msg.sender == rewardRouter, "Must be Reward Router"); 

>>        // Amount of reward token claimed plus any sent to this contract from grants.
        uint256 amountRewardToken = IERC20(rewardToken).balanceOf(address(this));

Note that per Sherlock's judging rules, comments in the code must be respected. As such, this issue remains valid because, per the code's comment, it is possible that tokens will be sent directly into the collector, leading to the issue mentioned in this report.

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

[WangSecurity]

To clarify, if the protocol wins some "competition", they get the grant as the prize and it can be sent into collector directly, bypassing the Reward Router?

In that case, the funds can be stuck in the contract, it can be a large grant, or not enough liquidity. As @gstoyanovbg said above the admin can temporarily add liquidity, but the difference between the rewards inside the Collector and the available liquidity can be too large. I agree that constraints are extreme, but the loss can be extremely high as well, hence, I believe it qualifies for Medium severity.

@gstoyanovbg do you have any counter-arguments?

[xiaoming9090]

To clarify, if the protocol wins some "competition", they get the grant as the prize and it can be sent into collector directly, bypassing the Reward Router?

@WangSecurity Yes, per the code's comment, the reward token can be sent to this contract (Collector) from grants. In this case, it will bypass the Reward Router and the reward tokens will be directly transferred into the collector contract.

[WangSecurity]

Then, the scenario described in the report is still viable, the contraints are extreme, but still the loss of funds is possible. Hence, medium severity is appropriate.

[gstoyanovbg]

Xiaoming9090 is right, it seems there is indeed such a scenario according to the comment in the source code. But according this comment of the sponsors don't sound like they are aware about it. However, despite that, sending a large sum to the collector doesn't seem very logical because a malicious user could frontrun the transaction and become a depositor in the vault in order to take a profit from this huge donation. After this they can immediately withdraw their funds from the vault.

Correct me if I'm wrong, but in order to send such a huge grant to the collector, doesn't the admin have to specify that they want to receive this huge grant at the collector's address? If that's the case, then we have a mistake from a trusted admin.

I still don't understand how there could be a loss of funds even if a large amount is sent somehow to the collector. As I mentioned earlier, the admin can provide the necessary liquidity and thus release the tokens. No matter how large the difference between the available and required liquidity is, the admin can still add the difference and release the tokens. If we're talking in specific numbers, how large of a grant do we expect that the required liquidity couldn't be gathered even with a loan?

Also, we should consider this comment from the sponsor, with which I agree. In reality, this problem cannot be fixed by changing the path because there are no suitable pools available.

[WangSecurity]

@gstoyanovbg do I understand correctly that in that case, the admin (or even the user) can take a flash loan of tokens used in the path, add liquidity, distribute rewards, withdraw liquidity and return the flash loan in the same transaction?

[gstoyanovbg]

Correct, this is a possible solution. Also a loan can be obtained in many different ways, even off-chain. Additionally, users of the protocol would be interested in distributing this large quantity of tokens among themselves. I'm mentioning all this to clarify why, in my opinion, there is no scenario where a large amount of tokens would be locked in the collector without finding a way to gather the required liquidity to release them.

[xiaoming9090]

@gstoyanovbg do I understand correctly that in that case, the admin (or even the user) can take a flash loan of tokens used in the path, add liquidity, distribute rewards, withdraw liquidity and return the flash loan in the same transaction?

@WangSecurity flash-loan will not help to solve the issue. Let's assume an OP/alETH velodrome pool.

Scenario 1 - Stuck reward tokens due to pools with insufficient liquidity (similar to the example in the report)

Assume there is a lack of alETH in the OP/alETH pool. First of all, I'm not aware of any method that could borrow alETH for free. If it is not free, it will lead to a loss of assets for the party who borrows the alETH due to borrowing fee. Also, no one would lend alETH (both on-chain and off-chain) to someone for free in the market.

Secondly, even if there is a method of flash-loaning alETH for free, the transaction you mentioned above will eventually revert at the end. Assume you flash-loaned X worth of alETH and add liquidity to the pool. The swap is carried out and X worth of alETH is transferred out from the pool, and distributed out to the users. Then, when you remove liquidity from the pool, you will not receive X worth of alETH back (otherwise, you will be printing money out of thin air) and won't be able to repay the flash-loan (TX reverted).

Scenario 2 - Bad slippage

Please do not forget that this report identifies two main concerns, and not just about stuck assets. Quote from my above report:

When a pool has insufficient liquidity or imbalanced liquidity (e.g., the pool's reserve_1 is much larger than the pool's reserve_2), the slippage will be very bad. Since the swap's routing path is hardcoded, the reward collector contract is forced to relax its slippage control and perform a swap against these pools, which incurs large slippage, leading to a loss for the protocol/users.

In this case, where there is a bad slippage or imbalanced liquidity in a pool, adding liquidity into the pool will not help to improve the slippage in a pool because no one can manipulate the pool to improve a slippage during a trade. There is no profitable way of manipulating the pool to improve the slippage or re-balance the liquidity during a trade.

[WangSecurity]

Thank you for that response, @gstoyanovbg do you see a scenario where it doesn't require paying additional funds to remove slippage or distribute rewards?

[gstoyanovbg]

@WangSecurity @xiaoming9090

Scenario 1 - Stuck reward tokens due to pools with insufficient liquidity (similar to the example in the report) Assume there is a lack of alETH in the OP/alETH pool. First of all, I'm not aware of any method that could borrow alETH for free. If it is not free, it will lead to a loss of assets for the party who borrows the alETH due to borrowing fee. Also, no one would lend alETH (both on-chain and off-chain) to someone for free in the market.

From what I understand, the question here is how to obtain the necessary amount of alETH. Here are a few options:

1. AlchemicTokenV2Base is an upgradeable contract, so a function can easily be added to mint as many alETH tokens as needed.
2. There is a flashLoan function with an instant fee of 0 (we discussed it in another discussion), controlled by the admin. This can be used to obtain the required amount of tokens.
3. The Alchemic protocol can be used - just need to provide the necessary collateral and take out a loan in alETH.
4. Swap another token for alETH in another pool.

Also it is not required to borrow exactly alETH, another currencies/tokens will do the job too, at the cost of small fees for swaps.

Secondly, even if there is a method of flash-loaning alETH for free, the transaction you mentioned above will eventually revert at the end. Assume you flash-loaned X worth of alETH and add liquidity to the pool. The swap is carried out and X worth of alETH is transferred out from the pool, and distributed out to the users. Then, when you remove liquidity from the pool, you will not receive X worth of alETH back (otherwise, you will be printing money out of thin air) and won't be able to repay the flash-loan (TX reverted).

Are you suggesting that will be received the opposite token? That's not a problem; It could be exchanged in another pool for alETH.

Of course, taking out a loan costs money in most cases unless someone offers it to you without additional fees (e.g., a friend). When swap operations are performed, there are also swap fees, typically in the range of 0.05%-0.1% depending on the pool. But to demonstrate that these are insignificant amounts, we need to establish a framework for the amount expected to end up in the collector somehow. My question is, what value do we accept as realistic? For example, is it realistic for someone to deposit tokens worth $1 billion, or are we rather talking about sums on the order of $1-3 million? In my opinion, assuming the sponsor made a mistake in sending too many tokens directly to the collector, it's their responsibility to ensure the necessary liquidity to release the tokens, and this doesn't have much to do with this report. The fact is that it is possible to release them, and the cost will be minimal or zero depending on where the liquidity is sourced from. But if you think it's useful to discuss in detail all possible ways to obtain the necessary liquidity and how much that would cost, I have no objections as long as we define clear boundaries and talk with specific numbers.

In this case, where there is a bad slippage or imbalanced liquidity in a pool, adding liquidity into the pool will not help to improve the slippage in a pool because no one can manipulate the pool to improve a slippage during a trade. There is no profitable way of manipulating the pool to improve the slippage or re-balance the liquidity during a trade.

Regarding the second scenario, the protocol has slippage protection. If there is too much slippage, transactions will be reverted. The admin can simply choose not to use Velodrome and implement working with another Dex. I don't see why the maximum allowable slippage needs to be increased. By the way, if we have an unbalanced pool, wouldn't every bot want to balance it and profit from it? I'm not sure I understand what the problem is here, and I would appreciate it if someone could clarify.

[WangSecurity]

Firstly, @gstoyanovbg it seems like you offered to use another pool ("4. Swap another token for alETH in another pool.") and then use another DEX ("simply choose not to use Velodrome and implement working with another Dex") but the problem we discuss here is that it's not possible in the current implementation if the funds are locked inside the collector. But, with all the information provided, I believe I can make a decision.

The admins are trusted and assumed to act in the best interest of the protocol. Therefore, if there is a big grant, I believe it's their responsibility to choose the best path with sufficient liquidity. In that way, they will implement a collector with appropriate swap path and at this point it doesn't matter if they deposit to the reward router or the collector directly.

Moreover, the admins are able to either mint any amount of the alTokens to themselves to provide sufficient liquidity.

But, the main reason is that it's TRUSTED admin's responsibility to ensure that if they win the grant, their collector's swap route is appropriate and will not cause reverts or high slippage.

Decision is to invalidate the report.

[xiaoming9090]

@WangSecurity

The admins are trusted and assumed to act in the best interest of the protocol. Therefore, if there is a big grant, I believe it's their responsibility to choose the best path with sufficient liquidity. In that way, they will implement a collector with appropriate swap path and at this point it doesn't matter if they deposit to the reward router or the collector directly.

The protocol team can ensure that the Velodrome pool has sufficient liquidity (e.g., sufficient alTokens) during development and deployment. However, the protocol team cannot ensure that there will be sufficient liquidity in the Velodrome after the deployment of the collector because the market is fluid, and the Velodrome pool is not controllable by the protocol team.

Anyone can trade on the Velodrome pool at any point in time. It is possible that someone might have already performed a swap using the Velodrome pool (OP/alTokens), unintentionally or with malicious intent, leaving insufficient alTokens available for the collector to execute the swap when the grants arrived (reach to contract). The sending of grants to the protocol and executing the swap are often not atomic and are not done within a single transaction. As such, within the timeframe where the grants are sent and the swap executed, there is a risk that this issue will happen.

In addition, the issue of bad slippage is not addressed. No one can guarantee that the slippage in the pool remains low forever. The slippage can go up or down depending on the trading activity in the Velodrome pool and is not controllable by the protocol. When the slippage becomes high, the protocol is forced to accept the bad slippage, leading to a loss (mentioned in the report) because there is no option for the protocol to update the hardcoded swap path or extracted the stuck OP from the collector.

Moreover, the admins are able to either mint any amount of the alTokens to themselves to provide sufficient liquidity.

This is not a valid mitigation or the right approach to the problem. This will increase the total supply of alTokens, which in turn will decrease the alToken's value. This will lead to a loss for the existing alTokens holders or the wider ecosystem that relies on alTokens to transact.

[WangSecurity]

Thank you for this comments. But, as per the comment you've sent before here that it's the rewards + tokens sent from grants.

And let's take a look at comments in the Reward Router at L34 and L36. As I understand them, it is the vault who gets the grants, then they're sent to the collector, swapped and distributed. And I believe it explains the comment in the collector that the reward tokens and tokens from grants are sent to the collector from the vault and then distributed (the main reasoning is that in the reward router it says "If vault is set to receive rewards from grants", hence, it's the vault who receives the grant, not the collector). I agree that the comment in the collector is ambiguous and tricky, but still I believe it's the correct behaviour that the grants are sent into vaults rather than the collector.

In that sense, the reverts of claimAndDonateRewards will lead to funds remain in the vault. Therefore, to mitigate the issue when there is no liquidity we can 1) make a new collector with better path (since the funds are in the vault, 2) decrease the reward amount using setRewardAmount and gradually distribute portions of rewards (while this still can trigger slippage and alTokens value will differ with each iteration, other paths can be used via different collectors).

@xiaoming9090 do you agree with the assumptions in the second paragraph (about vaults and where the grants are sent) or there is something I miss?