fix: security audit fixes for NFTDescriptor.sol

[rroland10]

Security Audit Report: NFTDescriptor.sol

Audited file: contracts/dex-periphery/base/NFTDescriptor.sol
Severity scale: Critical > High > Medium > Low > Informational

---

Vulnerability 1: uint8 Overflow in escapeQuotes() — Loop Counters and Quote Counter

Severity: High

Description: The escapeQuotes() function uses uint8 for both the loop iterator i and the quotesCount variable. Since uint8 can only hold values 0–255, a token symbol longer than 255 bytes causes the loop counter i to overflow back to 0, resulting in an infinite loop that exhausts all gas. Similarly, if a symbol contains more than 255 quote characters, quotesCount wraps to 0, silently skipping the escape logic and producing malformed JSON output that could break NFT metadata parsing or enable JSON injection.

Attack vector: A malicious ERC-20 token can set its symbol() to a string longer than 255 bytes (or with >255 quote chars). When a user mints a liquidity position with that token, constructTokenURI() enters an infinite loop, causing a denial-of-service that makes tokenURI() permanently revert for that position.

Fix: Changed uint8 quotesCount → uint256 quotesCount and all uint8 i loop counters → uint256 i, eliminating the overflow risk for any input length.

---

Vulnerability 2: Incorrect Protocol Branding — "Uniswap" in generateName()

Severity: Medium (Informational/Phishing Risk)

Description: The generateName() function hardcodes 'Uniswap - ' as the NFT name prefix. This is a Dex223 protocol, not Uniswap. This creates user confusion, potential phishing vectors (users may believe they are interacting with Uniswap), and brand/legal exposure. NFT marketplace displays (OpenSea, etc.) would show "Uniswap" as the position name despite the NFT belonging to Dex223.

Fix: Changed 'Uniswap - ' → 'Dex223 - ' to reflect the correct protocol identity.

---

Vulnerability 3: Division by Zero in tickToDecimalString() When tickSpacing == 0

Severity: High

Description: The function performs TickMath.MIN_TICK / tickSpacing and TickMath.MAX_TICK / tickSpacing without validating that tickSpacing > 0. If tickSpacing is 0 (due to a misconfigured pool or a crafted parameter), the EVM triggers a panic revert (division by zero), making the entire tokenURI() call permanently revert for that NFT position. While pools should always have non-zero tick spacing, this is a defense-in-depth issue — the library should not rely on upstream invariants.

Fix: Added require(tickSpacing > 0, 'NFTDesc: tickSpacing zero') at the start of the function for explicit early validation with a descriptive revert reason.

---

Vulnerability 4: Silent Misbehavior in adjustForDecimalPrecision() for Decimal Differences > 18

Severity: Medium

Description: The function computes difference = abs(baseTokenDecimals - quoteTokenDecimals). If difference > 18, the function falls through to the else branch and returns the raw sqrtRatioX96 without any adjustment. This silently produces wildly inaccurate price displays for exotic token pairs with extreme decimal differences (e.g., a token with 0 decimals paired with one that has 77 decimals — which is technically possible for non-standard ERC-20s). Rather than displaying wrong prices, the function should revert.

Fix: Added require(difference <= 18, 'NFTDesc: decimal diff > 18') to explicitly reject unsupported decimal configurations instead of silently producing garbage output.

---

Vulnerability 5: uint8 Digit Counter Overflow and Zero-Value Underflow in fixedPointToDecimalString()

Severity: High

Description: The function uses uint8 digits to count the number of digits in value. Two issues:

1. Overflow: If value has more than 255 digits (theoretically possible for extreme sqrt price ratios multiplied by 10**44), the uint8 counter wraps around, producing an incorrect digit count and corrupting all subsequent buffer index calculations — leading to out-of-bounds writes or garbage output.
2. Underflow: If value == 0, the while loop never executes, digits remains 0, and digits - 1 underflows. In Solidity 0.7.x (no automatic underflow checks), this wraps to 255, causing completely wrong buffer allocations.

Fix:

• Changed counter to uint256 digitCount to prevent overflow.
• Added require(digitCount > 0, 'NFTDesc: zero value') before the subtraction to guard against underflow.
• Cast back to uint8 only after validation: uint8 digits = uint8(digitCount - 1).

