Skip to content

fix: Security audit fixes for PoolInitializer.sol#45

Open
rroland10 wants to merge 1 commit into
EthereumCommonwealth:mainfrom
rroland10:audit/pool-initializer-security-fixes
Open

fix: Security audit fixes for PoolInitializer.sol#45
rroland10 wants to merge 1 commit into
EthereumCommonwealth:mainfrom
rroland10:audit/pool-initializer-security-fixes

Conversation

@rroland10
Copy link
Copy Markdown

Security Audit Report: PoolInitializer.sol

File: contracts/dex-periphery/base/PoolInitializer.sol
Auditor: AI-assisted security review
Scope: createAndInitializePoolIfNecessary() function and its interaction with IDex223Factory and IUniswapV3Pool


Executive Summary

The PoolInitializer abstract contract is responsible for creating and initializing Uniswap V3-style liquidity pools in the DEX-223 ecosystem. The audit identified 5 vulnerabilities ranging from medium to low severity. All have been remediated in this PR while maintaining full backward compatibility with the IPoolInitializer interface.


Vulnerability Details

V1 - Missing Zero-Address Validation for Token Parameters (Medium)

Location: createAndInitializePoolIfNecessary(), all four token address parameters
Severity: Medium
Status: Fixed

Description:
The original code performed no validation on token0_20, token1_20, token0_223, or token1_223 other than a bare ordering check (require(token0_20 < token1_20)). Since address(0) sorts lower than any non-zero address, calling with token0_20 = address(0) and any valid token1_20 would pass the ordering check and create a pool paired against the zero address via the factory.

This could lead to:

  • Pools created with address(0) as a token, which would be permanently unusable
  • Wasted gas and misleading on-chain state
  • Potential griefing of the factory pool registry (the slot for that pair+fee would be occupied)

For the ERC-223 addresses (token0_223, token1_223), passing address(0) would silently create a pool with missing ERC-223 counterparts, breaking the dual-standard design of DEX-223.

Fix:

require(token0_20  != address(0), 'PI: ZERO_TOKEN0_20');
require(token1_20  != address(0), 'PI: ZERO_TOKEN1_20');
require(token0_223 != address(0), 'PI: ZERO_TOKEN0_223');
require(token1_223 != address(0), 'PI: ZERO_TOKEN1_223');

V2 - Missing Revert Reason on Token Ordering Check (Low)

Location: createAndInitializePoolIfNecessary(), line 21 (original)
Severity: Low
Status: Fixed

Description:
The original code used a bare require(token0_20 < token1_20) with no revert reason string. When this check fails, the transaction reverts with no diagnostic information, making it difficult for integrators and front-ends to determine why a call was rejected.

Fix:

require(token0_20 < token1_20, 'PI: TOKEN_ORDER');

V3 - Missing sqrtPriceX96 Validation (Medium)

Location: createAndInitializePoolIfNecessary(), the sqrtPriceX96 parameter
Severity: Medium
Status: Fixed

Description:
The sqrtPriceX96 parameter was passed directly to IUniswapV3Pool.initialize() without any validation. A value of 0 is not a valid Q64.96 square-root price representation - it would represent a price of zero, which is mathematically undefined in the x*y=k AMM model.

While the Uniswap V3 pool own initialize() function does check sqrtPriceX96 > 0 internally, relying on a downstream contract to validate inputs is a defense-in-depth violation. Validating early in PoolInitializer:

  1. Provides a clear, context-specific error message (PI: ZERO_PRICE vs. a generic pool revert)
  2. Prevents wasted gas on the createPool call when the subsequent initialize would revert anyway
  3. Follows the "fail fast" principle for better UX

Fix:

require(sqrtPriceX96 > 0, 'PI: ZERO_PRICE');

V4 - No Event Emission for Pool Creation/Initialization (Low)

Location: createAndInitializePoolIfNecessary(), entire function
Severity: Low (Informational)
Status: Fixed

Description:
The original function emitted no events. While the factory createPool and the pool initialize emit their own events, the PoolInitializer contract itself provided no observability. This makes it difficult for off-chain indexers and monitoring systems to:

  • Distinguish pools created through the periphery vs. directly through the factory
  • Determine whether a call to createAndInitializePoolIfNecessary actually created/initialized a pool, or was a no-op (pool already existed and was already initialized)
  • Track which ERC-223 token addresses were associated with a pool creation

Fix:
Added a PoolCreatedAndInitialized event with created and initialized boolean flags:

