Consider extrinsic status when executing indirect calls

[Kailai-Wang]

We noticed this problem in https://github.com/litentry/litentry-parachain/issues/1092

Quote:

As far as I experimented, the IndirectCallsExecutor simply iterates over all extrinsics in a parentchain block and handles them, without checking the extrinsic status.

I intentionally misconfigured the parameter so that the extrinsic failed and threw out an error, but this extrinsic was still parsed. This seems incorrect.

We also have a workaround for this: https://github.com/litentry/litentry-parachain/pull/1193

A bit hacky though as we wanted to have as few changes as possible. We append a status flag to the found extrinsics during parentchain syncing and parse it in the indirect-calls-executor, with customised encoding/decoding of OpaqueExtrinsics:

// `indirect_calls_executor` looks for extrinsics in parentchain blocks without checking their status.
// We believe it's wrong, see https://github.com/litentry/litentry-parachain/issues/1092
//
// One solution is to change the block type to `sp_runtime::generic::Block` or `sp_runtime::generic::SignedBlock`,
// this will however affect many structs or files, like block_importer, block_import_dispatcher, consensus and so on.
//
// We use a hacky workaround here for the least possible changes:
// append an extra `status` flag to `OpaqueExtrinsic` (Vec<u8>) and adjust the codec of it
//
// We intentionally don't drop failed extrinsics to allow any potential (post-)processing of failed extrinsics

Please let us know if you are happy with it, if so we can submit a PR.

[clangenb]

Yeah, we definitely want to consider extrinsic statuses of parentchain extrinsics, happy to merge a PR! I will formulate some thoughts for this PR here.

I had problems understanding the comment:

// One solution is to change the block type to sp_runtime::generic::Block or sp_runtime::generic::SignedBlock, // this will however affect many structs or files, like block_importer, block_import_dispatcher, consensus and so on.

But, I think I get it now, it is because you silently change the data format that the OpaqueExtrinsic contains, sometimes it contains the xt_status, and sometimes it doesn't. This is indeed be a bit dangerous in the sense of: it needs to be documented well, so we know exactly where OpaqueExtrinsic contains the tx status, and where it doesn't.

Security issue: However, there is definitely one security issue that needs to be adressed:

// In the service, in the untrusted world.
for block in &block_chunk_to_sync {
    let block_events = self.parentchain_api.events(Some(block.block.hash()))?;
    events.push(block_events);
}

Here, we fetch the events of a particular block with a storage query. However, we don't check the corresponding storage proof. Hence, a malicious operator could modify the untrusted function that syncs the parentchain blocks, and do the following:

• feed in the correct parentchain blocks will be validated in the light-client and accepted because they are correct.
• create some fake events that might tell that an extrinsic failed even though it was successful. -> some state will be reverted in the worker (e.g. balance refunded) and you can do the same game over and over again and increase some onchain balance whereas the sidechain balance does not get drained.

To mitigate this, we need to pass in the full event list into the enclave to verify corresponding storage proof inside the enclave, and do the filtering for the events that we care about inside the enclave. So to add some pseudocode about what needs to be done to get the storage proofs:

let events_storage_key = storage_key(blalbal);
let events = self.parentchain_api.events(Some(block_hash));
let events_proof = self.parentchain_api.storage_proof(events_storage_key);

// we need to sync the blocks, events, events_proof together into the enclave
// and do the necessary filtering inside.

Also, in the future, we might want to detect, when the worker is still syncing the parentchain because then it won't care about the events. #1215 might help here, but I consider this a future problem.

[clangenb]

Also, I think I would like to extend the description maybe:

pub struct OpaqueExtrinsicWithStatus {
    pub xt: OpaqueExtrinsic,
    pub status: XtStatus,
}

// I think we can skip the dispatch info.
pub enum XtStatus {
  ExtrinsicSuccess,
  ExtrinsicFailed { dispatch_error: DispatchError },
}

[Kailai-Wang]

Thanks @clangenb ! We definitely haven't forgotten this issue and understand your concerns. However, we have been focusing on the product launch recently and didn't have time to look further into it

The potential risk isn't a big problem for us as of now because we are the only worker operators. But I strongly agree it should be solved later, we'll note this down and come back to this after the product launch.

[coax1d]

Closed by #1272