VaultUtils Manual Review Findings

VUS-01M: Inexistent Emission of Event

Type	Severity	Location
Standard Conformity		VaultUtils.sol:L432-L434, L475-L477, L490-L492, L531-L533

Description:

The referenced statements normally emit a VaultPositionSettled or VaultPendingPositionSettled event, however, when they are executed pre-emptively they do not.

Impact:

As this finding pertains an off-chain impact, its severity is unquantifiable.

Example:

Copy
489if (index == length) {
490    _removeExpiryFromArray(_expiry, false, true);
491    isPositionAtExternalExpirySettled[_expiry] = true;
492    return true;
493}
494
495if (_endLength > length) {
496    _endLength = length;
497}
498
499while(index < _endLength) {
500    IVaultUtils.OptionToken memory token = optionTokensAtExternalExpiry[_expiry][index];
501
502    address quotetoken = _getQuoteTokenForSettlement(token.optionTokenId);
503    address[] memory path = new address[](1);
504    path[0] = quotetoken;
505
506    uint16 underlyingAssetIndex = Utils.getUnderlyingAssetIndexByOptionTokenId(token.optionTokenId);
507
508    bool shouldContinue = true;
509
510    try IVault(vault).settleVaultPosition(
511        path,
512        underlyingAssetIndex,
513        token.optionTokenId
514    ) returns (bool isSettled) {
515        if (isSettled) IVault(vault).directPoolDeposit(quotetoken);
516    } catch {
517        shouldContinue = false;
518    }
519
520    if (shouldContinue) {
521        delete optionTokensAtExternalExpiry[_expiry][index];
522        index++;
523    } else {
524        break;
525    }
526}
527
528optionTokensAtExternalExpiryStart[_expiry] = index;
529
530if (index == length) {
531    _removeExpiryFromArray(_expiry, false, true);
532    isPositionAtExternalExpirySettled[_expiry] = true;
533    emit VaultPendingPositionSettled(address(vault), _expiry);
534}

Recommendation:

We advise the event to be properly introduced, ensuring that the event emittances are correct.

Alleviation (b02fae335f62cc1f5f4236fb4d982ad16a32bd26):

A proper event emission has been introduced in the two out of the four referenced storage mutations that were previously not accompanied by one, addressing this exhibit in full.

VUS-02M: Improper Handling of Vault Settlements

Type	Severity	Location
Logical Fault		VaultUtils.sol:L457, L515

Description:

The referenced statements will perform a direct pool deposit if the positions they attempted to settle were settled, however, they will proceed execution even if they were not settled.

Impact:

The exhibit has been set as informational given that the Vault::settleVaultPosition function will never yield false, causing this vulnerability to not manifest.

Example:

Copy
452try IVault(vault).settleVaultPosition(
453    path,
454    underlyingAssetIndex,
455    token.optionTokenId
456) returns (bool isSettled) {
457    if (isSettled) IVault(vault).directPoolDeposit(quotetoken);
458} catch Error(string memory reason) {
459    shouldContinue = false;
460
461    emit CatchError(reason);
462}

Recommendation:

We advise the code to ensure that the position was settled either by mandating that isSettled is true, or assigning shouldContinue to false if isSettled is false.

Alleviation (b02fae335f62cc1f5f4236fb4d982ad16a32bd26):

The code was instead updated to yield the value that the settlement occurred for, and to only perform a deposit if the value is non-zero.

As such, we consider this exhibit properly alleviated.

VUS-03M: Unscalable Array Based Mechanism

Type	Severity	Location
Logical Fault		VaultUtils.sol:L548-L556, L601-L607

Description:

The referenced functions which are invoked as part of sensitive flows (i.e. VaultUtils::settleSelfOriginatedPositions, VaultUtils::settleExternalOriginatedPositions) will iterate a theoretically unbounded array.

Impact:

If sufficient expiries are introduced to the options system of the Moby team without being processed, all aforementioned functions will become permanently inoperable as the block gas limit would be achieved when trying to invoke them.

Example:

Copy
594function _removeExpiryFromArray(uint256 _expiry, bool _isSelfOriginated, bool _isForSettlement) internal {
595    uint256[] storage targetArray = _isSelfOriginated
596        ? (!_isForSettlement ? selfOriginatedExpiries : selfExpiriesReadyForSettlement)
597        : (!_isForSettlement ? externalOriginatedExpiries : externalExpiriesReadyForSettlement);
598
599    uint256 arrlength = targetArray.length;
600
601    for (uint256 i = 0; i < arrlength; i++) {
602        if (targetArray[i] == _expiry) {
603            targetArray[i] = targetArray[arrlength - 1];
604            targetArray.pop();
605            break;
606        }
607    }
608}

Recommendation:

We advise this approach to be revised as the functions may become inoperable if the array becomes sufficiently large due to block gas limits.

The VaultUtils::_prepareExpiriesForSettlement function could accept a number of expiries it should process, while the VaultUtils::_removeExpiryFromArray function could accept an index to remove as it is always known or can be known when it is invoked.

Alleviation (b02fae335f):

The expiry removal mechanism was updated per our recommendation, and as an alleviation to the preparation loop a VaultUtils::migrateExpiryToSettle function was introduced permitting expiries to be migrated one-by-one.

A new vulnerability has been introduced as a result of the atomic function whereby the _expiry being migrated is not validated as being less than the currentTime permitting any expiry to be migrated for settlement.

Alleviation (a8720219a6):

The VaultUtils::migrateExpiryToSettle function has been removed from the codebase to eliminate the newly introduced vulnerability, however, a fix for the VaultUtils::_prepareExpiriesForSettlement has not been introduced as advised rendering this exhibit partially addressed.

Alleviation (a95db4124c):

The code of the VaultUtils::prepareExpiresToSettle function was updated to accept a start and end index, properly permitting the caller to define the precise subset of expiries that should be processed and thus alleviating this exhibit in full.