Prevent construct() from being called directly on implementation contracts#165
Prevent construct() from being called directly on implementation contracts#165
Conversation
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
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) (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) (45 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/5c6fb9d7068b0e88ea339f96a1baab206f68372e/src/mutability/Mutable.sol#L138-L143) has payable functions: - [MutablePauseTarget.fallback()](https://github.com/equilibria-xyz/root/blob/5c6fb9d7068b0e88ea339f96a1baab206f68372e/src/mutability/Mutable.sol#L139-L141) But does not have a function to withdraw the etherreentrancy-no-ethImpact: Medium
root/src/mutability/Mutator.sol Lines 34 to 46 in 5c6fb9d
root/src/mutability/Mutable.sol Lines 123 to 127 in 5c6fb9d
root/src/mutability/Mutable.sol Lines 97 to 113 in 5c6fb9d unused-returnImpact: Medium
root/src/token/types/Token6.sol Lines 43 to 45 in 5c6fb9d
root/src/distribution/Airdrop.sol Lines 46 to 51 in 5c6fb9d
root/src/token/types/Token.sol Lines 51 to 53 in 5c6fb9d
root/src/token/types/Token18.sol Lines 53 to 55 in 5c6fb9d
root/src/token/types/Token6.sol Lines 54 to 56 in 5c6fb9d
root/src/distribution/Airdrop.sol Lines 28 to 34 in 5c6fb9d
root/src/token/types/Token.sol Lines 40 to 42 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 34 to 46 in 5c6fb9d
root/src/token/types/Token18.sol Lines 43 to 45 in 5c6fb9d incorrect-modifierImpact: Low
root/src/attribute/Attribute.sol Lines 28 to 32 in 5c6fb9d calls-loopImpact: Low
root/src/mutability/Mutator.sol Lines 62 to 65 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 57 to 60 in 5c6fb9d reentrancy-benignImpact: Low
root/src/mutability/Mutator.sol Lines 57 to 60 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 62 to 65 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 34 to 46 in 5c6fb9d reentrancy-eventsImpact: Low
root/src/mutability/Mutable.sol Lines 116 to 120 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 62 to 65 in 5c6fb9d
root/src/mutability/Mutable.sol Lines 123 to 127 in 5c6fb9d
root/src/mutability/Mutator.sol Lines 57 to 60 in 5c6fb9d dead-codeImpact: Informational
root/src/number/types/UFixed18.sol Lines 288 to 290 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 295 to 297 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 295 to 297 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 287 to 289 in 5c6fb9d
root/src/mutability/Implementation.sol Lines 69 to 71 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 288 to 290 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 311 to 313 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 327 to 329 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 311 to 314 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 335 to 337 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 304 to 306 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 343 to 346 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 313 to 316 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 280 to 282 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 329 to 331 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 280 to 282 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 303 to 305 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 361 to 363 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 279 to 281 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 369 to 371 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 330 to 332 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 272 to 274 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 352 to 355 in 5c6fb9d
root/src/number/types/Fixed6.sol Lines 319 to 321 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 296 to 298 in 5c6fb9d
root/src/mutability/Implementation.sol Lines 74 to 76 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 312 to 314 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 320 to 323 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 345 to 348 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 336 to 339 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 362 to 364 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 320 to 322 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 264 to 266 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 303 to 305 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 304 to 307 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 322 to 324 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 271 to 273 in 5c6fb9d
root/src/mutability/Implementation.sol Lines 81 to 84 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 263 to 265 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 296 to 298 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 354 to 356 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 337 to 339 in 5c6fb9d
root/src/number/types/UFixed6.sol Lines 287 to 289 in 5c6fb9d
root/src/number/types/UFixed18.sol Lines 256 to 258 in 5c6fb9d
root/src/number/types/Fixed18.sol Lines 328 to 330 in 5c6fb9d solc-versionImpact: Informational
root/src/attribute/Attribute.sol Line 2 in 5c6fb9d
root/src/number/types/Fixed18.sol Line 2 in 5c6fb9d
root/src/vrgda/VRGDADecayMath.sol Line 2 in 5c6fb9d missing-inheritanceImpact: Informational
root/src/utils/OwnableStub.sol Lines 9 to 15 in 5c6fb9d
naming-conventionImpact: Informational
root/src/attribute/Pausable.sol Lines 34 to 36 in 5c6fb9d
root/src/mutability/Mutable.sol Lines 37 to 41 in 5c6fb9d
root/src/mutability/Mutable.sol Line 34 in 5c6fb9d
root/src/attribute/Ownable.sol Line 21 in 5c6fb9d
root/src/attribute/Ownable.sol Lines 42 to 44 in 5c6fb9d
root/src/mutability/Implementation.sol Line 22 in 5c6fb9d
root/src/attribute/Ownable.sol Lines 24 to 28 in 5c6fb9d
root/src/mutability/Implementation.sol Lines 25 to 29 in 5c6fb9d
root/src/attribute/Pausable.sol Line 23 in 5c6fb9d
root/src/attribute/Pausable.sol Lines 26 to 30 in 5c6fb9d
root/src/mutability/Implementation.sol Line 87 in 5c6fb9d
root/src/attribute/Attribute.sol Lines 20 to 24 in 5c6fb9d
root/src/attribute/Attribute.sol Line 17 in 5c6fb9d unimplemented-functionsImpact: Informational
root/src/attribute/Withdrawable.sol Lines 11 to 18 in 5c6fb9d
root/src/mutability/Implementation.sol Lines 13 to 88 in 5c6fb9d
root/src/attribute/Delegatable.sol Lines 12 to 20 in 5c6fb9d
root/src/attribute/Executable.sol Lines 12 to 21 in 5c6fb9d unindexed-event-addressImpact: Informational
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/attribute/Ownable.t.sol (1)
34-35: Extract this storage-slot write behind a shared helper/constant.This raw slot literal is now duplicated across several suites. If the
ImplementationStorageslot changes, these tests will start mutating the wrong word in a non-obvious way. Please centralize the slot in one test helper and call that instead of copying the magic value inline.♻️ Example cleanup
- vm.store(address(ownable), 0x3c57b102c533ff058ebe9a7c745178ce4174563553bb3edde7874874c532c200, bytes32(0)); + _clearConstructedFlag(address(ownable));bytes32 internal constant IMPLEMENTATION_STORAGE_SLOT = 0x3c57b102c533ff058ebe9a7c745178ce4174563553bb3edde7874874c532c200; function _clearConstructedFlag(address target) internal { vm.store(target, IMPLEMENTATION_STORAGE_SLOT, bytes32(0)); }test/attribute/Delegatable.t.sol (1)
40-41: LGTM! Consider using a shared constant for the storage slot.The storage manipulation correctly clears the
constructedflag to enable unit testing. Since the AI summary indicates multiple test files use the same pattern with this magic number, extractingImplementationStorageLocationinto a shared test helper or constant would reduce maintenance burden if the slot ever changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f4f30a7-b5fd-4ef8-a2ce-23107fe12f12
📒 Files selected for processing (10)
src/mutability/Implementation.solsrc/mutability/interfaces/IImplementation.soltest/attribute/Attribute.t.soltest/attribute/Delegatable.t.soltest/attribute/Executable.t.soltest/attribute/Ownable.t.soltest/attribute/Pausable.t.soltest/attribute/Withdrawable.t.soltest/mutability/Mutable.t.soltest/utils/OwnableStub.t.sol
66ff672 to
139a210
Compare
Unit Test Coverage ReportCoverage after merging sec-disable-impl-construct into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/attribute/Ownable.t.sol (1)
34-35: Extract this slot reset into a shared test helper.The same private ERC-7201 slot literal is now duplicated across multiple suites. Centralizing it in one helper would reduce drift and make it easier to swap these tests to a delegatecall-based harness later if you want them to follow production semantics more closely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d86074c0-d07a-4cfc-aa90-4e7aa1128954
📒 Files selected for processing (10)
src/mutability/Implementation.solsrc/mutability/interfaces/IImplementation.soltest/attribute/Attribute.t.soltest/attribute/Delegatable.t.soltest/attribute/Executable.t.soltest/attribute/Ownable.t.soltest/attribute/Pausable.t.soltest/attribute/Withdrawable.t.soltest/mutability/Mutable.t.soltest/utils/OwnableStub.t.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- test/utils/OwnableStub.t.sol
- test/attribute/Attribute.t.sol
- test/mutability/Mutable.t.sol
Sets a
constructedflag in the Implementation constructor's ERC-7201 storage slot, causing direct calls toconstruct()on the implementation to revert withImplementationAlreadyConstructedError. Proxy (delegatecall) usage is unaffected since the proxy's own storage is read.Ref: OZ
_disableInitializersNote
Medium Risk
Touches core upgrade/initialization plumbing by introducing a new constructed-lock in
Implementation, so mistakes could brick initialization if the flag is set or cleared incorrectly. Changes are small and covered by updated tests, but the area is security- and lifecycle-sensitive.Overview
Prevents calling
construct()directly on implementation contracts by adding a persistentconstructedflag inImplementation’s ERC-7201 storage, set during the implementation constructor via a new_disableInitializers()hook.construct()now reverts with the newImplementationAlreadyConstructedErrorwhen the implementation instance is already locked, while proxy/delegatecall flows continue to rely on the proxy’s own storage. Tests are updated to account for the lock (clearing the storage slot for direct unit tests) and a new regression test asserts directconstruct()on an implementation reverts.Written by Cursor Bugbot for commit 139a210. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests