Spearbit 89 Unsafe casting and overflow of negation (#341)

* Spearbit 89 Unsafe casting and overflow of negation * format * make uint128
Uniswap · Sep 5, 2024 · 6213761 · 6213761
1 parent d1afb1b
commit 6213761
Show file tree

Hide file tree

Showing 49 changed files with 70 additions and 53 deletions.
diff --git a/.forge-snapshots/Payments_swap_settleFromCaller_takeAllToMsgSender.snap b/.forge-snapshots/Payments_swap_settleFromCaller_takeAllToMsgSender.snap
@@ -1 +1 @@
-129459
+129465
diff --git a/.forge-snapshots/Payments_swap_settleFromCaller_takeAllToSpecifiedAddress.snap b/.forge-snapshots/Payments_swap_settleFromCaller_takeAllToSpecifiedAddress.snap
@@ -1 +1 @@
-131377
+131383
diff --git a/.forge-snapshots/Payments_swap_settleWithBalance_takeAllToMsgSender.snap b/.forge-snapshots/Payments_swap_settleWithBalance_takeAllToMsgSender.snap
@@ -1 +1 @@
-123612
+123588
diff --git a/.forge-snapshots/Payments_swap_settleWithBalance_takeAllToSpecifiedAddress.snap b/.forge-snapshots/Payments_swap_settleWithBalance_takeAllToSpecifiedAddress.snap
@@ -1 +1 @@
-123754
+123730
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_erc20_withClose.snap b/.forge-snapshots/PositionManager_increaseLiquidity_erc20_withClose.snap
@@ -1 +1 @@
-154735
+154710
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_erc20_withSettlePair.snap b/.forge-snapshots/PositionManager_increaseLiquidity_erc20_withSettlePair.snap
@@ -1 +1 @@
-153793
+153768
diff --git a/.forge-snapshots/PositionManager_increaseLiquidity_native.snap b/.forge-snapshots/PositionManager_increaseLiquidity_native.snap
@@ -1 +1 @@
-136186
+136161
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap b/.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap
@@ -1 +1 @@
-132042
+132017
diff --git a/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap b/.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap
@@ -1 +1 @@
-172746
+172721
diff --git a/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap b/.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap
@@ -1 +1 @@
-143428
+143403
diff --git a/.forge-snapshots/PositionManager_mint_native.snap b/.forge-snapshots/PositionManager_mint_native.snap
@@ -1 +1 @@
-339884
+339859
diff --git a/.forge-snapshots/PositionManager_mint_nativeWithSweep_withClose.snap b/.forge-snapshots/PositionManager_mint_nativeWithSweep_withClose.snap
@@ -1 +1 @@
-348264
+348239
diff --git a/.forge-snapshots/PositionManager_mint_nativeWithSweep_withSettlePair.snap b/.forge-snapshots/PositionManager_mint_nativeWithSweep_withSettlePair.snap
@@ -1 +1 @@
-347630
+347605
diff --git a/.forge-snapshots/PositionManager_mint_onSameTickLower.snap b/.forge-snapshots/PositionManager_mint_onSameTickLower.snap
@@ -1 +1 @@
-318202
+318177
diff --git a/.forge-snapshots/PositionManager_mint_onSameTickUpper.snap b/.forge-snapshots/PositionManager_mint_onSameTickUpper.snap
@@ -1 +1 @@
-318872
+318847
diff --git a/.forge-snapshots/PositionManager_mint_sameRange.snap b/.forge-snapshots/PositionManager_mint_sameRange.snap
@@ -1 +1 @@
-244441
+244416
diff --git a/.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap b/.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap
@@ -1 +1 @@
-374127
+374102
diff --git a/.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap b/.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap
@@ -1 +1 @@
-324233
+324208
diff --git a/.forge-snapshots/PositionManager_mint_withClose.snap b/.forge-snapshots/PositionManager_mint_withClose.snap
@@ -1 +1 @@
-375533
+375508
diff --git a/.forge-snapshots/PositionManager_mint_withSettlePair.snap b/.forge-snapshots/PositionManager_mint_withSettlePair.snap
@@ -1 +1 @@
-374733
+374708
diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap
@@ -1 +1 @@
-419680
+419655
diff --git a/.forge-snapshots/V4Router_Bytecode.snap b/.forge-snapshots/V4Router_Bytecode.snap
@@ -1 +1 @@
-7107
+7063
diff --git a/.forge-snapshots/V4Router_ExactIn1Hop_nativeIn.snap b/.forge-snapshots/V4Router_ExactIn1Hop_nativeIn.snap
@@ -1 +1 @@
-115155
+115185
diff --git a/.forge-snapshots/V4Router_ExactIn1Hop_nativeOut.snap b/.forge-snapshots/V4Router_ExactIn1Hop_nativeOut.snap
@@ -1 +1 @@
-115632
+115662
diff --git a/.forge-snapshots/V4Router_ExactIn1Hop_oneForZero.snap b/.forge-snapshots/V4Router_ExactIn1Hop_oneForZero.snap
@@ -1 +1 @@
-124447
+124477
diff --git a/.forge-snapshots/V4Router_ExactIn1Hop_zeroForOne.snap b/.forge-snapshots/V4Router_ExactIn1Hop_zeroForOne.snap
@@ -1 +1 @@
-130165
+130195
diff --git a/.forge-snapshots/V4Router_ExactIn2Hops.snap b/.forge-snapshots/V4Router_ExactIn2Hops.snap
@@ -1 +1 @@
-179173
+179203
diff --git a/.forge-snapshots/V4Router_ExactIn2Hops_nativeIn.snap b/.forge-snapshots/V4Router_ExactIn2Hops_nativeIn.snap
@@ -1 +1 @@
-169873
+169903
diff --git a/.forge-snapshots/V4Router_ExactIn3Hops.snap b/.forge-snapshots/V4Router_ExactIn3Hops.snap
@@ -1 +1 @@
-228160
+228190
diff --git a/.forge-snapshots/V4Router_ExactIn3Hops_nativeIn.snap b/.forge-snapshots/V4Router_ExactIn3Hops_nativeIn.snap
@@ -1 +1 @@
-218884
+218914
diff --git a/.forge-snapshots/V4Router_ExactInputSingle.snap b/.forge-snapshots/V4Router_ExactInputSingle.snap
@@ -1 +1 @@
-129459
+129465
diff --git a/.forge-snapshots/V4Router_ExactInputSingle_nativeIn.snap b/.forge-snapshots/V4Router_ExactInputSingle_nativeIn.snap
@@ -1 +1 @@
-114449
+114455
diff --git a/.forge-snapshots/V4Router_ExactInputSingle_nativeOut.snap b/.forge-snapshots/V4Router_ExactInputSingle_nativeOut.snap
@@ -1 +1 @@
-114895
+114901
diff --git a/.forge-snapshots/V4Router_ExactOut1Hop_nativeIn_sweepETH.snap b/.forge-snapshots/V4Router_ExactOut1Hop_nativeIn_sweepETH.snap
@@ -1 +1 @@
-121387
+121413
diff --git a/.forge-snapshots/V4Router_ExactOut1Hop_nativeOut.snap b/.forge-snapshots/V4Router_ExactOut1Hop_nativeOut.snap
@@ -1 +1 @@
-116688
+116714
diff --git a/.forge-snapshots/V4Router_ExactOut1Hop_oneForZero.snap b/.forge-snapshots/V4Router_ExactOut1Hop_oneForZero.snap
@@ -1 +1 @@
-125503
+125529
diff --git a/.forge-snapshots/V4Router_ExactOut1Hop_zeroForOne.snap b/.forge-snapshots/V4Router_ExactOut1Hop_zeroForOne.snap
@@ -1 +1 @@
-129420
+129446
diff --git a/.forge-snapshots/V4Router_ExactOut2Hops.snap b/.forge-snapshots/V4Router_ExactOut2Hops.snap
@@ -1 +1 @@
-179252
+179274
diff --git a/.forge-snapshots/V4Router_ExactOut2Hops_nativeIn.snap b/.forge-snapshots/V4Router_ExactOut2Hops_nativeIn.snap
@@ -1 +1 @@
-175136
+175158
diff --git a/.forge-snapshots/V4Router_ExactOut3Hops.snap b/.forge-snapshots/V4Router_ExactOut3Hops.snap
@@ -1 +1 @@
-229091
+229109
diff --git a/.forge-snapshots/V4Router_ExactOut3Hops_nativeIn.snap b/.forge-snapshots/V4Router_ExactOut3Hops_nativeIn.snap
@@ -1 +1 @@
-224999
+225017
diff --git a/.forge-snapshots/V4Router_ExactOut3Hops_nativeOut.snap b/.forge-snapshots/V4Router_ExactOut3Hops_nativeOut.snap
@@ -1 +1 @@
-220300
+220318
diff --git a/.forge-snapshots/V4Router_ExactOutputSingle.snap b/.forge-snapshots/V4Router_ExactOutputSingle.snap
@@ -1 +1 @@
-128698
+128716
diff --git a/.forge-snapshots/V4Router_ExactOutputSingle_nativeIn_sweepETH.snap b/.forge-snapshots/V4Router_ExactOutputSingle_nativeIn_sweepETH.snap
@@ -1 +1 @@
-120665
+120683
diff --git a/.forge-snapshots/V4Router_ExactOutputSingle_nativeOut.snap b/.forge-snapshots/V4Router_ExactOutputSingle_nativeOut.snap
@@ -1 +1 @@
-116041
+116059
diff --git a/src/PositionManager.sol b/src/PositionManager.sol
@@ -365,6 +365,7 @@ contract PositionManager is
         // the locker is the payer or receiver
         address caller = msgSender();
         if (currencyDelta < 0) {
+            // Casting is safe due to limits on the total supply of a pool
             _settle(currency, caller, uint256(-currencyDelta));
         } else if (currencyDelta > 0) {
             _take(currency, caller, uint256(currencyDelta));
@@ -417,6 +418,7 @@ contract PositionManager is
         if (payer == address(this)) {
             currency.transfer(address(poolManager), amount);
         } else {
+            // Casting from uint256 to uint160 is safe due to limits on the total supply of a pool
             permit2.transferFrom(payer, address(poolManager), uint160(amount), Currency.unwrap(currency));
         }
     }

diff --git a/src/V4Router.sol b/src/V4Router.sol
@@ -91,7 +91,7 @@ abstract contract V4Router is IV4Router, BaseActionsRouter, DeltaResolver {
                 _getFullCredit(params.zeroForOne ? params.poolKey.currency0 : params.poolKey.currency1).toUint128();
         }
         uint128 amountOut = _swap(
-            params.poolKey, params.zeroForOne, int256(-int128(amountIn)), params.sqrtPriceLimitX96, params.hookData
+            params.poolKey, params.zeroForOne, -int256(uint256(amountIn)), params.sqrtPriceLimitX96, params.hookData
         ).toUint128();
         if (amountOut < params.amountOutMinimum) revert V4TooLittleReceived(params.amountOutMinimum, amountOut);
     }
