Target
Bug Description
According to the docs (linked below) and as in any other investment scheme, the performance fee collected for user's earnings should be x% of profit, i.e. end_balance - start_balance.
Vault's collectFees() function collects fee at every deposit / withdraw event, by taking x% of the gains since last deposit/withdraw action. However, it does not account for losses in held funds, and therefore takes far too much fees from users. See scenario under POC section for example of loss for investor.
Impact
Permanent freezing of funds - user's funds are incorrectly consumed by the system as fees. Over time, since crypto markets are known as highly volatile, the majority of user funds will end up in the governance's hands.
Risk Breakdown
Likelyhood of exploit: Certain
Weakness: Incorrect handling of data
Severity: Critical
Recommendation
Two options:
As done in other projects, store the maximum share price for which performance fee has been paid per user. Therefore, fluctuations in price will not continue to scoop unjust fees.
In case of protocol losses, keep running total of (yield - losses) amount. Only charge performance fee if this delta is positive.
References
https://docs.brahma.fi/products/brahma-vaults/pmusdc/apy-calculation-and-fee - Documentation of fee system in Brahma https://github.com/code-423n4/2022-05-enso/blob/main/contracts/Strategy.sol - example of correct fee collection in Enso Finance
Proof of Concept
Assume 15% performance fee as deployed
User inserts 100$ to vault
Vault loses 30$, now 70$
User inserts additional 10$ to vault - prevVaultFunds updated to 80$
Vault gains 30$ back, fee sent = 0.15 * 30 = 4.5$
Vault now holds 105.5$
User withdraws entire holdings, 105.5$.
User permanently lost 4.5$ to incorrect performance fee collection.
Note that this applies to fees from both Vault and executor funds.
Immunefi has rejected the report, claiming that since bug relies on specific market movements and depends on when fee collection occurs. It is very disappointing that they did not understand that users will lose chunks of their deposits because markets never have a monotonous upward direction. They fail to differentiate between loss of yield and loss of original funds, and I still have no idea how this bug is not fixed.