EgisSecurity - StrategyPassiveManagerVelodrome.sol - `_addLiquidity` can be DoS'ed constantly

[sherlock-admin3]

EgisSecurity

high

StrategyPassiveManagerVelodrome.sol - _addLiquidity can be DoS'ed constantly

Summary

Vulnerability Detail

The protocol interacts with the Velodrome nftManager, when it attempts to _addLiquidity.

If we look at nftManager.mint:

 function mint(MintParams calldata params)
        external
        payable
        override
        checkDeadline(params.deadline)
        returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
    {
        if (params.sqrtPriceX96 != 0) {
            ICLFactory(factory).createPool({
                tokenA: params.token0,
                tokenB: params.token1,
                tickSpacing: params.tickSpacing,
                sqrtPriceX96: params.sqrtPriceX96
            });
        }
        PoolAddress.PoolKey memory poolKey =
            PoolAddress.PoolKey({token0: params.token0, token1: params.token1, tickSpacing: params.tickSpacing});

        ICLPool pool = ICLPool(PoolAddress.computeAddress(factory, poolKey));

        (liquidity, amount0, amount1) = addLiquidity(
            AddLiquidityParams({
                poolAddress: address(pool),
                poolKey: poolKey,
                recipient: address(this),
                tickLower: params.tickLower,
                tickUpper: params.tickUpper,
                amount0Desired: params.amount0Desired,
                amount1Desired: params.amount1Desired,
                amount0Min: params.amount0Min,
                amount1Min: params.amount1Min
            })
        );

        _mint(params.recipient, (tokenId = _nextId++));

        bytes32 positionKey = PositionKey.compute(address(this), params.tickLower, params.tickUpper);
        (, uint256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128,,) = pool.positions(positionKey);

        // idempotent set
        uint80 poolId = cachePoolKey(address(pool), poolKey);

        _positions[tokenId] = Position({
            nonce: 0,
            operator: address(0),
            poolId: poolId,
            tickLower: params.tickLower,
            tickUpper: params.tickUpper,
            liquidity: liquidity,
            feeGrowthInside0LastX128: feeGrowthInside0LastX128,
            feeGrowthInside1LastX128: feeGrowthInside1LastX128,
            tokensOwed0: 0,
            tokensOwed1: 0
        });

        refundETH();

        emit IncreaseLiquidity(tokenId, liquidity, amount0, amount1);
    }

The important part for this issue is at the end.

refundETH will attempt to transfer any ETH (native token) that the contract currently has, to msg.sender.

/// @inheritdoc IPeripheryPayments
    function refundETH() public payable override nonReentrant {
        if (address(this).balance > 0) TransferHelper.safeTransferETH(msg.sender, address(this).balance);
    }

At first glance this isn't a problem, as StrategyPassiveManagerVelodrome doesn't send any native tokens when calling the nftManager.

nftManager also can't receive native tokens, unless it's WETH that's calling the receive function.

receive() external payable {
        require(msg.sender == WETH9, "NW9");
    }

But there is another way to forcefully send native tokens to any address and that's using selfdestruct.

When a contract calls selfdestruct, its entire native token balance is sent to a specified address. Note that this is happens even if the contract can't receive native tokens, i.e missing receive/fallback or in the case of the nftManager it has a receive, but it only allows for WETH to call it.

Knowing this, if nftManager has any native balance, it will attempt to send it to msg.sender, in our case that is the StrategyPassiveManagerVelodrome.

This will make the tx revert, as StrategyPassiveManagerVelodrome doesn't have a receive/fallback function.

Because of this any calls to _addLiquidity can be DoS'ed at any time.

This is especially problematic for withdraw, as if the strategy isn't paused, it will call _addLiquidity, which as stated above can be DoSed.

The attack can be pulled off on any chain, as the attacker can simply spam deploying contracts, funding them with 1 wei, then self-destructing them and sending the funds to the nftManager.

This will be even easier to do when the protocol and Velodrome deploy on other chains other than OP, as the possibility of front-running a withdraw/deposit, makes the attack even easier to pull off.

Impact

DoS of _addLiquidity, which will directly DoS deposit/withdraw/setPositionWidth/unpause/moveTicks

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L28

Tool used

Manual Review

Recommendation

Add a receive function to the strategy and possibly add a onlyOwner withdraw function that will transfer out any native tokens that the strategy has accumulated.

[sherlock-admin3]

2 comment(s) were left on this issue during the judging contest.

WildSniper commented:

not high due to the following quote, "Inflicts serious non-material losses (doesn't include contract simply not working". And a valid medium due to this quote, "Breaks core contract functionality, rendering the contract useless or leading to loss of funds." really nice catch

DHTNS commented:

Invalid -> this kind of frontrunning is not possible all the time with an l2

[MirthFutures]

This cant happen on Base or OP where these contracts will exist. Low bug.

[sherlock-admin3]

Escalate

This is a valid Medium severity issue.

The attack does not rely on front-running whatsoever.

The attacker only has to forcefully send native tokens to the Velodrome pool, when the pool doesn't have a balance. He doesn't have to time the attack to DoS the protocol.

Example:

1. Attacker forcefully sends native tokens to the Velodrome pool.
2. When the pool has a balance, he will completely DoS Beefy, as the main functionality of _addLiquidity calls the Velodrome pool, which then will always attempt to refundETH, which will always revert, as stated in the issue.
3. Only when the pool calls refundETH on another address and succeeds will the pool be empty again, at which point the attacker will simply forcefully send native tokens again to continue the DoS on Beefy.
4. The impact is serious, because users won't be able to withdraw their funds, because on such action, contract try to reopen a position

The attacker only requires to know if the Velodrome pool is empty or not, if it's empty he will send native tokens to it, from this point, until the pool is empty again, almost all of Beefy's functionality will not be usable.

The attack is even more effective on Velodrome pools, with not much activity, as it will take more time for a refundETH call to succeed, as there are less tx's calling the pool.

