aman - The `unpause` function is not logically correct

[sherlock-admin3]

aman

medium

The unpause function is not logically correct

Summary

The Contest ReadMe state that

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold? Yes

The unpause function will always revert due to this check if (owner() == address(0)) revert NotAuthorized(); because the pause function can only be called when the strategy owner calls retireVault.

Vulnerability Detail

The strategy support the pausable feature , which allows the owner of strategy to pause and unpause the strategy. However in the StrategyPassiveManagerVelodrome the only instance where the strategy can be paused is inside the retireVault. lets have a look at retireVault code :

794:     function retireVault() external onlyOwner {
795:         if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized(); // supply == 10**3 => 1000 , 99 , 999 , <-
796:         panic(0,0);
797:         address feeRecipient = beefyFeeRecipient();
798:         // @audit : the pool will ot expire if tokenon zero transfer fail
799:         IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
800:         IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this)));
801:         _transferOwnership(address(0));
802:     }

As it can be observed that at line 801 the ownership of strategy has transferred to address(0). now lets have a look at unpause function :

820:     function unpause() external onlyManager {
821:         if (owner() == address(0)) revert NotAuthorized();
822:         _giveAllowances();
823:         _unpause();
824:         _setTicks();
825:         _addLiquidity();
826:     }

At line 821 we check that if the owner==address(0) then revert. So the use case where this function could be used is not possible in current implementation.

Impact

The unpause function will never be executed successfully.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add an other function which will allows the owner to pause the strategy. perform all the things which are done in retireVault but don't transfer the ownership to address(0).

[sherlock-admin4]

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

z3s commented:

Invalid; panic() calls _pause() too.

DHTNS commented:

Invalid -> works as intended if vault retired then no need to unpause