Brahma

--> CRIT - Brahma.Fi - Fee collection does not take previous losses into account 🚩 top of page Search Target https://etherscan.io/address/0x3c4Fe0db16c9b521480c43856ba3196A9fa50E08 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. bottom of page