To address the front running part in the above comment, front running isn't impossible in OP and as stated by OP themselves:

Once we decentralize the sequencer, whoever runs the sequencer would be able to determine the order of transactions, so some front running might be possible.

And:

Also, we may make the transaction pool public in the future.

If this happens, the attack becomes very easy to pull off and should even be considered High severity, but because currently, this isn't the case, this is a Medium severity bug and should be addressed.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[merlin-up]

Escalate

This is a valid Medium severity issue.

The attack does not rely on front-running whatsoever.

The attacker only has to forcefully send native tokens to the nftManager, when the contract doesn't have a balance. He doesn't have to time the attack to DoS the protocol.

Example:

1. Attacker forcefully sends native tokens to the nftManager.
2. When the contract has a balance, he will completely DoS Beefy, as the main functionality of _addLiquidity calls the nftManager, which then will always attempt to refundETH, which will always revert, as stated in the issue.
3. Only when the nftManager calls refundETH on another address and succeeds will the contract be empty again, at which point the attacker will simply forcefully send native tokens again to continue the DoS on Beefy.
4. The impact is serious, because users won't be able to withdraw their funds, because on such action, contract try to reopen a position

The attacker only requires to know if the nftManager is empty or not, if it's empty he will send native tokens to it, from this point, until the contract is empty again, almost all of Beefy's functionality will not be usable.

To address the front running part in the above comment, front running isn't impossible in OP and as stated by OP themselves:

Once we decentralize the sequencer, whoever runs the sequencer would be able to determine the order of transactions, so some front running might be possible.

And:

Also, we may make the transaction pool public in the future.

If this happens, the attack becomes very easy to pull off and should even be considered High severity, but because currently, this isn't the case, this is a Medium severity bug and should be addressed.

The functions that will be in the DoS:

1. deposit - Called during deposit to add all liquidity back to their positions
2. withdraw (when is not paused) - Withdraws the specified amount of tokens from the strategy as calculated by the vault
3. moveTicks - Function called to rebalance the position
4. setPositionWidth - Sets position width and readjusts positions
5. unpause - Unpause deposits, give allowances and add liquidity

Many of these functions can only be called during the calm period, but the user/manager will not be able to do this due to the DoS. The attacker can also conveniently choose the time to start and end this DoS. The attacker may not be a simple user, but a competitor of Beefy, i.e., another protocol.

[sherlock-admin3]

Escalate

This is a valid Medium severity issue.

The attack does not rely on front-running whatsoever.

The attacker only has to forcefully send native tokens to the nftManager, when the contract doesn't have a balance. He doesn't have to time the attack to DoS the protocol.

Example:

1. Attacker forcefully sends native tokens to the nftManager.
2. When the contract has a balance, he will completely DoS Beefy, as the main functionality of _addLiquidity calls the nftManager, which then will always attempt to refundETH, which will always revert, as stated in the issue.
3. Only when the nftManager calls refundETH on another address and succeeds will the contract be empty again, at which point the attacker will simply forcefully send native tokens again to continue the DoS on Beefy.
4. The impact is serious, because users won't be able to withdraw their funds, because on such action, contract try to reopen a position

The attacker only requires to know if the nftManager is empty or not, if it's empty he will send native tokens to it, from this point, until the contract is empty again, almost all of Beefy's functionality will not be usable.

To address the front running part in the above comment, front running isn't impossible in OP and as stated by OP themselves:

Once we decentralize the sequencer, whoever runs the sequencer would be able to determine the order of transactions, so some front running might be possible.

And:

Also, we may make the transaction pool public in the future.

If this happens, the attack becomes very easy to pull off and should even be considered High severity, but because currently, this isn't the case, this is a Medium severity bug and should be addressed.

The functions that will be in the DoS:

1. deposit - Called during deposit to add all liquidity back to their positions
2. withdraw (when is not paused) - Withdraws the specified amount of tokens from the strategy as calculated by the vault
3. moveTicks - Function called to rebalance the position
4. setPositionWidth - Sets position width and readjusts positions
5. unpause - Unpause deposits, give allowances and add liquidity

Many of these functions can only be called during the calm period, but the user/manager will not be able to do this due to the DoS. The attacker can also conveniently choose the time to start and end this DoS. The attacker may not be a simple user, but a competitor of Beefy, i.e., another protocol.

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.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/beefyfinance/cowcentrated-contracts/pull/20/files

[spacegliderrrr]

The escalation is factually incorrect. The attacker would not need to send funds to the pool, but rather to the NFTManager contract. Any user interaction with the NFTManager would resolve the issue. Here you can check how many daily transactions there are: https://basescan.org/address/0x827922686190790b37229fd06084350E74485b72

Hence, to cause a DoS to the Beefy stategy, a user would have to send eth after each and every transaction with the NFTManager. Which would also not guarantee it, due to the FIFO way OP/Base sequencer work.

Note that Head of Judging was advised prior to evaluating the severity. Given that none of the functions are time-sensitive, the issue is of low severity.

[merlin-up]

@spacegliderrrr I have updated the escalation comment to be correct since the escalation period has not yet ended. Your comment states: Any user interaction with the NFTManager would resolve the issue, but this will not actually resolve the issue. I believe it is not advisable to rely on someone else interacting with the NFTManager, i.e., calling the mint/increaseLiquidity functions, as interactions with the NFTManager can vary at different times. We also need to recognize that not all calls from other users who invoke these functions will be successful, as they too might not have a receive function, and their transactions might fail as well. It should also be understood that the attacker will send 1 wei to the NFTManager again, thereby prolonging this attack. The attacker may not be a simple user, but a competitor of Beefy, i.e., another protocol.

You also mentioned that no function is time-sensitive, but in the updated comment, I listed the functions that will be subject to the DoS - the deposit, withdraw, moveTicks, setPositionWidth, and unpause functions, which are almost all functions of the StrategyPassiveManagerVelodrome smart contract. If we put all this information together, I believe this is a serious vulnerability for the protocol and its users.

