Omniscia Moby Audit

PositionManager Code Style Findings

PositionManager Code Style Findings

PMR-01C: Improper Operation Location

Description:

The BasePositionManager::_swap function will invoke the Vault::swap function which in turn expects funds to have already been transmitted to the vault.

This is presently performed manually before invoking the BasePositionManager::_swap implementation which is ill-advised.

Example:

contracts/PositionManager.sol
758IERC20(_path[0]).safeTransfer(sourceVault, amountOut);
759amountOut = _swap(sourceVault, _path, _minOutWhenSwap, address(this));

Recommendation:

We advise the transfer operation to be relocated in the BasePositionManager::_swap function itself, ensuring consistency across the codebase.

Alleviation (a8720219a6a97e10b8d9c6a70c6345747f0fdcb3):

The transfer has been relocated to the upstream BasePositionManager dependency as advised, addressing this exhibit.

PMR-02C: Ineffectual Usage of Safe Arithmetics

Description:

The linked mathematical operation is guaranteed to be performed safely by surrounding conditionals evaluated in either require checks or if-else constructs.

Example:

contracts/PositionManager.sol
288require(msg.value >= executionFee, "PositionManager: Invalid execution fee");
289require(_path.length == 1 || _path.length == 2, "PositionManager: Invalid path length");
290require(_path[0] == weth, "PositionManager: Invalid path");
291

292_transferInETH(); // convert ETH to WETH
293uint256 amountIn = msg.value - executionFee; // get payment token after deducting executionFee

Recommendation:

Given that safe arithmetics are toggled on by default in pragma versions of 0.8.X, we advise the linked statement to be wrapped in an unchecked code block thereby optimizing its execution cost.

Alleviation (a8720219a6a97e10b8d9c6a70c6345747f0fdcb3):

An unchecked code block has been introduced to the referenced arithmetic operation, optimizing it as advised.

PMR-03C: Redundant Code Repetition

Description:

The referenced code block can be optimized by utilizing function pointers, greatly optimizing the legibility of the code.

Example:

contracts/PositionManager.sol
421if (isOpenPosition) {
422    try this.executeOpenPosition(index, key, _executionFeeReceiver, isPreviousCancelled) returns (bool _wasExecuted, bool _wasFlagRaised) {
423        if (!_wasExecuted) { 
424            shouldContinue = false;
425            shouldIncreaseIndex = false;
426        }
427

428        if (_wasExecuted && _wasFlagRaised) {
429            shouldContinue = false;
430            shouldIncreaseIndex = true;
431        }
432    } catch Error(string memory executeErrorMessage) {
433        
434        emit ExecuteErrorMessage(executeErrorMessage);
435

436        try this.cancelOpenPosition(key, _executionFeeReceiver) returns (bool _wasCancelled) {
437            if (_wasCancelled) {
438                isPreviousCancelled = true;
439            }
440        } catch Error(string memory cancelErrorMessage){
441            isPreviousCancelled = true;
442            emit CancelErrorMessage(cancelErrorMessage);
443        }
444    }
445} else {
446    try this.executeClosePosition(index, key, _executionFeeReceiver, isPreviousCancelled) returns (bool _wasExecuted, bool _wasFlagRaised) {
447        if (!_wasExecuted) {
448            shouldContinue = false;
449            shouldIncreaseIndex = false;
450        }
451

452        if (_wasExecuted && _wasFlagRaised) {
453            shouldContinue = false;
454            shouldIncreaseIndex = true;
455        }
456    } catch Error(string memory executeErrorMessage) {
457

458        emit ExecuteErrorMessage(executeErrorMessage);
459

460        try this.cancelClosePosition(key, _executionFeeReceiver) returns (bool _wasCancelled) {
461            if (_wasCancelled) {
462                isPreviousCancelled = true;
463            }
464        } catch Error(string memory cancelErrorMessage) {
465            isPreviousCancelled = true;
466            emit CancelErrorMessage(cancelErrorMessage);
467        }
468    }
469}

Recommendation:

We advise local function declarations to be introduced that represent executions and cancellations, each pointing to the correct implementation based on the isOpenPosition flag.

Alleviation (a8720219a6a97e10b8d9c6a70c6345747f0fdcb3):

The Moby team evaluated this optimization and opted to not apply it as it would be too complex to incorporate.

PMR-04C: Suboptimal Struct Declaration Styles

Description:

The linked declaration styles of the referenced structs are using index-based argument initialization.

Example:

contracts/PositionManager.sol
844OpenPositionRequest memory request = OpenPositionRequest(
845    _account,
846    _underlyingAssetIndex,
847    _optionIds,
848    _expiry,
849    _optionTokenId,
850    _minSize,
851    _path,
852    _amountIn,
853    _minOutWhenSwap,
854    _isDepositedInETH,
855    blockTime,
856    RequestStatus.Pending,
857    0,
858    0,
859    0
860);

Recommendation:

We advise the key-value declaration format to be utilized instead in each instance, greatly increasing the legibility of the codebase.

Alleviation (a8720219a6a97e10b8d9c6a70c6345747f0fdcb3):

The Moby team opted to instead employ a key-assignment declaration style which we consider optimal and to achieve a similar purpose to the key-value declaration style, rendering this exhibit addressed.

PMR-05C: Test Code

Description:

The referenced event represents a test event.

Example:

contracts/PositionManager.sol
163event Debug(uint256, uint256, uint256);

Recommendation:

We advise it to be omitted, optimizing the code's deployment cost.

Alleviation (a8720219a6a97e10b8d9c6a70c6345747f0fdcb3):

The test Debug event has been removed from the codebase as advised.

Prev

OptionsToken.sol (OTN-C)

Next

PositionValueFeed.sol (PVF-C)

On this page