SmartContract: restrict the number of allowed notifications by AnnaShaleva · Pull Request #3548 · neo-project/neo

AnnaShaleva · 2024-10-23T16:10:17Z

Description

The problem is described in nspcc-dev/neo-go#3490. In short words, an arbitrary number of smart contract notifications is allowed (up to VM's GasLimit and MaxBlockSystemFee, but it's huge), which results in a possibility to DoS the node. 10K of large notifications emitted in a single transaction result in block acceptance delays for both Go and C# nodes. 50K of large notifications may result in the node failure. I did these tests on privnet, you may find the results in nspcc-dev/neo-go#3490 (comment). Please, don't try to DoS mainnet/testnet using this vulnerability.

Port the solution from nspcc-dev/neo-go#3640, but an alternative solution is described in nspcc-dev/neo-go#3640 (comment) and in nspcc-dev/neo-go#3490 (comment). MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. nspcc-dev/neo-go#3490 (comment).

This PR is a subject of discussion, hence please, share your thoughts on the described problem and proposed solution. Once we agree on the solution, I'll add unit-tests to this PR.

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Unit-tests should be added once we agree on solution.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Fix the problem described in nspcc-dev/neo-go#3490. Port the solution from nspcc-dev/neo-go#3640. MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. nspcc-dev/neo-go#3490 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

cschuchardt88 · 2024-10-23T16:20:24Z

This needs to be in VM limit class.

vncoelho · 2024-10-23T16:21:53Z

src/Neo/SmartContract/Native/RoleManagement.cs

            list.AddRange(nodes);
            list.Sort();
            engine.SnapshotCache.Add(key, new StorageItem(list));
-            


Formatting job is failing without this change.

AnnaShaleva · 2024-10-23T16:22:11Z

This needs to be in VM limit class.

Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.

vncoelho · 2024-10-23T16:22:57Z

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

+            // Restrict the number of notifications for Application executions. Do not check
+            // persisting triggers to avoid native persist failure. Do not check verification
+            // trigger since verification context is loaded with ReadOnly flag.
+            if (IsHardforkEnabled(Hardfork.HF_Echidna) && Trigger == TriggerType.Application && notifications.Count == MaxNotificationCount)


Looks good to me, we just need to ensure .Count is correct calculated at this step.

Is trigger really needed? Limits shouldn't have a trigger.

Yes, it's needed, because we can't allow OnPersist and PostPersist failures (imagine that in future MaxNotificationCount may be reduced to some lower value). And for Verification trigger this check is useless by design, no notifications are possible with this trigger.

Ref. nspcc-dev/neo-go#3490 (comment).

cschuchardt88 · 2024-10-23T17:12:27Z

This needs to be in VM limit class.

Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.

All the same thing. Class is called ExecutionEngineLimits in Neo.VM namespace. How to use engine.Limit.MaxNotification

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

Jim8y · 2024-10-24T09:31:27Z

instead of limiting number of notifactions, i prefer to make the price related to the notifaction size. From @AnnaShaleva 's excellent benchmark, i think size * number is the real reason. Cause i think its a normal practice contract may need to send a lof of notifactions, but definately abnormal to send large notifactions.

AnnaShaleva · 2024-10-24T11:00:30Z

i think size * number is the real reason

Exactly, as described in nspcc-dev/neo-go#3490 (comment).

i prefer to make the price related to the notifaction size.

Yes, it's an alternative solution proposed in nspcc-dev/neo-go#3490 (comment). But we need to discuss it because even with extremely expensive notifications there's a chance that CNs will be killed by a single transaction. Yes, the user will have to pay for it, but CNs may not be able to process this transaction. Also, large number of notifications is harmful for both C# and Go RPC servers.

So to me, the restriction of the overall notifications number is required, but at the same time it can be combined with dynamic price solution.

The base branch was changed.

shargon · 2025-01-24T08:22:39Z

src/Neo.CLI/config.fs.mainnet.json

      "HF_Cockatrice": 5800000,
-      "HF_Domovoi": 5800000
+      "HF_Domovoi": 5800000,
+      "HF_Echidna": 5800001


@AnnaShaleva Should be removed from config

It comes from the branch change

Ping @AnnaShaleva

Will update.

src/Neo/SmartContract/Native/RoleManagement.cs

shargon · 2025-03-03T10:24:45Z

This should be included in 3.8

AnnaShaleva · 2025-03-03T11:15:50Z

Previously we’ve agreed that this solution is less preferable than #3600. @roman-khimov do you think we need to merge it?

shargon · 2025-03-03T12:03:17Z

Previously we’ve agreed that this solution is less preferable than #3600. @roman-khimov do you think we need to merge it?

Why not complementary?

roman-khimov · 2025-03-03T12:32:26Z

