[Security] Prevent duplicate mutable name collisions in Mutator.create#159
[Security] Prevent duplicate mutable name collisions in Mutator.create#159
Conversation
Slither reportStatic Analysis Report**THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [locked-ether](#locked-ether) (2 results) (Medium) - [reentrancy-no-eth](#reentrancy-no-eth) (3 results) (Medium) - [unused-return](#unused-return) (9 results) (Medium) - [incorrect-modifier](#incorrect-modifier) (1 results) (Low) - [calls-loop](#calls-loop) (2 results) (Low) - [reentrancy-benign](#reentrancy-benign) (3 results) (Low) - [reentrancy-events](#reentrancy-events) (4 results) (Low) - [dead-code](#dead-code) (44 results) (Informational) - [solc-version](#solc-version) (3 results) (Informational) - [missing-inheritance](#missing-inheritance) (2 results) (Informational) - [naming-convention](#naming-convention) (13 results) (Informational) - [unimplemented-functions](#unimplemented-functions) (4 results) (Informational) - [unindexed-event-address](#unindexed-event-address) (1 results) (Informational) ## locked-ether Impact: Medium Confidence: High - [ ] ID-0 Contract locking ether found: Contract [Mutator](https://github.com/equilibria-xyz/root/blob/f5103537c84304fd4b0182c63b967a4b8d2bfbcb/src/mutability/Mutator.sol#L13-L66) has payable functions: - [IMutator.upgrade(IImplementation,bytes)](https://github.com/equilibria-xyz/root/blob/f5103537c84304fd4b0182c63b967a4b8d2bfbcb/src/mutability/interfaces/IMutator.sol#L18) - [Mutator.upgrade(IImplementation,bytes)](https://github.com/equilibria-xyz/root/blob/f5103537c84304fd4b0182c63b967a4b8d2bfbcb/src/mutability/Mutator.sol#L51-L55) But does not have a function to withdraw the etherroot/src/mutability/Mutator.sol Lines 13 to 66 in f510353
reentrancy-no-ethImpact: Medium
root/src/mutability/Mutator.sol Lines 34 to 46 in f510353
root/src/mutability/Mutable.sol Lines 123 to 127 in f510353
root/src/mutability/Mutable.sol Lines 97 to 113 in f510353 unused-returnImpact: Medium
root/src/token/types/Token6.sol Lines 43 to 45 in f510353
root/src/distribution/Airdrop.sol Lines 46 to 51 in f510353
root/src/token/types/Token.sol Lines 51 to 53 in f510353
root/src/token/types/Token18.sol Lines 53 to 55 in f510353
root/src/token/types/Token6.sol Lines 54 to 56 in f510353
root/src/distribution/Airdrop.sol Lines 28 to 34 in f510353
root/src/token/types/Token.sol Lines 40 to 42 in f510353
root/src/mutability/Mutator.sol Lines 34 to 46 in f510353
root/src/token/types/Token18.sol Lines 43 to 45 in f510353 incorrect-modifierImpact: Low
root/src/attribute/Attribute.sol Lines 28 to 32 in f510353 calls-loopImpact: Low
root/src/mutability/Mutator.sol Lines 62 to 65 in f510353
root/src/mutability/Mutator.sol Lines 57 to 60 in f510353 reentrancy-benignImpact: Low
root/src/mutability/Mutator.sol Lines 57 to 60 in f510353
root/src/mutability/Mutator.sol Lines 62 to 65 in f510353
root/src/mutability/Mutator.sol Lines 34 to 46 in f510353 reentrancy-eventsImpact: Low
root/src/mutability/Mutable.sol Lines 116 to 120 in f510353
root/src/mutability/Mutator.sol Lines 62 to 65 in f510353
root/src/mutability/Mutable.sol Lines 123 to 127 in f510353
root/src/mutability/Mutator.sol Lines 57 to 60 in f510353 dead-codeImpact: Informational
root/src/number/types/UFixed18.sol Lines 288 to 290 in f510353
root/src/number/types/Fixed6.sol Lines 295 to 297 in f510353
root/src/number/types/UFixed6.sol Lines 295 to 297 in f510353
root/src/number/types/Fixed6.sol Lines 287 to 289 in f510353
root/src/mutability/Implementation.sol Lines 66 to 68 in f510353
root/src/number/types/Fixed18.sol Lines 288 to 290 in f510353
root/src/number/types/Fixed6.sol Lines 311 to 313 in f510353
root/src/number/types/Fixed6.sol Lines 327 to 329 in f510353
root/src/number/types/UFixed6.sol Lines 311 to 314 in f510353
root/src/number/types/Fixed6.sol Lines 335 to 337 in f510353
root/src/number/types/Fixed18.sol Lines 304 to 306 in f510353
root/src/number/types/Fixed6.sol Lines 343 to 346 in f510353
root/src/number/types/UFixed18.sol Lines 313 to 316 in f510353
root/src/number/types/UFixed18.sol Lines 280 to 282 in f510353
root/src/number/types/UFixed6.sol Lines 329 to 331 in f510353
root/src/number/types/Fixed18.sol Lines 280 to 282 in f510353
root/src/number/types/Fixed6.sol Lines 303 to 305 in f510353
root/src/number/types/Fixed6.sol Lines 361 to 363 in f510353
root/src/number/types/UFixed6.sol Lines 279 to 281 in f510353
root/src/number/types/Fixed6.sol Lines 369 to 371 in f510353
root/src/number/types/UFixed18.sol Lines 330 to 332 in f510353
root/src/number/types/UFixed18.sol Lines 272 to 274 in f510353
root/src/number/types/Fixed6.sol Lines 352 to 355 in f510353
root/src/number/types/Fixed6.sol Lines 319 to 321 in f510353
root/src/number/types/UFixed18.sol Lines 296 to 298 in f510353
root/src/mutability/Implementation.sol Lines 71 to 73 in f510353
root/src/number/types/Fixed18.sol Lines 312 to 314 in f510353
root/src/number/types/UFixed6.sol Lines 320 to 323 in f510353
root/src/number/types/Fixed18.sol Lines 345 to 348 in f510353
root/src/number/types/Fixed18.sol Lines 336 to 339 in f510353
root/src/number/types/Fixed18.sol Lines 362 to 364 in f510353
root/src/number/types/Fixed18.sol Lines 320 to 322 in f510353
root/src/number/types/UFixed18.sol Lines 264 to 266 in f510353
root/src/number/types/UFixed6.sol Lines 303 to 305 in f510353
root/src/number/types/UFixed18.sol Lines 304 to 307 in f510353
root/src/number/types/UFixed18.sol Lines 322 to 324 in f510353
root/src/number/types/UFixed6.sol Lines 271 to 273 in f510353
root/src/number/types/UFixed6.sol Lines 263 to 265 in f510353
root/src/number/types/Fixed18.sol Lines 296 to 298 in f510353
root/src/number/types/Fixed18.sol Lines 354 to 356 in f510353
root/src/number/types/UFixed6.sol Lines 337 to 339 in f510353
root/src/number/types/UFixed6.sol Lines 287 to 289 in f510353
root/src/number/types/UFixed18.sol Lines 256 to 258 in f510353
root/src/number/types/Fixed18.sol Lines 328 to 330 in f510353 solc-versionImpact: Informational
root/src/attribute/Attribute.sol Line 2 in f510353
root/src/number/types/Fixed18.sol Line 2 in f510353
root/src/vrgda/VRGDADecayMath.sol Line 2 in f510353 missing-inheritanceImpact: Informational
root/src/utils/OwnableStub.sol Lines 9 to 15 in f510353
naming-conventionImpact: Informational
root/src/attribute/Pausable.sol Lines 34 to 36 in f510353
root/src/mutability/Mutable.sol Lines 37 to 41 in f510353
root/src/mutability/Mutable.sol Line 34 in f510353
root/src/attribute/Ownable.sol Line 21 in f510353
root/src/attribute/Ownable.sol Lines 42 to 44 in f510353
root/src/mutability/Implementation.sol Line 21 in f510353
root/src/attribute/Ownable.sol Lines 24 to 28 in f510353
root/src/mutability/Implementation.sol Lines 24 to 28 in f510353
root/src/attribute/Pausable.sol Line 23 in f510353
root/src/attribute/Pausable.sol Lines 26 to 30 in f510353
root/src/mutability/Implementation.sol Line 76 in f510353
root/src/attribute/Attribute.sol Lines 20 to 24 in f510353
root/src/attribute/Attribute.sol Line 17 in f510353 unimplemented-functionsImpact: Informational
root/src/attribute/Withdrawable.sol Lines 11 to 18 in f510353
root/src/mutability/Implementation.sol Lines 13 to 77 in f510353
root/src/attribute/Delegatable.sol Lines 12 to 20 in f510353
root/src/attribute/Executable.sol Lines 12 to 21 in f510353 unindexed-event-addressImpact: Informational
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a security/operational footgun in the mutability system by preventing Mutator.create from deploying multiple mutables that share the same implementation.name(), which previously overwrote _nameToMutable and could orphan earlier mutables from future upgrades.
Changes:
- Add a new
MutatorMutableAlreadyExists()custom error toIMutator. - Add a duplicate-name guard in
Mutator.createbefore deploying a newMutable. - Add a regression test ensuring
createreverts when attempting to deploy a second mutable with an already-registered name.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/mutability/Mutator.sol |
Prevents duplicate mutable-name registrations by reverting before deployment and mapping update. |
src/mutability/interfaces/IMutator.sol |
Exposes a dedicated custom error for duplicate-name creation attempts. |
test/mutability/Mutator.t.sol |
Adds a regression test validating the revert on duplicate mutable name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Unit Test Coverage ReportCoverage after merging sec-f1-duplicate-name-brick into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Security report
What was broken
Mutator.createallowed deploying multiple mutables with the sameimplementation.name(). The_nameToMutablemapping was overwritten, so upgrades would target only the latest mutable and older ones could become upgrade-orphaned.What was fixed
MutatorMutableAlreadyExists()error toIMutator.Mutator.createbefore deployment.test_revertsOnDuplicateMutableName.Validation
forge test --match-path 'test/mutability/Mutator.t.sol' --match-test 'test_revertsOnDuplicateMutableName'forge test --match-path 'test/mutability/*.sol'Impact
Prevents accidental/procedural bricking of upgrade paths caused by name collisions.
Note
Low Risk
Small, localized change to mutable creation semantics plus a new revert case; low blast radius and covered by a targeted test.
Overview
Prevents deploying multiple mutables with the same
implementation.name()by adding a pre-deploy guard inMutator.createthat reverts when a name is already registered.Exposes the new
MutatorMutableAlreadyExists()error viaIMutatorand adds a regression test (test_revertsOnDuplicateMutableName) to ensure duplicate-name creation fails.Written by Cursor Bugbot for commit 48aff1d. This will update automatically on new commits. Configure here.