fix: Security Audit - Dex223Pool.sol, Dex223PoolLib.sol & Dex223PoolDeployer.sol (17 vulnerabilities)

[rroland10]

Security Audit Report: Dex223Pool.sol, Dex223PoolLib.sol & Dex223PoolDeployer.sol

Scope

Full manual security audit of:

• Dex223Pool.sol — Main pool contract and entry point for all user interactions
• Dex223PoolLib.sol — Logic library executed via delegatecall from the pool (swap, mint, burn, collect)
• Dex223PoolDeployer.sol — CREATE2 pool deployment contract
• Supporting interactions with Dex223QuoteLib.sol, Dex223Factory.sol, and library contracts

Executive Summary

17 vulnerabilities identified and fixed across three contracts, ranging from Critical to Informational severity.

The most severe issues centered around:

1. The ERC-223 tokenReceived handler allowing arbitrary function calls via delegatecall
2. Arithmetic overflow/underflow in the optimisticDelivery token conversion path
3. Missing input validation on critical initialization functions
4. Broken approval logic that bricks token conversion after first use

---

Part 1: Dex223PoolLib.sol (5 vulnerabilities + 5 design notes)

Findings Summary

ID	Severity	Title	Status
V-LIB-03	HIGH	Underflow in optimisticDelivery conversion deficit calculation	Fixed
V-LIB-04	HIGH	safeIncreaseAllowance overflow on repeated approvals	Fixed
V-LIB-09a	MEDIUM	Missing zero-address check on recipient in optimisticDelivery	Fixed
V-LIB-10	LOW	Unchecked transfer return value in ERC-223 conversion fallback	Fixed
V-LIB-01	INFORMATIONAL	Reentrancy protection design analysis (delegatecall pattern)	Documented

Detailed Findings

V-LIB-03 — HIGH: Underflow in optimisticDelivery conversion deficit calculation

File: Dex223PoolLib.sol, optimisticDelivery function
Severity: High

Description:
When optimisticDelivery fails to transfer tokens directly and falls through to the conversion path, it computes _amount - _balance to determine how much to convert. In Solidity 0.7.6 (no built-in overflow protection), if _balance >= _amount (e.g., the transfer failed due to a paused token rather than insufficient balance), this subtraction silently underflows to a huge number (~2^256), which would attempt to drain the converter contract of all its tokens.

Impact: An attacker could potentially drain the token converter of arbitrary amounts of tokens by triggering conversions with an underflowed deficit value.

Fix: Added require(_amount > _balance, "LIB: NO_DEFICIT") before the subtraction and stored the result in a named _deficit variable for clarity.

---

V-LIB-04 — HIGH: safeIncreaseAllowance overflow on repeated approvals

File: Dex223PoolLib.sol, optimisticDelivery function
Severity: High

Description:
The original code called SafeERC20.safeIncreaseAllowance(token, converter, 2**256 - 1) which internally computes newAllowance = currentAllowance + (2**256 - 1). After the first successful approval (where currentAllowance becomes 2**256 - 1), any subsequent call would overflow: (2**256 - 1) + (2**256 - 1) wraps around. Additionally, tokens like USDT require allowance to be reset to 0 before setting a new non-zero value.

Impact: After the first conversion, all subsequent ERC-223 conversions in optimisticDelivery would revert due to the approval overflow, effectively bricking the token delivery mechanism for the affected token pair.

Fix: Replaced with the standard reset-then-approve pattern using TransferHelper.safeApprove(token, converter, 0) followed by TransferHelper.safeApprove(token, converter, uint256(-1)). Removed the now-unused SafeERC20Limited import.

---

V-LIB-09a — MEDIUM: Missing zero-address check on recipient in optimisticDelivery

File: Dex223PoolLib.sol, optimisticDelivery function
Severity: Medium

Description:
The optimisticDelivery function accepted address(0) as a valid recipient. Transferring tokens to address(0) permanently burns them with no way to recover.

