blackhole - _addLiquidty can be reverted when guage is not alive

[sherlock-admin3]

blackhole

high

_addLiquidty can be reverted when guage is not alive

Summary

The gauge may not be active, but this is not validated in the _addLiquidity. This can lead to reverted transactions when deposit and withdraw functions are called, preventing users from withdrawing their tokens.

Vulnerability Detail

https://github.com/velodrome-finance/slipstream/blob/main/contracts/gauge/CLGauge.sol#L185

File: contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L300
        if (positionMain.nftId != 0) ICLGauge(gauge).deposit(positionMain.nftId); // @audit voter.isAlive()? "GK"
        if (positionAlt.nftId != 0) ICLGauge(gauge).deposit(positionAlt.nftId);

Impact

If the gauge is not active, transactions involving _addLiquidity may revert, causing users to be unable to withdraw their tokens.

Code Snippet

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

Tool used

Manual Review

Recommendation

It's recommended to add the validation to ensure the guage is alive and update the related code.

File: contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L300
+    if (IVoter(ICLGauge(gauge).voter()).isAlive(gauge)) {
        if (positionMain.nftId != 0) ICLGauge(gauge).deposit(positionMain.nftId);
        if (positionAlt.nftId != 0) ICLGauge(gauge).deposit(positionAlt.nftId);
+    }

File: contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L333
        if (positionMain.nftId != 0) {
            (,,,,,,,liquidity,,,,) = INftPositionManager(nftManager).positions(positionMain.nftId);
-            ICLGauge(gauge).withdraw(positionMain.nftId);
+            if (ICLGauge(gauge).stakedContains(address(this), positionMain.nftId)) ICLGauge(gauge).withdraw(positionMain.nftId);
        } 

        if (positionAlt.nftId != 0) {
            (,,,,,,,liquidityAlt,,,,) = INftPositionManager(nftManager).positions(positionAlt.nftId);
-            ICLGauge(gauge).withdraw(positionAlt.nftId);
+            if (ICLGauge(gauge).stakedContains(address(this), positionAlt.nftId)) ICLGauge(gauge).withdraw(positionAlt.nftId);
        }

File: contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L463~L464
-        ICLGauge(gauge).getReward(positionMain.nftId);
+        if (positionMain.nftId != 0 && ICLGauge(gauge).stakedContains(address(this), positionMain.nftId)) ICLGauge(gauge).getReward(positionMain.nftId);
-        ICLGauge(gauge).getReward(positionAlt.nftId);
+        if (positionAlt.nftId != 0 && ICLGauge(gauge).stakedContains(address(this), positionAlt.nftId)) ICLGauge(gauge).getReward(positionAlt.nftId);

[sherlock-admin2]

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

DHTNS commented:

Invalid -> velo guages dont die?

[z3s]

They can withdraw when it's paused if (!_isPaused()) _addLiquidity();

[lizhming]

Escalate

The gauge's status is controlled by the voter contract, so it can change after the strategy is deployed. https://github.com/velodrome-finance/slipstream/blob/main/contracts/gauge/CLGauge.sol#L185 Without the manager calling panic(), users cannot withdraw their funds if the gauge becomes inactive. This is a Denial of Service (DoS) bug.

[sherlock-admin3]

Escalate

The gauge's status is controlled by the voter contract, so it can change after the strategy is deployed. https://github.com/velodrome-finance/slipstream/blob/main/contracts/gauge/CLGauge.sol#L185 Without the manager calling panic(), users cannot withdraw their funds if the gauge becomes inactive. This is a Denial of Service (DoS) bug.

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.

[z3s]

So when gauge becomes inactive, TRUSTED admin calls panic(), and users can withdraw. I think it's the correct procedure.

[nevillehuang]

Agree with lead judges comments, users can still withdraw, they just cannot deposit, which is intended if gauge is not active. Additionally, external admins are deemed as trusted within contest READ.ME. Planning to reject escalation and leave issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• lizhming: rejected