Omniscia Moby Audit
PositionManager Manual Review Findings
PositionManager Manual Review Findings
PMR-01M: Incorrect Dry-Run of Ensuing Request (Double Processing of Position)
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | PositionManager.sol:L483, L502, L503, L505, L519, L525, L531, L593, L609, L624, L625, L629, L631, L638 |
Description:
The PositionManager queue is meant to adhere to a strict set of rules as defined in the PositionManager::executePositions function. Specifically, the queue should process as long as requests can be executed and if one failure is achieved, the queue should consume all failures beyond that and then halt.
The problem with the current approach is that the system will attempt to check if requests after the first failed one also fail by supplying an _isPreviousCancelled flag that is meant to "dry-run" the executions to check if they would succeed.
The system will perform multiple state mutations, asset transfers, and generally execute the position fully in each of the PositionManager::executeOpenPosition and PositionManager::executeClosePosition functions without actually marking the request as processed or yielding the execution fee to the caller.
Impact:
The system's queue will improperly execute the last position that should not be executed when processing failed requests.
This will result in the position being re-executed the next time the queue is invoked, thereby causing fund loss for the custodial PositionManager.
Example:
593function executeClosePosition(uint256 _requestIndex, bytes32 _key, address payable _executionFeeReceiver, bool _isPreviousCancelled) public nonReentrant returns (bool isExecuted, bool isFlagRaised) {594 require(msg.sender == address(this), "PositionManager: Invalid sender");595
596 ClosePositionRequest storage request = closePositionRequests[_key];597 if (request.status == RequestStatus.Executed || request.status == RequestStatus.Cancelled) { return (true, true); }598
599 uint40 processBlockTime = uint40(block.timestamp);600 require(request.blockTime + maxTimeDelay >= processBlockTime, "PositionManager: Request expired");601
602 uint40 expiry = Utils.getExpiryByOptionTokenId(request.optionTokenId);603 require (expiry > processBlockTime, "PositionManager: Option expired");604
605 (, address targetVault) = IController(controller).getVaultIndexAndAddressByTimeGap(processBlockTime, request.expiry);606 607 address optionsToken = IOptionsMarket(optionsMarket).getOptionsTokenByIndex(request.underlyingAssetIndex);608 609 IERC1155Base(optionsToken).safeTransferFrom(address(this), targetVault, request.optionTokenId, request.size, "");610
611 (uint256 amountOut, uint256 executionPrice) = IController(controller).pluginClosePosition(612 request.account,613 _requestIndex,614 request.optionTokenId,615 request.size,616 request.path[0],617 request.minAmountOut,618 address(this),619 targetVault620 );621 622 if (amountOut > 0) {623 if (request.path.length > 1) {624 IERC20(request.path[0]).safeTransfer(targetVault, amountOut);625 amountOut = _swap(targetVault, request.path, request.minOutWhenSwap, address(this));626 }627
628 if (request.withdrawETH) {629 _transferOutETHWithGasLimitFallbackToWeth(amountOut, payable(request.account));630 } else {631 IERC20(request.path[request.path.length - 1]).safeTransfer(request.account, amountOut);632 }633 }634
635 // @dev636 // If code processed to this point, it means that the request will be executed successfully.637 // But, if the previous request was cancelled, the current request should not be executed.638 if (_isPreviousCancelled) { return (false, false); }639 640 _transferOutETHWithGasLimitFallbackToWeth(executionFee, _executionFeeReceiver);641 642 request.status = RequestStatus.Executed;643 request.amountOut = amountOut;644 request.executionPrice = executionPrice;645 request.processBlockTime = processBlockTime;646
647 emit ExecuteClosePosition(648 request.account,649 request.underlyingAssetIndex,650 request.expiry,651 request.optionTokenId,652 request.size,653 request.path,654 amountOut,655 executionPrice,656 processBlockTime - request.blockTime657 );658
659 return (true, false);660}Recommendation:
We advise the system to instead force a revert if the _isPreviousCancelled flag is true, reverting any state changes performed by either PositionManager::executeOpenPosition and PositionManager::executeClosePosition.
The revert can yield a custom payload that is then decoded in the try-catch clause of the PositionManager::executePositions function, ensuring that the code stops processing the queue if the position would execute properly but had a previous position cancelled.
Alleviation (b02fae335f62cc1f5f4236fb4d982ad16a32bd26):
The dry-running capabilities of the PositionManager::executePositions function have been removed, instead opting to process all types of requests sequentially.
As the original double execution of a request is no longer applicable, we consider this exhibit fully alleviated.