[0xdeth]

To add several points to the validity of the issue.

1. It does affect time-sensitive functions, as moveTicks is time-sensitive. The longer it is DoS'ed, the longer the protocol can't rebalance its ticks.
2. The attack relies on nothing and requires no setup, because the Beefy strategy has no receive/fallback, an attacker can technically continue doing the indefinitely. This can very easily be automated by a bot.
3. The README of the contest states:

   On what chains are the smart contracts going to be deployed? any EVM-compatible network

While this is technically not true for Beefy's Velodrome strategy, as Velodrome will be deployed only on chains on the OP Superchain (for now), we can conclude that wherever Velodrome is deployed, Beefy will deploy the strategy on that chain as well.

Some of these chains have much less activity and popularity than others, which makes this attack much more effective.

Even on more popular chains, even if the DoS is shorter, in the long-term the attack will compound and have a bigger impact, as stated in point 2, the attack can technically be pulled off indefinitely, as there is no way to stop a malicious actor from making the attack.

4. As stated in the Sherlock docs for Medium severity issues

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

The issue describes how an attacker will DoS _addLiquidity, which for all intents and purposes, is the most important function of the protocol, as it is called in the 3 of its most important functions: deposit, withdraw, and moveTicks.

With all these points in mind and the fact that the attacker requires no setup and can continuously DoS Beefy's strategy to his heart's content, I believe this should be a Medium severity issue.

[spacegliderrrr]

It does affect time-sensitive functions, as moveTicks is time-sensitive. The longer it is DoS'ed, the longer the protocol can't rebalance its ticks.

moveTicks is not time-sensitive. Even if a position is out of range, not rebalancing for even an hour would not have any serious impact. Furthermore, rebalancer can always batch 2 transactions to guarantee they move the ticks.

While this is technically not true for Beefy's Velodrome strategy, as Velodrome will be deployed only on chains on the OP Superchain (for now), we can conclude that wherever Velodrome is deployed, Beefy will deploy the strategy on that chain as well.

Pure speculation. Not guaranteed that the contracts will be the same on different chains (if they even deploy on different chains). In fact, Base and Op deployments have their differences. We're working with what we have, not what we suppose we might have in the future.

The issue describes how an attacker will DoS _addLiquidity, which for all intents and purposes, is the most important function of the protocol, as it is called in the 3 of its most important functions: deposit, withdraw, and moveTicks.

Described above why moveTicks is not actually bricked. Same for withdraw - an admin can pause the strategy to make sure all withdraws go through at any time.

Anyhow, thanks everyone for making your case clear, let's kindly wait for HoJ to review everything and come to a conclusion

[IWildSniperI]

the problem here isn't preventing people from withdrawing the point here is that beefy's core functionality is farming liquidity giving higher yield, now tell me about APY when constantly preventing moveTicks

Pure speculation. Not guaranteed that the contracts will be the same on different chains (if they even deploy on different chains). In fact, Base and Op deployments have their differences. We're working with what we have, not what we suppose we might have in the future.

yes we work with what we have and we have this quote in the readme plus the contracts

"On what chains are the smart contracts going to be deployed? any EVM-compatible network"

[dhtns2122001]

The bug is low at best -

1. refundEth() is a public function which can simply be called first on the NFTManager by moveTicks() bot . Not to mention there are many bots keeping their eyes on NFTManager (free money). So if attacker tries to block the moveTicks function by constantly giving 1 wei to NFTManager then bot runners can very easily catch on to what's happening and call refundETH and then interact with moveTicks in same txn.
2. let's say the bots do nothing to prevent this attack, it is still dependant on order of txns on an L2. See-

consider the following 2 scenarios- scenario-1 a. Attacker donates 1 wei b. moveTicks is called ( successful DOS)

If in next block or same block someone does a write function on NFTManager(two other write function makes its balance 0) then the attacker will need to frontrun the moveTicks caller again . That's why its frontrunning dependant which is not guaranteed on Optimism , as Velo only exists on optimism(super chain) . Certainly does not exist on L1.

scenario-2 a. moveTicks is called b. Attacker donates 1 wei (not successful)

And if some attacker starts donating in every block(even with no economic incentives) the bots can easily do as mentioned in point 1.

"Giving higher yield" is not a functionality. Its a feature . The supposed "exploit" is only possible if admins make errors continously in bot management. Even then its not guaranted.

[0xdeth]

refundEth() is a public function which can simply be called first on the NFTManager by moveTicks() bot . Not to mention there are many bots keeping their eyes on NFTManager (free money).

This is an assumption that there are outside bots that will call the function, which is entirely OOS. Even if such bots existed, why would they call the function when the profit will be 1 wei? The gas costs for simply calling refundETH to get the 1 wei is so much more compared to the profit they will make. Bots are made with profit in mind and it makes no sense for someone to make such a bot.

So if attacker tries to block the moveTicks function by constantly giving 1 wei to NFTManager then bot runners can very easily catch on to what's happening and call refundETH and then interact with moveTicks in same txn.

The supposed "exploit" is only possible if admins make errors continously in bot management.

This means that the protocol would change their bog, which is a future assumption and cannot be taken as an argument and is OOS.

That's why its frontrunning dependant

I reiterate again, that the attack is not depending on front running. An attacker can make a bot that forcefully sends 1 wei to the NFTManager every time that his balance is 0, then he just waits for someone to interact with Beefy's Velodrome strategy and that user will be DoSed, he can do this as much as he wants, there is nothing to stop him.

Let's assume that either mint or increaseLiquidity is called every 1 minute on the NFTManager.

