shaka - UniswapSwapAdapter.swap() only works for paths formed by two tokens

[sherlock-admin]

shaka

medium

UniswapSwapAdapter.swap() only works for paths formed by two tokens

Summary

If UniswapSwapAdapter.swap() receives in the parameter _swapData a path with more than two tokens, it will revert.

Vulnerability Detail

In the UniswapSwapAdapter.swap() function the is a check to ensure that the path ends in _outputToken.
The path value will be right padded with zeroes to fit a 32 byte word. However, when extracting the value of the last token in the path, the length of the padding is hardcoded for a two token path.

function swap(address _outputToken, bytes calldata _swapData) external returns (uint256) {
    // Decode swap data
    (uint256 deadline, uint256 _amountIn, uint256 _amountOutMinimum, bytes memory _path) = abi.decode(
        _swapData,
        (uint256, uint256, uint256, bytes)
    );

    // Check that the outputToken is the final token in the path
    uint256 length = _swapData.length;
    address swapOutputToken = address(bytes20(_swapData[length - 41:length - 21])); // 👈 values hardcoded for two token path
    
	if (swapOutputToken != _outputToken) {
		// The keeper-inputted Output Token differs from what the contract says it must be.
		revert incorrectOutputToken();
	}
    // ...

Thus, if a path with more than two tokens is passed in the parameters, swapOutputToken will not match _outputToken and the transaction will revert.

Impact

For yield tokens whose Uniswap route to TAU involves more tokens the yield pull process will fail when SwapHandler.swapForTau() is called by a keeper.

Code Snippet

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/SwapAdapters/UniswapSwapAdapter.sol#L21-L35

Proof of concept

This test shows how a swap fails when we add a third token in the middle of the path.

describe.only("Swap", async () => {
    // let path: string;
    let basicSwapParams: string;
    const setupTokensFixture = deployments.createFixture(async () => {
      // Approve DAI
      await swapAdapter.connect(keeper).approveTokens(testDAI.address);
      expect(await testDAI.allowance(swapAdapter.address, swapRouter.address)).to.equal(ethers.constants.MaxUint256);

      // Get generic swap parameters
      basicSwapParams = buildUniswapSwapAdapterData(
        [testDAI.address, "0x1111111111111111111111111111111111111111", testUSDT.address],
        [3000, 3000],
        testDepositAmount,
        expectedReturnAmount,
        0,
      ).swapData;
    });

    beforeEach(async function () {
      await setupTokensFixture();
    });

    it.only("fails to swap if there are more than two tokens", async () => {
      await expect(swapAdapter.swap(testUSDT.address, basicSwapParams)).to.be.revertedWithCustomError(
        swapAdapter,
        "incorrectOutputToken",
      );
    });
});

Tool used

Manual Review

Recommendation

Adapt the code so that it works for any number of tokens int he path.

// Check that the outputToken is the final token in the path
uint256 length = _swapData.length;
-- address swapOutputToken = address(bytes20(_swapData[length - 41:length - 21]));
++ uint256 pathLength = _path.length;
++ uint256 rightPaddingLength = length - 160 - _path.length; // offset of 160 is the length of 3 uint256 params + bytes header
++ address swapOutputToken = address(bytes20(_swapData[length - (rightPaddingLength + 20):length - rightPaddingLength]));

Duplicate of #160