involontary replay attack when unshielding

[brenzi]

unshielding suffers from replay attacks, even without bad intentions:

If more than one worker is registered for a shard, these workers all send the unshield_funds(account, amount, shard) extrinsic. The pallet has no way to know that these are results of the same TrustedCallSigned::unshield() (which suffers another replay attack until we solve #89)

Of course, an adversary could also expoit this replay attack.

Mitigation: keep track of all confirmed TrustedCallSigned.

The substratee_registry::confirm_call(<TrustedCallSigned>) keeps a registry of confirmed calls. This registry could be a storage_map(Hash) -> u32 and counts the number of confirmations N per call.

If N > 0, we don't unshield because it has already been done

We might merge the confirm_call and unshield_funds extrinsic into one for that purpose. No need to spend two transactions for a single purpose

Please update the book too

[clangenb]

In fact, we need to merge the unshield and the confirm_call extrinsic. Otherwise, we mitigate nothing as the worker could simply withhold the confirm_call extrinsic.

[brenzi]

Thoughts on keeping a registry of unconfirmed calls instead:

• what happens with failing TrustedCalls? throw a way after N blocks?
• could save state space because it's not ever-growing
  might actually be a better idea....

[clangenb]

In fact, we need to merge the unshield and the confirm_call extrinsic. Otherwise, we mitigate nothing as the worker could simply withhold the confirm_call extrinsic.

The current implementation partly solves the above mentioned issue of having two separate extrinsics, as the enclave checks the inclusion of all enclave generated extrinsics.

[clangenb]

I would be in favour of keeping track of unconfirmed calls instead. I think it is more elegant and it does not matter for the client because:

• In both cases the client will see that a call is unconfirmed either because it is in the list of unconfirmed extrinsics or because we have the number of confirmations == 0.
• In both cases the client will receive an CallConfirmed event.

Idea: How about emitting an event upon throwing away an unconfirmed call?

[brenzi]

no need for that additional event I think. Go for it! register unconfirmed calls along with their block number!
We can do the purge logic later as part of on_initialize() (at beginning of block generation)

[clangenb]

Has been fixed with the pallet-substratee-registry update in #147.

As it was more straight forward to implement, we currently keep track of confirmed extrinsics instead of unconfirmed extrinsics.