002.md

csanuragjain

medium

MultiHop swap is implemented incorrectly

Summary

If canBatchSwap is called with swaps.length=1 then ideally multihop should revert mentioning an invalid swap. But due to incorrect implementation of isMultiHopSwap, contract will find user data eligible for multi hop swap

Vulnerability Detail

1. User call canCall function with sig as BATCH_SWAP
2. This makes call to canBatchSwap function
3. Lets say User call this with only 1 value in swap in calldata, which makes swaps.length=1

(
            ,
            IVault.BatchSwapStep[] memory swaps,
            IAsset[] memory assets,
            ,
            ,
        ) = abi.decode(data, (
                uint8, IVault.BatchSwapStep[], IAsset[], IVault.FundManagement, uint256[], uint256
            )
        );

4. isMultiHopSwap is now called which instantly returns true since swap.length=1 and loop runs for swaps.length-1 which is 0 times in this case

function isMultiHopSwap(IVault.BatchSwapStep[] memory swaps)
        internal
        pure
        returns (bool)
    {
        uint steps = swaps.length;
        for (uint i; i < steps - 1; i++) {
            if (swaps[i].assetOutIndex != swaps[i+1].assetInIndex)
                return false;
        }
        return true;
    }

5. canBatchSwap returns true which is incorrect as assets[0] will only be considered in this swap

Impact

Incorrect swap will be marked as a correct swap

Code Snippet

https://github.com/sentimentxyz/controller/blob/85d6eeef5ae5ab569c6e57c877aa0831db0db4b2/src/balancer/BalancerController.sol#L231-L242

Tool used

Manual Review

Recommendation

Add below check in isMultiHopSwap

require(swaps.length>1, "Invalid swap data");