DenTonylifer - StrategyPassiveManagerVelodrome does not take into account unharvested fees

[sherlock-admin2]

DenTonylifer

high

StrategyPassiveManagerVelodrome does not take into account unharvested fees

Summary

StrategyPassiveManagerVelodrome.sol does not take into account unharvested fees, which will lead to inaccurate calculations and losing fees, as the function balances() will return an incorrect value of total0.

Vulnerability Detail

Before every deposit/withdraw vault calls beforeAction() function. This is necessary for the calculation of amounts in deposit/withdraw functions in vault to be accurate:

/// @notice Called during deposit and withdraw to remove liquidity and harvest fees for accounting purposes.
    function beforeAction() external {
        _onlyVault();
        _claimEarnings();  
        _removeLiquidity();
    }

The protocol expects this function to harvest a fees (for accounting purposes). But instead this function actually increases the amount of unharvested fees (because of calling _claimEarnings()):

/// @notice Internal function to claim rewards from the gauge and collect them.
    function _claimEarnings() private {
        // Claim rewards
        uint256 feeBefore = IERC20Metadata(output).balanceOf(address(this));

        if (positionMain.nftId != 0) ICLGauge(gauge).getReward(positionMain.nftId);
        if (positionAlt.nftId != 0) ICLGauge(gauge).getReward(positionAlt.nftId);

        uint256 claimed = IERC20Metadata(output).balanceOf(address(this)) - feeBefore;
        fees = fees + claimed;

        emit ClaimedFees(claimed);
    }

Next, the function balances() is called. It is expected to return balance of this contract + balance of positions - feesUnharvested:

function balances() public view returns (uint256 token0Bal, uint256 token1Bal) {
        (uint256 thisBal0, uint256 thisBal1) = balancesOfThis();
        (uint256 poolBal0, uint256 poolBal1,,,,) = balancesOfPool();

        uint256 total0 = thisBal0 + poolBal0;
        uint256 total1 = thisBal1 + poolBal1;

        // For token0 and token1 we return balance of this contract + balance of positions - feesUnharvested.  
        return (total0, total1);
    }

But it does not take into account unharvested fees and only returns balance of this contract + balance of positions.

Also note:

This code comments are not outdated based on contextual evidence - the protocol clearly uses fees, and the comments and the order of calling some functions indicate that the protocol expects fees to be harvested.

Impact

As stated in the comments, fees is token0:

//* @return _amountLeft The amount of token0 left after fees.

// Calculate amount of token 0 to swap for fees.
            uint256 amountToSwap = _amount * fee.total / DIVISOR;
            _amountLeft = _amount - amountToSwap;

            // If token0 is not native, swap to native the fee amount.

In this case function balances() will always return too big value of total0. It will lead to incorrect calculation in deposit/withdraw in BeefyVaultConcLiq.sol. Also panic() finction relies on the value of this function and will not revert even if amount of token0 in the strategy is less that bal0, so this check will not revert, causing to bypassing slippage check:

if (bal0 < _minAmount0 || bal1 < _minAmount1) revert TooMuchSlippage();

Also note that fees is rewards from the gauge, not part of deposited funds, so they should not be withrawable. Here is attack scenario (some details omitted for simplicity):

• User deposits 1000 tokens, now he has 1000 shares
• Protocol receives rewards from the gauge
• token0.balanceOf(address(this)) = 1050, where user's deposit = 1000, and unharvested fees = 50 tokens

• User redeems 1000 shares and receives 1050 tokens: _amount0 = (_bal0 _shares) / _totalSupply = (1050 1000) / 1000 = 1050

      strategy.beforeAction();
  
      uint256 _totalSupply = totalSupply();
      _burn(msg.sender, _shares);
  
      (uint256 _bal0, uint256 _bal1) = balances();
  
      uint256 _amount0 = (_bal0 * _shares) / _totalSupply;
      uint256 _amount1 = (_bal1 * _shares) / _totalSupply;
  
      strategy.withdraw(_amount0, _amount1);
  
      IERC20Upgradeable(token0).safeTransfer(msg.sender, _amount0);

  • User steals all protocol's unharvested fees

    Code Snippet

    Link 1 Link 2

    Tool used

