OmnichainNonFungibleTokenDO Manual Review Findings

ONF-01M: Incorrect Encoding of Callback Payload

Type	Severity	Location
Logical Fault		OmnichainNonFungibleTokenDO.sol:L301

Description:

The IOmnichainNonFungibleTokenOperations::omnichainIncreaseBalanceCallback function selector as an operation will result in the OmnichainNonFungibleTokenDO::_omnichainCallback function being invoked which will attempt to decode a bool from the opData.

As the data supplied is empty rather than true, the decoding operation will fail causing a cross-chain LayerZero transaction that cannot be executed to manifest.

Impact:

Repeated omni-chain transfer attempts that were executed before the transaction was successfully processed will result in hanged cross-chain callback transactions.

Example:

Copy
298function _omnichainIncreaseBalance(DataPoint dp, bytes32 rid, OmnichainAddress from, OmnichainAddress to, uint256 tokenId) internal returns (bytes memory) {
299    if (_successfullOmnichainRequests[rid]) {
300        // Return data for callback call without any actions
301        return abi.encode(dp, IOmnichainNonFungibleTokenOperations.omnichainIncreaseBalanceCallback.selector, "");
302    }
303    _successfullOmnichainRequests[rid] = true;
304
305    // Now we can actually increase balance
306    (, address toAddress) = OmnichainAddresses.decode(to);
307    bytes32 diidTarget = _diid(dp, toAddress);
308    DiidData storage diiddTarget = _diidData(dp, diidTarget);
309
310    return _updateOmnichainBalancesOnTransfer(dp, from, toAddress, tokenId, diidTarget, diiddTarget);
311}

Recommendation:

We advise the code to properly encode the true status code so as to ensure that the callback is properly executed and acts as a "no-op" in case an omni-chain transfer was retried multiple times before it was successfully executed.

Alleviation (c6b23c23d8bcd8cce85049ad959cbd711a37126b):

The overall logic around omni-chain request statuses was updated to incorporate a CallResult value and the callback payload in both referenced statements will ensure that a bool value is encoded rather than an empty one, addressing this exhibit in full.

ONF-02M: Improper Refund Address

Type	Severity	Location
Logical Fault		OmnichainNonFungibleTokenDO.sol:L375

Description:

The referenced OmnichainProxy function invocation will assume that the caller themselves owns the same address on the target chain which is incorrect as many L2s employ address masking, especially in the context of smart contract refund recipients (i.e. multi-signature wallets).

Impact:

A cross-chain message's surplus or failure may result in unspent native funds being erroneously sent to an incorrect / inaccessible refund recipient, resulting in minor fund loss.

Example:

Copy
359function _retryIncreaseOmnichainBalance(bytes32 rid) internal nonReentrant {
360    PendingOmnichainTransfer memory pot = _pendingOmnichainTransfers[rid];
361    (uint32 toChainId, ) = OmnichainAddresses.decode(pot.to);
362
363    bytes memory data = abi.encode(rid, pot.from, pot.to, pot.tokenId);
364
365    bytes4 operation = IOmnichainNonFungibleTokenOperations.omnichainIncreaseBalance.selector;
366
367    bytes32 retryRid = _proxy.queryDataObjectWrite{value: msg.value}(
368        toChainId,
369        address(this), // Address of DO on target chain is same on all chains
370        pot.dp,
371        operation,
372        data,
373        OMNICHAIN_INCREASE_BALANCE_GAS_LIMIT,
374        OMNICHAIN_INCREASE_BALANCE_CALLBACK_GAS_LIMIT,
375        payable(_msgSender())
376    );
377    emit OmnichainRetryIncompleteOmnichainIncreaseBalanceSent(rid, retryRid);
378}

Recommendation:

We advise the system to permit a different refund address to be specified, ensuring that message refund operations are executed correctly.

Alleviation (c6b23c23d8bcd8cce85049ad959cbd711a37126b):

The refund address was updated to be the result of an input argument, alleviating this exhibit.

ONF-03M: Unbounded Callback Gas Cost

Type	Severity	Location
Logical Fault		OmnichainNonFungibleTokenDO.sol:L411-L416

Description:

The OmnichainNonFungibleTokenDO::_updateOmnichainBalancesOnTransfer function is meant to attempt EIP-721 style callbacks on the target recipient and issue a refund on the origin chain if the callbacks fail.

The current mechanism will invoke the EIP-721 callback of the recipient with an unbounded gas cost permitting the top-level call to ultimately run out of gas before a refund is signaled on the origin chain, effectively causing the funds to be lost permanently.

Impact:

A malicious or benign EIP-721 callback implementer that runs out of gas (i.e. unbounded loop) may cause an omni-chain transaction to be permanently stuck.

Example:

Copy
395function _updateOmnichainBalancesOnTransfer(
396    DataPoint dp,
397    OmnichainAddress from,
398    address to,
399    uint256 tokenId,
400    bytes32 diidTarget,
401    DiidData storage diiddTarget
402) private returns (bytes memory) {
403    _increaseBalance(diiddTarget, dp, OTHER_CHAIN_FROM_DIID, 1);
404
405    DpData storage dpd = _dpData(dp);
406    dpd.owners[tokenId] = to;
407
408    // Notify handlers
409    // We are making external call within try/catch, it will revert if any of the handlers fail, but we will catch and handle it
410    bool success;
411    try this.callIncreaseBalanceHandlersAndRevertOnFail(dp, from, to, tokenId) {
412        success = true;
413    } catch {
414        // Here we catch all errors, not only IncreaseBalanceCallbackHandlerFailed, because we need to send something back to original chain in any case
415        success = false;
416    }
417
418    if (!success) {
419        // Revert increase balance
420        _decreaseBalance(diiddTarget, dp, diidTarget, 1, tokenId);
421
422        dpd.owners[tokenId] = address(0);
423    }
424    return abi.encode(dp, IOmnichainNonFungibleTokenOperations.omnichainIncreaseBalanceCallback.selector, abi.encode(success));
425}

Recommendation:

We advise the system to accommodate for such a case by subtracting the gas necessary to signal a refund from the gas allocation of the OmnichainNonFungibleTokenDO::callIncreaseBalanceHandlersAndRevertOnFail function call.

Alleviation (c6b23c23d8bcd8cce85049ad959cbd711a37126b):

An explicit gas limit has been configured for the OmnichainNonFungibleTokenDO::callIncreaseBalanceHandlersAndRevertOnFail call performed, alleviating this exhibit.