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

Position Can Be Created Without Closing Previous Position on Type Change

rampage

Summary

In PerpetualVault.sol, when changing position types, a flaw in afterOrderCancellation() allows the creation of new positions without properly closing existing ones. This occurs due to incomplete cleanup of the next action state when an order is cancelled.

Vulnerability Details

The vulnerability manifests in the following sequence:

  1. The keeper initiates a position type change through PerpetualVault.sol#run():

  • Sets nextAction.selector to INCREASE_ACTION

  • Attempts to close the current position via _createDecreasePosition()

  1. If the order is cancelled, afterOrderCancellation() is triggered:

  • Cleans up flowData and flow

  • Critically fails to clear nextAction

  1. When runNextAction() is subsequently called:

  • Processes the lingering INCREASE_ACTION

  • Creates a new position while the old one remains open

This creates a scenario where multiple positions can exist simultaneously, which violates the protocol's intended behavior.

Key code sections remain as in original report:

​
/**
* @notice Off-chain script calls `run` function to change position type (long/short/close).
* @param isOpen If true, open a position; if false, close the current position.
* @param isLong If true, open a long position; if false, open a short position.
* @param prices GMX price data of market tokens from GMX API call.
* @param metadata Swap transaction data generated by off-chain script.
*/
function run(
bool isOpen,
bool isLong,
MarketPrices memory prices,
bytes[] memory metadata
) external nonReentrant {
_noneFlow();
_onlyKeeper();
if (gmxProxy.lowerThanMinEth()) {
revert Error.LowerThanMinEth();
}
flow = FLOW.SIGNAL_CHANGE;
​
if (isOpen) {
if (positionIsClosed) {
if (_isFundIdle() == false) {
revert Error.InsufficientAmount();
}
if (_isLongOneLeverage(isLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(isLong, acceptablePrice, prices);
}
} else {
if (beenLong == isLong) {
revert Error.NoAction();
} else {
// Close current position first and then open the requested position in the next action
-> 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);
}
}
}
} else {
if (positionIsClosed == false) {
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
} else {
revert Error.NoAction();
}
}
}
​
​
/**
* @notice Callback function triggered when an order execution on GMX is canceled due to an error.
* @param requestKey The request key of the executed order.
* @param orderType The type of order.
* @param orderResultData The result data of the order execution.
*/
function afterOrderCancellation(
bytes32 requestKey,
Order.OrderType orderType,
IGmxProxy.OrderResultData memory orderResultData
) external {
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}
_gmxLock = false;
​
if (orderResultData.isSettle) {
// Retry settle action.
nextAction.selector = NextActionSelector.SETTLE_ACTION;
} else if (orderType == Order.OrderType.MarketSwap) {
// If GMX swap fails, retry in the next action.
nextAction.selector = NextActionSelector.SWAP_ACTION;
// abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
nextAction.data = abi.encode(swapProgressData.remaining, swapProgressData.isCollateralToIndex);
} else {
if (flow == FLOW.DEPOSIT) {
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
} else if (flow == FLOW.WITHDRAW) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
} else {
-> // If signal change fails, the offchain script starts again from the current status.
-> delete flowData;
-> delete flow;
}
}
emit GmxPositionCallbackCalled(requestKey, false);
}
​
​
/**
* @notice `flow` is not completed in one tx. So in that case, set up the next action data
* and run the `nextAction`.
* @param prices GMX price data of market tokens from GMX API call
* @param metadata swap tx data. generated by off-chain script
*/
function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
​
if (_isLongOneLeverage(_isLong)) {
_runSwap(metadata, true, prices);
} else {
// swap indexToken that could be generated from the last action into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(_isLong, acceptablePrice, prices);
}
} else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
// swap indexToken that could be generated from settle action or liquidation/ADL into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);
} else if (_nextAction.selector == NextActionSelector.SWAP_ACTION) {
(, bool isCollateralToIndex) = abi.decode(
_nextAction.data,
(uint256, bool)
);
_runSwap(metadata, isCollateralToIndex, prices);
} else if (_nextAction.selector == NextActionSelector.SETTLE_ACTION) {
_settle();
} else if (_nextAction.selector == NextActionSelector.FINALIZE) {
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
_finalize(_nextAction.data);
} else if (positionIsClosed == false && _isFundIdle()) {
flow = FLOW.COMPOUND;
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
-> _createIncreasePosition(beenLong, acceptablePrice, prices);
}
} else {
revert Error.InvalidCall();
}
}
​

Impact

  • Protocol state consistency is compromised

  • Multiple positions can exist simultaneously

  • Potential for unexpected behavior in position management

  • Risk of financial losses due to unintended position interactions

Tools Used

Manual Code Review

Recommendations

Implement proper cleanup in afterOrderCancellation() by adding:

function afterOrderCancellation(
bytes32 requestKey,
Order.OrderType orderType,
IGmxProxy.OrderResultData memory orderResultData
) external {
if (msg.sender != address(gmxProxy)) {
revert Error.InvalidCall();
}
_gmxLock = false;
​
if (orderResultData.isSettle) {
// Retry settle action.
nextAction.selector = NextActionSelector.SETTLE_ACTION;
} else if (orderType == Order.OrderType.MarketSwap) {
// If GMX swap fails, retry in the next action.
nextAction.selector = NextActionSelector.SWAP_ACTION;
// abi.encode(swapAmount, swapDirection): if swap direction is true, swap collateralToken to indexToken
nextAction.data = abi.encode(swapProgressData.remaining, swapProgressData.isCollateralToIndex);
} else {
if (flow == FLOW.DEPOSIT) {
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
} else if (flow == FLOW.WITHDRAW) {
nextAction.selector = NextActionSelector.WITHDRAW_ACTION;
} else {
// If signal change fails, the offchain script starts again from the current status.
delete flowData;
delete flow;
+ delete nextAction;
}
}
emit GmxPositionCallbackCalled(requestKey, false);
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

invalid_afterOrderCancellation_do_not_reset_nextAction

Normal behavior, the keeper will retry to increase the position. That’s why there is this condition in `_createIncreasePosition`: ``` if (flow == FLOW.DEPOSIT) { amountIn = depositInfo[counter].amount; flowData = vaultReader.getPositionSizeInTokens(curPositionKey); } else { amountIn = collateralToken.balanceOf(address(this)); } ```

Support

FAQs

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

Give us feedback!