core/txpool/legacypool: Fix overdraft DoS attack in the legacy pool

[ehdus829]

An attacker can perform an overdraft attack by sending transactions in a batch state over the P2P network. This allows the attacker to evict other pending transactions from the txpool.

This vulnerability occurs because the overdraft-checking logic inside the ValidateTransactionWithState function does not take batch-state transactions into account.

Within this function, only the spending cost of pending transactions is considered. However, for batch-state transactions, the transactions are first moved into the queue state via the enqueueTx function, and only after all loops finish are they promoted to the pending state through the requestPromoteExecutables function.

As a result, an attacker can submit a series of batched transactions over the P2P network whose total value exceeds what the attacker can actually afford, causing them to be registered as pending transactions and pushing out other legitimate pending transactions from the txpool.

We have fully validated this issue using both local test code and a remotely exploitable PoC. This issue has already been reported through the Ethereum bug bounty program, and we are submitting this PR following a request to do so. Applying the patch included in this PR mitigates the described scenario.

[ehdus829]

We fixed the test coverage to properly account for the cumulative cost when submitting transactions in batch mode.

• TestPostponing:

  The test was modified to validate Postponing behavior by submitting batch transactions from two separate accounts. Each account adds transactions to the pool as single transactions with different costs, and the test verifies that Postponing works correctly.

  The previous version did not check whether the cumulative cost of batch transactions exceeded the account balance.

• TestStableUnderpricing:

  The wallet corresponding to keys[0] does not hold enough balance to cover the cumulative cost of transactions added to the pool.

  To add transactions successfully, keys[0] must have at least config.GlobalSlots * 100100 balance.

• TestPendingMinimumAllowance:

  When sending transactions in batch mode, the wallets in the keys mapping do not have sufficient balance to cover the cumulative cost.

  Although each wallet may submit more than AccountSlots transactions in batch mode, the eviction algorithm ensures that only AccountSlots transactions per account remain in the pool.

  Therefore, each wallet requires an initial balance of AccountSlots * 100100.

• TestQueueAccountLimiting:

  This test did not consider whether the cumulative cost of batch-submitted transactions fits within the account balance.

  The test pre-fills the queue with transactions and later calls runReorg, during which promoteExecutables caps the AccountQueue and removes transactions beyond the limit.

  As a result, the account must initially hold a balance of int64(testTxPoolConfig.AccountQueue + 5) * 100100.

  Please review when the team has time.

[healthykim]

Hi, to better understand the attack, could you share the code you used to perform the PoC? Also, since functions like promoteExecutables in queue.go currently check overdrafts based only on the current on chain balance, it might be better to update both for consistency.

[ehdus829]

Hey team, both versions of the PoC (the remote PoC and the local testing PoC) are fully completed. We can share the PoC with the Ethereum team anytime (we’ve also shared it before via the Ethereum bug bounty program). But we have some concerns about sharing the PoC in a public PR. Therefore, we’d like to hear the team’s thoughts on the preferred way to share it before moving forward.
Regarding the promoteExecutables function you mentioned, we also reviewed applying the same patch to it. But our view is different. As noted in the code comments, the goal of the ValidateTransactionWithState function is to act as a helper that verifies whether a transaction is valid based on the internal state of the pool, which is why we believe applying the patch described in the PR is appropriate there. In contrast, the promoteExecutables function, as stated in its comments, is responsible for promoting processable transactions from the future queue to pending transactions, and thus serves a different purpose.
For this reason, To keep each function aligned with its intended responsibility, we think it is best to leave promoteExecutables unchanged. And we don't anticipate additional security issues arising from keeping promoteExecutables as it is.

[ehdus829]

@rjl493456442 Should we request an additional code review? We're also wondering what the plan is for merging.

[ehdus829]

Since the previous conversation thread was related to an earlier patch version, I close that thread and resend the patch review provided above in this thread.

I revised the patch so that the queue cost delta is added on top of the existing ExistingExpenditure value, while managing a snapshot using the batchTxValidationState structure.

This patch should effectively mitigate the overdraft attack. When a transaction is first added in a batch state, the queue total cost is stored as a snapshot. When a subsequent transaction is added, even though the first transaction is already in the queue, its cost is added to the ExistingExpenditure value, and the system verifies whether the second transaction would exceed the sender’s available balance. As a result, even if an attacker attempts an overdraft attack across batchtransactions, the cost of the first transaction is already included in the ExistingExpenditure value, which prevents the attack.

Below are additional considerations to ensure that this patch does not interfere with normal transaction behavior:

Considerations

1. Gap-filling transactions are not rejected.
   Legitimate gap-filling transactions that fill nonce gaps, as previously mentioned, will not be rejected. This is because the existing queue transaction total cost is excluded from ExistingExpenditure.

2. Replacement transactions within the batch are correctly handled.
   If multiple transactions within the batch share the same nonce, they should be treated as replacement transactions, which requires calculating the delta cost. To support this, the patch records the nonce values of transactions entering the batch and includes them in the calculation.
   As a result:

   • Transactions that should be replaced normally will still be replaced correctly.
   • When replacing a transaction that already exists in the queue, the previous cost is excluded from ExistingCost, since including it would open an overdraft attack vector.

3. The following overdraft attack vectors were specifically considered:

   • Whether multiple transactions submitted in a batch could individually stay within the account balance but collectively exceed the available balance.
   • Whether replacing an already queued transaction (not a batch replacement) could be abused to exceed the available balance.

Additionally, while implementing this patch, I updated the test cases that were failing so that they now align with the intended behavior.

Please let me know if you have any feedback after reviewing the patch.