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

Withdrawing All ETH can break Settlement, Order Operations and the afterOrderExecution

codertjay

Summary

The GmxProxy contract allows the owner to withdraw all ETH through withdrawEth(), which can prevent two critical
operations: settling existing positions. This breaks key protocol functionality the operation require ETH for execution fees.

Also in the protocol we have a strong invariant in which afterOrderExecution which must not break which also requires some gas fees.
If there is an issue or if the admin likely went ahead to withdrawEth on gmxProxy before afterOrderExecution is called this could affect data flow in the perpetualvault and that requires some gas fees to process and as u know according to the dev on afterOrderExecution on perpetualVault that function must never break

The PerpetualVault

/**
* @notice Callback function that is called when an order on GMX is executed successfully.
* This function handles the post-execution logic based on the type of order that was executed.
* @dev This callback function must never be reverted. It should wrap revertible external calls with `try-catch`.
* @param requestKey The request key of the executed order.
* @param positionKey The position key.
* @param orderResultData Data from the order execution.
*/
function afterOrderExecution(
bytes32 requestKey,
bytes32 positionKey,
IGmxProxy.OrderResultData memory orderResultData,
MarketPrices memory prices
) external nonReentrant {
// @audit: only gmxProxy() can call this function
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}

The GmxProxy

function afterOrderExecution(bytes32 requestKey, Order.Props memory order, EventLogData memory eventData)
external
override
validCallback(requestKey, order)
{
bytes32 positionKey = keccak256(
abi.encode(
address(this), order.addresses.market, order.addresses.initialCollateralToken, order.flags.isLong
)
);
.......
// rest of code
IPerpetualVault(perpVault).afterOrderExecution(requestKey, positionKey, orderResultData, prices);
delete queue;
​

Vulnerability Details

Two critical functions require ETH balance for execution fees:

  1. Creating new orders: This is where the fees is added by the user creating the order

function createOrder(Order.OrderType orderType, IGmxProxy.OrderData memory orderData) public returns (bytes32) {
uint256 positionExecutionFee = getExecutionGasLimit(orderType, orderData.callbackGasLimit) * tx.gasprice;
require(address(this).balance >= positionExecutionFee, "insufficient eth balance");
gExchangeRouter.sendWnt{value: positionExecutionFee}(orderVault, positionExecutionFee);
// ... rest of order creation
}

However, the owner can drain all ETH before settle is called or afterOrderExecution

function withdrawEth() external onlyOwner returns (uint256) {
uint256 balance = address(this).balance;
payable(msg.sender).transfer(balance); // Withdraws ALL ETH
return balance;
}

  1. Settlement of existing positions:

function settle(IGmxProxy.OrderData memory orderData) external returns (bytes32) {
uint256 positionExecutionFee = getExecutionGasLimit(Order.OrderType.MarketDecrease, orderData.callbackGasLimit) * tx.gasprice;
require(address(this).balance >= positionExecutionFee, "insufficient eth balance");
gExchangeRouter.sendWnt{value: positionExecutionFee}(orderVault, positionExecutionFee);
// ... rest of settlement logic
}

Impact

HIGH - If owner withdraws all ETH:

  1. Existing positions cannot be settled due to insufficient ETH for execution fees

  2. AfterOrderExicution could break because it requires eth for transaction fees and would affect variables flow

Tools Used

  • Manual code review

Proof Of Concept

This test shows how the owner withdraw all eth which prevent creating position for jsut this test scenario but could also affect the afterOrderExecution after an order has been created

function test_withdrawEthCausesOrderCreationToFail() public {
​
address gmxProxyAddress = address(PerpetualVault(vault).gmxProxy());
GmxProxy gmxProxy = GmxProxy(payable(gmxProxyAddress));
​
// Setup: Create a deposit first
address alice = makeAddr("alice");
payable(alice).transfer(1 ether);
​
// Get USDC token
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
​
// Instead of using deal, we can either:
address whale = 0x489ee077994B6658eAfA855C308275EAd8097C4A; // Arbitrum USDC whale
vm.startPrank(whale);
uint256 amount = 1e10;
collateralToken.transfer(alice, amount);
vm.stopPrank();
​
// Now proceed with deposit
vm.startPrank(alice);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
​
console.log("The GmxProxy Contract balance before withdraw",address(gmxProxy).balance);
​
// Get owner and withdraw all ETH
address owner = gmxProxy.owner();
vm.startPrank(owner);
uint256 withdrawn = gmxProxy.withdrawEth();
vm.stopPrank();
​
console.log("The GmxProxy Contract balance after withdraw",address(gmxProxy).balance);
console.log("The GmxProxy Contract minEth", gmxProxy.minEth());
​
// Verify ETH was withdrawn
assertEq(address(gmxProxy).balance, 0, "Should have withdrawn all ETH");
​
// Try to create an order - should fail due to insufficient ETH
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](2);
data[0] = abi.encode(3380000000000000);
​
// Get owner of keeper to execute order
address keeperOwner = KeeperProxy(keeper).owner();
vm.startPrank(keeperOwner);
​
// This should revert because GmxProxy has no ETH for execution fees
vm.expectRevert();
KeeperProxy(keeper).run(vault, true, false, prices, data);
​
vm.stopPrank();
​
// Additional assertions to show impact
assertEq(address(gmxProxy).balance, 0, "Balance should still be zero");
​
}

The Log Gotten From Foundry Response

➜ 2025-02-gamma git:(main) ✗ forge test --mt test_withdrawEthCausesOrderCreationToFail -vvvv --via-ir --rpc-url arbitrum
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/KeeperProxy.t.sol:KeeperProxyTest
[PASS] test_withdrawEthCausesOrderCreationToFail() (gas: 721357)
Traces:
........
│ │ │ │ │ └─ ← [Return] true
│ │ │ │ └─ ← [Revert] LowerThanMinEth()
│ │ │ └─ ← [Revert] LowerThanMinEth()
│ │ └─ ← [Revert] LowerThanMinEth()
│ └─ ← [Revert] LowerThanMinEth()
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Return]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.38ms (3.92ms CPU time)
​
Ran 1 test suite in 4.82s (11.38ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
➜ 2025-02-gamma git:(main) ✗
​

Recommendations

Add ETH reserve requirement in withdrawEth():

This way if there is an active order owner shouldnt be able to withdraw all ethereum from the gmxProxy

function withdrawEth() external onlyOwner returns (uint256) {
uint256 balance = address(this).balance;
​
// Calculate required ETH for pending operations
uint256 requiredEth = 0;
​
// If there's a pending order that needs settlement
if (queue.requestKey != bytes32(0)) {
requiredEth += getExecutionGasLimit(
Order.OrderType.MarketDecrease,
2_000_000 // Default callback gas
) * tx.gasprice;
}
​
// Add minEth requirement
requiredEth += minEth;
​
require(balance > requiredEth, "Must maintain ETH reserve");
​
uint256 withdrawable = balance - requiredEth;
payable(msg.sender).transfer(withdrawable);
return withdrawable;
}

This ensures sufficient ETH remains for both settlement and new order operations.

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Support

FAQs

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