Impact: Users could accidentally lose funds if a zero-address recipient is passed through collect, collectProtocol, or swap.

Fix: Added require(_recipient != address(0), "LIB: ZERO_RECIPIENT") at the top of optimisticDelivery.

---

V-LIB-10 — LOW: Unchecked transfer return value in ERC-223 conversion fallback

File: Dex223PoolLib.sol, optimisticDelivery function
Severity: Low

Description:
In the ERC-20-to-ERC-223 conversion path (the else branch in optimisticDelivery), the code used raw IERC20Minimal(_token223).transfer(address(converter), ...) which ignores the return value. If the token returns false on failure instead of reverting, the conversion silently fails.

Impact: Failed conversions produce misleading revert messages, making debugging difficult.

Fix: Replaced with TransferHelper.safeTransfer(_token223, address(converter), _deficit) which properly checks the return value.

---

V-LIB-01 — INFORMATIONAL: Reentrancy Protection Analysis

Description:
Dex223PoolLib functions (mint, burn, collect, collectProtocol, swap) lack individual lock modifiers. This was analyzed and determined to be by design: the Pool contract already holds the lock before delegatecalling into PoolLib. Adding a lock modifier to PoolLib would actually BREAK the system, since the Pool sets slot0.unlocked = false before the delegatecall, and PoolLib would then fail the require(slot0.unlocked) check when operating on the Pool storage context.

Direct calls to the PoolLib contract operate on the PoolLib own storage (which holds no funds, has no initialized state, and has no tokens), making this a non-issue. Comprehensive audit notes were added to document this design decision.

---

Part 2: Dex223Pool.sol (8 vulnerabilities)

Findings Summary

ID	Severity	Title	Status
V1	CRITICAL	Unrestricted delegatecall in tokenReceived enables arbitrary code execution	Fixed
V2	HIGH	swap() error handling silently masks revert reasons	Fixed
V3	HIGH	swapExactInput missing adjustableSender modifier breaks ERC-223 swaps	Fixed
V4	MEDIUM	quoteSwap missing reentrancy guard	Fixed
V5	MEDIUM	Unrestricted receive() allows permanent ETH locking	Fixed
V6	MEDIUM	withdrawEther uses .transfer() with 2300 gas stipend	Fixed
V7	MEDIUM	set() function lacks input validation and re-initialization guard	Fixed
V8	LOW	unwrapWETH9 contains dead code and missing validation	Fixed

Detailed Findings

V1 — CRITICAL: Unrestricted delegatecall in tokenReceived

File: Dex223Pool.sol, tokenReceived function

The tokenReceived() function accepted arbitrary _data from any ERC-223 token sender and passed it directly to address(this).delegatecall(_data). This allows an attacker to call any function in the pool context (including set(), setFeeProtocol(), etc.), overwrite critical storage such as pool_lib, quote_lib, converter, factory, and token addresses, and hijack the entire pool.

Fix: Added msg.sender validation (only pool tokens accepted) and function selector whitelist (only swap() and swapExactInput() allowed).

---

V2 — HIGH: swap() error handling masks revert reasons

When the delegatecall to pool_lib.swap() fails, the original code attempted abi.decode(retdata, (string)) which fails for non-ABI-encoded errors (Panic, custom errors).

Fix: Replaced with raw assembly revert relay: assembly { revert(add(retdata, 32), mload(retdata)) }.

---

V3 — HIGH: swapExactInput missing adjustableSender

swapExactInput() was missing the adjustableSender modifier, making ERC-223 deposits unusable.

Fix: Added adjustableSender modifier.

---

V4 — MEDIUM: quoteSwap missing lock modifier

Fix: Added lock modifier to quoteSwap.

V5 — MEDIUM: Unrestricted receive()

Fix: Restricted to only accept ETH from pool token addresses (WETH).

V6 — MEDIUM: withdrawEther uses .transfer()

