Pallets where there is no deposit charged for filling up the storage

[conectado]

Problem

It was found during the audit that any forum user can use the forum's extrinsic create_thread to fill up the blockchain storage and exhaust the number of threads of a given category for a small fee since this requires no deposit.

This problem exists in two forms across the runtime: Either we allow with no deposit filling up the storage of a given module with no bound or with bounds.

In the bounded case, this would allow a denial of service attack where other users won't be able to use the storage.

In the unbounded case, this'd allow to grow the storage maliciously.

There are different permission levels that'd allow a user to abuse these extrinsics, either a lead or a worker/member.

When a lead can fill up a bounded storage is not really a problem because there is no other user that it can be denied of service but I leave those cases in here just to keep them in mind.

Pallets

• forum

  • create_thread: Any forum user can create a thread, the thread storage for a given category is bounded.
  • add_post: Any forum user can create a post, the post storage for a given thread is bounded.
  • create_category: The lead can create categories with no cost, the category storage is bounded. (Same goes for sub-categories)

• working_group

  • add_opening: A lead can use this extrinsic to create openings unbounded.
  • apply_on_opening: A working group member can apply multiple times, filling blockchain storage(when no stakes) unbounded.
  • add_opening + apply_on_opening + fill_opening: A lead can add an opening with no stakes, apply on it multiple times and fill it, and occupy completely the worker storage.

• membership

  • add_staking_account_candidate: A user could use multiple accounts to fill up the blockchain storage, unbounded.

• proposals_discussion

  • add_post: A member can add posts to any thread, unbounded.

• storage/data_directory

  • add_content: A member can use it to fill up the storage unbounded.

• content_directory

  • add_curator_group: A lead can add groups to fill up the storage unbounded.
  • add_curator_to_group: A lead can add curators to a group to fill it up bounded.

• service_discovery:

  • set_ipns_id: Any worker can use it to fill up storage unbounded.

• data_object_type_registry

  • register_data_object_type: A lead can use it to fill up storage unbounded.

[shamil-gadelshin]

Working group leaders have their stake locked. And it could be slashed any time by a council.

[bedeho]

• So certainly lets skip the last four major bullet points here,because they are going way.
• The forum is really tricky, because cleanup up after a thread will become quite expensive, as each post is a separate storage entry, so this could be hundreds of iterations. I also do not think it makes sense to have each poster cleanup each post individually, that would take too much coordination and wasted block space. The thread author is the best positioned person to decide when a thread is truly "done", and then to re

It seems the most practical way would be to something like

struct Thread {
posts: BTreeMap<PostId, Post>,
cleanup_payoff: Number /// <== we could also require posters to contribute to this with each post, so that bigger objects have greater reason to be cleaned up? but we must be careful, posting should be cheap... so tread carefully on this.
}

Now this object just grows to become big, within the given thread size bound, and the cleanup is constant time operation. SRlabs pointed out that even quite large reads/writes have negligible impact on benchmark, if any.

• I thought proposal discussions are auto cleaned up when proposal is executed?

[shamil-gadelshin]

I thought proposal discussions are auto cleaned up when proposal is executed?

Correct!

[conectado]

Working group leaders have their stake locked. And it could be slashed any time by a council.

Isn't it still risky to have the storage filled up for a long time until a council member is alerted of this problem?

I thought proposal discussions are auto cleaned up when proposal is executed?

However, it could be abused as long as the proposal is not executed or rejected and for some proposals I believe that could be a long time especially if it's pending constitution.

[conectado]

For the working group to prevent some user to apply multiple times we could check that the account hasn't applied for the same opening already.

[conectado]

Proposed mitigations(For non-lead problems):

forum

As @bedeho proposed we can have this struct for the thread we can have:

struct Thread {
posts: BTreeMap<PostId, Post>,
cleanup_payoff: Number
}

cleanup_payoff would be contributed by all the posters preventing spam and making an incentive to cleanup the thread.

The staking associated with creating the thread would also function as an anti-spam mechanism for the thread creation.

working_group

Checking that a user can't apply twice would prevent apply_on_opening to fill the storage.

This is not urgent since the storage would be cleaned up after the lead chose the workers but someone can use big amount of space during this period for a very small fee.

membership

As long as there is an existential deposit there would be a cost associated with having an account that would prevent add_staking_account_candidate from being abused. If the existential deposit isn't enough we could require an additional minimum balance.

