Wrong gas price calculation in eth_send_raw_unsigned_tx could lead to suboptimal transaction pricing

[howlbot-integration]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/eth_rpc.cairo#L304-L307

Vulnerability details

Proof of Concept

eth_rpc.cairo::eth_send_raw_unsigned_tx underprices transactions, which could lead to delayed or failed transactions in congested network conditions.

Take a look at this part of the code:

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/eth_rpc.cairo#L304-L307

let possible_priority_fee = tx.max_fee_per_gas - block_base_fee;
let priority_fee_is_max_priority_fee = is_nn(
    possible_priority_fee - tx.max_priority_fee_per_gas
);
let priority_fee_per_gas = priority_fee_is_max_priority_fee * tx.max_priority_fee_per_gas + (
    1 - priority_fee_is_max_priority_fee
) * possible_priority_fee;
let effective_gas_price = priority_fee_per_gas + block_base_fee;

The problem is in the calculation of priority_fee_per_gas. The logic attempts to choose between tx.max_priority_fee_per_gas and possible_priority_fee, but it does so incorrectly.

The issue is that priority_fee_is_max_priority_fee is set to 1 when possible_priority_fee is greater than or equal to tx.max_priority_fee_per_gas, which is the opposite of what we actually want. This means that when the possible_priority_fee is larger, the function will choose tx.max_priority_fee_per_gas instead of the smaller value.

As a result, in some cases, the priority_fee_per_gas could be set higher/lower than intended leading to users paying more/less in gas fees than necessary.

In most cases, transactions may be underpriced relative to what the user is willing to pay. In congested network conditions, transactions may end up being delayed or failing to be processed cos they're not utilizing the full gas price the user was willing to pay.

Short Note:

I discussed this issue with the sponsors and they confirmed this bug case

Recommended Mitigation Steps

The calculation of priority_fee_is_max_priority_fee should be inverted.

- let priority_fee_is_max_priority_fee = is_nn(
-     possible_priority_fee - tx.max_priority_fee_per_gas
- );
+ let priority_fee_is_max_priority_fee = is_nn(
+     tx.max_priority_fee_per_gas - possible_priority_fee
+ );

Assessed type

Error

[c4-judge]

dmvt marked the issue as selected for report

[dmvt]

ref: code-423n4/2024-09-kakarot-validation#125 (comment)

[obatirou]

This means that when the possible_priority_fee is larger, the function will choose tx.max_priority_fee_per_gas instead of the smaller value.

This is a wrong interpretation of our discord answer. The EIP1559 says that one should pick the smaller between possible and given max, which is the implemented behavior.

There is no wrong calculation here. This finding is invalid

[dmvt]

This is a wrong interpretation of our discord answer. The EIP1559 says that one should pick the smaller between possible and given max, which is the implemented behavior.

I missed this on my first review of the issue. The sponsor is correct here. The "max" is actually the variable defined in the first line highlighted by the warden let possible_priority_fee = tx.max_fee_per_gas - block_base_fee;, making the second line correct.

[c4-judge]

dmvt marked the issue as unsatisfactory:
Invalid