soliditywala - Excessive fee not returned

[sherlock-admin2]

soliditywala

high

Excessive fee not returned

Summary

The sendMessage() function in the AvailBridge smart contract exposes a vulnerability where users incur irreversible ether losses when mistakenly sending more than the calculated fee. The current implementation lacks a refund mechanism for excess ethers.

Vulnerability Detail

The sendMessage() function in the AvailBridge smart contract lacks a mechanism to refund users for excessive ether sent when the amount send for fees exceeds the amount returned by getFee(). The current implementation only ensures that the ethers received must be greater or equal to the calculated fee, but does not have any refunding mechanism resulting in the loss of excess funds.

Impact

Users mistakenly sending more ether than required for the transaction fee incur an irrecoverable loss of the excessive amount.

Code Snippet

        if (msg.value < getFee(length)) {
            revert FeeTooLow();
        }

Tool used

Manual Review

Recommendation

Implement a refund mechanism to return any excess ether sent by users. Adjust the relevant code as follows:

uint256 fee = getFee(length);
if (msg.value < fee) {
    revert FeeTooLow();
}

// Refund excess ether to the sender
uint256 excessEther = msg.value - fee;
if (excessEther > 0) {
    (bool success, ) = msg.sender.call{value: excessEther}("");
    if (!success) {
        revert RefundFailed();
    }
}

[sherlock-admin]

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

tsvetanovv commented:

Invalid. User mistake. See Sherlock documentation

takarez commented:

valid because { This is valid and a duplicate of issue 013}

[soliditywala]

Escalate I think this is a valid issue, here is the reason why.

Alice wants to sendMessage() and calculates fees required off chain when feePerByte = 1000 wei.

Lets say the messageLength = 4 and then fees calculated was 4 * 1000 = 4000 wei.

So she sends 4000 wei along when calling sendMessage().

Meanwhile, updateFeePerByte() was called by admin/governance and new fees was feePerByte = 500.

Both the tx are in mem-pool, and updateFeePerByte() was executed before Alice's sendMessage() due to higher gas fee.

Now because the new feePerByte = 500, the function should accept 4 * 500 = 2000 wei. But as the function is not designed to return excess value, Alice will lose 2000 wei.

[sherlock-admin]

Escalate I think this is a valid issue, here is the reason why.

Alice wants to sendMessage() and calculates fees required off chain when feePerByte = 1000 wei.

Lets say the messageLength = 4 and then fees calculated was 4 * 1000 = 4000 wei.

So she sends 4000 wei along when calling sendMessage().

Meanwhile, updateFeePerByte() was called by admin/governance and new fees was feePerByte = 500.

Both the tx are in mem-pool, and updateFeePerByte() was executed before Alice's sendMessage() due to higher gas fee.

Now because the new feePerByte = 500, the function should accept 4 * 500 = 2000 wei. But as the function is not designed to return excess value, Alice will lose 2000 wei.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[ShaheenRehman]

Escalate I think this is a valid issue, here is the reason why.

Alice wants to sendMessage() and calculates fees required off chain when feePerByte = 1000 wei.

Lets say the messageLength = 4 and then fees calculated was 4 * 1000 = 4000 wei.

So she sends 4000 wei along when calling sendMessage().

Meanwhile, updateFeePerByte() was called by admin/governance and new fees was feePerByte = 500.

Both the tx are in mem-pool, and updateFeePerByte() was executed before Alice's sendMessage() due to higher gas fee.

Now because the new feePerByte = 500, the function should accept 4 * 500 = 2000 wei. But as the function is not designed to return excess value, Alice will lose 2000 wei.

[sherlock-admin]

Escalate I think this is a valid issue, here is the reason why.

Alice wants to sendMessage() and calculates fees required off chain when feePerByte = 1000 wei.

Lets say the messageLength = 4 and then fees calculated was 4 * 1000 = 4000 wei.

So she sends 4000 wei along when calling sendMessage().

Meanwhile, updateFeePerByte() was called by admin/governance and new fees was feePerByte = 500.

Both the tx are in mem-pool, and updateFeePerByte() was executed before Alice's sendMessage() due to higher gas fee.

Now because the new feePerByte = 500, the function should accept 4 * 500 = 2000 wei. But as the function is not designed to return excess value, Alice will lose 2000 wei.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[QEDK]

This is a user input issue, we will provide a UI that will set the minimum fee automatically. Now, let's come to the question of whether excess fees should be returned - my simple view on this matter is no. The important question to consider here is, from the perspective of the protocol, what is the difference between a user that intentionally pays more than the minimum fee and someone who accidentally sends more than the minimum fee, the obvious answer is none.

Now coming to the question of refunds, why not then simply make it require(msg.value == getFee(length))? The additional refund gas costs + reentrancy concerns are not at all worth the risk. Finally to address the severity, this is at most a low-severity issue, that is my assessment.

[soliditywala]

I agree, UI will set the minimum fees, but it doesn't matter, because minimum fees at the time of signing the tx and execution can change, that's what the issue is about.

So you are thinking from protocol's perspective, sounds good, it make sense that protocol wants to generate more fees, but what about from the users perspective? Why should they pay more.

The gas and reentrancy check cost will be paid by caller, so it make sense to spend SOME gas to avoid losing MORE ethers.

[0xTenma]

Escalations points are false considering there is a modifier: whenNotPaused in the sendMessage function. When the admin updates the fee, they just have to pause the users from sending messages via that function for some time.

[soliditywala]

Well this behavior that the contract should be in paused state when updating the fees was not mentioned or documented anywhere I guess.

[r0ck3tzx]

This is not a valid issue, not even LOW/QA. The fee presented to the user is accepted by them, and they decide to proceed with it. In your example, Alice accepted the fee and decided to call the sendMessage function. There would be an issue if the user were charged a fee that they didn't accept.

[0xMR0]

This issue is simply user input mistake and should be invalid per sherlock rules.

[cvetanovv]

This issue is invalid. I agree with @QEDK, @r0ck3tzx, @0xMR0. This is a user mistake and according to Sherlock documentation is not a valid issue.

[Evert0x]

Result: Invalid Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ShaheenRehman: rejected