Description:
The root cause of this issue can have two similar impacts, which I will explain with 2 different scenarios:
Scenario 1:
A position is open on GMX and a user decices to withdraw, but due to market conditions, the position collateral will not be enough to withdraw the user's position, so the order's sizeDeltaInUsd gets set to the whole position size:
if (
sizeDeltaInUsd == 0
|| vaultReader.willPositionCollateralBeInsufficient(
prices, curPositionKey, market, _isLong, sizeDeltaInUsd, collateralDeltaAmount)
) {
@> sizeDeltaInUsd = sizeInUsd;
}
In the code snippet above we can see that if willPositionCollateralBeInsufficient function returns true, sizeDeltaInUsd gets set to the whole position size.
When GMX executes the MarketDecrease order request, PerpetualVault::afterOrderExecution will get called and this part of the function will trigger (the fees have been settled in previous order):
} else if (orderResultData.orderType == Order.OrderType.MarketDecrease) {
uint256 sizeInUsd = vaultReader.getPositionSizeInUsd(curPositionKey);
@> if (sizeInUsd == 0) {
delete curPositionKey;
}
if (flow == FLOW.WITHDRAW) {
nextAction.selector = NextActionSelector.FINALIZE;
uint256 prevCollateralBalance = collateralToken.balanceOf(address(this)) - orderResultData.outputAmount;
nextAction.data = abi.encode(prevCollateralBalance, sizeInUsd == 0, false);
In the snippet above we can see that only curPositionKey gets deleted, but positionIsClosed is not set to true, even though the position is closed on GMX.
Due to this, when a new user calls PerpetualVault::deposit, a MarketIncrease order request will be sent to GMX to increase the position, but the position is closed at this time, so GMX will open a new position, and the user, along with the next depositors, will pay the execution fee, when in reality, the position should be closed at this time and when there is no open position, depositors should not pay an execution fee.
Scenario 2, which results in users not receiving execution fee refund when they should:
Users who call withdraw function always pay execution fee when positionIsClosed is false, but they do not get refunded in the scenario where positionIsClosed is false and curPositionKey is 0 at the same time, which can happen when GMX executes a MarketDecrease order request and PerpetualVault::afterOrderExecution gets called, if the order completely closes the GMX position. This scenario happens when the flow is WITHDRAW. Here is the relevant code snippet:
} else if (orderResultData.orderType == Order.OrderType.MarketDecrease) {
uint256 sizeInUsd = vaultReader.getPositionSizeInUsd(curPositionKey);
@> if (sizeInUsd == 0) {
delete curPositionKey;
}
In the code snippet above, only curPositionKey is deleted, but positionIsClosed is not set to true, so if, after this withdrawal gets finalized, another user calls withdraw, he will pay an execution fee, but will not get refunded, when in reality, the GMX position is closed and the keeper did not open a new one yet.
Reminding you that this is how it's posssible for a single user to close the entire GMX position:
if (
sizeDeltaInUsd == 0
|| vaultReader.willPositionCollateralBeInsufficient(
prices, curPositionKey, market, _isLong, sizeDeltaInUsd, collateralDeltaAmount)
) {
@> sizeDeltaInUsd = sizeInUsd;
}
As you can see, if VaultReader::willPositionCollateralBeInsufficient returns true, the whole GMX position will be closed in the following order execution. This function can return true for multiple reasons:
In this scenario, there could be other users that possess shares, and if they call withdraw after the current MarketDecrease order executes, they will pay the execution fee and will not get refunded, even though the GMX position is closed and all remaining funds are in the vault.
Here is a code snippet from withdraw function, which shows that users will pay an execution fee (positionIsClosed is still false at this point) and then _withdraw gets called:
@> _payExecutionFee(depositId, false);
if (curPositionKey != bytes32(0)) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
_settle();
@> } else {
MarketPrices memory prices;
@> _withdraw(depositId, hex'', prices);
}
}
In the below code snippet we see that _handleReturn function gets called with the last parameter set to false. This is a problem, because in this case the position on GMX is closed, which means the funds are in the vault, so the execution fee should be refunded.
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);
}
function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
In the code snippet above, we can see that the third parameter is refundFee, which gets checked at the end of _handleReturn function:
@> if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
Impact:
Scenario 1:
All depositors, after the issue occurs, have to pay an execution fee, when in reality they should not pay because position should be closed until keeper opens it. Additionally, first depositor after the issue occurs basically creates a new position on GMX when the keeper did not plan it and, by design, all positions should only be opened by a keeper, based on signals.
Scenario 2:
Users who still have shares after a GMX position gets closed will call withdraw and will not receive execution fee refund.
Likelihood:
The issue can happen when a user wants to withdraw, but position collateral is not enough, and this can occur due to various market conditions, such as:
Proof of Concept:
Scenario 1:
Add the test provided below to PerpetualVault.t.sol
Run the test with this command: forge test --mt test_PositionIsClosed_NotSetToTrue --via-ir --rpc-url <YOUR_RPC_URL_HERE> -vv
In terminal you should see output like this:
Logs:
total amount after alice's withdrawal: 0
curPositionKey: 0
positionIsClosed: false
Read the comments in the provided test to understand what's happening
function test_PositionIsClosed_NotSetToTrue() external {
address keeper = PerpetualVault(vault2x).keeper();
address alice = makeAddr("alice");
depositFixtureInto2x(alice, 10e9);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3390000000000000);
vm.prank(keeper);
PerpetualVault(vault2x).run(true, true, prices, data);
PerpetualVault.FLOW flow = PerpetualVault(vault2x).flow();
assertEq(uint8(flow), 2);
assertEq(PerpetualVault(vault2x).positionIsClosed(), true);
(PerpetualVault.NextActionSelector selector,) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector), 0);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
uint256[] memory aliceDepositIds = PerpetualVault(vault2x).getUserDeposits(alice);
uint256 executionFee = PerpetualVault(vault2x).getExecutionGasLimit(false);
uint256 lockTime = 1;
PerpetualVault(vault2x).setLockTime(lockTime);
vm.warp(block.timestamp + lockTime + 1);
bytes[] memory swapData = new bytes[](2);
swapData[0] = abi.encode(0);
deal(alice, 1 ether);
vm.prank(alice);
PerpetualVault(vault2x).withdraw{value: executionFee * tx.gasprice}(alice, aliceDepositIds[0]);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, swapData);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
console.log("total amount after alice's withdrawal:", PerpetualVault(vault2x).totalAmount(prices));
console.log("curPositionKey:", uint256(PerpetualVault(vault2x).curPositionKey()));
console.log("positionIsClosed:", PerpetualVault(vault2x).positionIsClosed());
assertEq(PerpetualVault(vault2x).totalAmount(prices), 0);
assertEq(uint256(PerpetualVault(vault2x).curPositionKey()), 0);
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
address bob = makeAddr("bob");
deal(bob, 1 ether);
uint256 bobsEthBalanceBeforeDeposit = address(bob).balance;
depositFixtureInto2x(bob, 10e9);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
uint256 bobsEthBalanceAfterDeposit = address(bob).balance;
assertTrue(bobsEthBalanceAfterDeposit < bobsEthBalanceBeforeDeposit);
}
Scenario 2:
Add the test provided below to PerpetualVault.t.sol
Run the test with this command: forge test --mt test_ExecutionFee_NotRefunded --via-ir --rpc-url <YOUR_RPC_URL_HERE> -vv
In terminal you should see output like this:
Logs:
curPositionKey: 0
positionIsClosed: false
Read the comments in the provided test to understand what's happening
function test_ExecutionFee_NotRefunded() external {
address keeper = PerpetualVault(vault2x).keeper();
address alice = makeAddr("alice");
depositFixtureInto2x(alice, 10e9);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3390000000000000);
vm.prank(keeper);
PerpetualVault(vault2x).run(true, true, prices, data);
assertEq(PerpetualVault(vault2x).positionIsClosed(), true);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
address bob = makeAddr("bob");
deal(bob, 1 ether);
depositFixtureInto2x(bob, 10e9);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
uint256[] memory aliceDepositIds = PerpetualVault(vault2x).getUserDeposits(alice);
uint256 executionFee = PerpetualVault(vault2x).getExecutionGasLimit(false);
uint256 lockTime = 1;
PerpetualVault(vault2x).setLockTime(lockTime);
vm.warp(block.timestamp + lockTime + 1);
bytes[] memory swapData = new bytes[](2);
swapData[0] = abi.encode(0);
bytes4 selector = bytes4(
keccak256(
"willPositionCollateralBeInsufficient(((uint256,uint256),(uint256,uint256),(uint256,uint256)),bytes32,address,bool,uint256,uint256)"
)
);
vm.mockCall(address(reader), selector, abi.encode(true));
deal(alice, 1 ether);
vm.prank(alice);
PerpetualVault(vault2x).withdraw{value: executionFee * tx.gasprice}(alice, aliceDepositIds[0]);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, swapData);
GmxOrderExecuted2x(true);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, new bytes[](0));
console.log("curPositionKey:", uint256(PerpetualVault(vault2x).curPositionKey()));
console.log("positionIsClosed:", PerpetualVault(vault2x).positionIsClosed());
assertEq(uint256(PerpetualVault(vault2x).curPositionKey()), 0);
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
uint256[] memory bobDepositIds = PerpetualVault(vault2x).getUserDeposits(bob);
(, uint256 bobShares,,,,) = PerpetualVault(vault2x).depositInfo(bobDepositIds[0]);
assertGt(bobShares, 0);
IERC20 collateralToken = PerpetualVault(vault2x).collateralToken();
assertGt(collateralToken.balanceOf(vault2x), 0);
uint256 bobsEthBalanceBeforeWithdraw = address(bob).balance;
vm.prank(bob);
PerpetualVault(vault2x).withdraw{value: executionFee * tx.gasprice}(bob, bobDepositIds[0]);
uint256 bobsEthBalanceAfterWithdraw = address(bob).balance;
assertTrue(bobsEthBalanceAfterWithdraw < bobsEthBalanceBeforeWithdraw);
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
}
Recommended Mitigation:
Set positionIsClosed to true if after a MarketDecrease order execution, current position size is 0.
} else if (orderResultData.orderType == Order.OrderType.MarketDecrease) {
uint256 sizeInUsd = vaultReader.getPositionSizeInUsd(curPositionKey);
if (sizeInUsd == 0) {
delete curPositionKey;
+ positionIsClosed = true;
}
if (flow == FLOW.WITHDRAW) {
nextAction.selector = NextActionSelector.FINALIZE;
uint256 prevCollateralBalance = collateralToken.balanceOf(address(this)) - orderResultData.outputAmount;
nextAction.data = abi.encode(prevCollateralBalance, sizeInUsd == 0, false);
Additionally, consider setting the last parameter to true when calling _handleReturn in this situation:
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)) {
- _handleReturn(0, true, false);
+ _handleReturn(0, true, true);
}
