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

Incorrectly passed in parameter in VaultReader::getPnl() leads to wrong pnl value returned during withdrawals.

0xdice91

Summary

In VaultReader::getPnl() the call to gmxReader.getPositionInfo sets the usePositionSizeAsSizeDeltaUsd boolean in the call parameter to true this causes the value set for sizeDeltaUsd to be overridden
back to its previous state leading to a wrong Pnl being returned.

Vulnerability Details

During withdrawal of a deposit ID in the function _withdraw() before the pnl is retrieved the sizeDeltaInUsd is calculated using the share of the deposit ID and the positionData.sizeInUsd of the position on gmx. Then this value is passed into getPnl(), this is done to ensure that the Pnl being returned is based on the share of the deposit ID being withdrawn.

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)) { // beenLong && leverage == BASIS_POINTS_DIVISOR
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;
@> 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);
}
}

The function VaultReader::getPnl() calls gmxReader.getPositionInfo() which returns information about the position from which the pnl is extracted.

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

The issue here is that VaultReader::getPnl() calls gmxReader.getPositionInfo() with usePositionSizeAsSizeDeltaUsd set to true this causes the value passed into sizeDeltaUsd to be overridden to
positionInfo.position.sizeInUsd() this will make the pnl that is returned that of the whole position instead that of the share being withdrawn.

Looking at gmxReader.getPositionInfo()

function getPositionInfo(
DataStore dataStore,
IReferralStorage referralStorage,
Position.Props memory position,
MarketUtils.MarketPrices memory prices,
uint256 sizeDeltaUsd,
address uiFeeReceiver,
@> bool usePositionSizeAsSizeDeltaUsd
) internal view returns (PositionInfo memory) {
if (position.account() == address(0)) {
revert Errors.EmptyPosition();
}
​
​
PositionInfo memory positionInfo;
GetPositionInfoCache memory cache;
​
​
positionInfo.position = position;
cache.market = MarketStoreUtils.get(dataStore, positionInfo.position.market());
cache.collateralTokenPrice = MarketUtils.getCachedTokenPrice(positionInfo.position.collateralToken(), cache.market, prices);
​
​
@> if (usePositionSizeAsSizeDeltaUsd) {
@> sizeDeltaUsd = positionInfo.position.sizeInUsd(); //@audit it is overridden here is usePositionSizeAsSizeDeltaUsd is true
​
}
​
//More code....
}

The value passed in for sizeDeltaUsd from _withdraw() is replaced when usePositionSizeAsSizeDeltaUsd is set to true, leading to the wrong pnl being returned for the deposit ID.

Impact

Wrong sizeDeltaUsd is used to calculate the pnl of the deposit ID causing errors in calculations and therefore withdrawals.

Tools Used

manual review

Recommendations

VaultReader::getPnl() should be correct to ensure the correct pnl is returned during withdrawal.

function getPnl(
bytes32 key,
MarketPrices memory prices,
uint256 sizeDeltaUsd
) external view returns (int256) {
uint256 sizeInTokens = getPositionSizeInUsd(key);
if (sizeInTokens == 0) return 0;
PositionInfo memory positionInfo = gmxReader.getPositionInfo(
address(dataStore),
referralStorage,
key,
prices,
sizeDeltaUsd,
address(0),
-- true //@audit should be set to false else as it will be overrideen in gmx
++ false
);
​
return positionInfo.pnlAfterPriceImpactUsd;
}
Updates

Lead Judging Commences

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