Vfat - MultiSwapRouter

Users lose funds when swaps partially fill, as the router pre-pulls the full input but only the consumed portion is used. The remainder is stranded — either in the router (recoverable by admin only) or in an external router (unrecoverable entirely).

The router's _pullInput() unconditionally transfers the full amountIn from the caller into the router. Multiple swap paths are then vulnerable to partial fills where the pool consumes less than the full input:

1. Direct pool swaps (UniswapV3Pool / AlgebraPool). _swapAlgebraPool() and _swapUniswapV3Pool() call pool.swap() with amountSpecified = int256(amountIn). If liquidity is insufficient, the pool partially fills. The callbacks (uniswapV3SwapCallback(), algebraSwapCallback()) pay only the actual positive delta, not the full amount. The difference remains stranded in the router, recoverable only via admin sweep() to the fee collector.

2. UniswapV4. _swapUniswapV4() builds a SWAP_EXACT_IN_SINGLE + SETTLE_ALL + TAKE_ALL action sequence. SETTLE_ALL settles based on the actual swap delta, not the full amountIn. On partial fills, Permit2 pulls only the consumed amount from the router. The unconsumed tokens remain stranded in the router after the Permit2 approval is revoked at L622-624.

3. VelodromeUniversalRouter (CL route). _swapVelodromeUniversalRouter() pushes the full amountIn to the Velodrome router via safeTransfer before executing the swap. When using the CL path (command = 0x00), the underlying Slipstream pool can partially fill like any V3-style pool. The unconsumed tokens remain stranded on the Velodrome router itself, not on the MultiSwapRouter. The MSR admin has no ability to recover these tokens — they are permanently lost to the user unless the Velodrome router has its own sweep mechanism controlled by a separate admin.

Medium. Users permanently lose unconsumed input on partial fills. For direct pool and V4 swaps, stranded tokens remain in the router and are swept to the fee collector. For VelodromeUniversalRouter CL swaps, stranded tokens are sent to the external router and are entirely unrecoverable by the MSR admin.

For direct pool swaps and V4, revert when the consumed amount differs from amountIn to prevent partial fills entirely or compute the delta and transfer back any unused tokens.

For VelodromeUniversalRouter, switch from the token push pattern to approve-call-revoke with payerIsUser = true, consistent with other router-based swaps. This keeps unconsumed tokens in the MSR where they can at least be swept.

Fixed in PR#653 & PR659.

The pre-fee minimum for UniswapV4 final steps uses a ceiling division that doesn't match the floor-rounded fee deduction, rejecting valid swaps at the V4 TAKE_ALL level.

_requiredOutputBeforeFee() inverts the continuous fee formula via ceiling division, but _applyFeeAndTransfer() uses floor division for the actual fee:

// MultiSwapRouter.sol#L849 — ceiling of continuous inverse
return (minAmountOut * BPS_DENOMINATOR + denom - 1) / denom;

// MultiSwapRouter.sol#L299 — actual fee uses floor
fee = (totalOutput * feeBps) / BPS_DENOMINATOR;

Because the fee rounds down, the user retains more than the continuous formula predicts, so the ceiling-based inverse overshoots the true minimum by 1 unit.

Additionally, swapSplit() never applies this V4 pre-fee minimum — _executeSingleRoute() always passes 0 as stepMinOut, so the same V4 swap that reverts via swap() succeeds via swapSplit() with a single route.

Low. Swaps where the V4 output falls exactly at the 1-unit boundary between the true minimum and the ceiling-derived bound will revert unnecessarily via swap(), wasting gas. The inconsistency between swap() and swapSplit() complicates integrator routing logic. No funds are at risk.

Replace the ceiling formula with the correct discrete inverse that accounts for floor-rounded fee deduction, and apply the same logic in swapSplit() for consistency.

Alternatively consider passing 0like in swapSplit().

Fixed in commit 345d0b9c.

The router lacks two related validation checks that together allow an attacker to extract any ERC20 or ETH balance held by the router. First, _swapUniswapV3Pool() validates only tokenIn against the pool but not tokenOut. Second, neither _swap() nor _executeSingleRoute() enforce that each step's tokenIn matches the previous step's tokenOut. These can be exploited independently or combined for more complex drains.

