diff --git a/.changeset/swap-and-add-aggregated-slippage-fix.md b/.changeset/swap-and-add-aggregated-slippage-fix.md new file mode 100644 index 000000000..771c027a8 --- /dev/null +++ b/.changeset/swap-and-add-aggregated-slippage-fix.md @@ -0,0 +1,17 @@ +--- +"@uniswap/router-sdk": patch +--- + +Fix swap-and-add silently weakening EXACT_INPUT slippage protection on routes with >2 trades. + +`SwapRouter.encodeSwaps` previously toggled `performAggregatedSlippageCheck` whenever an +EXACT_INPUT trade had more than two routes, which set each individual swap's +`amountOutMinimum` to `0` and shifted enforcement onto a downstream `sweepToken` / +`unwrapWETH9` call. In the swap-and-add path there is no such trailing aggregate check; +the only remaining swap-output guard is `Position#mintAmountsWithSlippage`'s +`amount0Min` / `amount1Min`, which can legitimately drop well below the swap-derived +minimum when the position price range is wide or near a boundary. The result was that a +user's nominal 3% swap slippage could degrade to 30%+ effective slippage. Aggregation is +now suppressed when `isSwapAndAdd` is true, restoring per-swap slippage floors. + +Fixes #514. diff --git a/sdks/router-sdk/src/swapRouter.test.ts b/sdks/router-sdk/src/swapRouter.test.ts index b10eaaaed..d70962ae6 100644 --- a/sdks/router-sdk/src/swapRouter.test.ts +++ b/sdks/router-sdk/src/swapRouter.test.ts @@ -3,6 +3,7 @@ import { Pair, Route as V2Route, Trade as V2Trade } from '@uniswap/v2-sdk' import { encodeSqrtRatioX96, FeeAmount, + Multicall, nearestUsableTick, Pool, Position, @@ -17,6 +18,7 @@ import { SwapRouter, Trade } from '.' import { ApprovalTypes } from './approveAndCall' import { MixedRouteSDK } from './entities/mixedRoute/route' import { MixedRouteTrade } from './entities/mixedRoute/trade' +import { MulticallExtended } from './multicallExtended' import { ADDRESS_ZERO } from './constants' describe('SwapRouter', () => { @@ -1801,5 +1803,180 @@ describe('SwapRouter', () => { }) }) }) + + // Regression tests for #514: swap-and-add must not aggregate the slippage check. + // In a multi-route EXACT_INPUT swap-and-add there is no trailing `sweepToken` / + // `unwrapWETH9` that enforces the swap-derived `minimumAmountOut` against the + // router balance, so individual swap floors are the only swap-level guard the + // user has. Aggregating them silently weakens the user's slippage tolerance. + describe('#swapAndAddCallParameters aggregated-slippage regression (issue #514)', () => { + const pool_V3_0_WETH = makeV3Pool(token0, WETH, liquidity) + + // 3 routes from token0 -> WETH using only single-segment swaps so that + // each trade emits exactly one inner swap calldata. (A MixedRouteTrade + // with mixed protocols is intentionally avoided here because + // `partitionMixedRouteByProtocol` splits the route into multiple calls, + // making per-trade reasoning harder; the aggregate-sum check below + // covers that case implicitly.) + const amountIn = CurrencyAmount.fromRawAmount(token0, JSBI.BigInt(1000)) + const v2TradeA = V2Trade.exactIn(new V2Route([pair_0_1, pair_1_WETH], token0, WETH), amountIn) + const v2TradeB = V2Trade.exactIn(new V2Route([pair_0_1, pair_1_2, pair_2_WETH], token0, WETH), amountIn) + const v3Trade = V3Trade.fromRoute( + new V3Route([pool_V3_0_1, pool_V3_1_WETH], token0, WETH), + amountIn, + TradeType.EXACT_INPUT + ) + + const position = new Position({ + pool: pool_V3_0_WETH, + tickLower: -60, + tickUpper: 60, + liquidity: 1111111111, + }) + const addLiquidityOptions = { + recipient: '0x0000000000000000000000000000000000000006', + slippageTolerance, + deadline: 2 ** 32, + } + + // Decode the outer multicall (either bare `multicall(bytes[])` or the + // deadline-bearing `multicall(uint256,bytes[])` used by `MulticallExtended`) + // and pull the per-swap `amountOutMinimum` floor from every recognised + // V2/V3 swap call. Inner calls we don't recognise (`pull`, `approve`, + // `unwrapWETH9`, `sweepToken`, `mint`, ...) are ignored. + function decodePerSwapAmountOutMin(outerCalldata: string): JSBI[] { + const outerSelector = outerCalldata.slice(0, 10) + let inner: string[] + if (outerSelector === '0xac9650d8') { + // multicall(bytes[]) + inner = Multicall.decodeMulticall(outerCalldata) + } else { + // multicall(uint256,bytes[]) or multicall(bytes32,bytes[]) + const decoded = MulticallExtended.INTERFACE.parseTransaction({ data: outerCalldata }) + inner = decoded.args.data as string[] + } + + const mins: JSBI[] = [] + for (const data of inner) { + const selector = data.slice(0, 10) + let fn: ReturnType + try { + fn = SwapRouter.INTERFACE.getFunction(selector as any) + } catch { + continue + } + const decoded = SwapRouter.INTERFACE.decodeFunctionData(fn, data) + switch (fn.name) { + case 'exactInput': + case 'exactInputSingle': + mins.push(JSBI.BigInt(decoded.params.amountOutMinimum.toString())) + break + case 'swapExactTokensForTokens': + mins.push(JSBI.BigInt(decoded.amountOutMin.toString())) + break + default: + break + } + } + return mins + } + + it('preserves per-swap amountOutMinimum on 3-route EXACT_INPUT swap-and-add', async () => { + const trades = [v2TradeA, v2TradeB, await v3Trade] + const { calldata } = SwapRouter.swapAndAddCallParameters( + trades, + { slippageTolerance }, + position, + addLiquidityOptions, + ApprovalTypes.NOT_REQUIRED, + ApprovalTypes.NOT_REQUIRED + ) + + const mins = decodePerSwapAmountOutMin(calldata) + expect(mins).toHaveLength(3) + + // Each individual swap must enforce its own slippage floor. + for (const m of mins) { + expect(JSBI.greaterThan(m, JSBI.BigInt(0))).toBe(true) + } + + // The aggregated floor reported by the SDK must match the sum of + // per-swap `amountOutMinimum`s - that is, every wei of swap-level + // protection promised to the user is actually enforced on-chain. + const expectedAggregateMin = trades + .map((t) => t.minimumAmountOut(slippageTolerance).quotient) + .reduce((a, b) => JSBI.add(a, b), JSBI.BigInt(0)) + const actualAggregateMin = mins.reduce((a, b) => JSBI.add(a, b), JSBI.BigInt(0)) + expect(actualAggregateMin.toString()).toEqual(expectedAggregateMin.toString()) + }) + + it('regular #swapCallParameters still aggregates the slippage check (3+ EXACT_INPUT trades)', async () => { + // Control: the original optimization is preserved for the non-swap-and-add + // path, where a trailing `sweepToken` / `unwrapWETH9` enforces the aggregated minimum. + const trades = [v2TradeA, v2TradeB, await v3Trade] + const { calldata } = SwapRouter.swapCallParameters(trades, { + slippageTolerance, + recipient, + deadlineOrPreviousBlockhash: deadline, + }) + + const mins = decodePerSwapAmountOutMin(calldata) + expect(mins).toHaveLength(3) + for (const m of mins) { + expect(m.toString()).toEqual('0') + } + }) + + it('two-route swap-and-add is unchanged (numberOfTrades > 2 is false)', async () => { + // Edge case: 2 trades never tripped the aggregated heuristic, so the + // fix must not alter encoding for the 2-trade path. We assert that + // both swaps carry a non-zero floor, matching pre-fix behaviour. + const trades = [v2TradeA, await v3Trade] + const { calldata } = SwapRouter.swapAndAddCallParameters( + trades, + { slippageTolerance }, + position, + addLiquidityOptions, + ApprovalTypes.NOT_REQUIRED, + ApprovalTypes.NOT_REQUIRED + ) + + const mins = decodePerSwapAmountOutMin(calldata) + expect(mins).toHaveLength(2) + for (const m of mins) { + expect(JSBI.greaterThan(m, JSBI.BigInt(0))).toBe(true) + } + }) + + it('aggregate swap-and-add floor is never lower than the SDK-reported minimum', async () => { + // Property check that also covers MixedRouteTrade-with-sectioning by + // summing across however many inner swap calldatas are emitted. Pre-fix + // the per-swap values were all zero and this lower-bound was violated; + // post-fix the inequality always holds. + const mixedTrade = MixedRouteTrade.fromRoute( + new MixedRouteSDK([pair_0_1, pool_V3_1_WETH], token0, WETH), + amountIn, + TradeType.EXACT_INPUT + ) + + const trades = [v2TradeA, await v3Trade, await mixedTrade] + const { calldata } = SwapRouter.swapAndAddCallParameters( + trades, + { slippageTolerance }, + position, + addLiquidityOptions, + ApprovalTypes.NOT_REQUIRED, + ApprovalTypes.NOT_REQUIRED + ) + + const mins = decodePerSwapAmountOutMin(calldata) + const aggregateMin = mins.reduce((a, b) => JSBI.add(a, b), JSBI.BigInt(0)) + const documentedMin = trades + .map((t) => t.minimumAmountOut(slippageTolerance).quotient) + .reduce((a, b) => JSBI.add(a, b), JSBI.BigInt(0)) + + expect(JSBI.greaterThanOrEqual(aggregateMin, documentedMin)).toBe(true) + }) + }) }) }) diff --git a/sdks/router-sdk/src/swapRouter.ts b/sdks/router-sdk/src/swapRouter.ts index d9fca8027..1e54bd720 100644 --- a/sdks/router-sdk/src/swapRouter.ts +++ b/sdks/router-sdk/src/swapRouter.ts @@ -429,7 +429,16 @@ export abstract class SwapRouter { // 1. when there are >2 exact input trades. this is only a heuristic, // as it's still more gas-expensive even in this case, but has benefits // in that the reversion probability is lower - const performAggregatedSlippageCheck = sampleTrade.tradeType === TradeType.EXACT_INPUT && numberOfTrades > 2 + // 2. NEVER for swap-and-add: in that path, individual swap recipients are + // ADDRESS_ZERO (router custody) and there is no trailing `sweepToken` / + // `unwrapWETH9` call that enforces the aggregated `minimumAmountOut` + // against the router's balance. The only remaining downstream guard is + // `mint`'s `amount0Min` / `amount1Min`, which `Position#mintAmountsWithSlippage` + // may legitimately lower far below the swap-derived minimum when the + // position price range is wide or near a boundary. Aggregating in that + // case silently weakens the user's slippage tolerance - see #514. + const performAggregatedSlippageCheck = + sampleTrade.tradeType === TradeType.EXACT_INPUT && numberOfTrades > 2 && !isSwapAndAdd // flag for whether funds should be send first to the router // 1. when receiving ETH (which much be unwrapped from WETH) // 2. when a fee on the output is being taken