Invalid messages can be submitted and processed due to a lack of validation bug.

[hats-bug-reporter]

Github username: @Lightoasis
Twitter username: --
Submission hash (on-chain): 0x4db207640153cc1503b71d4356ffd08ae1d098f8338119b046bc93b854bcb89e
Severity: low

Description:
Description
This bug is caused by a lack of validation in function submitMessage to verify that the message.length of the submitted message is not 0 before accepting the message's submission and processing it. This allows empty and invalid messages to be submitted and processed as valid messages,

    function submitMessage(
        bytes32 destinationIdentifier,
        bytes calldata destinationAddress,
        bytes calldata message,
        IncentiveDescription calldata incentive
    ) checkBytes65Address(destinationAddress) external payable returns(uint256 gasRefund, bytes32 messageIdentifier) {
        if (incentive.refundGasTo == address(0)) revert RefundGasToIsZero();
        // Check that the application has set a destination implementation
        bytes memory destinationImplementation = implementationAddress[msg.sender][destinationIdentifier];
        // Check that the length is not 0.
        if (destinationImplementation.length == 0) revert NoImplementationAddressSet();
}

Attachments

1. Proof of Concept (PoC) File

Add this test to roundtrips.t.sol and run forge test.

  function test_escrow_wormhole_message(bytes calldata message) public {
    vm.assume(message.length == 0);

    IncentiveDescription storage incentive = _INCENTIVE;

    vm.recordLogs();
    (uint256 gasRefund, bytes32 messageIdentifier) = escrow.submitMessage{value: _getTotalIncentive(_INCENTIVE)}(
        _DESTINATION_IDENTIFIER,
        convertEVMTo65(address(99)),
        message,
        incentive
    );
    Vm.Log[] memory entries = vm.getRecordedLogs();

    (uint64 sequence, uint32 nonce, bytes memory payload, uint8 consistencyLevel) = abi.decode(
      entries[1].data,
      (uint64, uint32, bytes, uint8)
    );

    bytes memory validVM = makeValidVM(payload, uint16(uint256(_DESTINATION_IDENTIFIER)), bytes32(uint256(uint160(address(escrow)))));

    vm.recordLogs();
    escrow.processPacket(hex"", validVM, bytes32(uint256(0xdead)));
    entries = vm.getRecordedLogs();

    (sequence, nonce, payload, consistencyLevel) = abi.decode(
      entries[1].data,
      (uint64, uint32, bytes, uint8)
    );

    validVM = makeValidVM(payload, uint16(uint256(_DESTINATION_IDENTIFIER)), bytes32(uint256(uint160(address(escrow)))));

    escrow.processPacket(hex"", validVM, bytes32(uint256(0xdead)));

  }

Runnable file attached below.

Fix
Verify that the message.length != 0 before accepting the message's submission and proccessing it.

Files:

• roundtrip.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmbgjX7YJBsf2ou45gj3FGwhw1Tip7z8UDYRWAyyqjohj8)

[reednaa]

Sorry, I don't understand the issue.

Why should a message length of 0 not be allow? What if you just wanted to invoke a function on another contract? (or collect a piece of information of another chain to be sent back on ack?).

[0xisaacc]

This will prevent spam messages in general.

Also, if you wanted to invoke a function on another contract or collect a piece of information, the calldata of the transaction should be used.

[reednaa]

How does it prevent spam messages and who classifies which messages are spam?

[0xisaacc]

I believe that the messages submitted should actually contain value. If you don't feel the same way, that's fine.