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

Withdrawers are double paying fee and negative PnL

vinica_boy

Summary

Withdrawers are paying position fee and negative PnL once when calculating the collateral delta to create a decrease position and once again when getting the output from the decrease order.

Vulnerability Details

Consider the following scenario:

  • There is an opened position with collateral $1000 and position size $3000

  • User has 1/10 of vault shares and initiates a withdraw

  • He has to be eligible of 1/10 of the whole position - minus fees and PnL related to his part

  • When he tries to withdraw, the collateral delta for the corresponding decrease position will be lowered by those fees and PnL

  • Instead of requesting $100 from the collateral, collateral delta would be a smaller number, lets say $80

  • The problem comes from the GMX implementation, which will also calculate fees and PnL and substract them from those $80

  • This will make withdrawer double paying and getting less than what he deserve.

The following logic is handling withdrawal process in Gamma and account for the fees and pnl:

else {
IVaultReader.PositionData memory positionData = vaultReader.getPositionInfo(curPositionKey, prices);
uint256 collateralDeltaAmount = positionData.collateralAmount * shares / totalShares;
uint256 sizeDeltaInUsd = positionData.sizeInUsd * shares / totalShares;
// we always charge the position fee of negative price impact case.
uint256 feeAmount = vaultReader.getPositionFeeUsd(market, sizeDeltaInUsd, false) / prices.shortTokenPrice.max;
int256 pnl = vaultReader.getPnl(curPositionKey, prices, sizeDeltaInUsd);
if (pnl < 0) {
collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.max;
} else {
collateralDeltaAmount = collateralDeltaAmount - feeAmount;
}
uint256 acceptablePrice = abi.decode(metadata, (uint256));
_createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
}

We see that fee and pnl are once substracted from the collateralDeltaAmount.

Taking a look how order decrease is handled in GMX code:

Inside DecreasePositionUtils::decreasePosition() we have the following snippet to account for fees and collateral. It uses processCollateral(). Taking a look at how fees and PnL are accounted in processCollateral():

// pay for fees
(values, collateralCache.result) = payForCost(
params,
values,
cache.prices,
cache.collateralTokenPrice,
// use collateralTokenPrice.min because the payForCost
// will divide the USD value by the price.min as well
fees.totalCostAmountExcludingFunding * cache.collateralTokenPrice.min
);

totalCostAmountExcludingFunding includes position fee.

// pay for negative pnl
if (values.basePnlUsd < 0) {
(values, collateralCache.result) = payForCost(
params,
values,
cache.prices,
cache.collateralTokenPrice,
(-values.basePnlUsd).toUint256()
);
...

And if we check how payForCost() is implemented, we see that fees and negative pnl will be first subtracted from the output amount (which is the actual amount user will get) and only if it is unsufficient, it will be subtracted from the remaining collateral.

Impact

Loss of funds for withdrawers.

Tools Used

Manual review.

Recommendations

Consider implementing a similar approach to the deposit functionality, where fees and pnl are accounted after decreasing order. I think the problem here is mainly that collateralDeltaAmount is subtracted before the actual decrease.

Updates

Lead Judging Commences

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

Appeal created

vinica_boy Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!