---

Vulnerability 6: Off-by-One Error in getCircleCoord() — % 255 Should Be % 256

Severity: Low

Description: The function returns (sliceTokenHex(tokenAddress, offset) * tokenId) % 255. Since sliceTokenHex returns a uint8 (range 0–255), the product modulo 255 can never produce 255 as a result (since 255 % 255 == 0). This means the output range is [0, 254] instead of the expected [0, 255], creating a subtle bias. Additionally, the scale() function divides by inMx - inMn = 255 - 0 = 255, but the actual range only reaches 254, meaning the maximum output coordinate can never actually reach the maximum of the output range — the SVG circles can never reach certain positions.

Fix: Changed % 255 → % 256 for full byte range [0, 255], and correspondingly updated all scale() calls from inMx = 255 → inMx = 256 to maintain correct mapping.

---

Vulnerability 7: Missing Division-by-Zero Guard in scale()

Severity: Low

Description: The scale() function computes .div(inMx.sub(inMn)) without validating that inMx > inMn. If called with inMx == inMn, the subtraction yields 0 and the division reverts with an opaque SafeMath error. While current callers always pass valid ranges, adding an explicit guard provides defense-in-depth and a clear revert message.

Fix: Added require(inMx > inMn, 'NFTDesc: invalid scale range') at the start of the function.

---

Vulnerability 8: uint8 Overflow Risk in feeToPercentString() — numSigfigs Counter

Severity: Low

Description: The numSigfigs variable uses uint8, limiting it to 255. While uint24 fee has at most 7 digits (max value 16,777,215), making overflow practically impossible with current fee values, this is a defensive coding issue. If the fee type were ever widened or the function reused, the uint8 would silently overflow. Additionally, numSigfigs is later used in SafeMath operations that expect uint256, requiring implicit widening conversions.

Fix: Changed uint8 numSigfigs → uint256 numSigfigs and added explicit uint8() casts where needed for struct field assignments.

---

Additional Fix: Hardhat Compiler Configuration

Severity: Informational

Description: Added Solidity 0.8.19 compiler overrides for Revenue_old.sol and RevenueV1.sol in hardhat.config.ts. These files use pragma solidity ^0.8.13 but no compiler was configured for them, causing HH606 compilation failures that prevented the entire project from compiling.

---

Summary Table

#	Vulnerability	Severity	Status
1	uint8 overflow in escapeQuotes() loop/counter	High	Fixed
2	Incorrect branding "Uniswap" → "Dex223"	Medium	Fixed
3	Division by zero in tickToDecimalString()	High	Fixed
4	Silent misbehavior for decimal diff > 18	Medium	Fixed
5	uint8 digit counter overflow + zero-value underflow	High	Fixed
6	Off-by-one % 255 → % 256 in getCircleCoord()	Low	Fixed
7	Missing div-by-zero guard in scale()	Low	Fixed
8	uint8 overflow risk in feeToPercentString()	Low	Fixed
9	Missing hardhat compiler config for Revenue files	Informational	Fixed

Test Plan

• Verify contract compiles successfully with npx hardhat compile
• Test escapeQuotes() with token symbol > 255 bytes — should not revert or loop infinitely
• Test escapeQuotes() with symbol containing > 255 quote characters — should properly escape all quotes
• Test constructTokenURI() produces JSON with "Dex223" branding (not "Uniswap")
• Test tickToDecimalString() with tickSpacing = 0 — should revert with 'NFTDesc: tickSpacing zero'
• Test adjustForDecimalPrecision() with baseTokenDecimals = 0, quoteTokenDecimals = 77 — should revert with 'NFTDesc: decimal diff > 18'
• Test fixedPointToDecimalString() with edge-case sqrtRatioX96 values near MIN_SQRT_RATIO and MAX_SQRT_RATIO
• Test getCircleCoord() can produce output value 255 (was impossible before)
• Test scale() correctly maps [0, 256] input range to output ranges
• Test feeToPercentString() with standard fee tiers: 100, 500, 3000, 10000
• Verify all pre-existing tests still pass (no regressions)

Made with Cursor