1. NFTManager balance = 0.
2. Attacker sends 1 wei to the NFTManager.
3. For 1 minute almost all functionality on Beefy is unusable. deposit, withdraw, moveTicks, setPositionWidth, and unpause are unusable.
4. mint or increaseLiquidity is called and refundETH is called, so NFTManager balance = 0, again.
5. Attacker sends 1 wei to the NFTManager again and the cycle continues.
6. The attacker's tx's aren't reliant on front running or back running, whenever the 1 wei is sent, then there is a 1 minute window where Beefy's strategy is unusable.

There is no way to stop this attack from continuing endlessly. The attacker might DoS 10 Beefy transactions in a minute, he might DoS 1, he is still attacking the protocol and there is nothing stopping him from continuing.

Beefy is under constant DoS attacks, which might not have a large impact in the short term, but in the long run, the total time that Beefy is DoSed will be greater and it will start affecting more and more users. which directly affects Beefy.

[merlin-up]

We need to understand that we will have multiple StrategyPassiveManagerVelodrome smart contracts (not just one), which means the DOS vulnerability will be present in the deposit, withdraw, moveTicks, setPositionWidth, and unpause functions in all StrategyPassiveManagerVelodrome contracts deployed by the Beefy protocol.

From the comments, some arguments can be highlighted on how to avoid DOS:

1) Any user interaction with the NFTManager mint and increaseLiquidity functions would resolve the issue. The attacker will send 1 wei to the NFTManager again, thereby prolonging this attack. So, no, this will not resolve the issue.

2) An admin can pause the strategy to ensure all withdrawals go through at any time. In this case, all functions that call the _addLiquidity internal function will also be paused (deposit, moveTicks, setPositionWidth). This means that to simply withdraw tokens from the strategy, the smart contract must be completely paused. (Reminder, there will be more than one StrategyPassiveManagerVelodrome contract). I don't think pausing the strategy is the best idea.

3) Rebalancer can always batch 2 transactions to guarantee they move the ticks. Beefy will launch a bot that will keep moving the ticks at a certain cadence. However, since the Beefy protocol was unaware of this issue (they are aware now because Watsons submitted reports about it), they wouldn't have been able to react very quickly, and damage would have already been done. I don't think sending batched transactions is the best idea.

4) Many bots keeping their eyes on NFTManager to withdraw 1 wei from it. I don't think so.

The only proper solution for this issue is to add the receive() function to the StrategyPassiveManagerVelodrome contract, and then there is no need to come up with methods to avoid this attack.

[dhtns2122001]

Sers please stop twisting my statements I have never said -

Many bots keeping their eyes on NFTManager to withdraw 1 wei from it

what i said was -

Not to mention there are many bots keeping their eyes on NFTManager (free money).

notice the difference, free money means arbitrage . Velodrome/Uniswap NonfungiblePositionManagers are like second home to arbitraguers. Please look at number of txns

https://optimistic.etherscan.io/address/0xbB5DFE1380333CEE4c2EeBd7202c80dE2256AdF4

And every mint/IncreaseLiquidity and some multicall txns(pure bots) make the contracts eth balance 0.

And sers, I am not a villain, not "coming up" with reasons my reasons have been the same, only a low on an l2 but medium on l1. It's a finding that needs fixing for sure but its not a medium by sherlock standards imo . Rest is surely up to the judges.

[0xdeth]

@dhtns2122001 I'm not twisting your words and no one is vilifying you, you are giving your arguments and we are giving ours, this is an escalation after all.

And every mint/IncreaseLiquidity and some multicall txns(pure bots) make the contracts eth balance 0.

I agree and I gave a realistic example how the attack still works and will continue to work. The time that the protocol will be DoS'ed in the long term in will add up and be a bigger and bigger problem.

Let the HoJ decide, we've all gotten our points across.

[nevillehuang]

@0xdeth @spacegliderrrr @dhtns2122001 The likelihood of long term DoS seems to be dependent on how the outside possibility of "bots" can function. This comment provides a counter argument of how this can be mitigated. So both would be considered assumptions.

This comment here also makes it seem very unlikely for an attacker to constantly perfectly time deployment + selfdestruct to DoS users long-term, given there are large volume of transactions.

Is there any time-sensitive impact for such a DoS?

[spacegliderrrr]

None of the functions mentioned are time-sensitive

The DoS’ed functions are deposit/ withdraw/ moveTicks/ changePositionWidth/ unpause.

Note that moveTicks and changePositionWidth are admin-only functions which could still be executed by simply bundling 2 transactions (first to remove the eth from the NFTManager)

[dhtns2122001]

In full agreement with @spacegliderrrr 's comments .

[merlin-up]

@nevillehuang

Respectfully, the moveTicks() function is time-sensitive. From README file: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)? The is rebalancer that will keep moving the ticks at a certain cadence.

It's like a zero-day issue, and the attack can occur before the off-chain mechanism (rebalancer) starts bundling two transactions.

The Beefy protocol potentially will have multiple StrategyPassiveManagerVelodrome smart contracts with different LP tokens. Therefore, there is no need for front-running; continuing to send 1 wei to the NFTManager.

Suppose Beefy protocol deploys 10 StrategyPassiveManagerVelodrome with different LP tokens:

1) The attack will aim to DoS deposit, withdraw, moveTicks, setPositionWidth, and unpause in all 10 strategies. Examples of DoS attacks will last for a maximum of 1 minute, in my opinion is OOS, as transactions on the Optimism chain in nftManager often have differences in time of several minutes. Relying on a user to call mint in NFTManager at the exact moment of the attack and send their transaction from StrategyPassiveManagerVelodrome in the same block is not a solution but a future assumption.

2) The attack will aim to DoS moveTicks, which is called by the rebalancer at a certain cadence. Therefore, the rebalancer will move ticks for all 10 strategies at certain intervals, and the attack will likely occur during this time.

[dhtns2122001]

