Failed cross chain TapOFT messages aren't stored for retrying

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/tokens/BaseTapOFT.sol#L52-L65 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/tokens/BaseTapOFT.sol#L309-L322 https://github.com/Tapioca-DAO/tapioca-sdk-audit/blob/90d1e8a16ebe278e86720bc9b69596f74320e749/src/contracts/lzApp/NonblockingLzApp.sol#L23-L41

Vulnerability details

Impact

For almost all cross chain messages sent via LayerZero in the Tapioca ecosystem, if a call fails on the destination chain the message is saved for retrying again on the destination chain. This has 2 purposes:

1. By using a non-blocking channel, if a message fails it won't block any other messages
2. By saving a failed message on the destination chain it can be retried without having to pay the expensive cross chain LayerZero message fee again

However the TapOFT messages do not follow this pattern. If the respective message fails the call doesn't revert and the failed message isn't saved. The end result is that the user has paid the significant LayerZero fee for a message that has failed, and now has to pay the same same large fee to retry again rather than just being able to retry the failed message on the destination chain. This is a poor user experience and costs the end user more of the source chain gas token than it should.

Proof of Concept

In BaseTapOFT.sol any messages received from another chain via LayerZero are executed through nonBlockingLzReceive:

    function _nonblockingLzReceive(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) internal virtual override {
        uint256 packetType = _payload.toUint256(0);

        if (packetType == PT_LOCK_TWTAP) {
            _lockTwTapPosition(_srcChainId, _payload);
        } else if (packetType == PT_UNLOCK_TWTAP) {
            _unlockTwTapPosition(_srcChainId, _payload);
        } else if (packetType == PT_CLAIM_REWARDS) {
            _claimRewards(_srcChainId, _payload);

For the sake of argument, let's consider the PT_UNLOCK_TWTAP message that calls _unlockTwTapPosition:

        // Exit and receive tokens to this contract
        try twTap.exitPositionAndSendTap(tokenID) returns (uint256 _amount) {
            // Transfer them to the user
            this.sendFrom{value: address(this).balance}(
                address(this),
                _srcChainId,
                LzLib.addressToBytes32(to),
                _amount,
                twTapSendBackAdapterParams
            );
        } catch Error(string memory _reason) {
            emit CallFailedStr(_srcChainId, _payload, _reason);
        } catch (bytes memory _reason) {
            emit CallFailedBytes(_srcChainId, _payload, _reason);
        }

As you can see from this part of the method, if exiting fails the error is caught and an event is emitted rather than reverting the call. As a result the message isn't saved for retrying. This can be seen from the NonblockingLzApp.sol contract:

    // overriding the virtual function in LzReceiver
    function _blockingLzReceive(uint16 _srcChainId, bytes memory _srcAddress, uint64 _nonce, bytes memory _payload) internal virtual override {
        (bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft(), 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));
        // try-catch all errors/exceptions
        if (!success) {
            _storeFailedMessage(_srcChainId, _srcAddress, _nonce, _payload, reason);
        }
    }

    function _storeFailedMessage(uint16 _srcChainId, bytes memory _srcAddress, uint64 _nonce, bytes memory _payload, bytes memory _reason) internal virtual {
        failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
        emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
    }

    function nonblockingLzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64 _nonce, bytes calldata _payload) public virtual {
        // only internal transaction
        require(_msgSender() == address(this), "NonblockingLzApp: caller must be LzApp");
        _nonblockingLzReceive(_srcChainId, _srcAddress, _nonce, _payload);
    }

Here _storeFailedMessage is only called if nonblockingLzReceive reverts and therefore if _nonblockingLzReceive reverts.

Although PT_LOCK_TWTAP doesn't save the incoming message if the call fails, it does send the funds back to the user, so it isn't a complete waste. However I would suggest that it also reverts instead so that the message is saved and can be retried.

Tools Used

Manual review

Recommended Mitigation Steps

Rather than emitting events stating that the message received via LayerZero has failed, TapOFT should instead revert execution to ensure that the message is available on the destination chain to retry again. This saves the user from having to pay the expensive LayerZero fee again and conforms to the same approach used by the rest of the codebase.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Consider QA

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b

[djb15]

@dmvt As stated in the report, cross chain messages are expensive so there is a non-trivial monetary impact to users here. I believe this has sufficient impact to be a medium severity issue

[dmvt]

As stated in the report

No additional information provided. Ruling stands.