search

sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

5 stars 5 forks source link

bughuntoor - No way to claim rewards in emergency mode if output token is not native #57

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 3 months ago

bughuntoor

medium

No way to claim rewards in emergency mode if output token is not native

Summary

No way to claim rewards in emergency mode if output token is not native

Vulnerability Detail

In certain case of emergency, contract owner can call panic to enter emergency mode. This claims earning, removes liquidity, stops deposits and revokes outstanding approvals.

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

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

The problem is that in order to claim the rewards, if output token is not native it has to be swapped through the Velodrome router. However, since contract is in panic mode and approvals are revoked, no funds can be swapped through the router. In case of a permanent emergency (due to an issue with the Velodrome pool/Vault), these funds cannot be retrieved safely.

Impact

Lost/Stuck funds

Code Snippet

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

Tool used

Manual Review

Recommendation

upon calling panic, if output token is not native send the rewards to owner wallet

sherlock-admin3 commented 3 months ago

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

DHTNS commented:

Low -> good find but the situation is not that grim admin can unpause + harvest + panic again in a single txn

MirthFutures commented 3 months ago

Ya, the code is designed to panic in case of bad behavior of allowed contracts. Though technically this is true, it still should be this way in case the unirouter, the gauge or the nftManager have an issue and we need to revoke. We then fix or update whats needed and call unpause, harvest and panic. We can also actually write a keeper contract to this in one transaction if needed.