Decoding of user supplied calldata can fail unexpectedly before making the onCatalystCall call

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0xf589e38631045ac8160c778bc6fdbdd028b70693fca9cb3aa54490520b12b224
Severity: medium

Description:
Description
The CCI contract makes external call to a user supplied address during the cross chain swap. Before making the external onCatalystCall the CCI contract tried to decode the address and calldata for that call.

This is done in these functions:

• _handleReceiveAssetFallback
• _handleReceiveLiquidity
• underwrite

_handleReceiveAssetFallback

    function _handleReceiveAssetFallback(bytes32 sourceIdentifier, bytes calldata data) internal returns (bytes1 status) {
        bytes calldata fromVault = data[ FROM_VAULT_LENGTH_POS : FROM_VAULT_END ];
        address toVault = address(bytes20(data[ TO_VAULT_START_EVM : TO_VAULT_END ]));

        try ICatalystV1Vault(toVault).receiveAsset(
            ...
        ) returns(uint256 purchasedTokens) {
            uint16 dataLength = uint16(bytes2(data[CTX0_DATA_LENGTH_START:CTX0_DATA_LENGTH_END]));
            if (dataLength != 0) {
                address dataTarget = address(bytes20(data[ CTX0_DATA_START : CTX0_DATA_START+20 ]));
                bytes calldata dataArguments = data[ CTX0_DATA_START+20 : CTX0_DATA_START+dataLength ];
                
                ICatalystReceiver(dataTarget).onCatalystCall(purchasedTokens, dataArguments);
            }
            return 0x00;
        } catch (bytes memory err) {
            return _handleError(err);
        }
    }

https://github.com/catalystdao/catalyst/blob/main/evm/src/CatalystChainInterface.sol#L536-L537

It can be seen that this function tries to decode dataTarget address.

In case a user supplied invalid calldata (length < 20 bytes) then this decoding will fail abruptly, resulting in the CCI.receiveMessage call getting reverted.

Attachments

1. Proof of Concept (PoC) File

Test case was added to ExampleTest.t.sol

    function test_cross_chain_swap_() external {
        // We need to set address(CCI) as the allowed caller and address(GARP) as the destination.
        bytes memory approvedRemoteCaller = convertEVMTo65(address(CCI));
        bytes memory remoteGARPImplementation = abi.encode(address(GARP));
        // notice that remoteGARPImplementation needs to be encoded with how the AMB expectes it
        // and approvedRemoteCaller needs to be encoded with how GARP expects it.
        CCI.connectNewChain(
            DESTINATION_IDENTIFIER,
            approvedRemoteCaller,
            remoteGARPImplementation
        );

        ICatalystV1Vault(vault1).setConnection(
            DESTINATION_IDENTIFIER,
            convertEVMTo65(vault2),
            true
        );

        ICatalystV1Vault(vault2).setConnection(
            DESTINATION_IDENTIFIER,
            convertEVMTo65(vault1),
            true
        );

        // Get the token at index 0 from the vault
        address fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
        // Lets also get the to token while we are at it:
        address toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);

        // Make an account for testing
        address alice = makeAddr("Alice");
        uint256 swapAmount = 100 * 10 ** 18;

        payable(alice).transfer(_getTotalIncentive(_INCENTIVE));
        Token(fromToken).transfer(alice, swapAmount);
        vm.prank(alice);
        Token(fromToken).approve(vault1, swapAmount);

        // Define the route as a struct:
        ICatalystV1Structs.RouteDescription
            memory routeDescription = ICatalystV1Structs.RouteDescription({
                chainIdentifier: DESTINATION_IDENTIFIER,
                toVault: convertEVMTo65(vault2),
                toAccount: convertEVMTo65(alice),
                incentive: _INCENTIVE
            });

        // We need the log emitted by the mock Generalised Incentives implementation.
        vm.recordLogs();
        vm.prank(alice);
        ICatalystV1Vault(vault1).sendAsset{
            value: _getTotalIncentive(_INCENTIVE)
        }(
            routeDescription,
            fromToken,
            1,
            swapAmount,
            0,
            alice,
            0,
            hex"1234"
        );
        // Get logs.
        Vm.Log[] memory entries = vm.getRecordedLogs();
        // Decode log.
        (, , bytes memory messageWithContext) = abi.decode(
            entries[1].data,
            (bytes32, bytes, bytes)
        );
        // Get GARP message.
        (
            bytes memory _metadata,
            bytes memory toExecuteMessage
        ) = getVerifiedMessage(address(GARP), messageWithContext);
        // Process message / Execute the receiveAsset call. This delivers the assets to the user.
        vm.recordLogs();
        GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
        // We need to deliver the ack, so we need to relay another message back:
        entries = vm.getRecordedLogs();
        (, , messageWithContext) = abi.decode(
            entries[3].data,
            (bytes32, bytes, bytes)
        );

        console.logBytes(messageWithContext);

        (_metadata, toExecuteMessage) = getVerifiedMessage(
            address(GARP),
            messageWithContext
        );
        // Process ack
        vm.recordLogs();
        assertEq(Token(fromToken).balanceOf(alice), 0);
        GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);

        // Alice received back the tokens on source chain
        assertEq(Token(fromToken).balanceOf(alice), swapAmount);
    }

Output:

Logs:
  0x80000000000000000000000000000000000000000000000000000000001231238000000000000000000000000000000000000000000000000000000000123123011ffee9d0d3687d2081ed0c509d5324779884c29f97ffa7cbbb77854006735de41400000000000000000000000000000000000000000000000000000000000000000000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff211000000000000000000000000000000000000000000000000000000000000000000000002867f0000000000000001ff00140000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a1f3ea70ae1b30210c218bb5c15ea0f30b865095140000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a3e2ebec76faba3c0943e381a24c4b49d8374010140000000000000000000000000000000000000000000000000000000000000000000000000000000000000000bf0b5a4099f0bf6c8bc4252ebec548bae95602ea00000000000000000000000000000000000000000000000001529c1a82b723fc0100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000056bc75e2d63100000140000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a4ad4f68d0b91cfd19687c881e50f3a00242828c00000001000000021234

The ff in the log shows that the CCI.receiveMessage call failed silently.

2. Revised Code File (Optional)
   Consider validating that the calldata input if of sufficient lenght (atleast 20 bytes).

[PlamenTSV]

The calldata is user provided, so he only hurts himself.

[reednaa]

Adding calldata to the cross-chain swap is expected to be done by advanced users. As such, they should we aware of our encoding scheme for the calldata. When not needed, I prefer not having a check and in this case I am leaning not having the check.