1. V3 direct pool tokenOut spoofing. _validateV3PoolAndDirection() confirms tokenIn is either token0 or token1 but ignores step.tokenOut. The swap at _swapUniswapV3Pool() computes amountOut from pool deltas without reference to step.tokenOut. An attacker can reference a legitimate A/B pool, set tokenIn = A and tokenOut = X (an unrelated token held by the router), and _applyFeeAndTransfer() will transfer amountOut units of X to the attacker while the actual pool output B remains stranded. Note that _swapAlgebraPool() already validates both tokens via _validateAlgebraPoolAndDirection(), making only the V3 path vulnerable.

2. Step token discontinuity. _swap() pulls only the first step's input from the caller and threads a numeric currentAmount through subsequent steps without verifying steps[i].tokenIn == steps[i-1].tokenOut. Similarly, _executeSingleRoute() in swapSplit() applies no per-step continuity check. An attacker can construct a multi-step swap where step 0 swaps a small amount of their own token, then step 1 declares a completely different tokenIn corresponding to a valuable token the router holds. The router approves and spends its pre-existing balance of that token via _callRouterWithApproval(), transferring the proceeds to the attacker. This works with any dex type, not just V3 direct pools.

3. Combined exploitation. Both vectors can be chained: a V3 pool step with spoofed tokenOut manufactures an arbitrary currentAmount, then a subsequent step with a discontinuous tokenIn spends a different router-held token using that spoofed amount.

Low. Drains any ERC20 or ETH balance residing on the router (dust, accidental transfers, airdrops, stranded tokens from prior swaps). Does not directly compromise user funds passing through well-formed swaps. Requires the router to hold a non-trivial balance of the target token.

Validate tokenOut against the pool's opposite token in _validateV3PoolAndDirection():

 function _validateV3PoolAndDirection(
     address pool,
-    address tokenIn
+    address tokenIn,
+    address tokenOut
 ) internal view returns (bool zeroForOne) {
     ...
     if (tokenIn == token0) {
+        if (tokenOut != token1) revert InvalidTokenIn();
         return true;
     }
     if (tokenIn == token1) {
+        if (tokenOut != token0) revert InvalidTokenIn();
         return false;
     }
     revert InvalidTokenIn();
 }

Enforce per-step token continuity in _swap():

 function _swap(
     SwapStep[] calldata steps,
     uint256 amountIn,
     uint256 minAmountOut,
     address recipient,
     uint256 deadline
 ) internal returns (uint256 amountOut) {
     if (block.timestamp > deadline) revert Expired();
     if (steps.length == 0) revert InvalidStepsLength();

     _pullInput(steps[0].tokenIn, amountIn);

     uint256 currentAmount = amountIn;
     uint256 minOutBeforeFee = _requiredOutputBeforeFee(minAmountOut);
     for (uint256 i; i < steps.length; i++) {
+        if (i > 0 && steps[i].tokenIn != steps[i - 1].tokenOut) {
+            revert InvalidTokenPath();
+        }
         uint256 stepMinOut;
         if (i == steps.length - 1 && steps[i].dexType == DexType.UniswapV4) {
             stepMinOut = minOutBeforeFee;
         }
         currentAmount = _executeStep(steps[i], currentAmount, stepMinOut);
     }
     ...
 }

Apply the same continuity check in _executeSingleRoute() for swapSplit():

 function _executeSingleRoute(
     SwapStep[] calldata steps,
     uint256 amountIn
 ) internal returns (uint256 currentAmount) {
     currentAmount = amountIn;
     for (uint256 s; s < steps.length; s++) {
+        if (s > 0 && steps[s].tokenIn != steps[s - 1].tokenOut) {
+            revert InvalidTokenPath();
+        }
         currentAmount = _executeStep(steps[s], currentAmount, 0);
     }
 }

Fixed in PR#654.

_wrapWETH() validates the wrap amount against address(this).balance, the router's entire ETH balance, without distinguishing between ETH provided in the current call and pre-existing ETH. Combined with the lack of step token continuity, an attacker can wrap and extract ETH that was not supplied by their transaction.

_wrapWETH() checks address(this).balance < amountIn but does not track how much ETH belongs to the current operation versus pre-existing balances (from accidental transfers, selfdestruct forced sends, or refunds). Since WrapWETH and UnwrapWETH steps are exempt from the router allowlist check in _executeStep(), and no per-step token continuity is enforced, an attacker can:

