Volatile token price causes higher vault fees

[kitty-the-kat]

The docs for the currently deployed version of the Gro protocol (not the one that was reviewed) states:

When users withdraw funds from Vault, they pay a 0.5% fee as a contribution to the remaining HODLers.

This is not how _calcFees() in the GVault works. In fact, in a volatile market where the price moves up and down a lot but ends up at the same place after some time, fees will be charged to the amount that was gained every time the strategy calls the GVault's report(), which can mean a lot more fees than a simple one-time fee on withdrawal.

Technical Details

_calcFees() is called only once in GVault, from report(). _calcFees() is a bit of a misnomer because the function also takes those fees and sends them to the feeCollector address specified by the owner using setFeeCollector() while returning the gains amount minus fees. The gains are defined as the difference between all the strategy's assets (loose assets, LP tokens, and rewards) and the strategy's debt, which is a value stored in strategies[strategyAddress].totalDebt in the GVault. A key part of this is that when a strategy reports a loss, the totalDebt of the strategy is reduced to account for the loss, so it is as though the strategy received fewer assets to begin with. The combination of how fees are taken from gains and the goal post for how gains are calculated getting moved on every loss creates a problematic combination. Consider the following series of events:

1. Value of Strategy is $1000
2. Strategy loses $20 and report() is triggered
3. Strategy gains $20 and report() is triggered (pays vault fee on $20 gains)
4. Strategy loses $20 and report() is triggered
5. Strategy gains $20 and report() is triggered (pays vault fee on $20 gains)

The strategy has paid the vault fee twice even though the value in the strategy hasn't changed. It is only the volatility in the strategy's assets that caused the fees to be applied. This penalizes depositors for market volatility instead of penalizing them for withdrawing, which is how the Gro protocol is documented to work today. Note that this specific strategy is built on 3CRV and the Frax Curve metapool, so the value of the LP tokens should only increase and not be subject to such volatility.

By deducting fees at the time of profit, there is less value in the vault to compound and grow, which slightly reduces the appeal of the strategy.

Impact

Low. Fees create a penalty for depositors in volatile markets, but should instead only penalize depositors who withdraw. In actuality, because 3CRV should only increase in value, the two are similar.

Recommendation

Only apply a fee on withdrawal to enable to net vault value to continue compounding until a user withdraws their deposit. A fee on withdrawal would create a disincentive for attack vectors such as this yearn vault attack.

[kitty-the-kat]

Acknowledged - Known, but not considered an issue, so we have not intention to fix this. Reasoning - One of the goals of the G2 was to remove withdrawal fees from the protocol, which were there in order to minimize the attack surface of the protocol. We estimate that the impact of the vaults management fee will lower overall APY, and can cause small losses as described in your example overall, but the end user will likely be less impacted by this than a direct withdrawal fee. The comparison with 3crv is also not a good one to make; this vault does not only run "numbers go up" strategies, and are expected to have losses from time to time, this does not impact the value of 3crv, but the vault token as the number of 3crv diminishes during a loss, it is not the intention of the vault to safeguard from these situations, but rather the tranches.