integritee-network / worker

Integritee off-chain worker and sidechain validateer
Apache License 2.0
89 stars 47 forks source link

Verify if indirect invocation extrinsic has been successful on parentchain. #758

Open haerdib opened 2 years ago

haerdib commented 2 years ago

An extrinsic included in a parentchain block is currently executed without any checks. Hence, it will be executed on the worker side even if it was unsuccessful on the parentchain itself:

grafik See https://github.com/integritee-network/worker/runs/6558543131?check_suite_focus=true

Open question: how can we check for the success of an extrinsic? Ideally, without checking the parentchain state.

Thanks goes to @Gauthamastro for detecting this vulnerability

haerdib commented 2 years ago

https://polkadot.js.org/docs/api/cookbook/blocks/#how-do-i-determine-if-an-extrinsic-succeededfailed

Mapping extrinsic to its event: system.ExtrinsicSuccess and system.ExtrinsicFailed events.

// no blockHash is specified, so we retrieve the latest
const signedBlock = await api.rpc.chain.getBlock();

// get the api and events at a specific block
const apiAt = await api.at(signedBlock.block.header.hash);
const allRecords = await apiAt.query.system.events();

// map between the extrinsics and events
signedBlock.block.extrinsics.forEach(({ method: { method, section } }, index) => {
  allRecords
    // filter the specific events based on the phase and then the
    // index of our extrinsic in the block
    .filter(({ phase }) =>
      phase.isApplyExtrinsic &&
      phase.asApplyExtrinsic.eq(index)
    )
    // test the events against the specific types we are looking for
    .forEach(({ event }) => {
      if (api.events.system.ExtrinsicSuccess.is(event)) {
        // extract the data for this event
        // (In TS, because of the guard above, these will be typed)
        const [dispatchInfo] = event.data;

        console.log(`${section}.${method}:: ExtrinsicSuccess:: ${JSON.stringify(dispatchInfo.toHuman())}`);
      } else if (api.events.system.ExtrinsicFailed.is(event)) {
        // extract the data for this event
        const [dispatchError, dispatchInfo] = event.data;
        let errorInfo;

        // decode the error
        if (dispatchError.isModule) {
          // for module errors, we have the section indexed, lookup
          // (For specific known errors, we can also do a check against the
          // api.errors.<module>.<ErrorName>.is(dispatchError.asModule) guard)
          const decoded = api.registry.findMetaError(dispatchError.asModule);

          errorInfo = `${decoded.section}.${decoded.name}`;
        } else {
          // Other, CannotLookup, BadOrigin, no extra info
          errorInfo = dispatchError.toString();
        }

        console.log(`${section}.${method}:: ExtrinsicFailed:: ${errorInfo}`);
      }
    });
});

Simple enough. But how can we make that secure?

Interesting discussion: https://github.com/paritytech/substrate/discussions/10783

Gauthamastro commented 2 years ago

@haerdib In Polkadex, we created a storage value where successful transactions save messages in a vector, these can be relayed to enclave with storage read proofs. The vector in storage is cleared on every new block using on_initialize().

Enclave can verify the messages from blockchain using storage proofs.

haerdib commented 2 years ago

For now, the following ideas came to my mind :

First idea: Check the storage & it's proof before and after the extrinsic execution:

  1. Iterate through all parentchain extrinsic and pick the one we are interested in
  2. Get the relevant state from before the block has been executed
  3. Enclave calculates what the new state should look like if the extrinsics have been executed successfully
  4. Get the relevant state from after the block has been executed and compare to the should be storage.

For this to work, allowed extrinsic per block must be limited, otherwise one failed extrinsic might block the verification of another.

Second idea, as described by @Gauthamastro above, would be to add the successful ones to a temporary storage. But this needs the direct modification of the specific pallet that provides the extrinsic, I suppose? But it's less limiting and complicated than the state comparison.

Gauthamastro commented 2 years ago

Not necessarily, you can have the storage in teerex pallet ( in our case ocex) where other pallets can store the required data based on a standard.

For example

pub enum IngressMessages { Deposit(assetid, user, amount), GenericData(identifier, vec<u8>), .... }

haerdib commented 2 years ago

Solution proposal: