Raspy Fleece Mantaray - Incorrect Fee Calculation in DepositQueue

[sherlock-admin3]

Raspy Fleece Mantaray

High

Incorrect Fee Calculation in DepositQueue

Summary

In the DepositQueue.sol contract, the _handleReport function incorrectly passes the asset price (priceD18) to the calculateDepositFee function, which is designed to calculate fees based on share amounts. This mismatch between the expected parameter type and the actual value passed leads to incorrect fee calculations, potentially resulting in users being charged incorrect fees for their deposits.

Vulnerability Details

In the _handleReport function of the DepositQueue contract, there's a critical misuse of the calculateDepositFee function:

IFeeManager feeManager = vault_.feeManager();
uint224 feePriceD18 = uint224(feeManager.calculateDepositFee(priceD18));
// @audit calculating fee based on `price` but the `calculateDepositFee` function calculate `fee` base on `shares`
uint224 reducedPriceD18 = priceD18 - feePriceD18;

The issue is that calculateDepositFee is designed to take a share amount as input and return a fee amount in shares. However, in this code, it's being passed priceD18, which is a price value (in 18 decimal format), not a share amount.

This mismatch leads to several problems:

1. The fee calculation is based on the wrong input value, leading to incorrect fee amounts.
2. The resulting feePriceD18 is then subtracted from priceD18 to get reducedPriceD18, which is conceptually incorrect - you shouldn't subtract a fee amount (in shares) from a price.
3. This incorrect reducedPriceD18 is then used to calculate the actual shares users receive for their deposits.

Later in the function, the code calculates shares and fees:

uint256 shares = Math.mulDiv(assets, reducedPriceD18, 1 ether);
if (shares > 0) {
    shareManager_.allocateShares(shares);
}
uint256 fees = Math.mulDiv(assets, priceD18, 1 ether) - shares;
if (fees > 0) {
    shareManager_.mint(feeManager.feeRecipient(), fees);
}

Because reducedPriceD18 is calculated incorrectly, both the shares and fees values will be wrong.

Impact

This vulnerability has several serious implications:

1. Incorrect Fee Charges: Users may be charged incorrect fees for their deposits, either too high or too low depending on the relationship between price values and share amounts.

2. Incorrect Share Allocation: Users will receive an incorrect number of shares for their deposits, potentially receiving fewer shares than they should.

3. Accounting Inconsistencies: The mismatch between expected and actual fee calculations can lead to accounting inconsistencies in the protocol.

Recommendation

The fee calculation should be corrected to properly calculate fees based on shares, not prices. There are two potential approaches:

Option 1: Calculate shares first, then apply fee

IFeeManager feeManager = vault_.feeManager();
uint256 totalShares = Math.mulDiv(assets, priceD18, 1 ether);
uint256 feeShares = feeManager.calculateDepositFee(totalShares);
uint256 userShares = totalShares - feeShares;

// Store the effective price (shares per asset)
uint224 effectivePriceD18 = uint224(Math.mulDiv(userShares, 1 ether, assets));
$.prices.push(timestamp, effectivePriceD18);

// Later allocation remains the same
if (userShares > 0) {
    shareManager_.allocateShares(userShares);
}
if (feeShares > 0) {
    shareManager_.mint(feeManager.feeRecipient(), feeShares);
}

Option 2: Modify the FeeManager interface

If the calculateDepositFee function is intended to work with prices, then the interface and implementation should be updated to clarify this, and the function should be renamed to something like calculateDepositFeePrice to make its purpose clear.

Either way, it's crucial to ensure that the fee calculation logic is consistent with the expected inputs and outputs of the functions involved, and that the mathematical operations make conceptual sense (e.g., not subtracting shares from prices).