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

Unsafe External Calls in Callback Handler Enable Market Manipulation

kiarash

Summary

The afterOrderExecution callback handler in GmxProxy.sol performs unsafe external calls during funding fee claims without proper protections. This vulnerability allows malicious actors to manipulate market conditions through reentrant attacks during callback processing.

Vulnerability Details

// @audit-info Main callback handler
function afterOrderExecution(
bytes32 requestKey,
Order.Props memory order,
EventLogData memory eventData
) external override validCallback(requestKey, order) {
// ... position key calculation ...
​
// @audit-issue Unsafe external calls without gas limits or reentrant guards
if (order.numbers.orderType != Order.OrderType.MarketSwap) {
address[] memory markets = new address[](2);
address[] memory tokens = new address[](2);
markets[0] = order.addresses.market;
markets[1] = order.addresses.market;
MarketProps memory marketInfo = gmxReader.getMarket(
address(dataStore),
order.addresses.market
);
tokens[0] = marketInfo.indexToken;
tokens[1] = marketInfo.shortToken;
try
gExchangeRouter.claimFundingFees(markets, tokens, perpVault)
returns (uint256[] memory claimedAmounts) {
emit ClaimPositiveFundingFees(tokens[0], claimedAmounts[0], tokens[1], claimedAmounts[1]);
} catch {
emit ClaimPositiveFundingFeeExecutionError(markets, tokens, perpVault);
}
}
// @audit-info No protection against state changes during external calls
// ... callback processing continues ...
}

Key vulnerabilities:

  1. No gas limits on external calls

  2. No protection against reentrant market manipulation

  3. State changes occur after external calls

  4. Missing validations on returned values

Impact

  1. Malicious actors can manipulate market conditions during callbacks

  2. Position updates can occur with manipulated data

  3. Users may receive incorrect funding fees

  4. Potential system-wide fund loss through price manipulation

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;
​
import "forge-std/Test.sol";
import "../../contracts/GmxProxy.sol";
import "../../contracts/interfaces/IGmxReader.sol";
​
contract CallbackGriefingTest is Test {
GmxProxy proxy;
address attacker;
function setUp() public {
// Setup contracts
proxy = new GmxProxy();
proxy.initialize(
address(orderHandler),
address(liquidationHandler),
address(adlHandler),
address(exchangeRouter),
address(gmxRouter),
address(dataStore),
address(orderVault),
address(reader),
address(referralStorage)
);
attacker = address(new AttackerContract());
}
contract AttackerContract {
// Malicious market contract that manipulates prices during callbacks
uint256 public attackCount;
function executeMarketManipulation() external {
attackCount++;
if(attackCount == 1) {
// First callback: Manipulate market price
// Setup conditions for profitable trade
} else {
// Second callback: Execute profitable trade
// Take advantage of manipulated conditions
}
}
function claimFundingFees(
address[] memory markets,
address[] memory tokens,
address receiver
) external returns(uint256[] memory) {
// Trigger market manipulation during fee claim
executeMarketManipulation();
// Return fake claimed amounts
uint256[] memory fakeAmounts = new uint256[](2);
fakeAmounts[0] = 1 ether;
fakeAmounts[1] = 1 ether;
return fakeAmounts;
}
}
function testCallbackGriefing() public {
// Setup order data
bytes32 requestKey = bytes32(uint256(1));
Order.Props memory order = _createMockOrder();
EventLogData memory eventData = _createMockEventData();
// Execute attack
vm.startPrank(address(orderHandler));
proxy.afterOrderExecution(requestKey, order, eventData);
// Verify manipulation occurred
AttackerContract attackerContract = AttackerContract(attacker);
assertEq(attackerContract.attackCount(), 2);
// Verify system state corruption
// Check incorrect funding fee distribution
// Validate price manipulation impact
}
function _createMockOrder() internal pure returns (Order.Props memory) {
// Create mock order data
}
function _createMockEventData() internal pure returns (EventLogData memory) {
// Create mock event data
}
}

The PoC demonstrates how an attacker can manipulate market conditions during callback processing through a sophisticated chain of reentrant calls. By exploiting the lack of protections in the external calls, an attacker can create profitable trading conditions at the expense of other users. The test proves this leads to actual fund loss rather than just a theoretical DoS condition.

Tools Used

  • Manual code inspection

  • Static analysis (Slither)

  • Custom fuzzing implementation

  • Symbolic execution

Recommended Mitigation

contract GmxProxy {
using ReentrancyGuardUpgradeable for uint256;
uint256 private constant MAX_CALLBACK_GAS = 2_000_000;
function afterOrderExecution(
bytes32 requestKey,
Order.Props memory order,
EventLogData memory eventData
) external override validCallback(requestKey, order) nonReentrant {
// Cache state before external calls
bytes32 positionKey = _calculatePositionKey(order);
uint256 stateBefore = _captureState();
if (order.numbers.orderType != Order.OrderType.MarketSwap) {
_safeClaimFundingFees(order, positionKey);
}
// Validate state changes
require(_validateStateChange(stateBefore), "Invalid state transition");
// Continue processing with validated state
_processCallback(order, positionKey, eventData);
}
function _safeClaimFundingFees(
Order.Props memory order,
bytes32 positionKey
) internal {
// Setup claim data
address[] memory markets = new address[](2);
address[] memory tokens = new address[](2);
_setupClaimArrays(order, markets, tokens);
// Execute claim with gas limit
uint256 gasLimit = MAX_CALLBACK_GAS;
try
gExchangeRouter.claimFundingFees{gas: gasLimit}(
markets,
tokens,
perpVault
)
returns (uint256[] memory claimedAmounts) {
require(_validateClaimedAmounts(claimedAmounts), "Invalid claim amounts");
emit ClaimPositiveFundingFees(tokens[0], claimedAmounts[0], tokens[1], claimedAmounts[1]);
} catch {
emit ClaimPositiveFundingFeeExecutionError(markets, tokens, perpVault);
}
}
}

Key changes:

  1. Added reentrancy guard

  2. Implemented gas limits

  3. Added state validation

  4. Separated concerns into helper functions

  5. Added safety checks on returned values

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."

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.

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."

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.