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

`_withdraw` function uses `shortTokenPrice.max` instead of `shortTokenPrice.min` when computing negative PnL adjustment, leading to underestimation of losses and excessive collateral withdrawal

infect3d

Summary

Because Gamma create and manage GMX positions, it also uses GMX price functions to estimate assets values on GMX. GMX returns a min and max value when requesting a price.

When a user withdraws from the vault and the position has a negative PnL (loss), the function uses prices.shortTokenPrice.max to convert the loss amount from USD to token terms. This is incorrect as it should use prices.shortTokenPrice.min for conservatism in loss scenarios.

Using shortTokenPrice.max results in a smaller token amount subtracted from the withdrawal collateral (since dividing by a larger number yields a smaller result). This leads to users being able to withdraw more collateral than they should, effectively allowing them to avoid bearing their full share of losses.

Vulnerability details

When a withdrawal is initiated from the vault via the _withdraw function, the amount of collateral that can be withdrawn by a user is calculated based on their proportional share of the vault. If the vault's position has a negative PnL (a loss), this loss should reduce the amount of collateral the user can withdraw.

The issue occurs on L1112, where the negative PnL adjustment is calculated, and the problem is also present in the fee calculation on L1109:

File: contracts/PerpetualVault.sol
1089: function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
1090: uint256 shares = depositInfo[depositId].shares;
1091: if (shares == 0) {
1092: revert Error.ZeroValue();
1093: }
1094:
1095: if (positionIsClosed) {
1096: _handleReturn(0, true, false);
1097: } else if (_isLongOneLeverage(beenLong)) {
1098: uint256 swapAmount = IERC20(indexToken).balanceOf(address(this)) * shares / totalShares;
1099: nextAction.selector = NextActionSelector.SWAP_ACTION;
1100: // abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
1101: nextAction.data = abi.encode(swapAmount, false);
1102: } else if (curPositionKey == bytes32(0)) { // vault liquidated
1103: _handleReturn(0, true, false);
1104: } else {
1105: IVaultReader.PositionData memory positionData = vaultReader.getPositionInfo(curPositionKey, prices);
1106: uint256 collateralDeltaAmount = positionData.collateralAmount * shares / totalShares;
1107: uint256 sizeDeltaInUsd = positionData.sizeInUsd * shares / totalShares;
1108: // we always charge the position fee of negative price impact case.
1109: uint256 feeAmount = vaultReader.getPositionFeeUsd(market, sizeDeltaInUsd, false) / prices.shortTokenPrice.max;
1110: int256 pnl = vaultReader.getPnl(curPositionKey, prices, sizeDeltaInUsd);
1111: if (pnl < 0) {
1112: collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.max;
1113: } else {
1114: collateralDeltaAmount = collateralDeltaAmount - feeAmount;
1115: }
1116: uint256 acceptablePrice = abi.decode(metadata, (uint256));
1117: _createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
1118: }
1119: }

In both cases, the contract uses prices.shortTokenPrice.max as the divisor when converting USD amounts to token amounts. This is fundamentally incorrect for a loss scenario:

  1. When calculating losses, the contract should use the minimum token price for denominators (prices.shortTokenPrice.min) to ensure a more conservative estimation of the token equivalent of the loss.

  2. Using the maximum price (prices.shortTokenPrice.max) results in a smaller token amount being deducted from the user's withdrawal.

Impact

Users withdrawing when the position has a negative PnL will get more tokens that they should receive.

  • Likelihood: high, as this happens on every withdrawal when the position has a negative PnL

  • Impact: medium, as this impacts all users' funds and creates perverse economic incentives

Recommended Mitigation Steps

Change the calculations to use prices.shortTokenPrice.min for both the fee and negative PnL adjustments:

function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
uint256 shares = depositInfo[depositId].shares;
if (shares == 0) {
revert Error.ZeroValue();
}
if (positionIsClosed) {
_handleReturn(0, true, false);
} else if (_isLongOneLeverage(beenLong)) {
uint256 swapAmount = IERC20(indexToken).balanceOf(address(this)) * shares / totalShares;
nextAction.selector = NextActionSelector.SWAP_ACTION;
// abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
nextAction.data = abi.encode(swapAmount, false);
} else if (curPositionKey == bytes32(0)) { // vault liquidated
_handleReturn(0, true, false);
} 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;
+ collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.min;
} else {
collateralDeltaAmount = collateralDeltaAmount - feeAmount;
}
uint256 acceptablePrice = abi.decode(metadata, (uint256));
_createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
}
}

This change ensures that the contract uses the most conservative price when accounting for losses, which correctly protects the vault and remaining users from excessive withdrawals.

Updates

Lead Judging Commences

n0kto Lead Judge 8 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.