JP_Courses - `AvailBridge::sendMessage()` Sending messages of still VALID length `data.length == MAX_DATA_LENGTH` will trigger tx revert, effectively DoS-ing the passing of arbitrary data from Ethereum to Avail

[sherlock-admin]

JP_Courses

medium

AvailBridge::sendMessage() Sending messages of still VALID length data.length == MAX_DATA_LENGTH will trigger tx revert, effectively DoS-ing the passing of arbitrary data from Ethereum to Avail

Summary

https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L300

https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L302 AvailBridge::sendMessage() Sending messages of still VALID length data.length == MAX_DATA_LENGTH will trigger tx revert, effectively DoS-ing the passing of arbitrary data from Ethereum to Avail

The bug is related to the condition in the sendMessage() function, where the length of the input data is checked against MAX_DATA_LENGTH. The current condition checks if the length is greater than or equal to MAX_DATA_LENGTH, and my argument is that it should be strictly greater >, in line with the naming of the constant and the custom error message.

Vulnerability Detail

https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L38

It's clear from the existing protocol tests that they treat the value for constant MAX_DATA_LENGTH as an invalid length, which unfortunately is an incorrect assumption to make, unless the naming of MAX_DATA_LENGTH is changed to e.g. INVALID_DATA_LENGTH, which might be acceptable only IF 102,400 was an actual invalid length in terms of how the EVM operates, which is not the case. I assume that the protocol's business logic actually allows for max valid length of 102,400 without any issues, but failed to implement the logic correctly.

Here they are testing for exactly this constant value, treating it as an invalid value, in contrast to the clear implied meaning of the naming of both the constant and the custom error: Parameter bytes32[3200] calldata c_data means that total size of the encoded data would be 32 * 3200, which equals 102,400:

    function testRevertExceedsMaxDataLength_sendMessage(bytes32 to, bytes32[3200] calldata c_data, uint256 amount)
        external
    {
        bytes memory data = abi.encode(c_data);
        address from = makeAddr("from");
        vm.prank(from);
        vm.deal(from, amount);
        vm.expectRevert(IAvailBridge.ExceedsMaxDataLength.selector);
        bridge.sendMessage{value: amount}(to, data);
        assertEq(bridge.isSent(0), 0x0);
        assertEq(bridge.fees(), 0);
    }

Here they are testing for a successful message transfer, ensuring the max data length is BELOW the constant value: See line: vm.assume(data.length < 102_400 && amount >= bridge.getFee(data.length));:

    function test_sendMessage(bytes32 to, bytes calldata data, uint32 feePerByte, uint256 amount) external {
        vm.prank(owner);
        bridge.updateFeePerByte(feePerByte);
        vm.assume(data.length < 102_400 && amount >= bridge.getFee(data.length));
        address from = makeAddr("from");
        IAvailBridge.Message memory message = IAvailBridge.Message(0x01, bytes32(bytes20(from)), to, 2, 1, data, 0);
        vm.prank(from);
        vm.deal(from, amount);
        bridge.sendMessage{value: amount}(to, data);
        assertEq(bridge.isSent(0), keccak256(abi.encode(message)));
        assertEq(bridge.fees(), amount);
    }

I could have fixed the bug and ran these same tests, but slightly modified to cater for the bugfix, and then included the results, but the above explanations and logic should suffice, as it should be clear enough.

Impact

• The only reason this bug is medium severity(please change to high if appropriate) is because the protocol team could potentially turn around and say oh well we can just change the name of the MAX_DATA_LENGTH constant and magic, bug fixed. However, what would make such a move actually invalid would be if the 102,400 value is in fact an acceptable valid upper limit for the data length in terms of protocol business logic and other potential EVM/protocol constraints, which I confidently assume is the case. In other words, it should be clear that this is definitely a bug, and fixing it isn't as simple as changing the name but to fix the actual implementation logic.

• Denial of Service (DoS) / Disruption of Arbitrary Data Transfer: -- The bug allows messages with a VALID length equal to MAX_DATA_LENGTH to trigger a transaction revert. -- Since the sendMessage() function is intended for passing arbitrary data from Ethereum to Avail, the bug disrupts this functionality by preventing ALL messages with the maximum allowed length from being processed.

• Loss of Trust and Reputation: -- Users and other stakeholders may lose trust in the AvailBridge protocol if they observe disruptions and unreliability due to the bug.