The optimistically maximum time the attacker will be able to DOS will be like 30 secs(Even this is not guaranteed, look at the no. of txn for context). If the rebalancer keeps on retrying reverting txns it'll go through within a minute at max. + how will the attacker know when the moveTicks() call from beefy is coming? optimism does not have a public mempool. Beefy already has resistance to IL by having an alt position. The impermanent loss even in the most optimistic case will be dust amount. + the issue of having some small impermanent losses is known to liquidity providers & is inherent to AMM positions. So the impact is-

1. No funds at risk, maybe dust amount of IL
2. Max 30 secs of griefing on a gelato bot(even with no batching)

This kind of DOS attack is very easy to write theoretically but is a nigh impossible task to successfully execute on optimism unless beefy insider is the attacker. Historically sherlock has judged such frontrun dependent griefing vuln as low on L2s. Same is the case for IL in this contest look at the comments from wildSniper in #19 which is a simliarish vuln that leads to IL .

[dhtns2122001]

In a nutshell even this dust IL is only possible when the attacker donates before Beefy bot && no one else interacts with NFPostionManager contract before beefy bot does. hence it is dependent on order of txns in a optimism block which only op sequencer can do.

[0xdeth]

@nevillehuang

The likelihood of long term DoS seems to be dependent on how the outside possibility of "bots" can function. This https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/69#issuecomment-2148109944 provides a counter argument of how this can be mitigated. So both would be considered assumptions.

How can this be taken as an argument? The rebalancer is first, OOS and second it's a future assumption. This is like saying saying: "The issue can be easily mitigated if Velodrome remove refundETH". How can such a comment be taken as an argument for the validity of an issue? Sure it'll mitigate the issue, but how isn't it OOS and a future assumption, two things that cannot be taken into account when validating an issue?

This comment https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/69#issuecomment-2144031868 also makes it seem very unlikely for an attacker to constantly perfectly time deployment + selfdestruct to DoS users long-term, given there are large volume of transactions.

For the nth time, he doesn't have to be perfect and the attack doesn't rely on front-running.

I've explained this multiple times already here and I've given a realistic example of how it works.

@dhtns2122001 and @spacegliderrrr focus only the DoS on moveTicks and completely disregard the fact that almost the entire strategy is unusable during the time of the DoS. The attack will DoS deposit, withdraw, moveTicks, setPositionWidth, and unpause. The attacker doesn't have to time anything, by constantly sending 1 wei he will DoS Beefy for longer periods of time and the attack will be more and more impactful the longer he continues to do so.

Considering Beefy's strategy is based around this "rebalancing" and calling moveTicks to get the perfect possible yield, the DoS that affects the rebalancing will start to stack up and will lead to a bigger and bigger loss the longer the attacker continues.

I repeat once again, the attacker can continue pulling off the attack endlessly and he doesn't have to time anything and is not front run dependent. I've given an example of how this would work and will continue to work.

I'm getting tired of repeating the same thing over and over again. Let @nevillehuang decide and be done with it.

[dhtns2122001]

3. For 1 minute almost all functionality on Beefy is unusable. `deposit, withdraw, moveTicks, setPositionWidth, and unpause` are unusable.

@0xdeth I have disregarded your example because it is written on a fantasy blockchain . No EVM compatible chain has a block spawn time of 1 min . the slowest one is 12 seconds ,

Secondly it shows you do not understand how blocks & txns work. In your fantasy scenario lets go to step 2.

2. Attacker donates 1 wei.
3. Now what if in the same block Alice interacts with the NFPositionManager and its balance is again gone to 0.
4. Now all beefy users interact in the same block.

How will you make sure alice does not interact with the positionManager? This is possible on L1 by negating the effects of alice interaction and backrunning her and donating again but not possible on optimism. Hence for the nth time i say it is dependent on order of txn and max possible dos you can achieve is less than a minute. If you are confident please maintain https://basescan.org/address/0x827922686190790b37229fd06084350E74485b72 & https://optimistic.etherscan.io/address/0xbB5DFE1380333CEE4c2EeBd7202c80dE2256AdF4 contract's balance greater than 0 for 1 minute multiple times a day .

[dhtns2122001]

anyways said enough unsubbing from the issue.

[DenTonylifer]

Given that there will be multiple instances of StrategyPassiveManagerVelodrome, this increases the likelihood of successfully performing a DoS attack on the main functions of some strategy smart contracts. Additionally, the moveTicks function needs to be called at a certain cadence in all StrategyPassiveManagerVelodrome instances, and DoS attack can prevent this in some strategies. From my experience, I would say that escalation is valid.

[Dliteofficial]

@DenTonylifer you should probably follow the chain of discussion on this issue discussing its validity. The multiple instances of SPMV deployments, the moveTicks call frequency etc.

[nevillehuang]

I agree with @dhtns2122001 and @spacegliderrrr comments, I believe it is difficult to continuously perform such a DoS on chains without a public mempool, especially when there are transactions occuring < 1 minute (could be seconds) for the contracts. With the next user interaction, the DoS will be lifted, so it is unlikely that permanent DoS would occur.

The question here is would DoSing moveTicks() or any other functions involved for seconds-minutes have a time sensitive impact that results in loss of funds? Could you guys please explain in more detail why it is important to call this potential functions promptly that results in protocol specific impact leading to loss of funds/broken core contract functionality

[NicolaMirchev]

Hey, @nevillehuang

We believe that all the above statements are all true. We can also agree that the contract should be changed and this is a real issue. Here we enter in deep arguments with arbitrary possibilities. Something leading to loss of funds with such an attack path can be executed at any time by anyone and should be considered Critical severity. I believe Medium is a realistic severity here because there is a clear attack path, which execution could temporarily prevent users from interacting with the protocol (deposit/withdraw) and most importantly admin function moveTicks().

• Theoretically, we have chains, which are op-based with Veldrome being deployed and small user interaction. It is technically possible for a machine to constantly execute actions of sending 1 wei to a contract for a week, which is "DoS user withdrawing their funds, for at least a week" (a Sherlock requirement for an issue being Medium The issue causes locking of funds for users for more than a week). Here is example of attack cost for sending 1 wei for one week every:

