-
Notifications
You must be signed in to change notification settings - Fork 21.6k
core/txpool: check for overdraft when adding tx #28076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Defense against Mempurge Attack
|
I want to acknowledge that the patch code is also co-developed by my lab mates (@wangyibo0308) with guidance from my advisor (@tristartom, https://github.com/fs3l). |
|
Hi! Regarding the overdraft attack, we already have this check https://github.com/ethereum/go-ethereum/blob/master/core/txpool/validation.go#L227. It will be performed before the transaction is added into the txpool. |
Hello, this MemPurge attack can succeed because it evaded the overdraft check that you mentioned. Our code checking overdraft on this additional path fixed this issue. |
Disclaimer: We are not the designer of MemPurge Attack and we don't know if MemPurge is a confirmed bug. |
This comment was marked as spam.
This comment was marked as spam.
Remove the latent overdraft tx in pending pool.
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
| return false | ||
| } | ||
|
|
||
| // Check balance for overdraft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a better solution is to check the total cost of both pending list and queued list? So that we can ensure the promoted transactions are still covered by the fund somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we directly check the total cost of the pending and queued list, it may cause some queued transactions to be deleted incorrectly.
For example, a sender Alice sends 3 transactions, first, tx_1 deposits 10 ether to her account, then tx_2 transfers 5 Ether to Bob from Alice, and finally, tx_3 transfers 3 Ether to Bob again. If tx_2 and tx_3 come to the queued pool first without tx_1, the check of the total cost of the pending and queued list will delete those two without waiting for the first transaction tx_1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a valid issue, but can it also occur currently? These three transactions are in the pending list, and the last two are deleted solely because the first one has not been included yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these will not happen because the queued list now does not check for latent overdrafts of tx. Therefore, the last two txs can be admitted to the queued pool first, and when the first tx reaches the pending pool, the promote function moves the two previously arrived txs to the pending pool. The movement also does not check for latent overdrafts of tx. So in our code, we add the check in the promote function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, we think these two methods can both mitigate the attack and we are fine with implementing a check of latent overdraft in the queued list.
|
We'll put this on hold until we're "done" for now with other txpool-tweaks |
We understand the busy schedule on your side and that our code patch not being merged is due to administrative reasons not technical reasons. May we get a confirmation from you that our defense code posted at https://github.com/fs3l/go-ethereum-TNX-defense/blob/eb9bfb43705c2ab94088bb21b5bbf0c257720034/core/txpool/legacypool/legacypool.go#L887 have fixed the mempurge attack (since the attack has been confirmed as stated in the paper: https://eprint.iacr.org/2023/956.pdf). This will help Ethereum users who are interested in the mempool security adopt our patch. |
|
Hello, may we know if there is any update on this thread? Or is there a timeline on the proposed code-fix given the recent Geth development? Thanks. |
|
Hello! We have introduced a new policy based on our previous Defense1. Our new policy, Defense2, builds upon Defense1 and introduces a new admission method (AP) to secure the mempool against dual attacks by evicting and locking a victim mempool. This design enforces an upper bound on the attack damage under locking attacks and a lower bound on the attack cost under eviction attacks. Additionally, it successfully prevents the XT6 attack, which is still effective on the latest Geth, as detailed in the paper accepted by USENIX Security '24. We made the code changes in line 708-719 here: [link]. |
Defense against Mempurge Attack, described in this preprint: https://eprint.iacr.org/2023/956.pdf
The cause of MemPurge attacks is the following: The detection code of latent overdraft txs in Geth 1.11.4 does not kick in if arriving transactions are first future transactions in the future-tx pool and then turned into pending-but-overdraft txs.
We propose just one code change, that is, enforcing the account balance check when future transactions are moved to the pending pool.
We add only six lines of code to Geth 1.12.2 codebase from Line 887 to Line 894 as follows.
https://github.com/fs3l/go-ethereum-TNX-defense/blob/eb9bfb43705c2ab94088bb21b5bbf0c257720034/core/txpool/legacypool/legacypool.go#L887
We have tested our patch using the Geth benchmark:
https://github.com/ethereum/go-ethereum/blob/master/core/txpool/legacypool/legacypool_test.go
The performance overhead under this benchmark is negligible, as shown in the attachment.
result.pdf