Skip to content

fix(nns): Prevent large manage neuron proposals #4509

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

jasonz-dfinity
Copy link
Contributor

@jasonz-dfinity jasonz-dfinity commented Mar 25, 2025

The changes in this PR is already deployed as part of a security hotfix:

Compared to the original commit, this PR also adds a CHANGELOG entry (not unreleased_changelog, since it's already released)

Why

Manage Neuron proposals have low fees (0.01 ICP) and high limit for open proposals (100K). If they can be large (~2MiB), then it's easy to fill up the wasm memory with very low cost.

What

Prevent large fields in Command

When validating the manage neuron proposals (validate_manage_neuron_proposal), simply disallow problematic commands:

  • MakeProposal can be large since they can (and are intended to) contain WASMs
  • DisburseMaturity isn't intended to contain large data, but it has an opt blob. There is generally lack of validation on the content of manage neuron proposals at the time the manage neuron proposals are created, but rather the commands are validated at execution time. This needs to be improved in the future. In this PR (intended as a hotfix), we aim at making the attack impossible with as few lines as possible. Disabling DisburseMaturity in manage neuron proposals is OK since the command is still under development.
  • Disburse/DisburseTo: they also have a blob (AccountIdentifier::hash), but they are not disabled if the neuron is not_for_profit, and only DFINITY neurons have this flag (at genesis)

Lower the limit of open proposals

Change the limit of open neuron management proposals from 100K to 10K

@jasonz-dfinity jasonz-dfinity requested a review from a team as a code owner March 25, 2025 17:23
@github-actions github-actions bot added the fix label Mar 25, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

  1. Done.

  2. No canister behavior changes.

@jasonz-dfinity jasonz-dfinity added this pull request to the merge queue Mar 26, 2025
Merged via the queue into master with commit 9ff99ef Mar 26, 2025
21 checks passed
@jasonz-dfinity jasonz-dfinity deleted the jason/NNS1-3656 branch March 26, 2025 19:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants