search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.24k stars 3.6k forks source link

Preventing 0 fee Tx spamming - Consensus Min Fee #4527

Closed RiccardoM closed 3 years ago

RiccardoM commented 5 years ago

Summary of Bug

Currently inside the MsgSend#ValidateBasic method implementation, the sender's balance is not checked for validity.

This is currently allowing a specific address (cosmos1pv6s6z542kyudwq8zxznkeu6hm6yv4fgvpzvd3) to spam invalid transactions trying to send 1.0 ATOM while having only 0.9 ATOMs inside his wallet.

Some transactions' hashes are the following.

Version

https://github.com/cosmos/cosmos-sdk/commit/028bbef48d093242a2b9adb9bab34eecc97bf3fe

Steps to Reproduce

  1. Create a send transaction having the following data
    • Send amount greater than current balance
    • Fee amount set to zero
  2. Try broadcasting the tx

The tx will be broadcasted but the send won't be effective.

Possible solutions

Improve validate check

A plausible solution in order to avoid this would be to implement a check based on the current balance of the sender, and a higher end estimate of the gas that he is going to pay inside the ValidateBasic method. The algorithm could be something like the following

  1. Get the sender_current_balance
  2. Compute a higher end estimate of the gas that he is going to pay
  3. Get the amount of fee that he is going to pay
  4. If sender_current_balance < gas + fee + send_amount invalidate the transaction

I don't know if a higher estimate of the fees and gas is possibile of if there is a smarter way of implementing this.

After a discussion presented below, this has been judged the worst option due to the fact that ValidateBasic has to perform basic checks and should not have access to the keeper.

Use a new tx_fee_floor param

Another option could be to implement a tx_fee_floor param inside the SDK in order to set a minimum tx fee that all validators must have set. This could be the only way to force validators to set a non-zero minimum-tx-fee value and prevent spam.

For Admin Use

fedekunze commented 5 years ago

On the tx hashes that you mentioned above. If you check the log, it says "success":false. The ValidateBasic() function from messages only validates the fields of the message. The rest is checked on the ante handler.

rigelrozanski commented 5 years ago

It doesn't matter if the sender doesn't have enough balance, this is not a requirement for a transaction to be pass the ante-handler, but a requirement of running the transaction through checkTx (which does more than validate basic as it has store lookup capabilities, validate-basic shouldnot/cannot). Once the ante-handler has been run if the transaction fails, it's okay because the fees have been irrevocably been paid to run the transaction - if the ante-handler fails then the transaction should be failing at the ante-handler and checkTx shouldn't even be run. If no fees are provided then the ante-handler should fail.

If any of the above statements are broken and somehow the zero-fees transactions are making it past the ante-handler then there is bug in the antehandler.

related: https://github.com/cosmos/cosmos-sdk/issues/4528

edit. validators set the min fee parameter - think the reason we haven’t had this is because we don’t have global params which is where a “global” floor-bottom parameter should live (as some private chains may want to allow zero fee transactions).

alexanderbez commented 5 years ago

Just to reiterate what has already been expressed:

Now, generally I'm not immediately in favor of having a tx fee floor param in x/auth because I believe it should be up to the validator's sole discretion to accept what they wish. However, as it's been described in this issue, spamming can still occur with 0 fee txs so long as there is a validator to propose/accept them.

The simple solution would be to signal to these validator(s) that they should stop doing so (increase their min fees)! In fact, @sunnya97 already did this. However, it may still be advantageous to have a tx fee floor param. In such a case, I believe this must go through governance and should be relatively low. We must also carefully think about all crypto-economic impacts.

RiccardoM commented 5 years ago

Thank you all for taking part into the discussion of this problem.

I now understand what you all are saying about the validation of the transaction and I am going to change the title of this issue in order to make it more generic.

About the tx fee floor param, what could the cypto-economic impacts of such param?

alexanderbez commented 5 years ago

Thanks @RiccardoM. Mostly around picking the appropriate floor.

We should also update the proposal in the issue body to use the tx fee floor param as that seems like the best alternative.

RiccardoM commented 5 years ago

@alexanderbez I've updated the description with the new possible solution

alexanderbez commented 5 years ago

Thanks @RiccardoM! Something like this, I believe, will have to go through a governance proposal. Also thinking further on this, it would be probably in the network's best interest to have what I'll call a "consensus min fee" to be dynamic based on some formula instead of static. There has been discussion by Vitalik on the zcash repo.

rigelrozanski commented 5 years ago

An idea of how we could make it dynamic is by simply taking the median value of validators' min fee as the network min fee

alexanderbez commented 5 years ago

An idea of how we could make it dynamic is by simply taking the median value of validators' min fee as the network min fee

Interesting. This would require the ability of validators being able to know about other validator's min fees.

Building on this, and borrowing from convos with @ValarDragon and discussions in the zcash thread, how about the following parameterized and relatively simple consensus min fee:

Have some initial consensus min fee F and a target block fulfillment size S (e.g. 75%). Every N blocks check to see if the average block fulfillment size is below or above S. If below, reduce F by some factor. If above, increase F by some factor. Otherwise, leave unchanged. We can probably ignore empty blocks too.

rigelrozanski commented 5 years ago

hmm should we really ignore empty blocks though? I don't see a reason why empty blocks shouldn't just function in the same way any other block does in that proposed mechanism - empty blocks represent unused capacity of the blockchain

So this mechanism is interesting because as soon as a blockchain started getting spammed the mechanism would react to create a large fee.

