no - Lost Fund in `StrategyPassiveManagerVelodrome::retireVault()`

[sherlock-admin2]

no

high

Lost Fund in StrategyPassiveManagerVelodrome::retireVault()

Summary

Lost Fund in StrategyPassiveManagerVelodrome::retireVault()

Vulnerability Detail

function retireVault() external onlyOwner {
        if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized();
        panic(0,0);
        address feeRecipient = beefyFeeRecipient();
        IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
        IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this)));        
        _transferOwnership(address(0));
    }

Missing to transfer Output Token(from gauge) out of this contract. Output Token will be permanently stuck in this contract.

Impact

Output Token will be permanently stuck in this contract.

Code Snippet

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

Tool used

Manual Review

Recommendation

function retireVault() external onlyOwner {
        if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized();
        panic(0,0);
        address feeRecipient = beefyFeeRecipient();
        IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
        IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this))); 
+        IERC20Metadata(output).safeTransfer(feeRecipient, IERC20Metadata(output).balanceOf(address(this)));        
        _transferOwnership(address(0));
    }

[sherlock-admin4]

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

DHTNS commented:

Low -> Harvest() can be called permissionlessly to send output to rewardPool where its intended to go

[MirthFutures]

This is a low.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/beefyfinance/cowcentrated-contracts/pull/19

[0502lian]

Escalate After calling retireVault(), Allowances for rewardPool be removed. Calling Harvest() will revert. This should be a valid issue. The function call chain is as follows:

function retireVault() external onlyOwner {
        if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized();
@>        panic(0,0);
        address feeRecipient = beefyFeeRecipient();
        IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
        IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this)));
        _transferOwnership(address(0));
    }

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

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

function _removeAllowances() private {
        IERC20Metadata(output).forceApprove(unirouter, 0);
@>       IERC20Metadata(output).forceApprove(rewardPool, 0);
        IERC20Metadata(lpToken0).forceApprove(nftManager, 0);
        IERC20Metadata(lpToken1).forceApprove(nftManager, 0);
    }

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

DHTNS commented:

Low -> Harvest() can be called permissionlessly to send output to rewardPool where its intended to go

Especially since the owner has already transferred to address(0), the harvest function will never be successfully called.

[sherlock-admin3]

Escalate After calling retireVault(), Allowances for rewardPool be removed. Calling Harvest() will revert. This should be a valid issue. The function call chain is as follows:

function retireVault() external onlyOwner {
        if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized();
@>        panic(0,0);
        address feeRecipient = beefyFeeRecipient();
        IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
        IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this)));
        _transferOwnership(address(0));
    }

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

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

function _removeAllowances() private {
        IERC20Metadata(output).forceApprove(unirouter, 0);
@>       IERC20Metadata(output).forceApprove(rewardPool, 0);
        IERC20Metadata(lpToken0).forceApprove(nftManager, 0);
        IERC20Metadata(lpToken1).forceApprove(nftManager, 0);
    }

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

DHTNS commented:

Low -> Harvest() can be called permissionlessly to send output to rewardPool where its intended to go

Especially since the owner has already transferred to address(0), the harvest function will never be successfully called.

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.

[Karan9034]

Escalate After calling retireVault(), Allowances for rewardPool be removed. Calling Harvest() will revert. This should be a valid issue. The function call chain is as follows:

function retireVault() external onlyOwner {
        if (IBeefyVaultConcLiq(vault).totalSupply() != 10**3) revert NotAuthorized();
@>        panic(0,0);
        address feeRecipient = beefyFeeRecipient();
        IERC20Metadata(lpToken0).safeTransfer(feeRecipient, IERC20Metadata(lpToken0).balanceOf(address(this)));
        IERC20Metadata(lpToken1).safeTransfer(feeRecipient, IERC20Metadata(lpToken1).balanceOf(address(this)));
        _transferOwnership(address(0));
    }

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

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

function _removeAllowances() private {
        IERC20Metadata(output).forceApprove(unirouter, 0);
@>       IERC20Metadata(output).forceApprove(rewardPool, 0);
        IERC20Metadata(lpToken0).forceApprove(nftManager, 0);
        IERC20Metadata(lpToken1).forceApprove(nftManager, 0);
    }

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

