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

Incorrect call arguments when fetching position info

vinica_boy

Summary

When users withdraw we remove the PnL based on the decreased size from the collateral delta amount. In order to get only the corresponding PnL we use VaultReader::getPnl() and pass as argument sizeDeltaInUsd equal to the amount coresponding to current withdrawal. In vault reader we call gmxReader.getPositionInfo() and pass true as the last argument which is usePositionSizeAsSizeDeltaUsd. This will basically override passed sizeDeltaInUsd and return the whole position PnL.

Vulnerability Details

In PerpetualVault::_withdraw() we have the following logic to process user's withdrawal when position is opened:

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

And for VaultReader::getPnl() we have:

function getPnl(
bytes32 key,
MarketPrices memory prices,
uint256 sizeDeltaUsd
) external view returns (int256) {
uint256 sizeInTokens = getPositionSizeInUsd(key);
if (sizeInTokens == 0) return 0;
// @audit incorrect last argument
PositionInfo memory positionInfo = gmxReader.getPositionInfo(
address(dataStore),
referralStorage,
key,
prices,
sizeDeltaUsd,
address(0),
true
);
​
return positionInfo.pnlAfterPriceImpactUsd;
}

Taking a look into ReaderPositionUtils::getPositionInfo() in GMX code we can see the following snippet:

if (usePositionSizeAsSizeDeltaUsd) {
sizeDeltaUsd = positionInfo.position.sizeInUsd();
}

Impact

Instead of accounting only for PnL based on the position size decrease, we always account for the whole PnL which lead to loss of funds for withrawers.

Tools Used

Manual review.

Recommendations

Pass false as argument when calling getPositionInfo() and we want information only for part of the position.

Updates

Lead Judging Commences

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

finding_getPnL_use_usePositionSizeAsSizeDeltaUsd_and_substract_it

Likelihood: Medium/High, every withdrawal from a short or leveraged vault that is not liquidated and has a negative PnL. Impact: High, subtract collateralDeltaAmount from the entire PnL of the position instead of the delta amount PnL. DoS or loss of funds

Support

FAQs

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

Give us feedback!