The theory is that if we're to have #3552 done with MaxBlockSystemFee of like 20 and then have ExecutionFeeFactor of 30 again then 20 GAS is ~2034 notifications and we're fine as is. The problem is that we neither have a low MaxBlockSystemFee value nor we have a high ExecutionFeeFactor (and can't have it till #3600, which will happen no one knows when). Given the severity of the issue at the same time, I'm OK with some hard limit for 3.8/Echidna. At least it's simple and effective.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2025-03-03T13:46:52Z

Good, updated. Then let's consider merging this PR in 3.8.

I'm also voting for the merge since I agree that this solution is better than nothing. Later, when we have lower MaxBlockSystemFee and flexible opcode prices, we may revert notifications number constraint (in some next hardfork).

shargon · 2025-03-04T09:38:04Z

@neo-project/core please review

…ect#3548) * add hardofork HF_Echidna * Add entries to `Designation` event (neo-project#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (neo-project#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * add hardofork HF_Echidna * Add entries to `Designation` event (neo-project#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (neo-project#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * SmartContract: restrict the number of allowed notifications Fix the problem described in nspcc-dev/neo-go#3490. Port the solution from nspcc-dev/neo-go#3640. MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. nspcc-dev/neo-go#3490 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: fix format Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Update src/Neo/SmartContract/ApplicationEngine.Runtime.cs * Avoid notification creation * add hardofork HF_Echidna * Add entries to `Designation` event (neo-project#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (neo-project#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * format * Fixed typo * Added back neo-project#3397 * fix format * Update src/Neo/SmartContract/Native/RoleManagement.cs * Neo.CLI: revert configuration changes Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

Jimmy and others added 8 commits August 8, 2024 00:24

add hardofork HF_Echidna

bcc5fd0

Add entries to Designation event (#3397)

e63f10b

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

[Neo Core StdLib] Add Base64url (#3453)

9d16c53

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

add hardofork HF_Echidna

f746f8d

Add entries to Designation event (#3397)

8d7f9e8

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

[Neo Core StdLib] Add Base64url (#3453)

3fc8077

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

AnnaShaleva added Discussion Initial issue state - proposed but not yet accepted Waiting for Review Hardfork labels Oct 23, 2024

AnnaShaleva requested review from Jim8y, cschuchardt88, roman-khimov, shargon and vncoelho October 23, 2024 16:10

AnnaShaleva mentioned this pull request Oct 23, 2024

【Oct.24】Core Dev Regular Meeting Proposal Discussion #3547

Closed

SmartContract: fix format

0c72e57

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva mentioned this pull request Oct 23, 2024

core: restrict the number of allowed SC notifications nspcc-dev/neo-go#3640

Merged

vncoelho reviewed Oct 23, 2024

View reviewed changes

shargon reviewed Oct 23, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Outdated Show resolved Hide resolved

Update src/Neo/SmartContract/ApplicationEngine.Runtime.cs

ee21427

shargon reviewed Oct 23, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Show resolved Hide resolved

Avoid notification creation

4a6f36a

shargon reviewed Jan 24, 2025

View reviewed changes

Merge branch 'master' into restrict-notifications-count

e5e3b08

shargon reviewed Jan 24, 2025

View reviewed changes

src/Neo/SmartContract/Native/RoleManagement.cs Outdated Show resolved Hide resolved

Update src/Neo/SmartContract/Native/RoleManagement.cs

f43f296

shargon added the Bug Used to tag confirmed bugs label Mar 3, 2025

AnnaShaleva force-pushed the restrict-notifications-count branch 2 times, most recently from 8dd5b25 to 7345ac4 Compare March 3, 2025 13:32

Neo.CLI: revert configuration changes

c309f63

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva force-pushed the restrict-notifications-count branch from 7345ac4 to c309f63 Compare March 3, 2025 13:32

Merge branch 'master' into restrict-notifications-count

f5a0482

shargon approved these changes Mar 3, 2025

View reviewed changes

AnnaShaleva requested review from cschuchardt88, nan01ab and vncoelho March 3, 2025 15:46

Merge branch 'master' into restrict-notifications-count

564a033

shargon added Ready to Merge and removed Discussion Initial issue state - proposed but not yet accepted labels Mar 4, 2025

Wi1l-B0t approved these changes Mar 4, 2025

View reviewed changes

Merge branch 'master' into restrict-notifications-count

3c40720

NGDAdmin merged commit 6c9312a into master Mar 5, 2025
6 checks passed

NGDAdmin deleted the restrict-notifications-count branch March 5, 2025 08:28

Conversation

AnnaShaleva commented Oct 23, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

cschuchardt88 commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jim8y commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shargon commented Mar 3, 2025

Uh oh!

AnnaShaleva commented Mar 3, 2025

Uh oh!

shargon commented Mar 3, 2025

Uh oh!

roman-khimov commented Mar 3, 2025

Uh oh!

AnnaShaleva commented Mar 3, 2025

Uh oh!

shargon commented Mar 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AnnaShaleva Oct 23, 2024 •

edited

Loading

cschuchardt88 Oct 23, 2024 •

edited

Loading

cschuchardt88 commented Oct 23, 2024 •

edited

Loading

Jim8y commented Oct 24, 2024 •

edited

Loading

AnnaShaleva commented Oct 24, 2024 •

edited

Loading