Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize New Accounts with Nonce Equal to Block Number #326

Open
shawntabrizi opened this issue Mar 25, 2021 · 10 comments
Open

Initialize New Accounts with Nonce Equal to Block Number #326

shawntabrizi opened this issue Mar 25, 2021 · 10 comments
Assignees
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@shawntabrizi
Copy link
Contributor

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
Copy link

kevlu93 commented Mar 25, 2021

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

@xlc
Copy link
Contributor

xlc commented Mar 31, 2021

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

@stale
Copy link

stale bot commented Jul 7, 2021

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.

@stale stale bot added the A5-stale label Jul 7, 2021
@Lohann
Copy link

Lohann commented Dec 2, 2021

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

@stale stale bot removed the A5-stale label Dec 2, 2021
@shawntabrizi
Copy link
Contributor Author

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
Copy link

Lohann commented Dec 2, 2021

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
Copy link

Lohann commented Dec 2, 2021

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
Copy link

Lohann commented Dec 2, 2021

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
Copy link
Contributor Author

shawntabrizi commented Dec 2, 2021

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
Copy link
Contributor Author

shawntabrizi commented Dec 2, 2021

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.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed Z6-mentor labels Aug 25, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
* Correctly handle re-org in mapping sync

* Rename sync_one_level -> sync_one_block

* Use while let syntax to remove an expect unwrap
@ggwpez ggwpez added the I5-enhancement An additional feature request. label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

8 participants