AvailBridge.sol :: sendMessage() Users can submit excess fees, and the surplus is not refunded.

Summary

sendMessage() is employed to transmit a message from Ethereum to Avail, accompanied by a fee determined by the message length. However, if users submit an excess fee, the surplus amount is not refunded.

Vulnerability Detail

sendMessage() is used to send a message from Ethreum to Avail.

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);
    }

The code verifies whether msg.value < getFee(length) to ensure that the user-provided amount covers the fee. However, it fails to check if the sent amount exceeds the required fee. This oversight results in users paying more fees than necessary because the fee calculation uses msg.value instead of getFee(length).

fees += msg.value

POC

To run the POC, copy the provided code into AvailBridgeTest.t.sol.

function test_sendMessage_SendsExcessiveFee(bytes32 to, bytes calldata data) external {

        //the fee and the amount send by the user
        uint256 feePerByte = 1e15;
        uint256 amount = 1e18;

        vm.prank(owner);
        //sets the fee ratio
        bridge.updateFeePerByte(feePerByte);

        vm.assume(data.length < 102_400);

        address from = makeAddr("from");

        //create the message
        IAvailBridge.Message memory message = IAvailBridge.Message(0x01, bytes32(bytes20(from)), to, 2, 1, data, 0);

        vm.prank(from);
        vm.deal(from, amount);

        //send message
        bridge.sendMessage{value: amount}(to, data);

        //message is correctly registred
        assertEq(bridge.isSent(0), keccak256(abi.encode(message)));

        //fees are correctly sended
        assertEq(bridge.fees(), amount);

        //balance of the user
        assertEq(from.balance, 0);
    }

Impact

Loss of funds of the users.

Code Snippet

https://github.com/sherlock-audit/2023-12-avail/blob/1afb56b8d4dfbf5d3f21bed0ddf80a04730204b5/contracts/src/AvailBridge.sol#L300C5-L321C6

Tool used

Manual Review.

Recommendation

An effective solution involves utilizing getFee(length) instead of msg.value and subsequently refunding any excess amount to the user.

function sendMessages(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;
+       fees += getFee(length);
        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));

+       if(msg.value > getFee(length)) {
+           uint256 amountRefunded = msg.value - getFee(length);
+           (bool success, ) = msg.sender.call{value: amountRefunded}("");
+           require(success, "Transfer failed");
+       }

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

sherlock-audit / 2023-12-avail-judging

IvanFitro - AvailBridge.sol :: sendMessage() Users can submit excess fees, and the surplus is not refunded. #63