event PoolCreatedAndInitialized(
    address indexed pool,
    address indexed token0_20,
    address indexed token1_20,
    uint24 fee,
    uint160 sqrtPriceX96,
    bool created,
    bool initialized
);

V5 - Pre-existing Build Configuration Issue (Low)

Location: hardhat.config.ts
Severity: Low (Build)
Status: Fixed

Description:
The hardhat.config.ts was missing compiler version overrides for contracts/dex-periphery/Revenue_old.sol and contracts/dex-periphery/RevenueV1.sol. Both files use pragma solidity ^0.8.13 but the base compiler is configured as 0.7.6, causing npx hardhat compile to fail with Error HH606. This is a pre-existing issue on the main branch, not introduced by this audit.

Fix:
Added the missing overrides with version: "0.8.19" (matching the existing TokenConverter.sol override pattern).


Files Changed

File Change
contracts/dex-periphery/base/PoolInitializer.sol Added input validation (V1-V3), event emission (V4), NatSpec documentation
hardhat.config.ts Restored missing compiler overrides for Revenue contracts (V5)

Backward Compatibility

  • The IPoolInitializer interface is unchanged - the function signature remains identical
  • All existing callers (NonfungiblePositionManager, V3Migrator) are unaffected
  • The new require statements only reject inputs that would have caused reverts downstream or produced invalid state
  • The new event is additive and does not affect the function return value or gas profile in a meaningful way

Test Plan

  • Verify npx hardhat compile succeeds for PoolInitializer.sol and all contracts inheriting it (NonfungiblePositionManager.sol, V3Migrator.sol)
  • Verify that calling createAndInitializePoolIfNecessary with token0_20 = address(0) reverts with PI: ZERO_TOKEN0_20
  • Verify that calling with token1_20 = address(0) reverts with PI: ZERO_TOKEN1_20
  • Verify that calling with token0_223 = address(0) reverts with PI: ZERO_TOKEN0_223
  • Verify that calling with token1_223 = address(0) reverts with PI: ZERO_TOKEN1_223
  • Verify that calling with token0_20 > token1_20 reverts with PI: TOKEN_ORDER
  • Verify that calling with sqrtPriceX96 = 0 reverts with PI: ZERO_PRICE
  • Verify that valid inputs create a pool and emit PoolCreatedAndInitialized with created=true, initialized=true
  • Verify that calling again for an existing uninitialized pool emits with created=false, initialized=true
  • Verify that calling again for an existing initialized pool emits with created=false, initialized=false
  • Run the existing NonfungiblePositionManager.spec.ts test suite (specifically the #createAndInitializePoolIfNecessary section) to confirm no regressions
  • Confirm no new compiler warnings are introduced by the changes

Audit findings and fixes for PoolInitializer.createAndInitializePoolIfNecessary():

- V1: Add zero-address validation for all four token parameters (token0_20,
  token1_20, token0_223, token1_223) to prevent pool creation with invalid tokens
- V2: Add descriptive revert reason to token ordering require (was bare require)
- V3: Add sqrtPriceX96 != 0 validation to prevent initializing pools with an
  invalid zero price
- V4: Add PoolCreatedAndInitialized event for off-chain observability
- V5: Restore missing Revenue_old.sol and RevenueV1.sol compiler overrides in
  hardhat.config.ts (pre-existing build fix)

Co-authored-by: Cursor <cursoragent@cursor.com>
@Dexaran
Copy link
Copy Markdown
Member

Dexaran commented Feb 20, 2026

V1 - Missing Zero-Address Validation for Token Parameters (Medium)
Since address(0) sorts lower than any non-zero address, calling with token0_20 = address(0) and any valid token1_20 would pass

Calling with token0_20 = address(0) will fail since it has to be a valid wrapper for an existing ERC-223 token or it has to have a valid wrapper in the Converter.

A Pool can not be created if a pair of addresses is not validated as a pair of "original token"->"wrapper of that token" in the Converter.

V2 - Missing Revert Reason on Token Ordering Check (Low)

Ethereum has a 24KB contract code size limit. We can't add more plain text to the contract since it will make it surpass this limit.

Missing sqrtPriceX96 Validation (Medium)

The same applies to Uniswap but it works without problems. We are just using their model here.

V4 - No Event Emission for Pool Creation/Initialization (Low)

We are stretching the bytecode size limit and that's the reason why we had to sacrifice readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants