Summary
When user want to withdraw it can do it via withdraw function:
function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
_noneFlow();
flow = FLOW.WITHDRAW;
flowData = depositId;
if (recipient == address(0)) {
revert Error.ZeroValue();
}
if (depositInfo[depositId].timestamp + lockTime >= block.timestamp) {
revert Error.Locked();
}
if (EnumerableSet.contains(userDeposits[msg.sender], depositId) == false) {
revert Error.InvalidUser();
}
if (depositInfo[depositId].shares == 0) {
revert Error.ZeroValue();
}
depositInfo[depositId].recipient = recipient;
_payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle();
} else {
MarketPrices memory prices;
_withdraw(depositId, hex'', prices);
}
}
If there is current position key the function will first settle outstanding fees and update state before keeper proceed further in runNextAction :
function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
if (_isLongOneLeverage(_isLong)) {
_runSwap(metadata, true, prices);
} else {
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(_isLong, acceptablePrice, prices);
}
} else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);
}
.
.
.
Eventually the _withdraw will be invoked:
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;
nextAction.data = abi.encode(swapAmount, false);
} else if (curPositionKey == bytes32(0)) {
_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;
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);
}
}
As we can see if position is not closed and is not long with 1x leverage and current position key is not bytes32(0) the following code block will be used:
{
IVaultReader.PositionData memory positionData = vaultReader.getPositionInfo(curPositionKey, prices);
uint256 collateralDeltaAmount = positionData.collateralAmount * shares / totalShares;
uint256 sizeDeltaInUsd = positionData.sizeInUsd * shares / totalShares;
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 will get current data of the position from getPositionInfo and then it will calculate collateralDeltaAmount and sizeDeltaInUsd.
Then it will after getting fee amount calculate pnl of the position with provided and recently calculated sizeDeltaInUsd above and after that it will account that and _createDecreasePosition also with recently calculated collateralDeltaAmount and sizeDeltaInUsd.
Vulnerability Details
The issue arise in getPnl function and its design:
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
);
return positionInfo.pnlAfterPriceImpactUsd;
}
As we can see when trying to get position info from GMX with freshly provided sizeDeltaUsd the usePositionSizeAsSizeDeltaUsd is set to true which will use the position size as delta instead of provided and recently calculated sizeDeltaUsd.
Here is the relevant code snippet from getPositionInfo from gmx github contract:
if (usePositionSizeAsSizeDeltaUsd) {
sizeDeltaUsd = positionInfo.position.sizeInUsd();
}
Which will return incorrect value for pnlAfterPriceImpactUsd and with that the incorrect amount will be deducted from collateralDeltaAmount here:
if (pnl < 0) {
collateralDeltaAmount = collateralDeltaAmount - feeAmount - uint256(-pnl) / prices.shortTokenPrice.max;
} else {
collateralDeltaAmount = collateralDeltaAmount - feeAmount;
}
And eventually decrease position will be created with incorrect values because it uses the calculated value of sizeDeltaInUsd and collateralDeltaAmount with incorrectly amount of pnl deducted:
_createDecreasePosition(collateralDeltaAmount, sizeDeltaInUsd, beenLong, acceptablePrice, prices);
Which could lead to incorrect accounting for the protocol and potentially loss of funds for the user.
Impact
Due to incorrect calculation of pnl wrong data will be passed which could lead to incorrect accounting and loss of funds.
Tools Used
Manual Review.
Recommendations
Make sure not to override provided sizeDeltaUsd in getPnl :
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),
+ false
);
return positionInfo.pnlAfterPriceImpactUsd;
}
