Liquidity Management

Where It Happens

Within the createOrder() function:

Here, the contract calls:

without resetting the approval back to zero later. This may cause the allowance to remain above zero indefinitely, at least until it is overwritten by a future approval. Note that:

• The code never does safeApprove(..., 0) before re-approving with amountIn.

• Some ERC20 tokens do not allow changing an existing non-zero allowance to another non-zero value without first resetting to zero. This can lead to reverts if orderData.amountIn changes from call to call (depending on the token’s implementation).

• Even if the token allows changing approvals from non-zero to non-zero, the contract is then effectively giving the GMX router a continuing allowance—not just for the single order. If the GMX router were compromised or replaced, it could pull additional tokens that remain in GmxProxy later, beyond the intended single use.

Impact

1. Potential Unlimited Token Approval:

   • If multiple orders are created over time, the allowance might accumulate or remain at a higher level than intended. A malicious (or compromised) GMX router contract could subsequently transfer more tokens from GmxProxy if the contract has leftover token balances.

2. Compatibility Reverts:

   • Certain fee-on-transfer or strict ERC20 tokens revert if you attempt to call approve(...) from one non-zero allowance to a different non-zero value. This might break createOrder() calls for specific tokens.

3. Unexpected Over-Spending:

   • If at any point the contract receives more of orderData.initialCollateralToken than needed, the GMX router might be able to spend the difference (if the allowance remains or gets inadvertently increased).

Issue Spot:

• SafeApprove usage is commonly known to be risky if you do not reset approvals to zero before granting new non-zero approvals.

• Many standard references (including OpenZeppelin’s recommendations) advise a pattern of approve(spender, 0) before approve(spender, someAmount) to avoid revert-prone tokens or to ensure no indefinite leftover approvals.

• The current contract code does not follow this pattern, opening up both a potential security hole (unintended or indefinite approvals) and an incompatibility with stricter ERC20 tokens.

Recommended Remediation

1. Reset Approvals Before Re-Approving:

   IERC20(token).safeApprove(address(gmxRouter), 0);

   IERC20(token).safeApprove(address(gmxRouter), orderData.amountIn);

   ​

   This ensures only the needed allowance is active for each order and is valid for tokens that forbid changing a non-zero allowance directly to another non-zero.

2. Use a “Safe” Single-Use Pattern:

   • If the contract wants truly ephemeral approvals, it can do something like:

     IERC20(token).safeIncreaseAllowance(address(gmxRouter), orderData.amountIn);

     // Transfer ...

     IERC20(token).safeDecreaseAllowance(address(gmxRouter), orderData.amountIn);

     ​

   • Or forcibly zero the approval after the tokens are sent. This ensures the router never retains an ongoing allowance beyond each transaction.