sherlock-audit / 2023-12-avail-judging

4 stars 4 forks source link

Tricko - Wrong message length check in `AvailBridge.sendMessage()`. #64

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

Tricko

medium

Wrong message length check in AvailBridge.sendMessage().

Summary

The AvailBridge.sendMessage() function incorrectly checks the message length by verifying if data.length is greater or equal to 102400. This check should only revert data.length exceeds the 102400 size limit.

Vulnerability Detail

As we can see from AvailBridge.sendMessage() function (code snippet below), it will revert if data.length is greater than or equal to MAX_DATA_LENGTH.

function sendMessage(bytes32 recipient, bytes calldata data) external payable whenNotPaused {
    uint256 length = data.length;
    if (length >= MAX_DATA_LENGTH) {
        revert ExceedsMaxDataLength();
    }
    // ...
}

However, examining the bridge off-chain code reveals that the maximum length is 102400. Consequently, it should only revert if data.length surpasses the MAX_DATA_LENGTH. Due to this mismatch, valid messages will cause reverts in AvailBridge.sendMessage().

Impact

Message length is wrongly checked. Valid messages will be incorrectly rejected by AvailBridge.sendMessage().

Code Snippet

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

Tool used

Manual Review.

Recommendation

Consider changing the data length check, as shown below.

diff --git a/AvailBridge.sol b/AvailBridge.mod.sol
index 38c7c11..df1034c 100644
--- a/AvailBridge.sol
+++ b/AvailBridge.mod.sol
@@ -299,7 +299,7 @@ contract AvailBridge is
      */
     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
sherlock-admin commented 8 months ago

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

tsvetanovv commented:

Low

takarez commented:

invalid because { Thi is intended behavior i believe as to make sure it didn't even come close}