proposals_discussion

As pointed out by @bedeho this would be mitigated by the thread being removed when the proposal is resolved. However, during that span of time it allows a user for a small fee to occupy a lot of a storage, to prevent this we could require a stake to post in a thread that would be released when the thread is deleted.

[bedeho]

• working_group
  • Make sure to make the check membership based, not account based.
  • Just force the minimum stake to be some non-zero amount then, for all proposals. Specifically, and as in all of these cases, it has to be no less than transaction fee of the recover transaction to be effective.
• proposals_discussion
  • Should we change the datamodel to be the same as the new one in the forum? why or why not? it would certainly reduce the computational load of cleanup up a proposal discussion.
  • Where would the money go? To the proposer, or is it burned? It wont be feasible to iterate all discussion participants and pay them back, that will be too many database writes.
• blog: isn't this part of Olympia also?

In terms of how this should be implemented, I suggest the same pattern as @shamil-gadelshin is using in the bounty module: a module specific account.

[conectado]

* `proposals_discussion`
  
  * Should we change the datamodel to be the same as the new one in the forum? why or why not? it would certainly reduce the computational load of cleanup up a proposal discussion.

The drawback of using that model seems to be that adding or updating a post requires to read/write the whole thread and as we talked about how much that impacts performance when a thread grows in size requires careful evaluation.

  * Where would the money go? To the proposer, or is it burned? It wont be feasible to iterate all discussion participants and pay them back, that will be too many database writes.

Maybe we could burn it? Since we don't need to incentivize the deletion of the thread because that's done automatically when a proposal decision is reached.

blog: isn't this part of Olympia also?

But since everything that is changed in the blog is done through proposals it already requires stakes to prevent any kind of spam and also it goes through the voting process.

[shamil-gadelshin]

FYI: PostById from the forum is a double map. It is deleted using remove_prefix by thread_id. It is still an iteration but cheaper than kill one by one.

[bedeho]

FYI: PostById from the forum is a double map. It is deleted using remove_prefix by thread_id. It is still an iteration but cheaper than kill one by one.

No it is the same, I asked Shawn about this.

The drawback of using that model seems to be that adding or updating a post requires to read/write the whole thread and as we talked about how much that impacts performance when a thread grows in size requires careful evaluation.

Lets get some numbers for that asap then, because this will impact many modules going forward.

[conectado]

membership

As long as there is an existential deposit there would be a cost associated with having an account that would prevent add_staking_account_candidate from being abused. If the existential deposit isn't enough we could require an additional minimum balance.

As it turns out the existential deposit might be too low, even lower than than the transaction fee which might be insufficient to prevent an attack from filling up the storage.

As a solution we could add a fixed amount of tokens to be burned from the staking account candidate when storing it.

[conectado]

Working group