@@ -127,8 +127,16 @@ abstract contract V4Router is IV4Router, BaseActionsRouter, DeltaResolver {
                 _getFullDebt(params.zeroForOne ? params.poolKey.currency1 : params.poolKey.currency0).toUint128();
         }
         uint128 amountIn = (
-            -_swap(
-                params.poolKey, params.zeroForOne, int256(int128(amountOut)), params.sqrtPriceLimitX96, params.hookData
+            uint256(
+                -int256(
+                    _swap(
+                        params.poolKey,
+                        params.zeroForOne,
+                        int256(uint256(amountOut)),
+                        params.sqrtPriceLimitX96,
+                        params.hookData
+                    )
+                )
             )
         ).toUint128();
         if (amountIn > params.amountInMaximum) revert V4TooMuchRequested(params.amountInMaximum, amountIn);
@@ -151,7 +159,9 @@ abstract contract V4Router is IV4Router, BaseActionsRouter, DeltaResolver {
                 pathKey = params.path[i - 1];
                 (PoolKey memory poolKey, bool oneForZero) = pathKey.getPoolAndSwapDirection(currencyOut);
                 // The output delta will always be negative, except for when interacting with certain hook pools
-                amountIn = (-_swap(poolKey, !oneForZero, int256(uint256(amountOut)), 0, pathKey.hookData)).toUint128();
+                amountIn = (
+                    uint256(-int256(_swap(poolKey, !oneForZero, int256(uint256(amountOut)), 0, pathKey.hookData)))
+                ).toUint128();
 
                 amountOut = amountIn;
                 currencyOut = pathKey.intermediateCurrency;

diff --git a/src/base/DeltaResolver.sol b/src/base/DeltaResolver.sol
@@ -54,6 +54,7 @@ abstract contract DeltaResolver is ImmutableState {
         int256 _amount = poolManager.currencyDelta(address(this), currency);
         // If the amount is positive, it should be taken not settled.
         if (_amount > 0) revert DeltaNotNegative(currency);
+        // Casting is safe due to limits on the total supply of a pool
         amount = uint256(-_amount);
     }
 

diff --git a/src/libraries/SlippageCheck.sol b/src/libraries/SlippageCheck.sol
@@ -41,9 +41,13 @@ library SlippageCheck {
         // Thus, we only cast the delta if it is guaranteed to be negative.
         // And we do NOT revert in the positive delta case. Since a positive delta means the hook is crediting tokens to the user for minting/increasing liquidity, we do not check slippage.
         // This means this contract will NOT support _positive_ slippage checks (minAmountOut checks) on pools where the hook returns a positive delta on mint/increase.
-        int128 amount0 = delta.amount0();
-        int128 amount1 = delta.amount1();
-        if (amount0 < 0 && amount0Max < uint128(-amount0)) revert MaximumAmountExceeded(amount0Max, uint128(-amount0));
-        if (amount1 < 0 && amount1Max < uint128(-amount1)) revert MaximumAmountExceeded(amount1Max, uint128(-amount1));
+        int256 amount0 = delta.amount0();
+        int256 amount1 = delta.amount1();
+        if (amount0 < 0 && amount0Max < uint128(uint256(-amount0))) {
+            revert MaximumAmountExceeded(amount0Max, uint128(uint256(-amount0)));
+        }
+        if (amount1 < 0 && amount1Max < uint128(uint256(-amount1))) {
+            revert MaximumAmountExceeded(amount1Max, uint128(uint256(-amount1)));
+        }
     }
 }