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

Fee amount being divided by `prices.shortTokenPrice.max` causes users to pay a smaller position fee than they should

0xl33

Description:

In PerpetualVault::_withdraw function, if there is an open GMX position, feeAmount gets calculated and subtracted from user's collateralDeltaAmount:

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

In the code snippet above we can see that the return value from VaultReader::getPositionFeeUsd gets divided by prices.shortTokenPrice.max. This is wrong, because dividing by a bigger value leads to smaller feeAmount and this goes against the protocol and in favor of the user. Instead, prices.shortTokenPrice.min should be used. This way the fee amount will be in favor of the protocol.

Impact:

Users pay smaller position fee than they should, which leads to protocol/other users having to cover a part of it when price impact is actually negative (because fee amount is bigger when price impact is negative).

Recommended Mitigation:

Divide by mininum price instead of maximum price.

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;
+ uint256 feeAmount = vaultReader.getPositionFeeUsd(market, sizeDeltaInUsd, false) / prices.shortTokenPrice.min;
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);
}
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!