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

Unchecked External Calls in afterOrderCancellation() May Cause Execution Failures

Summary

The function afterOrderCancellation() in GmxProxy.sol makes an unchecked external call to transfer ERC20 tokens. If the token does not conform to the ERC20 standard or deliberately reverts, the entire execution flow could fail, leading to denial of service (DoS) and preventing order cancellations from completing.

Vulnerability Details

The function afterOrderCancellation() calls safeTransfer() on an ERC20 token without verifying whether the transfer succeeds:

IERC20(eventData.addressItems.items[0].value).safeTransfer(perpVault, eventData.uintItems.items[0].value);

If the token being transferred is a malicious ERC20 token or one that does not return a success value, this can cause the function to revert unexpectedly. This prevents order cancellations, effectively locking user funds and disrupting trading operations.

Attack Scenario

  1. A user deposits a malicious ERC20 token that reverts on transfer.

  2. An order cancellation is triggered.

  3. afterOrderCancellation() attempts to transfer the token.

  4. The transfer fails, and the function reverts, preventing the order from being canceled.

  5. The order remains in limbo, and the protocol cannot process future cancellations.

Impact

  • Denial of Service (DoS): The entire order cancellation process fails if the token transfer fails.

  • Funds Stuck: Users may be unable to retrieve assets if the order cannot be canceled.

  • Protocol Instability: If multiple transactions fail due to this issue, the entire GMX order system may become unreliable.

Tools Used

Manual Review

Recommendations

Use try/catch for external calls

try IERC20(token).safeTransfer(to, amount) {} catch {
revert("Token transfer failed");
}
Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope
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: Out of scope
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.