• • second -> $2540 for sending wei(1 wei 604800) + $12 096 (~$0.02 for gas 604800) = $14 636
• • 5 seconds -> $508 for sending wei(1 wei 120960) + $2419.2 (~$0.02 for gas 120960) = $2 927.2 As you can see the cost is not unrealistically high. Hacker organisations have billion of funds for exploiting projects. Even for a malicious millionaire it may be worth to act bad.

• Another thing is that we prevent the core functionality of moving the ticks, which could result in the position going out of range and user funds not collecting fees, which is a broken core contract functionality because of an exploiter's actions

[0xdeth]

Hey, @nevillehuang

To answer your second point: moveTicks and setPositionWidth are used to rebalance the ticks (they call _setTicks which is the function that sets the ticks) of the strategy to achieve maximal yield of the position. If we assume that the rebalance always tries to achieve maximum yield, then DoSing the functions will lower said yield. This will compound over time, as the attacker can DoS the functions hundreds or thousands of times, which in the long term will affect the strategy as it would have accumulated less fees. Considering this is what the strategy is build around, optimizing fees, this does break the most important functionality of the strategy.

@NicolaMirchev already answered the first point and how the DoS can be achieved at a higher frequency.

One thing to keep in mind, is that Beefy will have a strategy for each of Velodrome's concentrated pools, which are currently 36, we can assume that there will be more in the future as the protocol grows. With this attack, he affects 36 strategies, which means 36 instances of _setTicks, deposit, withdraw, unpause. The most important being _setTicks and withdraw, as _setTicks is the core of the contract and DoSing it would result in a lower yield and withdraw is the most important user facing function, as if it's DoSed, user funds will be frozen.

As explained by this comment, If an attacker is constantly sending wei to the NFTManager he will increase his chances of DoSing all the strategies, which means with a linear increase of his funds, he does exponentially more damage.

[merlin-up]

Firstly, I would like to clarify that sometimes there is a difference in time between transactions in the NonfungiblePositionManager smart contract, ranging from a few minutes to even cases where it's 10-15 minutes (Optimism chain). While generally the time difference between transactions is small, for moveTicks to not fail, it's necessary for the specific mint/increaseLiquidity call to occur before the moveTicks call, as otherwise, an attacker could simply send 1 wei again. Interactions with the NFTManager can vary at different times.

Regarding the moveTicks() function, it is called by the rebalancer to rebalance the position. Specifically, it removes liquidity from the main and alternative positions and adds the removed liquidity to the new range of those positions based on the current tick. According to the readme, the protocol should be able to work with any standard ERC20 tokens. Therefore, such a situation can easily occur with any volatile tokens or tokens in a low TVL pool. Additionally, since the same token can be in different strategies, the rebalancer will need to move the ticks in all strategies, which will be more challenging.

If moveTicks() is not called in time, users will experience out-of-range impermanent loss (the balance is always overweighted in the underperforming asset) and stop receiving yield.

[dhtns2122001]

I recommend watsons to do their due dilligence before putting out anything on a public escalation . Velodrome exists on MODE , BOB, but this beefy code can not work on those chains cause velodrome on MODE and BOB is starkly different from velodrome/ aerodome on OP/Base . NonFungiblePositionManager does not even exist on MODE/ BOB , please check on official docs Even if it did its a serious fantasy to assume no one will interact with it for 7 days.

No. of strategies existing does not even matter cause they interact with the same PositionManager.

As far as DOS moveTicks() is concerned two main points-

1. How will the attacker know when it's gonna be called? OP / base no public mempool? if he does As watsons say and keep sending 1 wei twice every block = 1wei / second on base Would cost him 86,400 wei a day + a gas cost of 0.00054 $ gas per wei sent ~= 50$ per day + a RPC cost to achieve that many txns. when he stands to gain nothing from it , not even achieving a DOS, best thing he would achieve is a few people complaining on beefy discord

2. Beefy does not have a single position, it has a alt position as well which tries to negate any impermanent loss(IL). Keep notice of the words tries to negate . Even if some IL does happen for 3-4 secs its gonna be very negligible/dust amounts . Not to mention IL does not lead to a valid find in AMMs , if IL happens it is considered admin error, no one can gurantee 0 impermanent losses in current AMM positions.

Not to mention all of this can be solved by making sure the bot batches two txns together. I recommend watsons to give a sufficient data to back their claims , not just fantasy numbers . Honestly i think lead judge should just pass their judgement on the finding as he sees fit, as watsons have repeatedly made false claims, tried to twist statements & even after all this useless chatter they have not put out any kind of concrete numbers about their finding. Its just a waste of everyone's time.

[0xdeth]

@dhtns2122001

NonFungiblePositionManager does not even exist on MODE/ BOB

I agree, this will still affect deployments on OP, no?

Even if it did its a serious fantasy to assume no one will interact with it for 7 days.

Read our comments again, no one said that no one is going to interact with it for 7 days.

No. of strategies existing does not even matter cause they interact with the same PositionManager.

That's exactly why sending the funds to the NFTManager will affect all of the strategies, since they all use the same NFTManager.

Would cost him 86,400 wei a day + a gas cost of 0.00054 $ gas per wei sent ~= 50$ per day + a RPC cost to achieve that many txns. when he stands to gain nothing from it,

It is DoS/griefing, he doesn't need to gain anything from it, that's why this should be Med, not High. the last lines of this comment have a reasonable purpose for the attack.

Not to mention all of this can be solved by making sure the bot batches two txns together

Again, I agree, but Velodrome removing refundETH will also fix the issue, how is this a valid argument in the context of Sherlock?

even after all this useless chatter they have not put out any kind of concrete numbers about their finding

Please read this

Honestly i think lead judge should just pass their judgement on the finding as he sees fit. Its just a waste of everyone's time.

