fix: Security Audit — CallbackValidation.sol

[rroland10]

Security Audit Report: CallbackValidation.sol

File: contracts/dex-periphery/base/CallbackValidation.sol
Auditor: AI-assisted security review
Severity Scale: Critical / High / Medium / Low / Informational

---

Executive Summary

A thorough security audit was performed on CallbackValidation.sol, a library used by the Dex223 periphery contracts (SwapRouter, LiquidityManagement, Quoter223) to validate that swap/mint callbacks originate from legitimate pool contracts. The library uses CREATE2-based address derivation to verify callers.

The audit identified 1 Medium and 2 Informational findings. All have been fixed in this PR.

---

Vulnerability 1 — Silent require Failure (Missing Revert Reason)

Field	Value
Severity	Medium
Location	CallbackValidation.sol:35
Status	Fixed

Description

The core require statement in verifyCallback had no revert reason string:

require(msg.sender == address(pool));

When a callback validation fails (i.e., a malicious or misconfigured contract calls a periphery callback), the transaction reverts with an empty error. This has several negative consequences:

1. Debugging difficulty: Developers and users cannot distinguish this revert from other empty reverts in the call stack (e.g., out-of-gas, failed low-level calls, assert failures). In a complex multi-hop swap through SwapRouter.exactInput(), an empty revert is nearly impossible to diagnose.

2. Off-chain tooling breakage: Monitoring systems, block explorers, and transaction simulation tools (Tenderly, Etherscan, etc.) cannot categorize or alert on this specific failure mode. This makes it harder to detect active exploitation attempts where an attacker tries to invoke callbacks from a non-pool address.

3. Increased attack surface for phishing: When users see a generic "Transaction reverted" error, they may retry with different parameters or increased gas, potentially falling victim to social engineering attacks that exploit the confusion.

4. Gas cost is negligible: Adding a short reason string ('CVF' = 3 bytes) costs only ~200 extra gas on the revert path, and zero gas on the success path. This is insignificant compared to the swap operation (~150k+ gas).

Fix Applied

require(msg.sender == address(pool), 'CVF');

The 'CVF' (Callback Validation Failed) error code follows the project's existing convention of short uppercase error codes (e.g., 'LOK', 'AI', 'TLU', 'TLM', 'TUM' used elsewhere in the codebase).

---

Vulnerability 2 — Incorrect NatSpec Documentation (Token Standard Ambiguity)

Field	Value
Severity	Informational
Location	CallbackValidation.sol:10-14
Status	Fixed

Description

The NatSpec documentation referenced "Uniswap V3" instead of "Dex223" and did not clarify that the tokenA/tokenB parameters must be ERC-20 addresses (not ERC-223 addresses).

This is critical context in the Dex223 dual-standard architecture because:

• The Dex223Factory.createPool() function populates the getPool mapping for all 8 combinations of ERC-20/ERC-223 token pairs (lines 104-112 of Dex223Factory.sol).
• However, PoolAddress.computeAddress() derives the pool address using only the ERC-20 token addresses (since pools are deployed via CREATE2 with keccak256(abi.encode(token0_erc20, token1_erc20, fee)) as the salt).
• If a developer mistakenly passes an ERC-223 token address to verifyCallback, the computed address would be wrong, causing the require to always fail — effectively breaking swaps and mints for that path.

The existing callers (SwapRouter.uniswapV3SwapCallback at line 165, Quoter223.uniswapV3SwapCallback at line 100, LiquidityManagement.uniswapV3MintCallback at line 42) all correctly pass ERC-20 addresses because the Path library encodes ERC-20 addresses. But future integrators who write custom periphery contracts could easily make this mistake without proper documentation.

Fix Applied

Updated NatSpec to:

• Reference "Dex223" instead of "Uniswap V3"
• Explicitly document that tokenA/tokenB must be ERC-20 addresses
• Add @dev tag explaining the CREATE2 validation mechanism

---

Vulnerability 3 — Missing Library-Level Documentation

Field	Value
Severity	Informational
Location	CallbackValidation.sol:8
Status	Fixed

Description

The library had only a single-line @notice with no @dev documentation explaining how the validation works. For a security-critical library that is the sole defense against callback spoofing attacks, this is insufficient.

Without clear documentation of the CREATE2-based verification mechanism, auditors and developers may not fully understand the security assumptions:

1. The validation assumes the POOL_INIT_CODE_HASH in PoolAddress.sol is correct and matches the actual deployed pool bytecode.
2. The validation assumes the factory address passed to the function is the canonical Dex223 factory.
3. The validation is only sound if CREATE2 address derivation is deterministic (which it is by the EVM specification, but this assumption should be documented).

Fix Applied

Added comprehensive @dev documentation explaining the validation mechanism and its security properties.

---

Additional Security Observations (No Code Changes Required)

Observation A — POOL_INIT_CODE_HASH Dependency

The security of CallbackValidation depends entirely on PoolAddress.POOL_INIT_CODE_HASH (currently 0x2ed8d490...). If this hash becomes stale (e.g., after a pool contract upgrade that changes bytecode), all callback validations would fail, breaking the entire periphery. This is a known operational risk, not a code vulnerability — but the hash should be verified against the actual Dex223Pool creation code after any compiler or contract changes.

Observation B — ERC-223 Token Address in Path Encoding

The SwapRouter and Quoter223 use Path.decodeFirstPool() to extract token addresses from the encoded path. The path encoding uses ERC-20 addresses by convention. If a user constructs a path with ERC-223 addresses instead, the verifyCallback would compute a wrong pool address and revert. With the new 'CVF' error code, this failure mode is now diagnosable.

Observation C — No extcodesize Check on Computed Pool Address

The verifyCallback function does not check whether the computed pool address has deployed code. If msg.sender happens to equal a CREATE2-derived address for a pool that hasn't been deployed yet, the validation would pass. However, this is not exploitable because:

1. An undeployed address cannot initiate a call (msg.sender must have code to call a function).
2. Even if CREATE2 address collision occurred (astronomically unlikely), the attacker would need to deploy a contract at that exact address.

---

Summary of Changes

File	Change
contracts/dex-periphery/base/CallbackValidation.sol	Added revert reason 'CVF' to require statement; updated NatSpec documentation to reference Dex223 and clarify ERC-20 token address requirement; added @dev documentation

Test Plan

• Verify CallbackValidation.sol compiles without errors under Solidity 0.7.6
• Verify SwapRouter.sol compiles without errors (uses CallbackValidation.verifyCallback)
• Verify LiquidityManagement.sol compiles without errors (uses CallbackValidation.verifyCallback)
• Verify Quoter223.sol compiles without errors (uses CallbackValidation.verifyCallback)
• Verify that a legitimate pool callback still succeeds (no behavioral change on happy path)
• Verify that a spoofed callback from a non-pool address reverts with 'CVF' error code
• Verify that POOL_INIT_CODE_HASH in PoolAddress.sol matches the keccak256 of Dex223Pool creation code
• Run existing test suite to confirm no regressions

Made with Cursor