paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 639 forks source link

Initialize New Accounts with Nonce Equal to Block Number #326

Open shawntabrizi opened 3 years ago

shawntabrizi commented 3 years ago

There is a well known issue that Substrate accounts which are killed could have their txs replayed if the lifetime is infinite.

To help mitigate this, we want to initialize a user's nonce with the number of the current block.

This means that assuming that an account does on average less than one tx per block, that none of it's transactions could be replayed even if the account was killed.

This can also be used as a mechanism to determine how old an account is (relatively). For example, we could start a democracy vote where only users with nonce lower than the current block number could vote. This means that it would not be possible for someone to create a bunch of new accounts after the vote is announced to then influence the outcome.

It would of course be possible to create a bunch of sleeper accounts initialized with a low nonce, but such activity could be monitored and accounted for before the vote takes place. Anyway, this second half is just an idea...

kevlu93 commented 3 years ago

I would be interested in helping out! New to the code base though so would need some pointers to get started.

xlc commented 3 years ago

This helps but immortal tx with nonce of 0 is still going to be replayable

stale[bot] commented 3 years ago

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Lohann commented 2 years ago

Can I take this one? I just faced this issue trying to figure why two transactions in different blocks have the exactly same hash and signature, block explores rely in the tx_hash as unique transaction identifier.

Tx hash: 0x313da69888b43486a5bf717958d02e7228467fa48654c8bd690fe38b56370113

https://polkascan.io/polkadot/block/7911545 https://polkascan.io/polkadot/block/7854172

shawntabrizi commented 2 years ago

That problem is not the same thing as mentioned in this issue.

A tx hash is literally a hash of the tx input data, and can be the same if the same inputs are used AFAIK.

The unique identifier for a tx is (block number, extrinsic index).

Lohann commented 2 years ago

Correct, they have the same hash because the account reuse the same nonce and call the same extrinsic twice. You can see here that the account 13Z4eAVmUt4we1K623cFgYHMnFSDC4cmuJUrGTuUEbZRuyf3 was reaped and created many times: https://polkascan.io/polkadot/account/13Z4eAVmUt4we1K623cFgYHMnFSDC4cmuJUrGTuUEbZRuyf3#balance-history

This is very counter intuitive, once in all other blockchain I know the transaction hash is global unique (bitcoin, ethereum, cardano, etc), using block_number + extrinsic_index as unique identifier is not very usual and should be better documented. Also I don't think the block explorers are aware about this issue, if you use polkascan for example and click in the transaction, it will display the same block instead show the tx in different blocks.

Lohann commented 2 years ago

In my opinion, is better to disable the Pallet::<T>::on_killed_account(who.clone()) functionality in the System Pallet until this issue if fixed, then create a migration which deletes all accounts without providers in the future. Because I consider the risk o replay attack worst than the risk of state bloating.

https://github.com/paritytech/substrate/blob/polkadot-v0.9.13/frame/system/src/lib.rs#L1080-L1085

I don't think people are aware that if their account is deleted then re-created, they can suffer replay attacks.

Lohann commented 2 years ago

I noticed that fixing the nonce initialization is not trivial:

So my suggestion is doing a band-aid solution to prevent replay attacks, maybe something like this:

frame/system/src/lib.rs#L1080-L1085

(1, 0, 0) => {
    // No providers left (and no consumers) and no sufficients. Account dead.

    if account.nonce == 0 {
        Pallet::<T>::on_killed_account(who.clone());
        Ok(DecRefStatus::Reaped)
    } else {
        // Mark the account to be reaped in a future migration
    }
},
shawntabrizi commented 2 years ago

same hash because the account reuse the same nonce and call the same extrinsic twice

This is not accurate. The tx hash does not include the nonce of the user or the account. Any two accounts will generate the same tx hash with the same tx and inputs. It is literally the hash of the tx bytes, not a unique identifier.

I don't think people are aware that if their account is deleted then re-created, they can suffer replay attacks.

This is a completely documented and known thing with functions already built into polkadot to prevent it.

All extrinsics have a configurable transaction lifetime or mortality. You can learn more about that here: https://wiki.polkadot.network/docs/build-protocol-info#transaction-mortality

Transaction hash is different than a transaction payload + signature, which could look the same if an immortal transaction is generated. But as noted in our documentation and done by all the APIs, this is not recommended to do ever.

shawntabrizi commented 2 years ago

I am not sure what to respond here. It is a part of the protocol, I am sure you and I both don't know every detail of Polkadot, however this is a feature which exists and when used correctly, can prevent tx replays.

Every wallet creator worth their salt in the Polkadot ecosystem should definitely make sure to provide only mortal transactions by default.

The document I provide above talks about both transaction mortality, and also mentions the misconception about transaction hash. I recommend you read the full document, then re-engage into the conversation.