VaultManager.sol - low decimal tokens and dust amounts can avoid fees

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0xb8af9370768b9b6a2cec363c1cf7c00ed5623a521582217a1cf4a730045b32df Severity: medium

Description: Description\ The vault manager contract invokes entry and exit fees on it's users which are configurable by each portfolio admin. However as the token and amounts are non-limited, a user can easily deposit or withdraw dust amounts of tokens to bypass those fees. On L2s with low tx costs (as of today, so costs are astronomically low and get rounded to $0.00) a user can seamlessly avoid paying fees.

Attack Scenario\ Each deposit and withdrawal are charged their respective fee when portfolio tokens are minted and burned:

• afterFeeAmount = feeModule()._chargeEntryOrExitFee(_mintAmount, exitFee);
• _mintAmount = feeModule()._chargeEntryOrExitFee(_mintAmount, entryFee);

The used calculation inside the charging function is plain and simple: (_tokenAmount * _feePercentage) / TOTAL_WEIGHT which is prone to precision loss to 0 on dust token amounts or such tokens amounts that multiplied by the percentage, still round down. Such amounts could generally be achieved with 2 decimal tokens like EURO.

The cooldown periods have no effect here since the cooldown stops deposits and withdrawals, but does not disable transfers. A user can use multiple addresses to bypass the cooldown and bypass these fees with dust amount operations.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation\ Invoke a minimum deposit check to prevent operations with dust amounts. It can be done inside _validateAndGetBalances

[langnavina97]

The concern regarding the possibility of bypassing fees with dust amount operations is not relevant. Our protocol does not mint dust amounts, and any transactions involving very small values would only result in dust. These small amounts do not significantly affect the overall system, ensuring that the impact of such operations is negligible. Therefore, the current implementation does not need modification in this regard.

@PlamenTSV