`encodeToeComposeMsg` Encoding is incorrect and messages won't work

[c4-bot-5]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/tapiocaOmnichainEngine/TapiocaOmnichainEngineCodec.sol#L85-L92 https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/tapiocaOmnichainEngine/TapiocaOmnichainEngineCodec.sol#L96-L109

Vulnerability details

The function encodeToeComposeMsg is written as follows

https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/tapiocaOmnichainEngine/TapiocaOmnichainEngineCodec.sol#L85-L92

    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);
    }

Decoding is written as follows:

https://github.com/Tapioca-DAO/tapioca-periph/blob/2ddbcb1cde03b548e13421b2dba66435d2ac8eb5/contracts/tapiocaOmnichainEngine/TapiocaOmnichainEngineCodec.sol#L96-L109

     *
     *           *    TapToken message packet   *
     * ------------------------------------------------------------- *
     * Name          | type      | start | end                       *
     * ------------------------------------------------------------- *
     * msgType       | uint16    | 0     | 2                         *
     * ------------------------------------------------------------- *
     * msgLength     | uint16    | 2     | 4                         *
     * ------------------------------------------------------------- *
     * msgIndex      | uint16    | 4     | 6                         *
     * ------------------------------------------------------------- *
     * tapComposeMsg | bytes     | 6     | msglength + 6             *
     * ------------------------------------------------------------- *
     *

Which means that the encoding is being done incorrectly

_msgType should be the first parameter

We can verify this (this is how I found it) via fuzz testing a random inputs and verifying that the decoded data matches

POC

The POC uses a custom foundry repo, I simply copied the files and fixed imports

The library TapiocaOmnichainEngineCodec is changed to always use internal functions

The source code is here: https://gist.github.com/GalloDaSballo/418d7c394de8bba4961332ce305d52aa

And the full repo is available here (invite only): https://github.com/GalloDaSballo/omnichain-lib-fuzz

The test is as follows:

        function encodeToeComposeMsg(bytes memory _msg, uint16 _msgType, uint16 _msgIndex, bytes memory _tapComposedMsg)
        public
    {
        bytes memory encoded =
            TapiocaOmnichainEngineCodec.encodeToeComposeMsg(_msg, _msgType, _msgIndex, _tapComposedMsg);
        (uint16 msgType_, uint16 msgLength_, uint16 msgIndex_, bytes memory tapComposeMsg_, bytes memory nextMsg_) =
            TapiocaOmnichainEngineCodec.decodeToeComposeMsg(encoded);

        /// @audit Seeing only one of these logs means the test failed
        t(_msgType == msgType_, "_msgType");
        t(BytesLib.equal(_msg, tapComposeMsg_), "_tapComposedMsg");
        t(_msgIndex == msgIndex_, "_msgIndex");

        t(false, "false"); /// @audit Seeing this log means the test passed
    }

Which simply encodes random values, and then decodes them, to verify that the decoding function returns the original values

Due to the incorrect order demonstrated above, the test will fail

Medusa logs will look like these:

⇾ [FAILED] Assertion Test: CryticTester.encodeToeComposeMsg(bytes,uint16,uint16,bytes)
Test for method "CryticTester.encodeToeComposeMsg(bytes,uint16,uint16,bytes)" resulted in an assertion failure after the following call sequence:
[Call Sequence]
1) CryticTester.encodeToeComposeMsg(419af62f033b453826c90feb7888fa282b118a8a067b126e723cf17589dd6aedeb57871aa349273cbdc038fcc9776b2f2cf54d6dff8f3d317e93053341cd8aade0ca21d6, 2, 12911, 4164) (block=2, time=2, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
[Execution Trace]
 => [call] CryticTester.encodeToeComposeMsg(419af62f033b453826c90feb7888fa282b118a8a067b126e723cf17589dd6aedeb57871aa349273cbdc038fcc9776b2f2cf54d6dff8f3d317e93053341cd8aade0ca21d6, 2, 12911, 4164) (addr=0xA647ff3c36cFab592509E13860ab8c4F28781a66, value=0, sender=0x0000000000000000000000000000000000010000)
         => [event] Log("_msgType")
         => [panic: assertion failed]

Indicating that the _msgType is being decoded incorrectly

This will make it so that messages built with encodeToeComposeMsg will always fail at decoding as they will result in gibberish bytes being decoded

Mitigation

Change the encoding function to:

    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);
    }

Please note that I am unsure if msg and _tapComposedMsg are supposed to be in this or the reversed order, as the documentation is ambigous!

And consider adding extensive fuzz and invariant testing for these libraries as they may have more edge cases that were missed

Assessed type

en/de-code

[c4-sponsor]

cryptotechmaker (sponsor) confirmed

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

0xRektora (sponsor) disputed

[0xRektora]

I agree that the naming should be different and documentation more explicit. Used in a sandboxed environment, you'd instantly know that there's a problem. However the encoding/decoding aren't done straightforward. Using a fuzzer might trigger a false positive. I'm not sure how this was setup but there should be a condition for the parameter passing. The _tapComposedMsg in encodeToeComposeMsg should always be empty if _msgIndex is 0, this is because it's the first message. If we take this into account, decodeToeComposeMsg will receive the right data structure, and index > 0 will not break too because we'd be passing a slice of the data to the next decodeToeComposeMsg call

nextMsg_ = BytesLib.slice(_msg, tapComposeOffset_, _msg.length - (tapComposeOffset_));

Which always make _tapComposedMsg of encodeToeComposeMsg empty, and match the decodeToeComposeMsg structure.

encodeToeComposeMsg passes previous messages to concatenate every compose message. Better wording could've been used between function/variable names.

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid