Dots - Withdraw function is missing onlyCalmPeriods modifier

[sherlock-admin3]

Dots

high

Withdraw function is missing onlyCalmPeriods modifier

Summary

Withdraw function is missing onlyCalmPeriods modifier

Vulnerability Detail

The onlyCalmPeriods modifier is a critical safety component of the strategy, as it is used to prevent deposits and harvests when the current price is not within a certain deviation of twap. This is to protect against price manipulation by an attacker during their transaction, possibly using flash loans or other means.

Both the harvest function as well as withdraw do not check whether the system is in a calm period or not. This means that users can still interact with the system, even though it is not safe to do so. This can lead to unexpected behavior and potential losses for the users.

Impact

This issue can lead to unexpected slippage for the users as well as potential losses for the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L226-L240

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L427-L435

function withdraw(uint256 _amount0, uint256 _amount1) external {
        _onlyVault();

        // Liquidity has already been removed in beforeAction() so this is just a simple withdraw.
        if (_amount0 > 0) IERC20Metadata(lpToken0).safeTransfer(vault, _amount0);
        if (_amount1 > 0) IERC20Metadata(lpToken1).safeTransfer(vault, _amount1);

        // After we take what is needed we add it all back to our positions. 
        if (!_isPaused()) _addLiquidity();

        (uint256 bal0, uint256 bal1) = balances();

        // TVL Balances after withdraw
        emit TVL(bal0, bal1);
    }

function harvest() external {
        _harvest(tx.origin);
    }

Tool used

Manual Review

Recommendation

-function withdraw(uint256 _amount0, uint256 _amount1) external {
+function withdraw(uint256 _amount0, uint256 _amount1) external onlyCalmPeriods {
        _onlyVault();

        // Liquidity has already been removed in beforeAction() so this is just a simple withdraw.
        if (_amount0 > 0) IERC20Metadata(lpToken0).safeTransfer(vault, _amount0);
        if (_amount1 > 0) IERC20Metadata(lpToken1).safeTransfer(vault, _amount1);

        // After we take what is needed we add it all back to our positions. 
        if (!_isPaused()) _addLiquidity();

        (uint256 bal0, uint256 bal1) = balances();

        // TVL Balances after withdraw
        emit TVL(bal0, bal1);
    }

-function harvest(address _callFeeRecipient) external {
+function harvest(address _callFeeRecipient) external onlyCalmPeriods {
     _harvest(_callFeeRecipient); 
 } 

-function harvest() external {
+function harvest() external onlyCalmPeriods {
     _harvest(tx.origin); 
 } 

Duplicate of #74

[sherlock-admin3]

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

DHTNS commented:

Low -> same as #79 fails to identify attack path no slippage present here?