-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor TrailsRouter to accept Multicall3 address as a constructor argument #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
88d6545
4a2588a
be3f011
a0193b2
230b419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,10 +21,18 @@ contract TrailsRouter is IDelegatedExtension, ITrailsRouter, DelegatecallGuard, | |||||
| using SafeERC20 for IERC20; | ||||||
|
|
||||||
| // ------------------------------------------------------------------------- | ||||||
| // Immutable Variables | ||||||
| // State Variables | ||||||
| // ------------------------------------------------------------------------- | ||||||
|
|
||||||
| address public immutable MULTICALL3 = 0xcA11bde05977b3631167028862bE2a173976CA11; | ||||||
| address public MULTICALL3; | ||||||
|
||||||
| address public MULTICALL3; | |
| address public immutable MULTICALL3; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor parameter should accept a immutable assignment instead of a mutable state variable. With immutable variables, the address is encoded in the deployed bytecode rather than stored in contract storage. This is critical for a contract primarily used via delegatecall, as it prevents storage slot conflicts with the calling contract. Consider: address public immutable MULTICALL3; constructor(address _multicall3) { MULTICALL3 = _multicall3; }
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation: The constructor should validate that _multicall3 is not the zero address to prevent deployment with an invalid multicall address. Consider adding: if (_multicall3 == address(0)) revert InvalidMulticall3Address(); This pattern is already used in TrailsRouterShim (see TrailsRouterShim.sol:34).
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -253,7 +253,7 @@ contract TrailsRouterShimTest is Test { | |||||||||
|
|
||||||||||
| // Verify sentinel by re-etching TrailsRouter and validating via delegated entrypoint | ||||||||||
| bytes memory original = address(shimImpl).code; | ||||||||||
| vm.etch(holder, address(new TrailsRouter()).code); | ||||||||||
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); | ||||||||||
|
||||||||||
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); | |
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); | |
| // Set MULTICALL3 storage slot (slot 0) to the correct address after etching | |
| vm.store(holder, bytes32(uint256(0)), bytes32(uint256(uint160(0xcA11bde05977b3631167028862bE2a173976CA11)))); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical bug: vm.etch only copies runtime bytecode, not storage. Since MULTICALL3 is now a storage variable (not immutable), it will be uninitialized (address(0)) at the holder address after this etch operation. Any subsequent operations that rely on MULTICALL3 will fail. The original code worked because immutable variables are embedded in the bytecode itself. Either revert MULTICALL3 to immutable, or explicitly set the storage slot after etching: vm.store(holder, bytes32(uint256(0)), bytes32(uint256(uint160(0xcA11bde05977b3631167028862bE2a173976CA11))));
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); | |
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); | |
| // Explicitly set MULTICALL3 storage slot after etch | |
| vm.store(holder, bytes32(uint256(0)), bytes32(uint256(uint160(0xcA11bde05977b3631167028862bE2a173976CA11)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading section header: This section is labeled "State Variables" but should be "Immutable Variables" (if the critical security issue is fixed and MULTICALL3 is made immutable). If MULTICALL3 remains mutable, the comment should explain why this is necessary despite the security risks of storage slot conflicts during delegatecall usage.