add_opening: While this function can be abused only by the leader we still want to prevent a malicious leader to fill up the storage before the council can take action. I think if we charge a deposit for adding an opening that can disincentivize from creating a new opening. So we could limit the number of simultaneously active opening to something like MaxWorkerNumber * 2 or MaxWorkerNumber + MAX_OPENINGS. (Note: this was also suggested in Joystream/audits#12)

[shamil-gadelshin]

Fixing this issue will result in adding several new "deposits value" settings.
We have already acquired a lot of different deposits, stakes, costs, pay-offs, etc., bound to different pallets. It is difficult to manage them all in case of inflation and/or new token issuance.

What if we start binding new prices to some well-known constant (let's say CENTS that we already have)?

We could easily update all the prices changing only a single constant, and won't lose tuning flexibility for individual setting (price). We could start with new values and then gradually migrate the rest of the system. The only downside I can see - we lose "pretty values" using the automatic calculation (it's a minor issue, and we could fix it using a simple rounding function). @bedeho @conectado

[conectado]

What if we start binding new prices to some well-known constant (let's say CENTS that we already have)?

We could easily update all the prices changing only a single constant, and won't lose tuning flexibility for individual setting (price). We could start with new values and then gradually migrate the rest of the system. The only downside I can see - we lose "pretty values" using the automatic calculation (it's a minor issue, and we could fix it using a simple rounding function). @bedeho @conectado

This sounds good, to fine tune each individual setting we would have different multiplier for each deposit?

One downside from this approach is that you can't adjust the value for the weight to fee constant(assuming it depends on the value of CENTS) without changing the value of the deposits but this could be a desired property since those values should be related.

[shamil-gadelshin]

This sounds good, to fine tune each individual setting we would have different multiplier for each deposit?

One downside from this approach is that you can't adjust the value for the weight to fee constant(assuming it depends on the value of CENTS) without changing the value of the deposits but this could be a desired property since those values should be related.

Yes, you are correct.

[conectado]

* `proposals_discussion`
  
  * Should we change the datamodel to be the same as the new one in the forum? why or why not? it would certainly reduce the computational load of cleanup up a proposal discussion.

To check the viability of this model I created #2234 to benchmark DB weights to see what changes we should or shouldn't make to the weight function to account for the bigger storage size for a thread record.

  Where would the money go? To the proposer, or is it burned? It wont be feasible to iterate all discussion participants and pay them back, that will be too many database writes.

Since there's no need to incentivize the deletion of a thread we can burn the deposit.

[conectado]

membership

As long as there is an existential deposit there would be a cost associated with having an account that would prevent add_staking_account_candidate from being abused. If the existential deposit isn't enough we could require an additional minimum balance.

As it turns out the existential deposit might be too low, even lower than than the transaction fee which might be insufficient to prevent an attack from filling up the storage.

As a solution we could add a fixed amount of tokens to be burned from the staking account candidate when storing it.

Working group

add_opening: While this function can be abused only by the leader we still want to prevent a malicious leader to fill up the storage before the council can take action. I think if we charge a deposit for adding an opening that can disincentivize from creating a new opening. So we could limit the number of simultaneously active opening to something like MaxWorkerNumber * 2 or MaxWorkerNumber + MAX_OPENINGS. (Note: this was also suggested in Joystream/audits#12)

We decided to solve both of these problem by requiring both extrinsics to have a minimum stake. This way we can use that stake as an anti-spam mechanism and as an incentive to cleanup working group's openings or membership's staking account candidates.

The council, for the sake of simplicity, won't have a lock, we trust that since a council member that wants to abuse the add_opening extrinsic to bloat the state has to go through the voting system making such an attack much easier to detect early and harder to fulfill.

[conectado]

Proposal Discussion & Forum pallet posts

Proposed mitigations(For non-lead problems):

forum

As @bedeho proposed we can have this struct for the thread we can have:

struct Thread {
posts: BTreeMap<PostId, Post>,
cleanup_payoff: Number
}

cleanup_payoff would be contributed by all the posters preventing spam and making an incentive to cleanup the thread.

* `proposals_discussion`
  
  * Should we change the datamodel to be the same as the new one in the forum? why or why not? it would certainly reduce the computational load of cleanup up a proposal discussion.
  * Where would the money go? To the proposer, or is it burned? It wont be feasible to iterate all discussion participants and pay them back, that will be too many database writes.

In #2234 analyzed the cost of saving a big object such as having a Thread with all the Post in the storage with regards to write/reads. We realized that there is a clear correlation between an object size and time for read or write if we follow down this path.

This means that if we do this we should add certain complexity to the weight functions of those extrinsics(or in general) to take that into account, the different ways we could do that are discussed in the ticket.

We decided to use another way to deal with the problems in proposal_discussion and forum that doesn't neeed to modify the weight functions.

The model we decided to use is the following:

• Each post is either editable or non-editable, decided in the moment of creation
• A non-editable post doesn't need to be stored, so it isn't.
• An editable post is stored, that requires a fee.
• Once a post is deleted the same fee is paid to the deleter.
• Multiple posts will be possible to delete with a single extrinsic call.
• The creator of a post can delete it.
• After a certain number of blocks after a post is last edited/creatad anyone can delete it. As long as the thread doesn't exists.

We expect that:

• Since there is a fee for a post that use the storage there isn't any vector for an attacker to fill the storage.
• Most posts will probably be non-editable, since they are cheaper, preventing the cleanup of a thread's posts from occupying too much block weight.
• That there is an incentive to delete a post after a thread close to recover the fee.
• Even if a post creator doesn't keep track of all their posts, an advanced user can cleanup multiple old undeleted posts to recover their fee, in that way we have more chances of cleanup.
• Deleting a thread is cheap since we don't need to cleanup the posts when doing so.

[shamil-gadelshin]

Fixed.