KiteWeb3 - Incorrect Data Length Validation in ```AvailBridge::sendMessage``` function

[sherlock-admin]

KiteWeb3

medium

Incorrect Data Length Validation in AvailBridge::sendMessage function

Summary

The AvailBridge::sendMessage function contains an if statement that checks whether the length of the data being sent is greater than or equal to the MAX_DATA_LENGTH constant. The current implementation uses a >= comparison, which is inconsistent with the project requirements that specify the data must not exceed 100KB (102,400 bytes) that is the Ethereum calldata limits (ref. https://avail-project.notion.site/The-Avail-Bridge-Public-Copy-a00c2aa4937d496ea346d02a6bb119ff - paragraph: Bridging extrinsics specification). This discrepancy can lead to unexpected reverts for messages that are exactly 100KB in size.

Vulnerability Detail

This mismatch leads to these vulnerabilities:

• Expectation Mismatch: Users may expect to be able to send messages of exactly 100KB, but the current condition will revert such transactions.
• Wasted Gas: Transactions that are reverted due to this condition result in unnecessary gas expenditure for the users.
• User Experience: The inconsistency between the contract's behaviour and the project requirements can lead to confusion and a negative user experience.

Impact

The impact of this issue is primarily on user experience and user loss of funds in reverted transactions. While it does not pose a direct security threat to the funds or the integrity of the smart contract, it can lead to frustration and decreased trust if users' legitimate transactions are unexpectedly reverted.

Code Snippet

Link to the code snippet: https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L300C5-L321C6

uint256 private constant MAX_DATA_LENGTH = 102_400;

function sendMessage(bytes32 recipient, bytes calldata data) external payable whenNotPaused {
        uint256 length = data.length;
@>        if (length >= MAX_DATA_LENGTH) {
             revert ExceedsMaxDataLength();
        }
        // ensure that fee is above minimum amount
        if (msg.value < getFee(length)) {
            revert FeeTooLow();
        }
        uint256 id;
        unchecked {
            id = messageId++;
        }
        fees += msg.value;
        Message memory message = Message(
            MESSAGE_TX_PREFIX, bytes32(bytes20(msg.sender)), recipient, ETH_DOMAIN, AVAIL_DOMAIN, data, uint64(id)
        );
        // store message hash to be retrieved later by our light client
        isSent[id] = keccak256(abi.encode(message));

        emit MessageSent(msg.sender, recipient, id);
    }

Tool used

Manual Review

Recommendation

The condition in the sendMessage function should be updated to reflect the intended behaviour as specified in the project requirements. The corrected condition should use a > comparison instead of >=:

function sendMessage(bytes32 recipient, bytes calldata data) external payable whenNotPaused {
        uint256 length = data.length;
-        if (length >= MAX_DATA_LENGTH) {
+.      if (length > MAX_DATA_LENGTH) {
            revert ExceedsMaxDataLength();
        }
        // ensure that fee is above minimum amount
        if (msg.value < getFee(length)) {
            revert FeeTooLow();
        }
        uint256 id;
        unchecked {
            id = messageId++;
        }
        fees += msg.value;
        Message memory message = Message(
            MESSAGE_TX_PREFIX, bytes32(bytes20(msg.sender)), recipient, ETH_DOMAIN, AVAIL_DOMAIN, data, uint64(id)
        );
        // store message hash to be retrieved later by our light client
        isSent[id] = keccak256(abi.encode(message));

        emit MessageSent(msg.sender, recipient, id);
    }

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low

takarez commented:

invalid because { duplicate of issue 074}