DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

close other’s position leads withdraw tokens

Summary

} else if (orderResultData.orderType == Order.OrderType.MarketDecrease) {
uint256 sizeInUsd = vaultReader.getPositionSizeInUsd(curPositionKey);
if (sizeInUsd == 0) {
delete curPositionKey;
}
function afterLiquidationExecution() external {
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}
depositPaused = true;
uint256 sizeInTokens = vaultReader.getPositionSizeInTokens(curPositionKey);
if (sizeInTokens == 0) {
delete curPositionKey;
}

The curPositionKey is only initialized when closing all positions or during liquidation, so using this, an attacker can withdraw more tokens.

Steps to Reproduce

Since the test code does not work, here’s an explanation of the attack sequence.

  1. The attacker opens a new small position.

  2. The attacker waits until another user creates a position.

The curPositionKey variable is set when opening a new position and executing the afterOrderExecution function.

  1. The attacker calls the withdrawal function. The following conditions are required:

function withdraw(address recipient, uint256 depositId) public payable nonReentrant {
_noneFlow();
flow = FLOW.WITHDRAW;
flowData = depositId;
...
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle(); // Settles any outstanding fees and updates state before processing withdrawal
}

Since curPositionKey is already set, the WITHDRAW_ACTION will be executed.

} else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
...
uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);
}
function _withdraw(uint256 depositId, bytes memory metadata, MarketPrices memory prices) internal {
uint256 shares = depositInfo[depositId].shares;
...
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);
}
}

If positionIsClosed is false and _isLongOneLeverage(beenLong) is false, the _createDecreasePosition function will be executed. The issue here is that curPosition belongs to another user's position, and that position is always larger than the attacker's position. By exploiting this, the attacker can close a larger position and make a profit.

References

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

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