1. Execute a swap through a permissionless pool on an allowlisted factory that returns an attacker-chosen currentAmount.
2. Use that currentAmount as input to a WrapWETH step, which wraps that many wei from the router's global ETH balance.
3. Receive the resulting WETH (or unwrap it back to ETH) through _applyFeeAndTransfer().

This bypasses the admin-only sweepETH() path intended to recover stranded ETH.

Low. Any ETH accumulated on the router can be extracted by any caller. Loss is bounded to the router's pre-existing ETH balance and does not directly drain user funds.

Introduce per-call ETH accounting to isolate ETH from the current operation. Record the initial balance at the start of swap()/swapSplit() and pass it through to _wrapWETH():

uint256 initialEthBalance = address(this).balance - msg.value;

Then in _wrapWETH(), replace the current balance check:

uint256 callEthBalance = address(this).balance - initialEthBalance;
if (callEthBalance < amountIn) {
    revert InsufficientEthBalance();
}

Additionally, enforcing step token continuity (steps[i].tokenIn == steps[i-1].tokenOut) as recommended in other findings would prevent arbitrary WrapWETH insertion into multi-step sequences.

Token continuity checks (PR #654) and amountIn == msg.value validation in _pullInput already block the attack path to _wrapWETH.

_executeBridgeAndRefund() calls the allowlisted bridge contract with fully user-supplied calldata, allowing any function on that contract to be invoked. Unlike swap paths where the MSR constructs calldata internally, bridge callers control the function selector entirely.

In _executeBridgeAndRefund(), the bridge call is:

(bool success, bytes memory result) =
    bridgeContract.call{ value: bridgeNativeValue }(bridgeCalldata);

The bridgeCalldata is passed through verbatim from the external caller. While bridgeContract is validated against the router allowlist via allowlist.requireAllowed(bridgeContract), no restriction is placed on which function is called.

Bridge contracts typically expose multiple entry points beyond their bridging function — admin functions, token recovery, alternative transfer modes, or proxy upgrade functions. An allowlisted bridge contract with a secondary function that transfers tokens to a caller-specified address (rather than bridging cross-chain) could be misused.

Low. Requires the allowlisted bridge contract to have a secondary function exploitable within the approve-call-revoke flow. The consumption check limits the blast radius to the current call's deposited amount. However, the attack surface is broader than necessary.

Add a function selector allowlist to the RouterAllowlist contract, mapping (bridgeContract, bytes4 selector) → bool. Validate the first 4 bytes of bridgeCalldata against this allowlist before executing the call:

bytes4 selector = bytes4(bridgeCalldata[:4]);
allowlist.requireAllowedSelector(bridgeContract, selector);

With a selector allowlist in place, the nonReentrant guard on swapAndBridge() and bridge() becomes likely unnecessary — the bridge contract is allowlisted and can only execute pre-approved functions, matching the trust model of swap() and swapSplit() which operate under the same allowlist without a reentrancy guard.

Fixed in 05c175fd.

In _swapAndCollectFee(), the minSwapOut slippage check is performed on the raw swap output before protocol fees are deducted. This is inconsistent with the regular swap path in _applyFeeAndTransfer(), where slippage is checked after fees, and can cause users to receive less than their intended minimum.

In the swap-and-bridge flow, _swapAndCollectFee() validates the swap output against minSwapOut at line if (currentAmount < minSwapOut), then subsequently deducts fees from the output amount. This means a user setting minSwapOut to protect against slippage may still receive minSwapOut - fee worth of tokens forwarded to the bridge.

In contrast, the standard swap path uses _applyFeeAndTransfer(), which first calculates amountAfterFee = totalOutput - fee and then checks if (amountAfterFee < minAmountOut). This correctly ensures the user receives at least minAmountOut after all deductions.

The discrepancy means that in the bridge path, the effective minimum output is lower than what the user specified by up to feeBps / BPS_DENOMINATOR of the output amount (currently up to 1%).

Low. Users bridging through the swap-and-bridge flow may receive less than their intended minimum output amount. The difference is bounded by the protocol fee (up to 1%), but the slippage protection does not function as expected.

Move the minSwapOut check to after the fee is deducted, or adjust the check to account for the fee, consistent with how _applyFeeAndTransfer() handles it in the regular swap path.

Fixed in 05c175fd.

RouterAllowlist._setRouter() accepts any address without validation, including address(0) and EOAs. When a swap step targets these invalid routers, the low-level call in _callRouter() succeeds silently (no code executed), producing zero output while input tokens remain stranded in the router.

_setRouter() does not validate that the router address is a contract. _callRouter() performs router.call{value: value}(data) and only reverts if success == false. Since low-level calls to address(0) or EOAs return success = true, the swap step completes as a no-op. Input tokens pulled into the router are only recoverable via admin sweep() to the fee collector, not returned to the user.

Informational. Requires admin misconfiguration (allowlisting an invalid address) combined with a caller submitting a swap with insufficient minAmountOut protection.

Validate in _setRouter() that router != address(0) and router.code.length > 0 when allowed == true.

Fixed in PR#656.

The router's accounting assumes standard ERC20 transfer behavior across multiple paths. Fee-on-transfer, rebasing, and reflection tokens can cause mismatches between expected and actual balances, allowing callers to extract pre-existing router-held tokens or causing reverts.

Three distinct paths are affected:

1. Direct pool swaps trust pool deltas for output accounting. _swapUniswapV3Pool() and _swapAlgebraPool() compute amountOut from the pool's returned deltas rather than measuring actual balance change. If the output token charges transfer fees, the router receives less than reported, and _applyFeeAndTransfer() consumes pre-existing router balance to cover the shortfall. Router-based swaps avoid this via _balanceOf() / _balanceDelta(), but direct pool swaps bypass that safeguard.

2. Input pull does not verify actual received amount. _pullInput() transfers the full nominal amountIn via safeTransferFrom without measuring the balance delta. For fee-on-transfer input tokens, the router receives less than amountIn but approves and spends the full nominal amount downstream in _callRouterWithApproval(), drawing from any pre-existing tokenIn balance on the router.

3. Balance-delta output accounting is unsafe for rebasing tokens. _swapViaRouter() measures output via _balanceDelta(). For rebasing or reflection tokens, balance changes during the external call get misattributed as swap output (positive rebase leaks router holdings) or cause underflow reverts (negative rebase) in _balanceDelta().

In all three cases, exploitation requires the router to hold a pre-existing balance of the affected token and a non-standard token to be used in the swap path.

Informational. Callers can extract pre-existing router-held balances (dust, accidental transfers, stranded tokens) when non-standard tokens are involved. Does not directly compromise user-deposited funds. Loss is capped at the router's holdings of the affected token.

Either explicitly block non-standard tokens or apply consistent balance-before/after measurement across all paths:

• In _pullInput(), measure actual received amount and revert if it differs from amountIn.
• In _swapUniswapV3Pool() and _swapAlgebraPool(), use _balanceOf() / _balanceDelta() instead of pool deltas, consistent with router-based swaps.
• In _balanceDelta(), add an explicit underflow check to produce a clear revert instead of a silent panic.

If non-standard tokens are intentionally unsupported, document this restriction explicitly.

Fixed in PR#657.

_swapUniswapV3Pool() and _swapAlgebraPool() compute amountOut from the pool's returned swap deltas rather than measuring the router's actual tokenOut balance increase. If the reported delta overstates what the router received (e.g., fee-on-transfer output tokens), _applyFeeAndTransfer() may consume pre-existing tokenOut dust held by the router.

Both _swapUniswapV3Pool() and _swapAlgebraPool() calculate output as:

uint256 amountOut = zeroForOne ? uint256(-amount1) : uint256(-amount0);

This trusts the pool's accounting. Router-based swaps (_swapViaRouter()) instead use _balanceOf() / _balanceDelta() to reconcile actual received amounts. Direct pool swaps bypass this safeguard, so any mismatch between reported and received amounts could cause _applyFeeAndTransfer() to pull from pre-existing router balances.

Informational. Exploitation requires a validated pool with a fee-on-transfer output token and pre-existing tokenOut dust on the router. Loss is capped at whatever dust balance exists for that token.

Use balance-before/after measurement consistent with router-based swaps instead of relying on pool deltas.

Fixed in PR#657.

In _applyFeeAndTransfer(), the fee is computed as fee = (totalOutput * feeBps) / BPS_DENOMINATOR. When totalOutput * feeBps < 10_000, integer division truncates the fee to zero. Traders can split volume across multiple small calls that each stay below this threshold to avoid paying protocol fees entirely.

Informational. Protocol fee under-collection relative to the configured feeBps. Most relevant for low-decimal or high-unit-value output tokens.

Enforce a minimum fee of 1 unit when feeBps > 0 and totalOutput > 0 to eliminate the zero-fee region.

Acknowledged — skipped as negligible (fee on a dust-sized swap rounding to zero has no practical impact).

In _executeStep(), the tokenIn == address(0) check is skipped for AlgebraPool and UniswapV3Pool dex types, even though these pool-level swap functions do not support native ETH.

The check that reverts when tokenIn == address(0) is excluded for UniswapV3Pool and AlgebraPool. These direct pool swap functions (IUniswapV3Pool.swap() and their Algebra equivalents) operate on ERC20 tokens exclusively and do not handle native ETH. Passing address(0) as tokenIn for these dex types would result in a failed swap or unexpected behavior downstream. Including them in the address(0) check would provide a clearer revert.

Informational.

Remove AlgebraPool and UniswapV3Pool from the exclusion list in the tokenIn == address(0) check, or add separate validation within their respective swap functions.

Fixed in PR#656.

The natspec on _swapAlgebra() states that for classic Algebra (QuickSwap V3), dexData should be empty. However, the code only checks step.dexData.length == 32 for the Algebra Integral branch and falls through to the classic branch for any other length, including non-empty values.

The function branches on step.dexData.length == 32 to distinguish Algebra Integral from classic Algebra. Any dexData that is not exactly 32 bytes (including non-empty values of other lengths) will execute the classic Algebra path. The natspec stating dexData should be empty is misleading since the code does not enforce that constraint.

Informational.

Update the natspec or enforce dexData to be empty.

Fixed in commit fd5136d4. Reflect actual behavior (any dexData length != 32 takes the classic path).

In _isAlgebraPoolFromFactory(), after checking poolByPair, the function calls _tryComputeAlgebraPoolAddress() to compute the deterministic pool address. This is unnecessary because Algebra pools are either registered in the poolByPair mapping or in the customPoolByPair mapping.

The function first queries poolByPair on the factory. If that fails or returns a different address, it falls through to _tryComputeAlgebraPoolAddress(), which computes a deterministic CREATE2 address. Matching a CREATE2 address does not prove the pool exists. Since all legitimate pools should be registered in either poolByPair or customPoolByPair, this computed address check adds no security value and could potentially validate a non-existent pool.

Informational.

Consider removing the _tryComputeAlgebraPoolAddress() check. If a pool is not found in poolByPair, skip directly to the customPoolByPair check when a customDeployer is provided, and return false otherwise.

Fixed in commit f9ef2c04. Removed redundant computePoolAddress check in Algebra pool validation.

Admin.setAdmin() allows setting the admin to address(0), which would permanently brick all allowlist management. While MultiSwapRouter.setAdmin() guards against this.

// Admin.sol#L46-L51 — no zero-address check
function setAdmin(address newAdmin) external onlyAdmin {
    emit AdminSet(admin, newAdmin);
    admin = newAdmin;
}

// MultiSwapRouter.sol#L344-L349 — has zero-address check
function setAdmin(address newAdmin) external {
    if (msg.sender != admin) revert NotAdmin();
    if (newAdmin == address(0)) revert InvalidAddress();
    ...
}

Informational. An admin mistake could permanently lock out RouterAllowlist management. No routers, factories, or hooks could ever be added or removed.

Add a zero-address check to Admin.setAdmin() or override it in RouterAllowlist to match MultiSwapRouter.

Fixed in PR#656.

Several minor inefficiencies and code duplication patterns were identified across both contracts.

1. Redundant first iteration in _validateAndSumAmounts(). _validateAndSumAmounts() derives tokenIn and tokenOut from routes[0] at L242-L243, then loops from i = 0 and checks every route against those values. The i = 0 iteration compares routes[0] against itself — always true. The loop could start at i = 1.

2. Unused amountIn in Algebra callback data. _swapAlgebraPool() encodes amountIn in the callback data, but algebraSwapCallback() skips it during decoding. The callback pays amountToPay from the pool's reported delta, not the encoded value. The encoding and decoding of this unused field wastes gas.

3. Wrap/unwrap uses step.router instead of wrappedNative directly. _wrapWETH() and _unwrapWETH() validate that step.router == wrappedNative, then call IWETH(step.router). Using IWETH(wrappedNative) directly would eliminate the step.router != wrappedNative check and simplify both functions.

4. Duplicated swap-and-pop pattern in RouterAllowlist. _setRouter(), _setFactory(), and _setHook() each implement identical swap-and-pop array management logic. This could be consolidated into a single reusable internal function or replaced with OpenZeppelin's EnumerableSet.

Informational. Minor gas overhead and code maintainability concerns. No security impact.

Implement the suggested changes.

Partially fixed in PR#658. Swap-and-pop pattern in RouterAllowlist kept as is.

_executeBridgeAndRefund() takes a user-supplied bridgeAmount, checks it against available, then approves exactly that amount. This check is unnecessary — the approval itself caps what the bridge can pull, and the existing consumption check and refund logic handle all cases.

In _executeBridgeAndRefund(), the flow is:

uint256 available = IERC20(bridgeToken).balanceOf(address(this)) - initialBridgeTokenBalance;
if (bridgeAmount > available) {
    revert BridgeAmountExceedsAvailable(bridgeAmount, available);
}
_approve(bridgeToken, bridgeContract, bridgeAmount);

Both swapAndBridge() and bridge() already know the effective amount available after pulling input and collecting fees. Each entry point should compute this amount and pass it directly to _executeBridgeAndRefund, removing the bridgeAmount parameter from both the internal function and the external interface.

Informational. Simplifies the external interface, removes a redundant parameter and check, and makes the approval inherently safe by construction (always set to exactly what the current call deposited post-fee).

Compute the effective post-fee amount in each entry point (swapAndBridge from _swapAndCollectFee's output, bridge from amountReceived - fee) and pass it to _executeBridgeAndRefund as the approval amount. Remove the bridgeAmount parameter from both the external functions and the internal helper, along with the BridgeAmountExceedsAvailable check and update BridgeDidNotConsumeTokens check.

Fixed in 05c175fd.

swapAndBridge() accepts a deadline parameter and reverts if block.timestamp > deadline, but bridge() has no deadline mechanism. Transactions can sit in the mempool and execute at an arbitrary future time.

Informational. Users cannot protect against stale bridge transactions executing after conditions have changed.

Add a deadline parameter to bridge() consistent with swapAndBridge().

Fixed in 05c175fd.

Both swapAndBridge() and bridge() compute _preCallEthBalance = address(this).balance - msg.value and store it in a uint256 private storage variable. The only consumer is _executeBridgeAndRefund(), which reads it once for the ETH refund calculation. This costs a cold SSTORE (~5k gas) on write and a warm SLOAD (~100 gas) on read per bridge call, versus near-zero cost for passing a stack variable through the call chain. The value also persists in storage after the function returns, which is unnecessary.

Informational. ~5k gas overhead per bridge call with no functional benefit over a function parameter.

Remove the _preCallEthBalance storage variable. Compute the value locally in each entry point and pass it as a parameter to _executeBridgeAndRefund.

Fixed in 05c175fd.

_executeBridgeAndRefund() queries the bridgeToken balance twice, the first while computing the available amount (line 480) and again to assign balanceBeforeBridge (line 491).

478:         // Cap bridgeAmount to what this call actually deposited (post-fee)
479:         uint256 available =
480:             IERC20(bridgeToken).balanceOf(address(this))
481:                 - initialBridgeTokenBalance;
482:         if (bridgeAmount > available) {
483:             revert BridgeAmountExceedsAvailable(bridgeAmount, available);
484:         }
485:
486:         // Approve bridge contract for exactly bridgeAmount
487:         _approve(bridgeToken, bridgeContract, bridgeAmount);
488:
489:         // Record balance before bridge call for consumption check
490:         uint256 balanceBeforeBridge =
491:             IERC20(bridgeToken).balanceOf(address(this));

Gas.

Query the balance once and use this value in both locations.

Fixed in 05c175fd.