Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Governance Fee Calculation Logic

verbotenviking

Summary

The PerpetualVault contract calculates governance fees based on a direct comparison between withdrawal amount and deposit amount, which can be problematic in certain edge cases. This implementation might lead to incorrect fee calculations, especially in scenarios with complex position value fluctuations.

Vulnerability Details

The issue is in the _transferToken function in PerpetualVault.sol (lines 1745-1750):

uint256 fee;
if (amount > depositInfo[depositId].amount) {
fee = (amount - depositInfo[depositId].amount) * governanceFee / BASIS_POINTS_DIVISOR;
if (fee > 0) {
collateralToken.safeTransfer(treasury, fee);
}
}

The function calculates fees by directly comparing the withdrawal amount with the original deposit amount. This approach has several potential issues:

  1. It doesn't account for rounding errors that might occur during share price calculations

  2. It doesn't properly handle scenarios where positions might have fluctuated significantly in value during the holding period

  3. The implementation assumes a linear profit/loss model which may not accurately reflect complex trading strategies

  4. The fee calculation doesn't account for differences that might result from deposits/withdrawals in a multi-user environment

Impact

These issues could lead to:

  1. Users being charged incorrect fees (either too much or too little)

  2. Loss of protocol revenue if fees aren't charged when they should be

  3. Users being charged fees even in scenarios where they should not be (e.g., in certain edge cases with complex value fluctuations)

  4. Inconsistent fee application across different withdrawal scenarios

While this might not lead to direct fund loss, it could create unfair fee distributions and undermine the protocol's economic model.

Tools Used

Manual code review

Recommendations

Implement a more robust fee calculation mechanism that considers the actual value accretion based on share price:

function _transferToken(uint256 depositId, uint256 amount) internal {
uint256 originalDepositAmount = depositInfo[depositId].amount;
uint256 originalShares = depositInfo[depositId].shares;
// Calculate what the original shares would be worth now
uint256 currentValueOfOriginalDeposit = originalShares * totalAmount() / totalShares;
// Calculate profit based on share value change, not direct amount comparison
int256 profit = int256(amount) - int256(currentValueOfOriginalDeposit);
uint256 fee = 0;
if (profit > 0) {
fee = uint256(profit) * governanceFee / BASIS_POINTS_DIVISOR;
if (fee > 0) {
collateralToken.safeTransfer(treasury, fee);
}
}
try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {
// Transfer succeeded
} catch {
// Handle failed transfer (see previous recommendation)
}
emit GovernanceFeeCollected(address(collateralToken), fee);
}

Additionally:

  1. Consider implementing a minimum fee threshold to avoid charging negligible fees

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!