Can tasks (experimental) and `AuthorizeCall` open transaction pool spam attack, by crafting different transactions that are valid.

[gui1117]

Context: Authorize call, tasks

AuthorizeCall is a transaction extension meant to replace the usage of ValidateUnsigned by allowing authorize an extrinsic solely based on its call.

AuthorizeCall is placed first in the transaction extension pipeline, when a transaction is validated AuthorizeCall will validate the call and transmute the origin to Origin::Authorized.
Because later extension in the transaction extension pipeline are mostly ignored, the user can choose different value, this creates different transaction for the same authorized call.

Tasks is an experimental feature, there is a ValidateUnsigned logic validating the call frame_system::Call::do_task { task}, if the task is valid then the transaction is valid.
If a task contains a parameter that allows multiple valid value (like a limit of number of item to migrate) then choosing different value create different transaction for the same task.

Potential attack

So these 2 cases given a valid call, somebody can create multiple transactions that is actually encoding one valid operation. The attacker generate millions of such transactions, send them, the node will receive them and given they are all valid they will hold them in their transaction pool.

When building a block, one transaction will be included, making all those duplicate invalid. At this moment the transaction pool is free again.

Another situation is when such call is marking the validity of the transaction as: Invalid::Future. Then when receiving the million transactions the node will hold the transactions for the future, and this will saturate the pool? I don't really know how future is handled in the pool.

Solutions

There is no issue with tasks if none of its parameter allow to encode the same kind of task with a different call. (to illusatrate we can imagine the task: migrate_storage(limit: u32), this task would be considered harmful and must never be written, instead the task should be migrate_storage and hardcode some limit). If this is important we must make it clear in the doc.
This is even trickier given parity-scale-codec doesn't enforce a single valid encoding for one type. E.g. BTreeSet, can be encoded in multiple way and the decoding will be valid. If such type is used in a task, then there is as many possible encoding of this task as there is factorial of the number of element possible encoding.

For AuthorizeCall, if the attack is legit, then AuthorizeCall is not a replacement for unsigned and must not be used. Because transaction extensions like CheckMortality allows for 2^16 transactions for the same call, CheckNonce allows for 2^32 transactions for the same call (nonce being skipped for the origin Origin::Authorized, any nonce is valid).
So maybe AuthorizeCall would have to be reworked.

[gui1117]

In the transaction pool transactions are identified by the tag they provide, not their hash. I think this answers the question, there is probably no issue, I will think a bit longer and probably try to put this information more visible in the doc.

[bkchr]

The transaction pool is distinguishing transactions based on the provides tags. The tags means it is the same transaction (even if the hash is different).

So, for AuthorizeCall it depends on the authorize logic that it presents secure provides tags. While these things like authorize make it seem "easy" to write transaction validation, the truth is that you still need to use your brain or you going to get dosed :)

For Task we need to change the validation logic to check the source to come from the local node and reject all the other transactions. This should be the easiest solution.