fix: security audit fixes for PositionValue.sol#47
Open
rroland10 wants to merge 1 commit into
Open
Conversation
- Add missing FullMath import (compilation reliability) - Add LowGasSafeMath for overflow-safe arithmetic in total() and _fees() - Use assembly unchecked subtraction for fee growth delta calculations in _fees() to correctly handle modular arithmetic wrapping - Use assembly unchecked subtraction in _getFeeGrowthInside() for all three tick position branches to match Uniswap V3 core Tick library behavior Co-authored-by: Cursor <cursoragent@cursor.com>
Member
I don't see any reason to dig into Uniswap's math since it works in production for years without any problems. We have higher chances of introducing bugs while trying to fix theoretical problems which don't cause any accidents in production.
Works for years in Uniswap. Let's not make changes just for the sake of changing something.
Another report related to Uniswap's math...
Math... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Audit Report:
PositionValue.solSummary
Comprehensive security audit of
contracts/dex-periphery/base/PositionValue.sol— a library that computes total token value (principal + fees) for Uniswap V3 NFT positions. This PR addresses 5 vulnerabilities spanning missing imports, arithmetic overflow/underflow, and modular arithmetic mishandling.Vulnerabilities Found & Fixes Applied
V-01: Missing
FullMathImport (Severity: High — Compilation Reliability)Location: Top-level imports
Description: The
_fees()function callsFullMath.mulDiv()butFullMathis never explicitly imported. The library resolves only through transitive imports (viaLiquidityAmounts.sol->FullMath.sol), making compilation dependent on import ordering and internal compiler behavior. Any refactoring of upstream dependencies could silently break this contract.Fix: Added explicit
import '../../libraries/FullMath.sol';import.V-02: Arithmetic Overflow in
total()(Severity: Medium)Location:
total()function, line 30Original Code:
return (amount0Principal + amount0Fee, amount1Principal + amount1Fee);Description: In Solidity 0.7.6 (pre-0.8.0), the
+operator does not have built-in overflow protection. Ifamount0Principal + amount0Feeexceedstype(uint256).max, the result silently wraps around to a much smaller number. While unlikely in normal operation, a position with extremely large fee accruals could trigger this, returning an incorrect (drastically underreported) total value.Fix: Replaced raw
+withLowGasSafeMath.add()which reverts on overflow:return (amount0Principal.add(amount0Fee), amount1Principal.add(amount1Fee));V-03: Fee Growth Underflow Revert in
_fees()(Severity: High — Can Lock Fee Queries)Location:
_fees()function, lines 131 and 139Original Code:
poolFeeGrowthInside0LastX128 - feeParams.positionFeeGrowthInside0LastX128Description: Uniswap V3 fee growth counters are designed to use modular (wrapping) arithmetic — they intentionally overflow and wrap around
uint256. The core pool'sTick.getFeeGrowthInside()uses Solidity 0.7.6 behavior where subtraction wraps, but this library's_fees()function performs the same subtraction with checked arithmetic. When the pool's fee growth counter wraps around pastuint256.max(by design), the subtractionpool - positionunderflows, causing the transaction to revert. This means fee queries become permanently broken for affected positions, effectively locking out any fee-dependent operations.Fix: Use inline assembly for unchecked subtraction that correctly wraps on underflow, accessing
positionFeeGrowthInside0LastX128(offset 0xC0) andpositionFeeGrowthInside1LastX128(offset 0xE0) from the FeeParams memory struct.V-04: Fee Growth Underflow Revert in
_getFeeGrowthInside()(Severity: High — Can Lock Fee Queries)Location:
_getFeeGrowthInside()function, lines 155-165 (all three tick position branches)Description: This is the same modular arithmetic issue as V-03 but in the fee growth inside calculation. The Uniswap V3 core
Tick.sollibrary'sgetFeeGrowthInside()function relies on unsigned integer wrapping for these exact same subtractions (see coreTick.sollines 68-94). The periphery's re-implementation uses checked subtraction in Solidity 0.7.6, which will revert when fee growth values wrap — exactly the scenario the core protocol was designed to handle.Fix: Wrapped all three branches in assembly blocks for unchecked subtraction, matching the core Tick library's intended modular arithmetic behavior.
V-05: Arithmetic Overflow in
_fees()Addition (Severity: Medium)Location:
_fees()function, lines 134 and 143Original Code:
FullMath.mulDiv(...) + feeParams.tokensOwed0;Description: The addition of computed uncollected fees and previously owed tokens (
tokensOwed0/tokensOwed1) uses raw+without overflow protection. WhileFullMath.mulDivitself is safe, the subsequent addition could overflow if the combined value exceedsuint256.max, silently wrapping to an incorrect small value.Fix: Replaced with
LowGasSafeMath.add():FullMath.mulDiv(...).add(feeParams.tokensOwed0);Files Changed
contracts/dex-periphery/base/PositionValue.solCompilation Verification
PositionValue.solcompiles cleanly with Solidity 0.7.6 (hardhat config)Revenue_old.sol/RevenueV1.sol(pragma mismatch) andDex223PoolLib.sol/MockTimeDex223Pool.sol(identifier errors) are not affected by this changeTest Plan
PositionValue.solcompiles without errors under Solidity 0.7.6PositionValueTest.solcompiles and all existing tests passtotal()with large principal + fee values nearuint256.maxto confirm overflow reverts correctlyfees()with fee growth counters that have wrapped pastuint256.maxto verify correct modular arithmetic_getFeeGrowthInside()for all three tick position branches (tickCurrent < tickLower,tickLower <= tickCurrent < tickUpper,tickCurrent >= tickUpper) with wrapped fee growth valuestokensOwedamountstotalGas(),principalGas(), andfeesGas()fromPositionValueTest.solpositionFeeGrowthInside0LastX128andpositionFeeGrowthInside1LastX128in the FeeParams struct