My only comment on this mechanism I don't think that the multiplicative factors would necessarily be appropriate... for instance if the blockchain has little use for a long time dragging the min fees to the lower possible limit, it should still be able to react to be being spammed quickly, I'd almost want the factor to be based on a long term historical average... the greater the spike/drop in usage the disproportionately faster the rate would change. make sense?

alexanderbez commented 5 years ago

Yeah on second thought, it may be advantageous to not avoid empty blocks. Wrt to drastic spikes in spam, we can possibly tune N to be small enough. Also, there is room to play with the multiplicative factor -- maybe we can parameterize this factor based on some recent historic input.

rigelrozanski commented 5 years ago

love it

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sahith-narahari commented 4 years ago

@alexanderbez do you think this can be added to sdk, and set as true or false in genesis. Few zones may be intrested to use this

alexanderbez commented 4 years ago

Of course, that's why the issue is still open, just not enough bandwidth to tackle it. We also need to implement this in such a way that allows applications to opt-in this functionality, because for example, in Gaia, this would need to go through a gov proposal I think. Let's slate this for 0.41.

aaronc commented 3 years ago

We would really like to get this done. I'm marking as 0.42 nice-to-have and will start on a draft PR.

aaronc commented 3 years ago

So the basic algorithm described here is pretty straightforward with just a few parameters.

My only comment on this mechanism I don't think that the multiplicative factors would necessarily be appropriate... for instance if the blockchain has little use for a long time dragging the min fees to the lower possible limit, it should still be able to react to be being spammed quickly, I'd almost want the factor to be based on a long term historical average... the greater the spike/drop in usage the disproportionately faster the rate would change. make sense?

I do think this is an important thing to keep in mind that we doesn't necessarily have an obvious solution to. I think the general idea would be to increase the fee quicker when gas usage spikes quicker so we might look at rate of change. But this would make one part of EIP 1559 quoted below more difficult:

When the network exceeds the target per-block gas usage, the base fee increases slightly and when capacity is below the target, it decreases slightly. Because these base fee changes are constrained, the maximum difference in base fee from block to block is predictable. This then allows wallets to auto-set the gas fees for users in a highly reliable fashion. It is expected that most users will not have to manually adjust gas fees, even in periods of high network activity.

So if we have an algorithm that increases fees quickly to respond to spikes, it will make fee estimation in wallets harder which I think is an important UX consideration. But if we don't do these quick fee ramp ups, it leaves newer, less mature zones vulnerable to cheap spam attacks at periods of lower activity. Any thoughts on how we reconcile these concerns?

hxrts commented 3 years ago

One consideration is there may be private/POA networks that don't want fees at all, a Cosmos Cash chain for instance. So we may want to have an override parameter.

I linked to a few resources here, the escalator algorithm has the ramp-up property you describe.

clevinson commented 3 years ago

There was a lot of active discussion on this issue today during our architecture call. Notes here.

alexanderbez commented 3 years ago

Happy to help out in either implementation or review here 👍

robert-zaremba commented 3 years ago

Seams to be related to: Preventing 0 fee Tx spamming - Consensus Min Fee #4527

tac0turtle commented 3 years ago

closing in favor of #8917

DesMarie commented 1 year ago

Summary of Bug

Currently inside the MsgSend#ValidateBasic method implementation, the sender's balance is not checked for validity.

This is currently allowing a specific address (cosmos1pv6s6z542kyudwq8zxznkeu6hm6yv4fgvpzvd3) to spam invalid transactions trying to send 1.0 ATOM while having only 0.9 ATOMs inside his wallet.

Some transactions' hashes are the following.

  • 9B0064B69F3C49ECA35DDE48D8FCE2378A638E0385DB489E12FF265D9018C825
  • EA4230A264C5CCD84EAD767F5FDB5172B72483428E0B85A899A684F768B77098
  • 1DF213731B007438E8DC749C9207264BD88F59D7CA2DDFF7F16B8F93122CC9EC
  • Many many others

Version

028bbef

Steps to Reproduce

  1. Create a send transaction having the following data

    • Send amount greater than current balance
    • Fee amount set to zero
  2. Try broadcasting the tx

The tx will be broadcasted but the send won't be effective.

Possible solutions

Improve validate check

~ A plausible solution in order to avoid this would be to implement a check based on the current balance of the sender, and a higher end estimate of the gas that he is going to pay inside the ValidateBasic method. The algorithm could be something like the following 1. Get the sender_current_balance 2. Compute a higher end estimate of the gas that he is going to pay 3. Get the amount of fee that he is going to pay 4. If sender_current_balance < gas + fee + send_amount invalidate the transaction I don't know if a higher estimate of the fees and gas is possibile of if there is a smarter way of implementing this. ~ After a discussion presented below, this has been judged the worst option due to the fact that ValidateBasic has to perform basic checks and should not have access to the keeper.

Use a new tx_fee_floor param

Another option could be to implement a tx_fee_floor param inside the SDK in order to set a minimum tx fee that all validators must have set. This could be the only way to force validators to set a non-zero minimum-tx-fee value and prevent spam.

For Admin Use

  • [ ] Not duplicate issue
  • [x] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [x] Contributor assigned/self-assigned
DesMarie commented 1 year ago

``

alexanderbez commented 1 year ago

@DesMarie there is a globalfee module that chains use to have a consensus-wide min fee. The thing is, when the fee is transferred it should fail, thus CheckTx should fail and the tx should never enter the mempool. So having a balance check doesn't buy you anything.