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

Negative PnL being divided by `prices.shortTokenPrice.max` causes users to receive more funds than they should

0xl33

Description:

In PerpetualVault::_withdraw function, if there is an open GMX position and if pnl gets calculated as negative, when adjusting collateralDeltaAmount the negative PnL gets divided by prices.shortTokenPrice.max:

} 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);
}

This is wrong, because dividing by a bigger value leads to smaller negative pnl and this goes against the protocol and in favor of the user. Instead, prices.shortTokenPrice.min should be used. This way the subtracted pnl will be bigger, and the collateralDeltaAmount will be in favor of the protocol.

Consider this example:

  • collateralDeltaAmount = 10000

  • feeAmount = 10

  • pnl = -1000

  • shortTokenPrice.max = 1.02

  • shortTokenPrice.min = 0.98

Current result (in favor of user):
collateralDeltaAmount = 10000 - 10 - 1000 / 1.02 = 9009,61

Correct result (in favor of protocol):
collateralDeltaAmount = 10000 - 10 - 1000 / 0.98 = 8 969,6

Difference between results = 9009,61 - 8 969,6 = 40,01

Of course the difference between results would be smaller if the gap bettwen min and max prices is smaller, but my point still stands.

Impact:

Users receive more funds than they should by not paying a part of their negative PnL.

Recommended Mitigation:

Divide the negative PnL by prices.shortTokenPrice.min, instead of prices.shortTokenPrice.max.

Example fix:

} 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;
+ collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.min;
} else {
collateralDeltaAmount = collateralDeltaAmount - feeAmount;
}
uint256 acceptablePrice = abi.decode(metadata, (uint256));
_createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_withdraw_use_prices.shortTokenPrice.max_instead_of_min

Likelihood: Low/Medium, every withdraw, position opened, not liquidated, beenShort or not 1, and the difference between minPrice and maxPrice is significant. Impact: Low, small part of feeAmount and PnL not deducted from collateralDeltaAmount.

Support

FAQs

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

Give us feedback!