c4-bot-9 commented 4 months ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarAssetXChainModule.sol#L90-L97 https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/tapiocaOmnichainEngine/TapiocaOmnichainEngineCodec.sol#L91

Vulnerability details

Description

In the depositYBLendSGLLockXchainTOLP() function, after lending to Singularity, it attempts to send SGL tokens cross-chain and execute the lock action upon receiving by using a composed message. Therefore, this function decodes the passed compose message to change the lockData.fraction to the received token amount, and then encodes that message again afterward

(uint16 msgType_,, uint16 msgIndex_, bytes memory tapComposeMsg_, bytes memory nextMsg_) =
        TapiocaOmnichainEngineCodec.decodeToeComposeMsg(data.lockAndParticipateSendParams.lzParams.sendParam.composeMsg);

        LockAndParticipateData memory lockData = abi.decode(tapComposeMsg_, (LockAndParticipateData));
        lockData.fraction = toftAmount;

        data.lockAndParticipateSendParams.lzParams.sendParam.composeMsg =
            TapiocaOmnichainEngineCodec.encodeToeComposeMsg(abi.encode(lockData), msgType_, msgIndex_, nextMsg_);

The decodeToeComposeMsg() function decodes with the order from left to right, which means tapComposeMsg_ precedes nextMsg_ in the whole encoded messages.

function decodeToeComposeMsg(bytes memory _msg)
    internal
    pure
    returns (
        uint16 msgType_,
        uint16 msgLength_,
        uint16 msgIndex_,
        bytes memory tapComposeMsg_,
        bytes memory nextMsg_
    )
{
    // TODO use bitwise operators?
    msgType_ = BytesLib.toUint16(BytesLib.slice(_msg, 0, 2), 0);
    msgLength_ = BytesLib.toUint16(BytesLib.slice(_msg, MSG_TYPE_OFFSET, 2), 0);

    msgIndex_ = BytesLib.toUint16(BytesLib.slice(_msg, MSG_LENGTH_OFFSET, 2), 0);
    tapComposeMsg_ = BytesLib.slice(_msg, MSG_INDEX_OFFSET, msgLength_);

    uint256 tapComposeOffset_ = MSG_INDEX_OFFSET + msgLength_;
    nextMsg_ = BytesLib.slice(_msg, tapComposeOffset_, _msg.length - (tapComposeOffset_));
}

However, encodeToeComposeMsg() function will move the next message to the beginning of the total composed message.

 function encodeToeComposeMsg(bytes memory _msg, uint16 _msgType, uint16 _msgIndex, bytes memory _tapComposedMsg)
    internal
    pure
    returns (bytes memory)
{
    return abi.encodePacked(_tapComposedMsg, _msgType, uint16(_msg.length), _msgIndex, _msg);
}

Therefore, the order of child messages will be changed in depositYBLendSGLLockXchainTOLP function.

Impact

Cross-chain execution of Layerzero v2 will compose with the wrong order of actions, leading to unexpected results of DOS.

Tools Used

Manual review

Recommended Mitigation Steps

Move _tapComposedMsg to the end of the encoded message in encodeToeComposeMsg as following:

 function encodeToeComposeMsg(bytes memory _msg, uint16 _msgType, uint16 _msgIndex, bytes memory _tapComposedMsg)
    internal
    pure
    returns (bytes memory)
{
    return abi.encodePacked(_msgType, uint16(_msg.length), _msgIndex, _msg, _tapComposedMsg);
}

Assessed type

en/de-code

c4-judge commented 3 months ago

dmvt marked the issue as primary issue

c4-sponsor commented 3 months ago

0xRektora (sponsor) disputed

0xRektora commented 3 months ago

The encodeToeComposeMsg will be used to encode a message from last to end, so indexes MSG_3(MSG_2(MSG_1)). This is why _tapComposedMsg is the first packed var.

Those 2 functions are a bit ambiguous, see https://github.com/code-423n4/2024-02-tapioca-findings/issues/74 for more details.