search

poanetwork / posdao-contracts

Smart contracts for POSDAO (Proof of Stake Decentralized Autonomous Organization consensus), a DPOS consensus implemented in Solidity and running within EVM with swappable BFT consensus
Other
106 stars 50 forks source link

Removed a reference to deleted reportBenign #36

Closed vkomenda closed 5 years ago

vkomenda commented 5 years ago

Isn't the removed bit dead code after https://github.com/poanetwork/posdao-contracts/commit/4825aba66020d1e3b5e9566d98a0e932e84f74f5#diff-e4a492be7f662d6148e33349be7eacffL77?

varasev commented 5 years ago

Removing the function from the contract doesn't mean that some validator can't try to call unexisting function and consume block's gas with that failing call (there will be approximately 25000 gas spent for such a calling). So, it's more reliable to leave that condition.

vkomenda commented 5 years ago

Possibly you are right but I still think that if all valdators comply to AuRa+PoS then there shouldn't be any such calls from correct validators. The only possibility of getting such a call is from an incompatible version of Parity client or from a malicious node. In the former case that client will most likely be incompatible with PoS, and in the latter case the validator will be reported and removed. You can still keep this catch for the non-existent method but if a malicious validator wants to make correct validators spend gas on handling calls to a non-existent method, that validator can pick from an infinite number of possibilities, so protecting ourselves from one method is clearly not enough.

varasev commented 5 years ago

I mean another case: imagine that some validator uses their ability to use 0 gas price. In this case, the validator can just request unexisting reportBenign function with zero gas price and will fail, but the gas spent for this fail transaction will be deducted from the block gas limit (because fail transactions spend gas anyway). If the validator makes a lot of those fail transactions, they will reach the gas limit and blockade other normal transactions. TxPermission prevents such cases.

afck commented 5 years ago

But if they do that maliciously, couldn't they just make a service transaction call to method foo or anything else, and do the same amount of damage?

varasev commented 5 years ago

No, they cannot do that with zero gas price because TxPermission rules don't allow that.

vkomenda commented 5 years ago

No, they cannot do that with zero gas price because TxPermission rules don't allow that.

So, if we disallow reportBenign and make it impossible to call with 0 gas price, wouldn't we fix the problem with bogus calls by making them costly?

afck commented 5 years ago

Exactly: I don't understand why the nonexisting reportBenign needs a different handling than the nonexisting foo.

varasev commented 5 years ago

Exactly: I don't understand why the nonexisting reportBenign needs a different handling than the nonexisting foo.

Ah, I see. The very last condition in allowedTxTypes already satisfies the rule. Sorry, I'm stupid this evening :)

The condition for reportBenign is really redundant here. Approve.