[Security] Remove unintended payable surface from Mutator.upgrade#161
[Security] Remove unintended payable surface from Mutator.upgrade#161
Conversation
Unit Test Coverage ReportCoverage after merging sec-f4-upgrade-payable-decision into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Slither reportStatic Analysis Report**THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [locked-ether](#locked-ether) (1 results) (Medium) - [reentrancy-no-eth](#reentrancy-no-eth) (2 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 [MutablePauseTarget](https://github.com/equilibria-xyz/root/blob/ed46bcb6e674558340a41710b3d1a8cc0d47af35/src/mutability/Mutable.sol#L138-L143) has payable functions: - [MutablePauseTarget.fallback()](https://github.com/equilibria-xyz/root/blob/ed46bcb6e674558340a41710b3d1a8cc0d47af35/src/mutability/Mutable.sol#L139-L141) But does not have a function to withdraw the etherreentrancy-no-ethImpact: Medium
root/src/mutability/Mutable.sol Lines 123 to 127 in ed46bcb
root/src/mutability/Mutable.sol Lines 97 to 113 in ed46bcb unused-returnImpact: Medium
root/src/token/types/Token6.sol Lines 43 to 45 in ed46bcb
root/src/distribution/Airdrop.sol Lines 46 to 51 in ed46bcb
root/src/token/types/Token.sol Lines 51 to 53 in ed46bcb
root/src/token/types/Token18.sol Lines 53 to 55 in ed46bcb
root/src/token/types/Token6.sol Lines 54 to 56 in ed46bcb
root/src/distribution/Airdrop.sol Lines 28 to 34 in ed46bcb
root/src/mutability/Mutator.sol Lines 34 to 43 in ed46bcb
root/src/token/types/Token.sol Lines 40 to 42 in ed46bcb
root/src/token/types/Token18.sol Lines 43 to 45 in ed46bcb incorrect-modifierImpact: Low
root/src/attribute/Attribute.sol Lines 28 to 32 in ed46bcb calls-loopImpact: Low
root/src/mutability/Mutator.sol Lines 54 to 57 in ed46bcb
root/src/mutability/Mutator.sol Lines 59 to 62 in ed46bcb reentrancy-benignImpact: Low
root/src/mutability/Mutator.sol Lines 59 to 62 in ed46bcb
root/src/mutability/Mutator.sol Lines 54 to 57 in ed46bcb
root/src/mutability/Mutator.sol Lines 34 to 43 in ed46bcb reentrancy-eventsImpact: Low
root/src/mutability/Mutator.sol Lines 54 to 57 in ed46bcb
root/src/mutability/Mutable.sol Lines 116 to 120 in ed46bcb
root/src/mutability/Mutator.sol Lines 59 to 62 in ed46bcb
root/src/mutability/Mutable.sol Lines 123 to 127 in ed46bcb dead-codeImpact: Informational
root/src/number/types/UFixed18.sol Lines 288 to 290 in ed46bcb
root/src/number/types/Fixed6.sol Lines 295 to 297 in ed46bcb
root/src/number/types/UFixed6.sol Lines 295 to 297 in ed46bcb
root/src/number/types/Fixed6.sol Lines 287 to 289 in ed46bcb
root/src/mutability/Implementation.sol Lines 66 to 68 in ed46bcb
root/src/number/types/Fixed18.sol Lines 288 to 290 in ed46bcb
root/src/number/types/Fixed6.sol Lines 311 to 313 in ed46bcb
root/src/number/types/Fixed6.sol Lines 327 to 329 in ed46bcb
root/src/number/types/UFixed6.sol Lines 311 to 314 in ed46bcb
root/src/number/types/Fixed6.sol Lines 335 to 337 in ed46bcb
root/src/number/types/Fixed18.sol Lines 304 to 306 in ed46bcb
root/src/number/types/Fixed6.sol Lines 343 to 346 in ed46bcb
root/src/number/types/UFixed18.sol Lines 313 to 316 in ed46bcb
root/src/number/types/UFixed18.sol Lines 280 to 282 in ed46bcb
root/src/number/types/UFixed6.sol Lines 329 to 331 in ed46bcb
root/src/number/types/Fixed18.sol Lines 280 to 282 in ed46bcb
root/src/number/types/Fixed6.sol Lines 303 to 305 in ed46bcb
root/src/number/types/Fixed6.sol Lines 361 to 363 in ed46bcb
root/src/number/types/UFixed6.sol Lines 279 to 281 in ed46bcb
root/src/number/types/Fixed6.sol Lines 369 to 371 in ed46bcb
root/src/number/types/UFixed18.sol Lines 330 to 332 in ed46bcb
root/src/number/types/UFixed18.sol Lines 272 to 274 in ed46bcb
root/src/number/types/Fixed6.sol Lines 352 to 355 in ed46bcb
root/src/number/types/Fixed6.sol Lines 319 to 321 in ed46bcb
root/src/number/types/UFixed18.sol Lines 296 to 298 in ed46bcb
root/src/mutability/Implementation.sol Lines 71 to 73 in ed46bcb
root/src/number/types/Fixed18.sol Lines 312 to 314 in ed46bcb
root/src/number/types/UFixed6.sol Lines 320 to 323 in ed46bcb
root/src/number/types/Fixed18.sol Lines 345 to 348 in ed46bcb
root/src/number/types/Fixed18.sol Lines 336 to 339 in ed46bcb
root/src/number/types/Fixed18.sol Lines 362 to 364 in ed46bcb
root/src/number/types/Fixed18.sol Lines 320 to 322 in ed46bcb
root/src/number/types/UFixed18.sol Lines 264 to 266 in ed46bcb
root/src/number/types/UFixed6.sol Lines 303 to 305 in ed46bcb
root/src/number/types/UFixed18.sol Lines 304 to 307 in ed46bcb
root/src/number/types/UFixed18.sol Lines 322 to 324 in ed46bcb
root/src/number/types/UFixed6.sol Lines 271 to 273 in ed46bcb
root/src/number/types/UFixed6.sol Lines 263 to 265 in ed46bcb
root/src/number/types/Fixed18.sol Lines 296 to 298 in ed46bcb
root/src/number/types/Fixed18.sol Lines 354 to 356 in ed46bcb
root/src/number/types/UFixed6.sol Lines 337 to 339 in ed46bcb
root/src/number/types/UFixed6.sol Lines 287 to 289 in ed46bcb
root/src/number/types/UFixed18.sol Lines 256 to 258 in ed46bcb
root/src/number/types/Fixed18.sol Lines 328 to 330 in ed46bcb solc-versionImpact: Informational
root/src/attribute/Attribute.sol Line 2 in ed46bcb
root/src/number/types/Fixed18.sol Line 2 in ed46bcb
root/src/vrgda/VRGDADecayMath.sol Line 2 in ed46bcb missing-inheritanceImpact: Informational
root/src/utils/OwnableStub.sol Lines 9 to 15 in ed46bcb
naming-conventionImpact: Informational
root/src/attribute/Pausable.sol Lines 34 to 36 in ed46bcb
root/src/mutability/Mutable.sol Lines 37 to 41 in ed46bcb
root/src/mutability/Mutable.sol Line 34 in ed46bcb
root/src/attribute/Ownable.sol Line 21 in ed46bcb
root/src/attribute/Ownable.sol Lines 42 to 44 in ed46bcb
root/src/mutability/Implementation.sol Line 21 in ed46bcb
root/src/attribute/Ownable.sol Lines 24 to 28 in ed46bcb
root/src/mutability/Implementation.sol Lines 24 to 28 in ed46bcb
root/src/attribute/Pausable.sol Line 23 in ed46bcb
root/src/attribute/Pausable.sol Lines 26 to 30 in ed46bcb
root/src/mutability/Implementation.sol Line 76 in ed46bcb
root/src/attribute/Attribute.sol Lines 20 to 24 in ed46bcb
root/src/attribute/Attribute.sol Line 17 in ed46bcb unimplemented-functionsImpact: Informational
root/src/attribute/Withdrawable.sol Lines 11 to 18 in ed46bcb
root/src/mutability/Implementation.sol Lines 13 to 77 in ed46bcb
root/src/attribute/Delegatable.sol Lines 12 to 20 in ed46bcb
root/src/attribute/Executable.sol Lines 12 to 21 in ed46bcb unindexed-event-addressImpact: Informational
|
Security report
What was broken
Mutator.upgradewas markedpayableeven though it did not forward value and had no ETH recovery path. This left accidental ETH-trap surface.Rationale review
Git history shows ETH forwarding on
upgradewas explicitly removed in commit84cf9fb("updates should not be payable"), indicating payable behavior is not intended in current design. #155.What was fixed
payablefromMutator.upgrade.payablefromIMutator.upgrade.test_revertsUpgradeWithValueconfirming ETH-bearing calls fail.Validation
forge test --match-path 'test/mutability/Mutator.t.sol' --match-test 'test_revertsUpgradeWithValue'forge test --match-path 'test/mutability/*.sol'Impact
Eliminates accidental ETH locking risk on upgrade entrypoint while preserving current upgrade semantics.