Use the current block number as the nonce of the new account

[NingLin-P]

There is a potential replay attack as mentioned in the upstream doc, where after an account is reaped and later recreated, its nonce is reset to 0, and the previously signed transaction can be replayed if it is immortal or not yet expired since the nonce matched.

This PR fixes the issue by using the current block number as the nonce of the new account, so the previously signed transaction will never match the nonce of the new account.

TODO: I added a unit to ensure the encode/decode of Nonce and TypeWithDefault is the same so we don't need to worry about compatibility with Mainnet and Taurus, but I will test it in a local dev network to double check.

Code contributor checklist:

• I have read, understood and followed contributing guide

[vedhavyas]

Looks good but lets move the DefaultNonceProvider to runtime primitives so we dont have to re-define it everywhere

[vedhavyas]

Thinking on this more, the is core substrate problem so shouldn't this be automatically handled in the upstream instead ?

[teor2345]

This seems like a useful fix, but I’m not sure what the exact security constraints are. Let’s document them somewhere?

[teor2345]

Is this a probabilistic defence, or a guaranteed defence?
Is it unlikely that an account/transaction could have a nonce greater than or equal to the block number, or is it impossible? Or do we need to add one to the default to make it impossible?

Let’s document the exact security constraints somewhere, because the linked document doesn’t go into that level of detail. Maybe in the protocol specs?

Also, do we need to document the security risks of skipping nonces (or throwing away a lot of draft transactions)?

[NingLin-P]

Is this a probabilistic defence, or a guaranteed defence?

It is a guaranteed defense.

Is it unlikely that an account/transaction could have a nonce greater than or equal to the block number, or is it impossible? Or do we need to add one to the default to make it impossible?

It is possible to have a nonce greater than the block number, this PR only changes the default nonce of newly created account. In main, the nonce of a new account will start with 0, with this PR, it will start with current_block_number, and the next nonce is current_block_number + 1, current_block_number + 2, ..., etc.

Let’s document the exact security constraints somewhere, because the linked document doesn’t go into that level of detail. Maybe in the protocol specs?

Yeah, I will update the protocol spec 👍

Also, do we need to document the security risks of skipping nonces (or throwing away a lot of draft transactions)?

Actually, I don't have any security risk in mind. This PR doesn't change the nonce of the existing account or add filter to reject transaction with nonce less than the block number, it only change the default nonce value of the new account. When drafting a new transaction, the user/tool/wallet usually should fetch the account's current nonce onchain to fill the transaction, this PR should not break this process.

But if there are tools/programs that internally transfer funds to create a new account and then draft transaction of the new account without querying its nonce onchain but simply assuming its nonce is start with 0, this kind of scenario will be broken...

[teor2345]

Is this a probabilistic defence, or a guaranteed defence?

It is a guaranteed defense.

Is it unlikely that an account/transaction could have a nonce greater than or equal to the block number, or is it impossible? Or do we need to add one to the default to make it impossible?

It is possible to have a nonce greater than the block number, this PR only changes the default nonce of newly created account. In main, the nonce of a new account will start with 0, with this PR, it will start with current_block_number, and the next nonce is current_block_number + 1, current_block_number + 2, ..., etc.

So this defence won't always work?

For example, in this situation:

1. the account's nonce X is currently greater than the block number
2. it gets reaped
3. it gets re-created with nonce current_block_number
4. all transactions with nonces between current_block_number and X can be replayed

Also, do we need to document the security risks of skipping nonces (or throwing away a lot of draft transactions)?

Actually, I don't have any security risk in mind. This PR doesn't change the nonce of the existing account or add filter to reject transaction with nonce less than the block number, it only change the default nonce value of the new account. When drafting a new transaction, the user/tool/wallet usually should fetch the account's current nonce onchain to fill the transaction, this PR should not break this process.

The situation I described above is more likely if a tool does not query the current nonce, and the tool/user makes a lot of transactions, using up nonces faster than blocks are created.

Does this need user/developer security documentation?

But if there are tools/programs that internally transfer funds to create a new account and then draft transaction of the new account without querying its nonce onchain but simply assuming its nonce is start with 0, this kind of scenario will be broken...

Does this need user/developer documentation?

[NingLin-P]

For example, in this situation

Indeed, this situation can still happen but it is even harder to trigger though.

Does this need user/developer security documentation?
Does this need user/developer documentation?

Yes, if we decided to take this approach, then we definitely need to document out this behavior change.

[NingLin-P]

Thinking on this more, the is core substrate problem so shouldn't this be automatically handled in the upstream instead ?

While upstream has outlined the issue already in their doc, I'm not sure why they did not fix it, but my guess is

• It is not a critical issue and requires several conditions to trigger
• The fix will change the behavior, where the nonce does not start with 0, which may not be expected for all substrate-based chain
• Anyone who wants to fix the issue can plugin the fix like we did in this PR

[vedhavyas]

It is not a critical issue and requires several conditions to trigger

Can we not replicate the same conditions on our chain as well ?

The fix will change the behavior, where the nonce does not start with 0, which may not be expected for all substrate-based chain

This is also a main concern of mine since for

• EVM there is no reaping of accounts, so nonce should always be the last nonce but setting the current block height might break somethings
• Same goes with Substrate but not to an extent I believe.

Here is an alternative solution I have been thinking if we are unable to replicate same conditions as polkadot
Maybe when an account is reaped, we can store nonce for that account and then re-use it when account is back active again?
Just essentially replace block_number with last nonce but I'm not sure if we can pass specific account info to achieve this

Or maybe we can also enforce only mortal transactions though I believe the problem still exists. Very curious what the polkadot conditions are that makes this not a critical issue 🤔

[NingLin-P]

Can we not replicate the same conditions on our chain as well ?

Emmm... please see https://wiki.polkadot.network/docs/transaction-attacks#replay-attack for what conditions are required to trigger the issue, and I think the issue still exists in polkadot, the reason I guess it is not a critical issue for polkadot is it is a known but won't fix issue.

Maybe when an account is reaped, we can store nonce for that account and then re-use it when account is back active again?

This essentially means storing permanent state in the runtime and defeat the purpose of ED, the attacker can easily bloat up the state by continuously transferring funds in and out of an account with the cost of some tx fee.

The issue and solution were brought by the audit team let's continue the discussion in Slack with them.