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

[L-01] Incorrect Execution Order of `nextAction.selector` in `run()` Function

phaseTwo

Summary

In the PerpVault contract, the run function incorrectly sets nextAction.selector before the previous position is fully closed. This can cause unexpected behavior during execution, as the contract assumes a new position is ready to be opened while the previous one is still in progress. The correct logic should ensure that nextAction.selector is only set after the current position is fully closed.

Vulnerability Details

The run function is responsible for handling position changes (open, close, switch between long and short) based on off-chain execution signals. However, there is a logical flaw in the function:

  • When switching from a long to a short position (or vice versa), the function immediately sets nextAction.selector = NextActionSelector.INCREASE_ACTION; before ensuring that the previous position is completely closed.

  • This premature action could lead to conflicts in execution where the contract assumes a new position is ready when the old position has not yet been finalized.

  • The issue primarily occurs in the else block when the contract identifies that a position is currently open (positionIsClosed == false).

Code Snippet from run Function

else { //if a position is not closed
if (beenLong == isLong) { // Already in the same position, do nothing
revert Error.NoAction();
} else { // Different position detected
// INCORRECT ORDER OF EXECUTION
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(isLong);
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
}
}

Issue Explanation

  • nextAction.selector is set before _createDecreasePosition (which is responsible for closing the existing position).

  • If _createDecreasePosition fails or takes time, the next action might attempt to increase a position that is still open, leading to unexpected contract state behavior.

Impact

  • State Inconsistency: The contract might try to open a new position while the old one is still closing, causing race conditions.

  • Potential Execution Failures: If a position is still in the process of closing, but the contract believes it has already closed, subsequent actions may fail or revert.

  • Keeper Coordination Issues: The off-chain keeper script may be misled into executing orders in an invalid sequence.

Tools Used

  • Manual code review

Recommendation

To mitigate this issue, the contract should ensure that nextAction.selector is only set after the existing position has been fully closed.

Suggested Fix

else { //if a position is not closed
if (beenLong == isLong) { // Already in the same position, do nothing
revert Error.NoAction();
} else { // Different position detected
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
// Move this after position closure to avoid premature execution
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(isLong);
}
}

Key Fixes:

  • Ensure position is fully closed before setting nextAction.selector.

  • Move the assignment of nextAction.selector to after _createDecreasePosition() executes.

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.

Support

FAQs

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