fix: security audit fixes for Dex223QuoteLib.sol

[rroland10]

Security Audit Report: Dex223QuoteLib.sol

Scope

Full security audit of contracts/dex-core/Dex223QuoteLib.sol — the delegatecall library used by Dex223Pool.quoteSwap() to simulate swap execution and return estimated output amounts.

---

Vulnerabilities Found & Fixes Applied

V-QL-02 — safeIncreaseAllowance Overflow on Repeated Approvals (HIGH)

Location: optimisticDelivery(), line ~373 (original)

Description:
The original code called SafeERC20.safeIncreaseAllowance(token, converter, 2**256 - 1) to approve the token converter. The safeIncreaseAllowance function internally computes newAllowance = currentAllowance + value. On the second invocation (when currentAllowance > 0), adding 2**256 - 1 causes a uint256 overflow. In Solidity 0.7.6 (which has no built-in overflow protection), this wraps silently, producing a near-zero or incorrect allowance value.

Impact: After the first successful approval, subsequent conversion attempts will fail or set an incorrectly small allowance, potentially blocking token deliveries.

Fix: Replaced with TransferHelper.safeApprove(token, converter, 0) followed by TransferHelper.safeApprove(token, converter, uint256(-1)). This reset-then-approve pattern is the standard approach and also handles tokens like USDT that require the allowance to be zero before setting a new value.

---

V-QL-03 — Underflow in Conversion Path Deficit Calculation (MEDIUM)

Location: optimisticDelivery(), line ~362 (original)

Description:
When the initial transfer() fails and the code enters the conversion fallback path, it computes _amount - _balance to determine how many tokens need to be converted. If _balance >= _amount (which can happen if the transfer failed for reasons other than insufficient balance — e.g., a paused token, a blacklisted recipient, or a reentrancy guard), this subtraction underflows in Solidity 0.7.6, producing a huge value (close to 2**256). This would attempt to convert an astronomical number of tokens from the converter, either draining it or reverting with a confusing error.

Impact: Potential drain of the token converter contract, or opaque revert errors that are impossible to debug.

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

---

V-QL-04 — Missing Zero-Address Recipient Check in optimisticDelivery (MEDIUM)

Location: optimisticDelivery(), beginning of function

Description:
The optimisticDelivery function accepts an arbitrary _recipient address and transfers tokens to it. If _recipient == address(0), most ERC-20 tokens will accept the transfer but the tokens are permanently burned (sent to the zero address with no way to recover them).

Impact: Permanent loss of pool funds if optimisticDelivery is called with a zero-address recipient (e.g., through a misconfigured swap call).

Fix: Added require(_recipient != address(0), "QLIB: ZERO_RECIPIENT") at the start of the function.

---

V-QL-05 — Unsafe Raw transfer() in ERC-223 Conversion Fallback and Unnecessary Import (LOW)

Location: optimisticDelivery(), ERC-223-to-ERC-20 conversion branch; import section

Description:
Two issues:

1. The ERC-223 fallback path used a raw IERC20Minimal(_token223).transfer(address(converter), ...) which ignores the return value. If the ERC-223 token returns false on failure instead of reverting, the conversion silently fails and the subsequent safeTransfer of the output token reverts with the opaque "ST" error.
2. The SafeERC20Limited.sol import is no longer needed since safeIncreaseAllowance was replaced with TransferHelper.safeApprove.

Impact: Silent failures in the conversion path lead to confusing error messages; unnecessary import increases bytecode size.

Fix: Replaced raw transfer() with TransferHelper.safeTransfer() which checks the return value. Removed the SafeERC20Limited import. Both changes are consistent with the identical fixes applied in Dex223PoolLib.sol (V-LIB-04, V-LIB-10).

---

Audit Notes (Informational — No Code Changes Required)

V-QL-01 — State Mutation + Revert Pattern Safety Analysis (INFORMATIONAL)

Description:
quoteSwap() intentionally mutates storage state (slot0, liquidity, fee growth, protocol fees, ticks, oracle observations) during swap simulation, then reverts via inline assembly to return the computed amounts in the revert data. When called via delegatecall from Dex223Pool.quoteSwap(), the revert rolls back ALL state changes within the delegatecall frame.

Assessment: This pattern is safe as long as (a) quoteSwap always terminates with a revert (guaranteed by the assembly blocks at the end — both if and else branches revert), and (b) the function is only invoked via delegatecall (enforced by the Pool contract). Direct calls operate on the QuoteLib's own uninitialized storage and always revert, so they are harmless.

---

V-QL-06 — Dead Code Analysis (INFORMATIONAL)

Description:
Functions _modifyPosition, _updatePosition, balance0, balance1, checkTicks, and _blockTimestamp are defined in QuoteLib but are NOT used by quoteSwap(). The optimisticDelivery function is also dead code in the context of quoteSwap (the function reverts before reaching any token delivery).

Assessment: These functions exist for storage layout consistency with Dex223PoolLib.sol. Since QuoteLib is used via delegatecall, its storage variable declarations must match the Pool's layout exactly. The functions are retained to maintain this correspondence and for potential future use. The fixes to optimisticDelivery were still applied since the function exists in the contract and could theoretically be called if the contract structure changes.

---

V-QL-07 — No Access Control (INFORMATIONAL)

Description:
Anyone can call quoteSwap() directly on the deployed QuoteLib contract (not via delegatecall from the Pool).

Assessment: This is safe because (1) the function always reverts, so no state changes persist, and (2) direct calls operate on the QuoteLib's own uninitialized storage (not the Pool's). No access control is needed.

---

Summary of Changes

ID	Severity	Description	Fix
V-QL-02	HIGH	safeIncreaseAllowance overflow on repeated approvals	Replace with safeApprove(0) + safeApprove(max)
V-QL-03	MEDIUM	Underflow in conversion deficit calculation	Add require(_amount > _balance) guard
V-QL-04	MEDIUM	Missing zero-address recipient check	Add require(_recipient != address(0))
V-QL-05	LOW	Unsafe raw transfer() + unnecessary import	Use TransferHelper.safeTransfer, remove import
V-QL-01	INFO	State mutation + revert pattern	Documented as safe (audit note)
V-QL-06	INFO	Dead code in QuoteLib	Documented rationale (storage layout consistency)
V-QL-07	INFO	No access control on QuoteLib	Documented as safe (always reverts)

Test Plan

• Verify contract compiles successfully with npx hardhat compile (confirmed: PASS)
• Verify quoteSwap still returns correct swap estimates via eth_call simulation
• Verify optimisticDelivery rejects address(0) as recipient
• Verify optimisticDelivery handles the case where _balance >= _amount gracefully (no underflow)
• Verify converter approval works correctly on first call (allowance set to max)
• Verify converter approval works correctly on subsequent calls (reset to 0, then max)
• Verify ERC-223 fallback conversion path properly reverts on failed transfers (via safeTransfer)
• Verify no storage layout changes between QuoteLib and PoolLib/Pool (struct ordering, variable ordering)
• Run existing test suite to ensure no regressions: npx hardhat test