Flag storage prefixes that collide with an inherited base's reserved prefix#1773
Merged
shargon merged 11 commits intoJun 26, 2026
Merged
Conversation
…prefix NC4056 (StorageKeyCollisionAnalyzer) only compared a contract's own StorageMap/LocalStorageMap prefixes against each other, so it missed the more common footgun: a contract that inherits Ownable or Pausable and then builds a StorageMap with that base class's reserved prefix. Ownable stores its owner under 0xFF and Pausable its paused flag under 0xFE, so a derived contract that reuses those prefixes silently shares the base's storage namespace and can corrupt the owner or paused state with no diagnostic. Seed the per-class prefix set with the reserved prefixes of any inherited framework base (Ownable 0xFF, Pausable 0xFE) before scanning the contract's own StorageMap fields, so the existing collision check fires when a derived contract reuses one. Reuses the existing NC4056 rule; the message names the base class whose prefix is being reused. A contract that does not inherit the base is unaffected, so there are no false positives. Adds tests for both reserved prefixes plus negatives (a non-inheriting contract using 0xFF, and an inheriting contract using a different prefix).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master-n3 #1773 +/- ##
==========================================
Coverage 82.70% 82.70%
==========================================
Files 305 305
Lines 27076 27076
Branches 3769 3769
==========================================
Hits 22394 22394
Misses 3530 3530
Partials 1152 1152 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ajara87
previously approved these changes
Jun 24, 2026
shargon
reviewed
Jun 24, 2026
shargon
left a comment
shargon
left a comment
Member
There was a problem hiding this comment.
I love it, could you add also Tokens nep17 and 11?
Contributor
Author
|
Added the reserved prefixes for Ownable2Step, PausableOwnable, AccessControl, and RoyaltyNep11Token too. |
ajara87
approved these changes
Jun 24, 2026
shargon
approved these changes
Jun 24, 2026
Wi1l-B0t
reviewed
Jun 24, 2026
| ["global::Neo.SmartContract.Framework.AccessControl"] = [0xFB], | ||
| ["global::Neo.SmartContract.Framework.TokenContract"] = [0x00, 0x01], | ||
| ["global::Neo.SmartContract.Framework.Nep17Token"] = [0x00, 0x01], | ||
| ["global::Neo.SmartContract.Framework.Nep11Token`1"] = [0x02, 0x03, 0x04], |
Contributor
There was a problem hiding this comment.
Why 'Nep11Token`1', not 'Nep11Token' ?
Contributor
Author
There was a problem hiding this comment.
It uses Roslyn's MetadataName. Generic types include their arity there, so Nep11Token is Nep11Token`1.
Wi1l-B0t
approved these changes
Jun 26, 2026
Member
|
@shargon could you merge? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NC4056(StorageKeyCollisionAnalyzer) only compared a contract's ownStorageMap/LocalStorageMapprefixes against each other, so it missed the more common footgun: a contract that inheritsOwnableorPausableand then builds aStorageMapwith that base class's reserved prefix.Ownablestores its owner under0xFFandPausableits paused flag under0xFE. A derived contract that reuses those prefixes silently shares the base's storage namespace — corrupting the stored owner or paused state — with no diagnostic today.How
Before scanning the contract's own
StorageMapfields, seed the per-class prefix set with the reserved prefixes of any inherited framework base (Ownable→0xFF,Pausable→0xFE). The existing collision check then fires when a derived contract reuses one. This reuses the existingNC4056rule (no new diagnostic id); the message names the base class whose prefix is being reused.A contract that does not inherit the base is unaffected and may use any prefix, so there are no false positives.
Testing
Adds positive tests for both reserved prefixes and negatives: a non-inheriting contract using
0xFF(no warning) and an inheriting contract using a different prefix (no warning). Full analyzer suite green (171).