[High] Missing check to prevent multiple bundle submissions in one single consensus block

[jakoblell]

Issue description

As of now there is no check to prevent submission of multiple bundles for a domain in one single consensus block.

In the (unlikely but not totally impossible) case an attacker has a large number of winning proof_of_election solutions within a short period of time, this may allow directly reaching ConfirmationDepth within a short sequence of consensus blocks. If all the consensus chain blocks required for reaching ConfirmationDepth are generated by the attacker (possible if the attacker is also operating a large farmer), honest operators will not be able to submit a fraud proof before the invalid domain execution receipt is considered final.

Risk

Attackers may be able to mint SSC tokens via invalid domain execution receipts.

Mitigation suggestion

Ensure that only one single submit_bundle call for a domain can be dispatched per consensus block. This will ensure that honest operators have at least 18 blocks to submit a fraud proof.

[nazar-pc]

We definitely expect to have multiple bundles per consensus block for the same domain, this is not a bug.

Also consensus moves forward even if it has invalid bundles in it. Operators will be punished, but the data will stay in consensus history forever.

Can you elaborate what is the issue here exactly?

[jakoblell]

We definitely expect to have multiple bundles per consensus block for the same domain, this is not a bug.

If having multiple bundles/calls to submit_bundle for the same domain within one single consensus block are to be expected, this can't directly be blocked. However in that case I'd recommend changing the meaning of ConfirmationDepth (which is used for determining if a message from a domain is to be accepted via is_domain_info_confirmed) so that it will require at least that number of blocks on the consensus chain.

Also consensus moves forward even if it has invalid bundles in it. Operators will be punished, but the data will stay in consensus history forever.

Can you elaborate what is the issue here exactly?

The security of the domain consensus system heavily depends on the ability of honest domain operators to submit a fraud proof before reaching ConfirmationDepth. If this number of domain bundles can be reached in a small number of consensus blocks, chances are that a malicious farmer can generate this sequence of consensus blocks without dispatching any fraud proofs, thus reaching ConfirmationDepth without giving honest domain operators any chance to submit a fraud proof before it is too late.

[nazar-pc]

it will require at least that number of blocks on the consensus chain

I'm not sure if we already have an issue for that, but it is definitely the case to make this transition, it just didn't happen yet implementation-wise.

[jfrank-summit]

I'm not sure if we already have an issue for that, but it is definitely the case to make this transition, it just didn't happen yet implementation-wise.

I believe #2289 is the relevant issue.

[dariolina]

I still believe this issue is a misunderstanding.

If this number of domain bundles can be reached in a small number of consensus blocks,

Every submit_bundle call does not directly extend the block tree and push domain blocks out of confirmation depth regardless of whether it is measured in consensus or domain blocks. Even if a domain submit bundles every second, and consensus blocks are produced every 6 slots it does not mean that domain chain grows at 6x the pace. All the bundles produced between two consensus blocks reference the SAME parent domain block so only cause the domain chain grow by 1, at the same pace (or slower) than consensus chain.
cc @NingLin-P

[NingLin-P]

All the bundles produced between two consensus blocks reference the SAME parent domain block so only cause the domain chain grow by 1, at the same pace (or slower) than consensus chain.

Right, if the consensus block doesn't contain bundle there will be no domain block produced, if there are bundles, no matter one or more, only one domain block will be produced. One domain block corresponds to one ER so there is only one valid ER available for the operator to submit with the bundle, even if there are multiple bundles in the next consensus block they all have the same ER.

One edge case is after fraud proof is submitted, some bad ERs will be pruned/reverted, so there will be multiple valid ERs available to submit. For the honest farmer, it will only accept the bundle with the same ER that extends the current head ER in its tx pool, so the consensus block produced by the honest farmer contains bundle with the same ER.

But because we don't have explicit checks during block execution to ensure all bundles have the same ER, it is possible for the malicious farmer to construct a block containing multiple bundles and each contains an ER that extends another ER in the block, essentially submitting multiple different ERs and forward the challenge period more than 1. In fact, we were plan to utilize this to handle #1673.

[jakoblell]

Maybe I'm missing something but in the code I don't see any strong protection against submitting multiple execution receipts with consecutive domain_block_number values in one single consensus chain block. These execution receipts are probably invalid but this doesn't matter if the attacker can submit so many of them within one single block that the first invalid execution receipt is already finalized before anybody can submit a fraud proof.

Should be possible to use the HeadReceiptExtended storage entry to ensure that the block tree can only be extended one single time per consensus chain block, as of now it is only checked at all when there are multiple submissions for the same domain_block_number but not for the case of multiple consecutive domain_block_number values above.

[vedhavyas]

@jakoblell This issue of operators being able to submit multiple consecutive bundles within the same consensus block does not exist.

To expand more on the flow of bundle submission, Once the bundle is submitted to the tx-pool of the consensus chain, since its an unsigned call, tx-pool does validate unsigned extrinsic here
Within the validate_bundle, we validate the execution receipt by comparing the current HeadReceipt + 1 with the domain_block_number from the Execution receipt. Since the HeadReceiptNumber is not updated to state during the tx-pool validate_unsigned check, we will only allow bundles that is carrying the ER with domain_block_number == HeadReceiptNumber + 1.

Assuming the latest HeadReceiptNumber is x
If multiple bundles are submitted with incrementing domain_block_number, x+1, x+2, x+3 etc... only the first bundle carrying domain_block_number x+1 will be accepted and rest will be rejected with InFuture error.

So for a given consensus block, there wont be any bundles with ER that consecutively extend the domain block more than 1.

Please confirm if I missed anything

[vedhavyas]

any thoughts on the above @jakoblell ?

[jakoblell]

@jakoblell This issue of operators being able to submit multiple consecutive bundles within the same consensus block does not exist.

To expand more on the flow of bundle submission, Once the bundle is submitted to the tx-pool of the consensus chain, since its an unsigned call, tx-pool does validate unsigned extrinsic here Within the validate_bundle, we validate the execution receipt by comparing the current HeadReceipt + 1 with the domain_block_number from the Execution receipt. Since the HeadReceiptNumber is not updated to state during the tx-pool validate_unsigned check, we will only allow bundles that is carrying the ER with domain_block_number == HeadReceiptNumber + 1.

This may be correct for the tx-pool validation but this doesn't matter if the attacker is producing the consensus chain block (which is a totally permissionless operation and just requires a valid proof of archival storage submission). In that case, the attacker can skip the tx pool validation and all other nodes will only run the validation via pre_dispatch during block execution - and at that time the HeadReceiptNumber may have already been updated via a previous submit_bundle call within the same consensus chain block.

Therefore I strongly recommend adding an extra on-chain check to ensure that HeadReceiptNumber can only be updated one single time per consensus block.

[vedhavyas]

Thank you for explanation

I have sent a PR to patch this here - #2790
Please have a look @jakoblell

[jakoblell]

Confirmed fixed in #2790, closing issue.