irisnet / irishub

A BPoS blockchain that enables cross-chain interoperability through a unified service model -- built with Cosmos-SDK
Apache License 2.0
304 stars 127 forks source link

[Bounty] [Eligible $300] Gas Rewards enables cheap fully filled blocks #922

Closed ValarDragon closed 5 years ago

ValarDragon commented 5 years ago

Impact: An adversary with minimal amounts of money (enough money to make 1 honest tx per block) can prevent any other txs from getting into a block. This applies with every validator being fully honest.

Problem: No limits are placed on Iris fee refunds. See these lines: https://github.com/irisnet/irishub/blob/874ed887cead838e03e3ef60e99a9abedbb2fda3/baseapp/fee.go#L65-L77

The core of the problem is that I can create valid txs with high gas wanted, but low gas used. This is because there is no concept of "MaxGasWantedPerTx" in the mempool, except for the block's gas cap, and there is no such logic getting ran in CheckTx anywhere in the Iris app.go or the ante handler which I could find. This means that I can produce a single transaction that has its gas wanted field as the entire block's amount of gas. (This is in general a desirable property) Consequently, I can specify sufficient fees such that proposers would accept / propose a block only containing my tx according to tendermint mempool logic. (As it would have sufficient fees allocated for its gas wanted) In Tendermint, txs are never ran before being included in a block, only check txs get ran. The block is built based on the sum of gasWanted fields. (The sum of the gas wanted fields cannot be greater than the block gas limit)

It follows that I can make a tx with a small amount of gas used, but with gas wanted = block gas limit, and correct amount of fees for that much gas. This will get proposed by any honest validator as a block with just one tx. I then get refunded everything but the gas corresponding to gasUsed due to the lack of limits in the cited iris logic. Since GasUsed is small, the cost to me to effectively waste the networks block is negligible. Thus I can keep on using this strategy to make all blocks only have 1 tx. This is quite high impact. Also note that without validators modifying their mempools, its impossible for circuit breakers or governance procedures to proceed, as they cannot get their tx included in the block.

A note about prioritization within the mempool, which is what the attack depends on: If mempools sorted by fee, it would be fairly trivial to make my large gas wanted tx always first. Since Tendermint doesn't currently do this, it'd just require a lot of p2p spam to get my tx prioritized first. (As its essentially FIFO if it passes CheckTx) The way blocks are built is if the next tx in this FIFO queue would overflow the gas wanted, no more txs are included. Therefore this high gas wanted will block more txs from getting in even if its not the first. (e.g. if the queue is <honest tx>, <my tx>, <honest tx>, only the first honest tx would get proposed, further causing small blocks at no cost to me)

Just to give a bit of context, this isn't an issue in Ethereum as there is no pipelining in its consensus. The txs are ran before proposing, so the gasUsed field is known. In Tendermint, since its a multi-round BFT algorithm, there is some degree of pipelining. The block doesn't get ran until its committed, to avoid extra computation under round increments, and to mitigate bandwidth requirements. The commit happens in the following blocks proposal.

Proposed Solution The suggested mitigation is to limit gas refunds to be within some factor of gas wanted. This would be changing the gas refunded logic to be: gas refunded = max(gas_used * factor, gas_wanted), instead of being the difference between gas wanted and gas used as it currently is.

-- Note that I do work on both the Tendermint and cosmos-sdk projects, however per the rules of the bug bounty I should be eligible as I had no part in writing this fee refund code. In fact, I have a proposal for the cosmos-sdk for how to do this exactly the same way I proposed here. https://github.com/cosmos/cosmos-sdk/issues/2150

abelliumnt commented 5 years ago

Thanks very much for your information. Actually this is a known issue. We will start to undertake this issue soon.

ValarDragon commented 5 years ago

Is there a list of known issues / could you guide me on where to look for known issues such as this? I couldn't find anything on it. It would be nice to see such a list to avoid spending the time on write-ups for known problems, especially when the fix is quite simple.

wukongcheng commented 5 years ago

I have filed some issues begin with [known issues]. And from now on, we will also use the github issue instead of the internal jira to register bugs. Thanks!

haifengxi commented 5 years ago

This vulnerability is a well known issue in our core dev team, and, as you pointed out, it's related to a known issue of Cosmos-SDK gas mechanism. We have now amended the program policy and started posting known issues to GitHub. Sorry about the confusion.

How about we mark this report of yours as Eligible with an award of $300? @ValarDragon

ValarDragon commented 5 years ago

That works for me, thank you!

haifengxi commented 5 years ago

Related issues: https://github.com/irisnet/irishub/issues/982 and https://github.com/irisnet/irishub/issues/996 will solve this issue.