I agree, but he asked us to add our comments and I presume yours as well, we are adding them.

We have no interest in attacking you, you are the one getting emotional. All of us have given our arguments, let @nevillehuang decide.

[dhtns2122001]

Again, I agree, but Velodrome removing `refundETH` will also fix the issue, how is this a valid argument in the context of Sherlock?

bot batching two txns & velodrome doing changes are not the same thing. Bot batching two txns is valid cause the responsibility to prevent IL is entirely on off chain manager of beefy

[dhtns2122001]

- - second -> $2540 for sending wei(1 wei * 604800) + $12 096 (~$0.02 for gas * 604800) = $14 636
- - 5 seconds -> $508 for sending wei(1 wei * 120960) + $2419.2 (~$0.02 for gas * 120960) = $2 927.2  
        As you can see the cost is not unrealistically high. Hacker organisations have billion of funds for exploiting projects. Even for a malicious millionaire it may be worth to act bad.

these comments from @NicolaMirchev sufficiently show how watsons are misinformed, they don't know their own attack vector, it costs like 50-60 bucks to send 86400 wei in a day (1 wei / second). https://basescan.org/tx/0x30f46ee98bbca5ac5af99698f16f09c8e5ae901fe07c07f08d3d3ecb09de6392 look at the gas cost. So it only costs like 350-450 bucks to send 1 wei per second for 1 week straight. Not whatever amount you have put in. I am not getting emotional, i am stating facts. U guys are putting out wrong data and wasting time. Again please do due diligence

[0xdeth]

The example is using Optimism as the chain and it's prices.

You are correct that if the attack happens on Base it would be cheaper.

[dhtns2122001]

https://optimistic.etherscan.io/tx/0x078b4c7f691931d243faf49994b0baad02947df55666c1b7d57c41b5d6c9ef98

still couldn't be more wrong. Please do due dilligence and stop wasting time

[dhtns2122001]

point is attacker has no financial incentive to gain from this & no possibility of successfully doing the DOS.

[dhtns2122001]

Honestly my last 24 hours comments are not there to show ur finding is low severity (that i think it is from the get go), they are there to correct the fantasy numbers / claims you are putting up and are ruining the judging experience and delaying the results un intentionally. Please stop doing that in future contests , where concrete data can be used, use that and not make up numbers. Putting up claims without proper due dilligience harms everyone & severely worsens the judging . The MODE/BOB chain fiasco could have been prevented by 1 visit to velo docs.

[merlin-up]

Even if some IL does happen for 3-4 secs its gonna be very negligible/dust amounts .

Why do you assume that impermanent loss will only last for 3-4 seconds and will only be a small amount? IL depends on how much the token price drops or increases and how much the position is out of range. So, if

1. the price drops significantly and the position is out of range even for a short period, it will lead to high impermanent loss after moving ticks.
2. If the price does not drop significantly, but the position is out of range for some time, it will also lead to a loss of funds and no yield generation. The IL also depends on the amount of tokens deposited into the strategy.

No. of strategies existing does not even matter cause they interact with the same PositionManager.

I think it does. Imagine we have multiple strategies with token A. In such a situation, the rebalancer would need to call moveTicks for all strategies with token A. This increases the chances that some strategies' contracts will be in DOS longer than others.

[dhtns2122001]

I think it does. Imagine we have multiple strategies with token A. In such a situation, the rebalancer would need to call `moveTicks` for all strategies with token A. This increases the chances that some strategies' contracts will be in DOS longer than others.

No that's not how bots work, they batch txns , imagine having a bot with a for loop?

Again IL is not a bug.

Any kind of fee loss due to your finding will only be limited to 3-4 seconds , even if you are very lucky & DOS it for a few blocks beefy will have a much wider range than most LPs due to having two positions in the same pool .

During volatile periods you cant even DOS it for 3-4 secs cause the number of txns to positionManager will also drastically increase.

As @nevillehuang has said , DOS wont be more than a minute + beefy has two positions + no incentive for attacker + bot can easily bypass the DOS ,sherlock only considers Loss of funds / 7 day dos.

[dhtns2122001]

Here's some data for @nevillehuang to help make up his mind - current velo epoch(1 week period) has now been going on for ~6 days( it starts every Thursday) 6 days = 5,18,400 seconds Fees accured by the best pool = 38,500 $ ~= 0.075 $ fees per second Now assume beefy has 10% share of this pool so fee for beefy per second = 0.0075 $ / second Now lets assume beefy has 500 depositers in the strategy with equal amounts for simplicity so fee share per depositer = 0.0075 $ / 500 = 0.000015 $ per depositer

Now if the attacker sends 1 wei and is able to achieve dos on moveTicks for lets say 1 block i.e 3 seconds it would cost to the depositer = 0.000015 $ * 3 = 0.000045 $

while it would cost the attacker ~= 0.005 $ to do the DOS (OP gas) His attack cost would be 100x the damage he causes to a Beefy depositer ( which is dust). Even if 100 strategies with same token exist he would still be in a loss as I have taken the pool with max fee collected other pools do significantly less fee generation.

This data is as of block number 121280505

[WangSecurity]

Firstly, I see lots of Watsons repeat the same arguments all over again. Please avoid it, if someone already mentioned your thought/opinion, you can simply leave a reaction under their comment, otherwise, the discussion becomes unnecessarily long and repeating the same arguments doesn't make any of the sides more right. If any of the arguments were mentioned, they will be taken into consideration for final judgment.

Secondly, what if the token price got out of the ticks set by the rebalancer, the contract stops accruing any yield at all or the yield becomes smaller? (again, to avoid too many unnecessary discussions, no need to bring up other points and arguments not related to the question).

[0xdeth]

@WangSecurity

To address your question:

if the token price got out of the ticks set by the rebalancer

It stops accruing fees and is considered "inactive".

[Dliteofficial]

what if the token price got out of the ticks set by the rebalancer, the contract stops accruing yield at all or the yield becomes smaller?

