fix: security audit fixes for PeripheryImmutableState.sol#43
Open
rroland10 wants to merge 1 commit into
Open
Conversation
- Add zero-address validation for _factory and _WETH9 constructor params - Add PeripheryImmutableStateInitialized event for deployment verification - Add comprehensive NatSpec documentation for constructor and contract Co-authored-by: Cursor <cursoragent@cursor.com>
Member
If we will improperly set the variables of our contracts upon deployment then the whole system will not work. It's not something that we will not notice during the launch. Introducing an extra unnecessary check will only increase the bytecode size without any positive effect.
Unnecessary increase of the bytecode size.
Yea, its reasonable to add the documentation. We can address it a bit later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Audit:
PeripheryImmutableState.solSummary
Comprehensive security audit of
contracts/dex-periphery/base/PeripheryImmutableState.sol— the abstract base contract that stores immutablefactoryandWETH9addresses used by all periphery contracts (SwapRouter, NonfungiblePositionManager, Quoter223, V3Migrator, PoolInitializer, LiquidityManagement, PeripheryPayments).Vulnerabilities Found
1. [MEDIUM] Missing Zero-Address Validation in Constructor
Severity: Medium
Type: Input Validation
Location:
PeripheryImmutableState.constructor(address, address)Description:
The constructor accepts
_factoryand_WETH9parameters without validating that they are notaddress(0). Both variables are declared asimmutable, meaning once set at deployment they can never be changed. If either is accidentally set to the zero address during deployment, the entire contract is permanently broken with no recovery path.Impact:
IDex223Factory(factory).getPool(...)andIDex223Factory(factory).createPool(...)would revert or return incorrect results. This breaks SwapRouter (all swap functions), NonfungiblePositionManager (mint, increase/decrease liquidity), PoolInitializer (pool creation), LiquidityManagement (add liquidity), V3Migrator (migration), and Quoter223 (all quote functions). The entire periphery layer becomes non-functional.receive()function in PeripheryPayments checksrequire(msg.sender == WETH9), which would block all native ETH receipts.unwrapWETH9()would call.balanceOf()and.withdraw()on address(0), causing reverts. Thepay()function's WETH9 branch would attempt to deposit/transfer on a zero address. V3Migrator'sreceive()would also be broken, preventing ETH refunds.Affected child contracts (7):
ERC223SwapRouter(SwapRouter.sol)DexaransNonfungiblePositionManager(NonfungiblePositionManager.sol)ERC223Quoter(Quoter223.sol)V3Migrator(V3Migrator.sol)PoolInitializer(PoolInitializer.sol)LiquidityManagement(LiquidityManagement.sol)PeripheryPayments(PeripheryPayments.sol)Fix: Added
require(_factory != address(0), 'FACTORY_ZERO')andrequire(_WETH9 != address(0), 'WETH9_ZERO')checks at the top of the constructor.2. [LOW] No Event Emission on Immutable State Initialization
Severity: Low
Type: Observability / Off-Chain Monitoring
Location:
PeripheryImmutableState.constructor(address, address)Description:
The constructor does not emit any event when the immutable state is initialized. Since
factoryandWETH9areimmutable(not stored in contract storage in the traditional sense), off-chain monitoring tools, indexers, and deployment verification scripts cannot easily discover which addresses were configured at deployment time without parsing constructor arguments from the deployment transaction input data.Impact:
Fix: Added
event PeripheryImmutableStateInitialized(address indexed factory, address indexed WETH9)and emit it in the constructor after setting the immutable values.3. [INFORMATIONAL] Missing NatSpec Documentation on Constructor
Severity: Informational
Type: Code Quality / Documentation
Location:
PeripheryImmutableState.constructor(address, address)and contract-levelDescription:
The constructor parameters lacked NatSpec
@paramdocumentation, and the contract-level documentation only had@titleand@noticebut was missing@devdocumentation explaining the immutability guarantees and implications.Fix: Added comprehensive NatSpec including
@devat the contract level,@notice,@param, and@devon the constructor.Changes Made
Compilation Verification
Verified that the changes compile successfully with
npx hardhat compile(Solidity 0.7.6, optimizer enabled with 5000 runs). No new errors or warnings introduced. Pre-existing compilation errors in unrelated test files (MockTimeDex223Pool.sol,Dex223PoolLib.sol) remain unchanged.Test Plan
npx hardhat compile(Solidity 0.7.6)_factory = address(0)reverts with'FACTORY_ZERO'_WETH9 = address(0)reverts with'WETH9_ZERO'PeripheryImmutableStateInitializedevent is emitted with correct indexed addresses on successful deploymentfactoryandWETH9public getters return the correct addresses post-deploymentMade with Cursor