Fix security issue: transaction validity controls must be executed in STF

[librelois]

PR #482 introduce some security issues: the validity check of a transaction is no longer performed in the State Transition Function (in block execution context).

Before this PR, the validate_unsigned function was called in the STF via the call to the pre_dispatch_unsigned method of the SignedExtension feature: https://github.com/paritytech/substrate/blob/master/primitives/runtime/src/traits.rs#L929-L937.

Now, the validate_self_contained function does this job, but it is not called in the STF.

Thus, a malicious validator can submit a valid block with invalid transactions, and for example replay transactions from another ChainId.

So of course, it would be enough to modify the implementation of SelfContainedCall by the Call type of the runtime to call the validation function in apply_self_contained.
But apply_self_contained does not allow to return a TransactionValidityError, the only way to make the block fail is to return None, this is not satisfactory.

I chose instead to do as substrate and add a pre_dispatch_self_contained function, which is used to check the validity of a transaction in the context of a block execution.
This also has the advantage of allowing to treat differently the nonce check depending on whether you are in the pool or in the block, like substrate does (see CheckNonce: https://github.com/paritytech/substrate/blob/master/frame/system/src/extensions/check_nonce.rs#L75-L123).

The controls in StackExecutor have been removed because they must be done upstream.
Indeed, in case of failure of a check in the StackExecutor, the extrinsic is failed but the block remains valid, but the issuer will not be charged because the checks return before. This opens a security hole allowing to spam the blockchain without paying fees.

[sorpaas]

Please just keep the changes of the two files in primitives/self-contained and revert others. The rest of the changes look slightly confusing to me and it might be better to review them in a separate PR.

Indeed, in case of failure of a check in the StackExecutor, the extrinsic is failed but the block remains valid, but the issuer will not be charged because the checks return before. This opens a security hole allowing to spam the blockchain without paying fees.

No. The issuer will always be charged in case of error. It only does not pay fees if the call succeed.

[sorpaas]

Change this to return TransactionValidity. I don't know why not?

[librelois]

The type TransactionValidity is only of interest for the pool, the struct ValidTransaction contains data that are only used by the pool, these data have no meaning in the context of the execution of a block.

[librelois]

Please just keep the changes of the two files in primitives/self-contained and revert others.

This is not possible. At least the changes in the ethereum pallet are necessary at the same time.

No. The issuer will always be charged in case of error. It only does not pay fees if the call succeed.

No, the fees are only applied if you reach at least the withdraw_fee call. In the case of signed substrate extrinsics this is done in the pre_dispatch phase by the SignedExtra of the transaction-payment pallet.
But in the case of an ethereum transaction, it is done in the stack Runner after the checks, so if a check fails the fee is not charged.
Moreover, these checks do not cause the block to be rejected, whereas they should for security reasons, so they must be done before entering the call (in pre_dispatch phase).

[nanocryk]

Here is an attack scenario :

A malicious collator produce a block with an Ethereum transaction having an invalid nonce.
The transaction will be executed by other clients and fail here :
https://github.com/paritytech/frontier/blob/master/frame/evm/src/runner/stack.rs#L95

However failing here don't make the block invalid, and as you can see no fees have been charged yet (withdraw_fee is called just after this check).
Thus, it's possible for a malicious collator to pollute a block with invalid Ethereum transactions for free, and other clients will not refuse it. (note: the collator actually don't need to be malicious, we've already encountered multiple times bugs in the block authorship pipeline that failed to filter out correctly invalid transaction that ended up in a block)

[sorpaas]

Thus, it's possible for a malicious collator to pollute a block with invalid Ethereum transactions for free, and other clients will not refuse it. (note: the collator actually don't need to be malicious, we've already encountered multiple times bugs in the block authorship pipeline that failed to filter out correctly invalid transaction that ended up in a block)

Can you show an actual attack routine with this? Should be fairly straightforward -- spin up a dev node, submit an extrinsic to evm pallet, and see if the balance drops. The fee, in case of error, is supposed to be handled by outer Substrate dispatchable logic. If we're doing that incorrectly then there can be bigger problems.

[librelois]

The fee, in case of error, is supposed to be handled by outer Substrate dispatchable logic.

Substrate dispatchable logic manages the fees via pre_dispatch and post_dispatch of SignedExtra (here: https://github.com/paritytech/substrate/blob/master/primitives/runtime/src/generic/checked_extrinsic.rs#L68-L88), so it's not managed by substrate anymore since we use our own extrinsic.

If we're doing that incorrectly then there can be bigger problems.

In fact this is one more problem, we would have to reimplement in the frontier extrinsic the pre_dispatch/post_dispatch logic, and manage the fees in it. This PR reintroduces pre_dispatch, but does not manage the fees in it, this is a small change I can add in this PR

[sorpaas]

I think that means we should have no problem in pallet-evm at least. I'd prefer if you can revert the changes in that pallet. For the problem in pallet-ethereum, I think we need to panic error after the do_transact function. And make sure you covered all validation cases in validate_self_contained so that panic never happens.

[sorpaas]

For the record, on the more "Substrate-alike" pallet-evm, we're definitely not able to pre-validate all errors before execution because we just don't have access to those life-cycles. And it shouldn't panic in case of those errors, because that's not how you usually do it in Substrate. We might be able to move some of the logic as "extensions" in the future, but I don't think that can be all of them because we still need to accept some of those parameters and do some runtime things.

[sorpaas]

In fact this is one more problem, we would have to reimplement in the frontier extrinsic the pre_dispatch/post_dispatch logic, and manage the fees in it. This PR reintroduces pre_dispatch, but does not manage the fees in it, this is a small change I can add in this PR

Are you sure? As I recall we do the fee logic inside the evm executor. In case of valid Ethereum transactions, Substrate is hands off in terms of fee handling.

[librelois]

I think we need to panic error after the do_transact function

If we do that we'll make the block invalid if the transaction is reverted, which is not compliant.
It is necessary to panic if and only if the transaction is invalid, which is what substrate does when we return a TransactionValidityError, in all other cases, we should not panic.

we're definitely not able to pre-validate all errors before execution because we just don't have access to those life-cycles.

We don't need to pre-validate all the errors, only those that should make the block invalid. That is, the ones that prevent fees from being charged or ensure that there is no replay (otherwise we can force a user to pay fees against his will).
For all other errors, the extrinsic is a failure but in a valid block, and the author is charged, so it's ok.

We might be able to move some of the logic as "extensions" in the future, but I don't think that can be all of them because we still need to accept some of those parameters and do some runtime things.

If we do like the SignedExtension of substrate it would be defined by the runtime and configurable, so I think we can do all the validity checks of a transaction. You will have understood that, by valid transaction we mean: transaction that does not imply to reject the block. The notion of valid transaction is totally different from the notion of success or failure of the execution of the call that this transaction contains.

Are you sure? As I recall we do the fee logic inside the evm executor. In case of valid Ethereum transactions, Substrate is hands off in terms of fee handling.

In fact we return Pays::No in case of valid transaction, in order to skipper the management of the substrate fees.
And in case the transaction is invalid, we must panic to reject the block, which will be the case when this PR will be merged, so it will be good for the fees.

[sorpaas]

I haven't fully read your comment, but just to clear some misunderstandings first -- in case of an Ethereum transaction revert we'd still return Ok for the Substrate dispatchable. Anything that will result in state changes will never error out in terms of Substrate dispatchable.

[sorpaas]

@librelois Invalid transaction vs transaction revert should actually be like this:

• Invalid transaction, which happens if the nonce is not valid, the gas limit is too high, etc.
  • In pallet-evm, those should be validated within the dispatchable. In case of error, it should return a dispatchable error. Fee deduction is handled by outer Substrate dispatchable.
  • In pallet-ethereum, those should be pre-validated in extrinsic's validate and dispatch. We do not need to validate them again within the dispatchable, but if there's anything left-over or assumptions broken, it should straight panic. Fee deduction should not be handled because of panic.
• Transaction revert, which is an Ethereum concept.
  • In both pallet-evm and pallet-ethereum, they should execute to Ok, same as a passing transaction.
  • Fee deduction should be handled within EVM executor, same as a passing transaction

[sorpaas]

The current PR actually looks okay for me now, but you just need to fix https://github.com/paritytech/frontier/pull/495/files#r727997940.

It doesn't appear to fix the second vulnerability you mentioned though. Just let me know if you'd just want to proceed with this now -- I'd actually prefer we fix that in a separate PR.

[librelois]

  * In `pallet-evm`, those should be validated within the dispatchable. In case of error, it should return a dispatchable error. Fee deduction is handled by outer Substrate dispatchable.

Exactly no, it is a security flaw to do so. If the transaction is invalid then the block must be rejected. Otherwise it allows a dishonest validator to insert an invalid transaction (e.g. a replay), which will charge the originator of the original transaction against his will.
This is why the transaction validity checks in the evm pallet should panic if they fail.

[librelois]

It doesn't appear to fix the second vulnerability you mentioned though. Just let me know if you'd just want to proceed with this now -- I'd actually prefer we fix that in a separate PR.

For me there are only vulnerabilities left for projects that use the evm pallet directly without going through the ethereum pallet. But this is not our case, so you can merge as is, I'll see if I can spend some time on the remaining vulnerabilities in another PR.

[sorpaas]

There's no such thing as "Ethereum transactions" in pallet-evm. That pallet only handles Substrate extrinsics. Those replay attack and things like that can only happen in pallet-ethereum. This is why we need panic in pallet-ethereum while it is perfectly safe to just return dispatchable errors in pallet-evm.

[sorpaas]

For me there are only vulnerabilities left for projects that use the evm pallet directly without going through the ethereum pallet. But this is not our case, so you can merge as is, I'll see if I can spend some time on the remaining vulnerabilities in another PR.

Just to clear this confusion. That second vulnerability I referred to is in pallet-ethereum, not pallet-evm. Because we're still returning dispatchable errors in transaction application but not panic, it will not pay fees. And this can result in spamming attacks. pallet-evm is safe on the other hand. I think you got confused on the point that pallet-evm does not handle Ethereum transactions at all.

[librelois]

There's no such thing as "Ethereum transactions" in pallet-evm. That pallet only handles Substrate extrinsics. Those replay attack and things like that can only happen in pallet-ethereum.

Haa ok, since we don't use frontier that way, I couldn't possibly know. Ok, so that means no more vulnerability, wonderful :)

[librelois]

That second vulnerability I referred to is in pallet-ethereum, not pallet-evm. Because we're still returning dispatchable errors in transaction application but not panic, it will not pay fees. And this can result in spamming attacks.

No my PR corrects that, that's what this PR is all about. Returning a TransactionValidityError in the apply method causes the block to be rejected, and that's what it should do. Look at the context of the apply call in the executive pallet ;)

[sorpaas]

@librelois I mean we're still leaving the possibility of that. Yes if we can confirm all cases are covered in validate and pre_dispatch then things are fine, but on the other hand we'd need to be defensive and panic if the conditions do not meet. I'll submit a PR shortly after I merge this, to show you what I means.

[librelois]

we'd need to be defensive and panic if the conditions do not meet.

The only way to know if the conditions are not met is to do the validity checks of the transaction, which I do in pre_dispatch. And if these checks are invalid then the executive pallet will panic for us.