If the price goes completely out of the range set by the rebalancer, the positions do not accrue yields because liquidity aren't supplied to the current range. This goes on until the rebalancer rebalances the ticks by calling moveTicks() in calm periods.

[WangSecurity]

Thank you for the responses. I'm trying to give as detailed response as possible.

Firstly, the bug can be done perpetually to cause constant DOS, but as LSW correctly noted, this will be easily solved if the user interacts with Velodrome, which will send all the ETH balance from the contract. But, the attacker can still send funds again, and repeat this step all over again when the Velodrome's nftManager balance is 0. But, based on the rules, in these cases we have to consider it as a one-time DOS, despite the fact it can be repeated perpetually.

Secondly, I know that in the future Beefy may deploy on other chains, work with more Velodrome pools. But, these are just assumptions and there' no guarantee it will ever happen. Hence, let's look at the current situation and the current state, not at what it could be at some point in the future.

Thirdly, there's no guarantee that this DOS would last less than 7 days even considering it as one-time DOS. But, the LSW has shown that it in reality it almost impossible, and I believe we can take it into consideration.

Fourthly, let's not consider bots that will monitor nftManager for free funds and call refundEth each time. We don't know about them anything. I agree it's fair point, but I think it's quite hyperbolised argument and we should consider what we have here.

Fifthly, the protocol is about yield strategies and it's main purpose is to make more yield. In an extremely edge case of this vulnerability, it indeed may lead to the situation, when the protocol's both strategies are out of range, but the rebalancer cannot change the ticks due to this DOS. In that case, it can lead to protocol not earning yield at all short term (second-couple of minutes). Since, again, this protocol is about yield strategies, i believe if it doesn't accrue any yield, it should be considered loss of funds. With that being said, the loss and the attack are extremely constrained but possible (remember, Sherlock's rules don't take likelihood into consideration).

Sixthly, fair point the rebalancer can call refundEth on nftManager and then change the ticks. Fair point, but we don't know how this mechanism will operate and what and how it does it actions. Hence, don't see it as a strong argument.

With that all being said, I hope I answered all of your points and opinions. Again, if there is no new information, no need to repeat the old one.

Returning to 5th point, in that extreme case, I believe move ticks action is indeed time-sensitive. Hence, planning to validate the report with medium severity, please tell me what should be the duplicates, only #121?

[spacegliderrrr]

@WangSecurity Could you elaborate on “ but we don't know how this mechanism will operate and what and how it does it actions”. Rebalancer is simply a trusted wallet. There’s no reason it wouldn’t be able to batch 2 transactions. Given that it’s trusted, it should be expected for it to act properly.

[Dliteofficial]

Fifthly, the protocol is about yield strategies and it's main purpose is to make more yield. In an extremely edge case of this vulnerability, it indeed may lead to the situation, when the protocol's both strategies are out of range, but the rebalancer cannot change the ticks due to this DOS. In that case, it can lead to protocol not earning yield at all short term (second-couple of minutes). Since, again, this protocol is about yield strategies, i believe if it doesn't accrue any yield, it should be considered loss of funds. With that being said, the loss and the attack are extremely constrained but possible (remember, Sherlock's rules don't take likelihood into consideration).

@WangSecurity This assessment is not accurate. I provided that specific explanation of what happens when the price is out of range and what happens to the yield for a reason and it is because I knew it could impact your assesment of the report.

When price is out of range, it is by design that the protocol doesn't earn yield until the ticks are rebalanced. In extreme instances like when there is really high volatility "uncalm periods", the liquidity isn't deployed. If it isn't deployed and not in use, yield doesn't accrue and until balance is restored, the strategy would not earn any yield. Depending on the duration of the uncalm period, yield will not be accumulated for that long. This vulnerability will only able to block the yield from being accumulated for like 12 secs at best (please see the explanation about transaction time above). So compared to the provisions made by the protocol, yield won't be accumulated for longer when there is no DOS attack rather when the price is being manipulated resulting in an uncalm period.

Plus, how is the loss of yield that was never earned because liquidity isn't deployed considered a loss of funds? I honestly don't understand. I know there is a rule of Sherlock that negates that, assuming I can find it. I'll tag it here. In summary, this is nothing but design and opportunity loss

[iamnmt]

@WangSecurity

Adding to your fifth point, it would required excessive constraints for the DOS to be considered preventing the strategy from earning yield.

The moveTicks function will revert on two occasions: not in calm period, the DOS is success. So in order for the DOS to be considered preventing the strategy from earning yield, the rebalancer can not call moveTicks in calm period (because if moveTicks reverts in uncalm period then it is considered a design choice) and the LP position is out of range.

When the moveTicks is called successfully, the LP position will be centered to the current tick and tickLower, tickUpper are updated accordingly. So in order for the DOS to be considered preventing the strategy from earning yield, the DOS has to success continuously from the last time moveTicks is called successfully to the time the rebalancer calls moveTicks when the current tick is out of the LP position (from the last moveTicks), in calm period.

Quoting from Sherlock's rule:

Additional constraints related to the issue may decrease its severity accordingly.

[merlin-up]

The moveTicks function, like the deposit function, is called during the calm period. The isCalm function checks if the current tick is within a certain deviation from the TWAP tick (the TWAP of the last twapInterval 120 seconds from the pool). The uncalm period does not prevent a DOS attack. Once the current tick gets close to the TWAP tick, the calm period begins and the rebalancer will not be able to call moveTicks because of the DOS attack.

Besides yield, if the rebalancer cannot call moveTicks and depending on the duration of the DOS attack, if the token price moves further out of range, the impermanent loss will be greater because the rebalancer couldn't call moveTicks. Let's say the rebalancer calls moveTicks when IL = x, but the transaction fails due to a DOS attack. Some time passes, and the token price drops further. Now, when the DOS attack ends and the rebalancer calls moveTicks, the IL will be greater than it was before.