Manual Review

Recommendation

Change the code as follows:

/// @notice Called during deposit and withdraw to remove liquidity and harvest fees for accounting purposes.
    function beforeAction() external {
        _onlyVault();
+       harvest();
-       _claimEarnings();  
        _removeLiquidity();
    }

/** 
     * @notice Returns total token balances in the strategy.
     * @return token0Bal The amount of token0 in the strategy.
     * @return token1Bal The amount of token1 in the strategy.
    */
function balances() public view returns (uint256 token0Bal, uint256 token1Bal) {
        (uint256 thisBal0, uint256 thisBal1) = balancesOfThis();
        (uint256 poolBal0, uint256 poolBal1,,,,) = balancesOfPool();

+       uint256 total0 = thisBal0 + poolBal0 - fees;
-       uint256 total0 = thisBal0 + poolBal0;
        uint256 total1 = thisBal1 + poolBal1;

        // For token0 and token1 we return balance of this contract + balance of positions - feesUnharvested.  
        return (total0, total1);
    }

Duplicate of #71

[sherlock-admin3]

Escalate

I don't understand why my report was marked as a duplicate of https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/42

The report https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/42 only shows the discrepancy between the code and the comments, but my report shows an incorrect calculation if fee token (output) is one of the lpToken, because balances() function relies on balanceOf(). Look at the Impact section - I clearly explain why this will lead to a loss of funds, my report is not about "incorrect code comment".

My report should be duplicate of https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/71: same root cause, same issue explanation, same impact, same recommendation.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[merlin-up]

Escalate

Watson mentioned: Report should be a duplicate of #71.

[sherlock-admin3]

Escalate

Watson mentioned: Report should be a duplicate of #71.

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.

[DenTonylifer]

I don't understand why my report was marked as a duplicate of https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/42

The report https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/42 only shows the discrepancy between the code and the comments, but my report shows an incorrect calculation if fee token (output) is one of the lpToken, because balances() function relies on balanceOf(). Look at the Impact section - I clearly explain why this will lead to a loss of funds, my report is not about "incorrect code comment". The code comments in my report is ONLY to show that the incorrect calculation in the balances() function is not a design choice, and nothing more.

My report should be duplicate of https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/71: same root cause, same issue explanation, same impact, same recommendation.

Also note, that https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/42 only identifies root cause:

• In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

In contrast, I clearly demonstrated the impact and attack path.

[Dliteofficial]

I believe this is not a duplicate of 71 because the root causes are not the same. 71 is as a result of the fact that one of the LPtokens can be the output token causing broken accounting

[DenTonylifer]

As stated in the comments, fees is token0 : In this case function balances() will always return too big value of total0

Based on the comments, I assumed that output might be the same as lptoken0. Do you agree with this assumption?

[nevillehuang]

Agree this is a duplicate of #71. While not really clear, watson gave a sufficient example of the fee being charged as token0. Since fee is charged in output token, this implicitly implies the fee being charged as token0 and hence the resulting issue.

However, whether or not the escalation is accepted depends on validity of #71, which will be separately considered. Per sherlock rules for escalation. Please avoid continuing discussions here and monitor discussions for #71 instead

1. Escalation will not be accepted in case the reward distribution is not affected. Example: If an escalation says This issue must be a duplicate of #23 and if the main issue 23 is already a low/invalid issue and is not escalated. Then that escalation would be rejected since it would not affect reward distribution.

[nevillehuang]

Based on planned escalation resolution in issue #71, planning to accept escalation and make issue a duplicate of #71

[WangSecurity]

Result: Medium Duplicate of #71

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• merlin-up: accepted