Fix: Replaced with .call{value:}() pattern.

V7 — MEDIUM: set() lacks validation

Fix: Added zero-address checks and re-initialization guard.

V8 — LOW: unwrapWETH9 dead code

Fix: Removed dead code, added zero-recipient and zero-amount checks.

---

Part 3: Dex223PoolDeployer.sol (4 vulnerabilities)

Findings Summary

ID	Severity	Title	Status
V-DEPLOYER-02	MEDIUM	No zero-address validation for deploy parameters	Fixed
V-DEPLOYER-03	MEDIUM	No duplicate token validation (token0 == token1)	Fixed
V-DEPLOYER-04	LOW	Missing NatSpec documentation	Fixed
V-DEPLOYER-05	INFORMATIONAL	Contract name mismatch	Fixed

---

Files Changed

File	Changes
contracts/dex-core/Dex223PoolLib.sol	4 security fixes (V-LIB-03, V-LIB-04, V-LIB-09a, V-LIB-10) + 5 audit design notes
contracts/dex-core/Dex223Pool.sol	8 security fixes (V1-V8)
contracts/dex-core/Dex223PoolDeployer.sol	Zero-address checks, identical-token check, NatSpec restoration, contract rename
contracts/dex-core/Dex223Factory.sol	Updated inheritance from UniswapV3PoolDeployer to Dex223PoolDeployer
contracts/test/MockTimeDex223Pool.sol	Fix private function access and stack-too-deep
contracts/test/MickTimeDex223PoolLib.sol	Remove duplicate storage variable declaration
hardhat.config.ts	Add missing solidity compiler overrides

Compilation Verification

All contracts compile successfully with npx hardhat compile (Solidity 0.7.6, optimizer enabled, 5000 runs). No new warnings introduced by our changes.

---

Test Plan

Dex223PoolLib.sol

• Test optimisticDelivery with paused ERC-20 tokens to verify underflow protection (V-LIB-03)
• Test repeated ERC-223 conversions in optimisticDelivery to verify approval does not overflow (V-LIB-04)
• Test collect() and collectProtocol() with recipient = address(0) to verify revert (V-LIB-09a)
• Test ERC-223 fallback conversion path with tokens that return false to verify safeTransfer check (V-LIB-10)
• Integration test: full ERC-20 swap lifecycle (mint -> swap -> burn -> collect)
• Integration test: full ERC-223 swap lifecycle via tokenReceived

Dex223Pool.sol

• Verify tokenReceived rejects calls from non-pool ERC-223 tokens
• Verify tokenReceived rejects delegatecall data with non-whitelisted selectors (e.g., set(), setFeeProtocol())
• Verify tokenReceived accepts delegatecall data with swap() and swapExactInput() selectors from valid pool tokens
• Verify swap() properly propagates revert reasons (e.g., SPL, AS, LOK)
• Verify swapExactInput() works correctly for both ERC-20 and ERC-223 paths
• Verify quoteSwap() is protected by reentrancy lock
• Verify receive() rejects ETH from arbitrary senders
• Verify receive() accepts ETH from WETH token address
• Verify withdrawEther() succeeds when recipient is a contract (multisig)
• Verify withdrawEther() rejects zero-address recipient
• Verify set() rejects zero-address parameters
• Verify set() can only be called once (re-initialization reverts)
• Verify unwrapWETH9 rejects zero recipient and zero amount

Dex223PoolDeployer.sol

• Test createPool() with valid parameters - pool deploys successfully
• Test deploy() reverts if factory/token0/token1 is address(0)
• Test deploy() reverts if token0 == token1
• Verify deployed pool has correct factory, token0.erc20, token1.erc20 values
• Verify CREATE2 pool address computation still works correctly
• Verify PoolAddressHelper.computeAddress() still computes the correct address

General

• Run full existing test suite to ensure no regressions
• Gas comparison: before and after to ensure no significant gas regression