darkart - Logical Error in Pausing Mechanism

[sherlock-admin3]

darkart

high

Logical Error in Pausing Mechanism

Summary

The function employs a logical OR (||) operator to check for two pausing conditions: strategy-specific pause and global pause. However, due to the OR operator, even when the strategy is paused, transactions can still be processed if there's no global pause. This bypasses the intended functionality of restricting access during a paused state.

Vulnerability Detail

The _whenStrategyNotPaused function utilizes a logical OR (||) operator to check for two pausing conditions:

• paused(): This checks if the specific strategy contract itself is paused.
• factory.globalPause(): This checks for a global pause flag set by a factory contract.

However, due to the OR operator, the issue arises when the strategy is paused (paused() is true) but there's no global pause (factory.globalPause() is false). In this scenario:

• The first condition (paused()) evaluates to true.
• The second condition (factory.globalPause()) evaluates to false.
• Because of the OR operator (||), as long as one condition is true, the entire if statement evaluates to false. This incorrectly allows the function's logic to proceed even though the strategy is intended to be paused.

Impact

Critical functionalities within the strategy that are guarded by this modifier might become accessible even when the strategy is paused. This could potentially lead to unexpected behavior or security vulnerabilities.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/StratFeeManagerInitializable.sol#L84-L86 https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/StratFeeManagerInitializable.sol#L77-L80

Tool used

Manual Review

Recommendation

Implement the refactoring approach suggested across all functions that rely on pausing logic, including _whenStrategyNotPaused and potentially _isPaused. Here's an example :

modifier_whenStrategyNotPaused() {
    require(!paused() && !factory.globalPause(), "StrategyPaused");
    _;
}

[sherlock-admin4]

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

z3s commented:

Invalid; Your assumption is wrong: Because of the OR operator (||), as long as one condition is true, the entire if statement evaluates to false.

DHTNS commented:

Invalid -> watson seems confused between 'AND' & 'OR' operators