bughuntoor - If keeper pauses the strategy and `owner` is renounced, it will result in permanent lock of the strategy

[sherlock-admin4]

bughuntoor

medium

If keeper pauses the strategy and owner is renounced, it will result in permanent lock of the strategy

Summary

If keeper pauses the strategy and owner is renounced, it will result in permanent lock of the strategy

Vulnerability Detail

The strategy can be paused via the panic function, which has a onlyManager modifier. The modifier allows only owner or keeper to call the function.

    function panic(uint256 _minAmount0, uint256 _minAmount1) public onlyManager {
        _claimEarnings();
        _removeLiquidity();
        _removeAllowances();
        _pause();

        (uint256 bal0, uint256 bal1) = balances();
        if (bal0 < _minAmount0 || bal1 < _minAmount1) revert TooMuchSlippage();
    }

    modifier onlyManager() {
        if (msg.sender != owner() && msg.sender != keeper()) revert NotManager();
        _;
    }

However, the problem is that the function responsible for unpausing requires owner to not be renounced. Hence, if keeper has to temporarily pause the strategy due to an emergency, they will not be able to later unpause it and would result in a permanent lock

    function unpause() external onlyManager {
        if (owner() == address(0)) revert NotAuthorized();
        _giveAllowances();
        _unpause();
        _setTicks();
        _addLiquidity();
    }

Impact

Permanent DoS

Code Snippet

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

Tool used

Manual Review

Recommendation

Remove the unnecessary check for renounced owner

[sherlock-admin4]

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

z3s commented:

Invalid; panic() doesn't renounce the owner.

DHTNS commented:

Low -> depends on admin error

[deadrosesxyz]

Escalate

Although both functions are restricted, different parties can call them (keeper != owner). Something as simple as bad timing of transactions (e.g. keeper pausing pool at the same time owner is renouncing) can lead to permanent lock of the strategy. Although the likelihood is relatively low, I believe the high impact justifies for Medium severity.

[sherlock-admin3]

Escalate

Although both functions are restricted, different parties can call them (keeper != owner). Something as simple as bad timing of transactions (e.g. keeper pausing pool at the same time owner is renouncing) can lead to permanent lock of the strategy. Although the likelihood is relatively low, I believe the high impact justifies for Medium severity.

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.

[MirthFutures]

Escalate

Although both functions are restricted, different parties can call them (keeper != owner). Something as simple as bad timing of transactions (e.g. keeper pausing pool at the same time owner is renouncing) can lead to permanent lock of the strategy. Although the likelihood is relatively low, I believe the high impact justifies for Medium severity.

Can I ask why this would be a high impact? During panic all the funds are recovered to the strategy and users can just withdraw? There is no loss of funds if the owner is renounced (which admin is trusted to not do)?

[nevillehuang]

In addition to sponsor comments above, this "issue" relies on admin errors (e.g. communication lapses) between the trusted owner and keeper. Planning to reject escalation and leave issue as it is

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• deadrosesxyz: rejected