Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

[Contracts] Overflowing bounded DeletionQueue #13702

Merged
paritytech-processbot merged 48 commits into from
Mar 31, 2023
Merged

[Contracts] Overflowing bounded DeletionQueue #13702

paritytech-processbot merged 48 commits into from
Mar 31, 2023

Conversation

@pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Mar 24, 2023 •
edited by athei
Loading

Currently the DeletionQueue is limited to DeletionQueueDepth, which is set to 128 for the test runtime. If more contracts are terminated within a short time, further attempts to terminate a contract will fail. If a public contract with an in-contract governance system is supposed to be terminated, an attacker can prevent this by terminating 128 fake contracts shortly before that. This will make the self-termination fail (and prevent freeing up the account balance + storage deposit of the contract). Launching another attempt to self-terminate may require a significant effort (e.g. votes from hundreds of stakeholders via the self-governance of the contract), which may be impractical, especially after multiple failed attempts.

This PR updates the storage to use a StorageMap instead of the current Bounded Vec used to keep track of contracts marked for deletion.

cumulus companion: paritytech/cumulus#2409

@pgherveou pgherveou requested a review from athei as a code owner March 24, 2023 08:37
@pgherveou pgherveou marked this pull request as draft March 24, 2023 08:37
@pgherveou pgherveou changed the title [Contracts review] Overflowing bounded DeletionQueue allows DoS against contract termination [Contracts] Overflowing bounded DeletionQueue allows DoS against contract termination Mar 24, 2023
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
@pgherveou pgherveou marked this pull request as ready for review March 27, 2023 12:33
@pgherveou pgherveou added the A0-please_review Pull request needs code review. label Mar 27, 2023
pgherveou
pgherveou commented Mar 27, 2023
View reviewed changes
athei
athei reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather superficial first round of review:

Let's move all the deletion queue stuff into the storage file. I want to prevent cluttering the lib file.

For the migration: We should not add a migration in this PR. We should:

  1. Merge this PR without migration
  2. Wait for #13638
  3. Create PR that adds the migration for this PR and the missing migration for #13369 as a single version 10.

@pgherveou pgherveou added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. C1-low PR touches the given topic and has a low impact on builders. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 28, 2023
@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 30, 2023

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 30, 2023

Error: "Check reviews" status is not passing for paritytech/cumulus#2409

@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 30, 2023

bot rebase

Merge remote-tracking branch 'origin/master' into pg/contracts-review…
ef324ec 
…-overflowing-bounded-deletionqueue-allows-dos-against-contract-termination
@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 30, 2023

Rebased

@command-bot
Copy link

command-bot bot commented Mar 30, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2619291 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2619291/artifacts/download.

@athei
Copy link
Member

athei commented Mar 31, 2023

bot rebase

Merge remote-tracking branch 'origin/master' into pg/contracts-review…
7d39202 
…-overflowing-bounded-deletionqueue-allows-dos-against-contract-termination
@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 31, 2023

Rebased

@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 31, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit de4cca4 into master Mar 31, 2023
@paritytech-processbot paritytech-processbot bot deleted the pg/contracts-review-overflowing-bounded-deletionqueue-allows-dos-against-contract-termination branch March 31, 2023 11:03
@pgherveou pgherveou changed the title [Contracts] Overflowing bounded DeletionQueue allows DoS against contract termination [Contracts] Overflowing bounded DeletionQueue Mar 31, 2023
@athei athei mentioned this pull request May 4, 2023
Merged
@Polkadot-Forum
Copy link

Polkadot-Forum commented May 11, 2023

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-41-v0-9-42/2828/1

@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@valentinfernandez1 valentinfernandez1 mentioned this pull request Jun 23, 2023
Closed
7 tasks
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
@CanonicalJP CanonicalJP mentioned this pull request Oct 23, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@agryaznov agryaznov Awaiting requested review from agryaznov

@koute koute Awaiting requested review from koute

2 more reviewers

@athei athei athei approved these changes

@juangirini juangirini juangirini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pgherveou @athei @Polkadot-Forum @juangirini @bkchr