Refactor TrailsRouter to accept Multicall3 address as a constructor argument #83
Conversation
shunkakinoki
commented
Nov 27, 2025
shunkakinoki
commented
- Updated TrailsRouter contract to initialize MULTICALL3 address via constructor instead of as an immutable variable.
- Modified deployRouter function to pass Multicall3 address during TrailsRouter deployment.
- Added a mock AlwaysFailingMulticall3 for testing purposes to simulate failure scenarios in tests.
- Updated relevant test cases to reflect changes in constructor and deployment logic.
…rgument - Updated TrailsRouter contract to initialize MULTICALL3 address via constructor instead of as an immutable variable. - Modified deployRouter function to pass Multicall3 address during TrailsRouter deployment. - Added a mock AlwaysFailingMulticall3 for testing purposes to simulate failure scenarios in tests. - Updated relevant test cases to reflect changes in constructor and deployment logic.
…icall3 address - Adjusted gas usage for various test cases in TrailsRouter and TrailsRouterTest to reflect recent performance changes. - Updated expectedRouterAddress function in TrailsRouterDeploymentTest and TrailsRouterShimDeploymentTest to incorporate the Multicall3 address in the CREATE2 address calculation.
src/TrailsRouter.sol
Outdated
| // ------------------------------------------------------------------------- | ||
|
|
||
| address public immutable MULTICALL3 = 0xcA11bde05977b3631167028862bE2a173976CA11; | ||
| address public MULTICALL3; |
There was a problem hiding this comment.
Why is this no longer immutable?
There was a problem hiding this comment.
Thank you! Fixed 🙏
There was a problem hiding this comment.
Pull request overview
This PR refactors the TrailsRouter contract to accept the Multicall3 address as a constructor argument instead of using a hardcoded immutable variable. However, this refactoring introduces critical security vulnerabilities by changing MULTICALL3 from an immutable to a mutable state variable.
Key Changes:
- Modified TrailsRouter to initialize MULTICALL3 via constructor parameter instead of hardcoded immutable
- Updated deployment scripts to pass the Multicall3 address during contract creation
- Added AlwaysFailingMulticall3 mock to simplify failure testing (replacing storage manipulation approach)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TrailsRouter.sol | CRITICAL: Changed MULTICALL3 from immutable to mutable state variable and added constructor - introduces storage slot conflicts in delegatecall context |
| script/TrailsRouter.s.sol | Updated deployRouter to pass Multicall3 address as constructor argument |
| test/TrailsRouter.t.sol | Added AlwaysFailingMulticall3 mock and simplified failure tests by removing storage manipulation |
| test/TrailsRouterShim.t.sol | CRITICAL BUG: Updated vm.etch calls with constructor argument, but storage is not copied, leaving MULTICALL3 uninitialized |
| test/script/TrailsRouter.s.t.sol | Updated CREATE2 address calculation to include constructor argument in initCode |
| test/script/TrailsRouterShim.s.t.sol | Updated CREATE2 address calculation to include constructor argument in initCode |
| .gas-snapshot | Reflects gas cost changes from deployment modifications and simplified test mocks |
Critical Issues Identified:
- Security vulnerability: MULTICALL3 as a mutable state variable creates storage slot 0 conflicts when TrailsRouter is used via delegatecall, potentially allowing the address to be overwritten
- Test bugs: vm.etch only copies bytecode, not storage, so MULTICALL3 will be address(0) in etched contexts
- Missing validation: Constructor doesn't validate that _multicall3 is not address(0)
Recommendation: Revert MULTICALL3 to immutable while keeping the constructor parameterization. Immutable variables are stored in bytecode, not storage, making them safe for delegatecall usage and eliminating the need for storage initialization in tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/TrailsRouter.sol
Outdated
| // ------------------------------------------------------------------------- | ||
|
|
||
| address public immutable MULTICALL3 = 0xcA11bde05977b3631167028862bE2a173976CA11; | ||
| address public MULTICALL3; |
There was a problem hiding this comment.
Critical security issue: Changing MULTICALL3 from immutable to a regular state variable introduces storage slot 0 conflicts when TrailsRouter is used via delegatecall (its primary use case). This could lead to the MULTICALL3 address being accidentally overwritten by the calling contract's storage operations at slot 0, potentially redirecting multicall operations to an attacker-controlled address. The immutable modifier should be restored, as it ensures the address is stored in contract bytecode rather than storage, avoiding this vulnerability.
| address public MULTICALL3; | |
| address public immutable MULTICALL3; |
| constructor(address _multicall3) { | ||
| MULTICALL3 = _multicall3; | ||
| } |
There was a problem hiding this comment.
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; }
| constructor(address _multicall3) { | ||
| MULTICALL3 = _multicall3; | ||
| } |
There was a problem hiding this comment.
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).
| // 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); |
There was a problem hiding this comment.
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. This means any subsequent calls to validateOpHashAndSweep that perform multicall operations will fail or behave unexpectedly. 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); | |
| // Set MULTICALL3 storage slot (slot 0) to the correct address after etching | |
| vm.store(holder, bytes32(uint256(0)), bytes32(uint256(uint160(0xcA11bde05977b3631167028862bE2a173976CA11)))); |
| // Verify via TrailsRouter delegated validation | ||
| bytes memory original = address(shimImpl).code; | ||
| vm.etch(holder, address(new TrailsRouter()).code); | ||
| vm.etch(holder, address(new TrailsRouter(0xcA11bde05977b3631167028862bE2a173976CA11)).code); |
There was a problem hiding this comment.
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)))); |
src/TrailsRouter.sol
Outdated
| // State Variables | ||
| // ------------------------------------------------------------------------- |
There was a problem hiding this comment.
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.
- Updated the MULTICALL3 state variable in the TrailsRouter contract to be immutable, ensuring it can only be set once during contract deployment. This change enhances gas efficiency and contract integrity.
- Adjusted gas usage metrics for various test cases in TrailsRouter and TrailsRouterTest to reflect recent performance optimizations. - Minor updates made to individual test cases for consistency and accuracy in gas calculations.
…ariables - Changed the comment for the MULTICALL3 variable to clarify its immutable status, enhancing code readability and consistency with naming conventions.