Low -> Harvest() can be called permissionlessly to send output to rewardPool where its intended to go

Especially since the owner has already transferred to address(0), the harvest function will never be successfully called.

harvest function needs to be called before the owner decides to call retireVault. Seems like Low/Info finding since there is a way to withdraw output tokens before ownership is transferred to address(0) and doesn't cause loss of funds. Also provided owner is trusted, we might assume they wouldn't call retireVault without calling harvest. Would like to hear lead judge's views.

[nevillehuang]

Fees can be harvested before vault is retired, and even if admins perform this, it would still be invalid based on the following sherlock rule, 5.3

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Planning to reject escalation and leave issue as it is

[0502lian]

Fees can be harvested before vault is retired, and even if admins perform this, it would still be invalid based on the following sherlock rule, 5.3

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Planning to reject escalation and leave issue as it is

I’m sorry, I cannot agree with you. Calling Harvest() is not a necessary task for the admin, especially since retireVault includes some of Harvest()’s functionalities through _claimEarnings(). Calling Harvest() is also not a prerequisite for calling retireVault, nor is it the intended design of the system; otherwise, Harvest() could be fully included within retireVault().

Another clear piece of evidence is that in StrategyPassiveManagerUniswap, there is no need to call Harvest() before calling retireVault. Therefore, in StrategyPassiveManagerVelodrome, it is also unnecessary to call Harvest() before calling retireVault.

This is a clear bug that results in a loss of funds when the protocol is terminated, which is different from the special cases you mentioned where pausing a collateral is necessary. Otherwise, any admin action causing loss of funds wouldn’t be considered a valid issue. Meanwhile if this weren’t the issue, the sponsor wouldn’t have needed to confirm and fix this problem.

[nevillehuang]

Admin can have incorrect call orders which wouldn't amount to a valid issue, and as mentioned in my above comment, the plan would be to reject escalation and leave issue as it is.

2. Admin could have an incorrect call order.

[0502lian]

I know this rule.

Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue.

Here, setWithdrawAddress() is a prerequisite for calling withdrawAll(). However, in this contract, calling Harvest() is not an admin job. Just like I mentioned before, especially since retireVault includes some of Harvest()’s functionalities through _claimEarnings(). Calling Harvest() is also not a prerequisite for calling retireVault, nor is it the intended design of the system; otherwise, Harvest() could be fully included within retireVault().

We must acknowledge that the admin now calls Harvest() to circumvent this bug just because we have already pointed out its existence. Doing so is necessary to avoid the loss of funds. If we hadn’t pointed out this bug and the Dev Team was unaware of this issue, it is highly likely that in a real-world scenario, they would have directly called retireVault(), and the loss would have occurred.

This situation is completely different from the case where the admin must set the Withdraw Address before calling withdrawAll(). Therefore,It is at least worth considering as a medium-severity issue.

[nevillehuang]

I still stand by my comments above. It is assumed that admin will take the necessary steps when retiring a vault, and even if they do not, sherlock rules deem they can break these assumptions. Still planning to reject escalation and leave issue as it is.

[0502lian]

The auditor’s responsibility is to find vulnerabilities or bugs that could break the protocol, while the judge’s responsibility is to determine, according to Shorlock’s rules, whether an attack (or loss) scenario is likely to occur in reality. For example, if an admin (trusted) sets incorrect parameters or if an admin has an incorrect call order, such as forgetting to set WithdrawAddress() before calling withdrawAll(), these scenarios assume that the admin (trusted) makes a mistake.

Returning to this case, if calling Harvest() is a necessary step before calling retireVault(), then I am mistaken, and this loss will not occur. However, if the issue lies within the retireVault() code and calling Harvest() can circumvent this problem, it seems like we are trying our best to break the auditor’s findings. This contest appears to be heading in that direction, which is somewhat unfortunate.

[WangSecurity]

Still, based on the rules we have to assume the admin will call functions in the order, so it doesn't cause any harm to the protocol. In that case, it's calling harvest before retireVault and we have to assume the admin will keep this order. Hence, decision remains the same, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0502lian: rejected

[spacegliderrrr]

Fixed. output token is now also transferred upon retiring the vault.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.