Code Snippet

    /**
     * @notice  Emits a corresponding arbitrary messag event on Avail
     * @dev     This function is used for passing arbitrary data from Ethereum to Avail
     * @param   recipient  Recipient of the message on Avail
     * @param   data  Data to send
     */
    function sendMessage(bytes32 recipient, bytes calldata data) external payable whenNotPaused {
        uint256 length = data.length;
        if (length >= MAX_DATA_LENGTH) { /// @audit-issue should be `>`, in line with the naming of the `MAX_DATA_LENGTH` constant as well as the naming of the custom error message
            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

VSC. Manual Review

Recommendation

• EVM Word Size and Efficiency: The choice of the max acceptable data length may also be influenced by considerations such as EVM word size and gas efficiency. Simply changing the constant's name without addressing the implemented logic may not align with these underlying technical considerations.

To fix the bug, you should update the condition in the sendMessage() function to use the strict greater-than > operator instead of greater-than-or-equal-to >=.

This change aligns with the naming of the constant(and custom error) and intention of the protocol for allowing all message lengths less OR equal to the max data length, i.e. <= MAX_DATA_LENGTH.

-   if (length >= MAX_DATA_LENGTH) {
+   if (length > MAX_DATA_LENGTH) {
        revert ExceedsMaxDataLength();
    }

[dappconsulting]

Post contest deadline I checked for evidence to support my argument even more, and here's what I found. The below completely obliterates any objection to my argument in terms of "we can simply just change the name of the length constant, as a bugfix":

I've found evidence that the protocol itself supports the fact that data: Option<Bytes>, // must not exceed 100KB (Ethereum calldata limits), as can be seen in the code snippet below which was taken from the reference link: Reference: https://avail-project.notion.site/The-Avail-Bridge-Public-Copy-a00c2aa4937d496ea346d02a6bb119ff

bridge.sendMessage(
  to: H256,
  domain: UCompact, // must not exceed u32
  asset_id: Option<UCompact>,
  value: Option<UCompac>,
  data: Option<Bytes>, // must not exceed 100KB (Ethereum calldata limits)
}

• In the AvailBridge contract, MAX_DATA_LENGTH is defined as 102_400, which is equivalent to 100 KB (100 * 1024). Therefore, the contract attempts to ensure that the length of the data field in the sendMessage() function does not EXCEED(>) this specified limit, however the implemented logic is incorrect. The specified limit is clearly still a VALID data length value. Only data lengths greater than the MAX_DATA_LENGTH are invalid lengths, and the function should revert only for those cases. Therefore the only valid bugfix would be to change from >= to >.

Again:

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();
    }
    // ... rest of the function
}

[sherlock-admin]

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

tsvetanovv commented:

Low

takarez commented:

invalid because { minor impact and may be intended behavior}

[dappconsulting]

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

tsvetanovv commented:

Low

takarez commented:

invalid because { minor impact and may be intended behavior}

It's definitely not intended protocol behaviour. It is simply a logic error. Definitely a valid bug regardless, which definitely should be fixed.

Low impact? Whenever anyone sends a message of max length, which is still a VALID length, it will be DoS-ed. Why would that be low impact and not medium/high?

[QEDK]

Let's assume that X bytes are allowed, if we allow only X - 1 bytes, that might be unintended but not an attack vector. If X bytes are allowed and user is able to send X + 1, that would be an attack vector as the messages are effectively unexitable. Now coming to the issue and its severity itself, we are discussing atm on what the number should be, we intentionally <= as a precaution to prevent adverse scenarios where someone is able to send unexitable messages.

[dappconsulting]

we intentionally <= as a precaution to prevent adverse scenarios where someone is able to send unexitable messages.

Hey, thanks for your response. I dont fully understand the last part "we intentionally <= as a precaution to prevent adverse scenarios where someone is able to send unexitable messages." ?

Could you elaborate on this please?

Here is my argument and logic, please read carefully:

If a valid MAX message length X is still a valid message length to be used in the protocol(as it should be), then using this max length in the protocol should not be a problem at all, otherwise the logical decision would be to use X-1 as the max valid message length.

To me it is clear that the max message length in your protocol, let's say it's X, is clearly the max valid message length, and therefore it should not be logically intact argument to say that only message lengths < X are allowed, or all message lengths >= X are not allowed. We are excluding a valid max length here, which is logically incorrect.

Yes, if an attacker sends mesage length of X+1, then we have a problem/attack vector risk. Cool. Not all bugs are attack vectors, some are protocol coding bugs which cannot be exploited by attackers but can cause massive damage to the protocol or users, or lower damage, depending, and in some cases if the protocol is lucky, no damage. When we audit smart contracts we hunt for all bugs, both exploitable bugs and non-exploitable bugs. They are both equally valid class of bugs, and potentially equally dangerous in some cases.

Getting back to this bug. To me it's clearly and simply a "logic" bug because a VALID max message length is excluded from being able to be used by users of the protocol, implying that the max length is not a valid acceptable length to be used. If that was the case, then it should be made clear in the codebase that X is not a valid message length, it exceeds the valid max length by 1.

In terms of clear & accurate comments in codebases: uint256 constant MAX_DATA_LENGTH = 102400; // max valid allowed data length, anything ABOVE this value is invalid

This means: lets allow users to send messages with data lengths of <= MAX_DATA_LENGTH or lets exclude all messages with data lengths > MAX_DATA_LENGTH.

If a max data length is still a valid length, then it should 100% of the time be treated as a valid length, otherwise it's not a valid max length. X cannot be sometimes valid max and sometimes invalid max.

So unless it can be proven in this codebase that the definition of this max data length constant is supposed to be the start of the INVALID data lengths beyond the last/max valid length, then this bug report is valid, and only the severity still in question.

Unless I'm missing the point of your argument? Please elaborate if that's the case.

thanks