Summary
When keeper changes position type, PerpetualVault.sol#afterOrderCancellation() function does not delete next action.
So keeper script can run next action. Then, new position of changed type is created even though previous position is not closed.
Vulnerability Details
We say that PerpetualVault.sol#run() function is called by keeper to change position type.
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) {
...
} else {
if (beenLong == isLong) {
revert Error.NoAction();
} else {
@> 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 {
...
}
}
As we can see above, next action is queued and then it creates order.
Here, we say that _isLongOneLeverage(beenLong) = false.
But order is canceled by some errors, then PerpetualVault.sol#afterOrderCancellation() function is called.
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) {
nextAction.selector = NextActionSelector.SETTLE_ACTION;
} else if (orderType == Order.OrderType.MarketSwap) {
nextAction.selector = NextActionSelector.SWAP_ACTION;
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 {
@>
@> delete flowData;
@> delete flow;
}
}
emit GmxPositionCallbackCalled(requestKey, false);
}
Here, it deletes flowData and flow because orderType = MarketDecrease and flow = SIGNAL_CHANGE.
But it does not delete nextAction.
The nextAction remaines to INCREASE_ACTION.
Then, keeper script calls runNextAction().
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 {
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);
}
} ...
}
So new position is created even though previous position is not closed.
Impact
The protocol is broken from new created position without closing previous position.
Tools Used
Manual Review
Recommendations
Modify PerpetualVault.sol#afterOrderCancellation() function as follows.
function afterOrderCancellation(
bytes32 requestKey,
Order.OrderType orderType,
IGmxProxy.OrderResultData memory orderResultData
) external {
...
if (orderResultData.isSettle) {
...
} else if (orderType == Order.OrderType.MarketSwap) {
...
} 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 {
delete flowData;
delete flow;
++ delete nextAction;
}
}
emit GmxPositionCallbackCalled(requestKey, false);
}
