sherlock-audit / 2023-03-taurus-judging

4 stars 0 forks source link

GimelSec - `swap()` will be reverted if `path` has more tokens. #160

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

GimelSec

high

swap() will be reverted if path has more tokens.

Summary

swap() will be reverted if path has more tokens, the keepers will not be able to successfully call swapForTau().

Vulnerability Detail

In test/SwapAdapters/00_UniswapSwapAdapter.ts:

    // Get generic swap parameters
    const basicSwapParams = buildUniswapSwapAdapterData(
      ["0xyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy", "0xzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"],
      [3000],
      testDepositAmount,
      expectedReturnAmount,
      0,
    ).swapData;

We will get:

000000000000000000000000000000000000000000000000000000024f49cbca
0000000000000000000000000000000000000000000000056bc75e2d63100000
0000000000000000000000000000000000000000000000055de6a779bbac0000
0000000000000000000000000000000000000000000000000000000000000080
000000000000000000000000000000000000000000000000000000000000002b
yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy000bb8zzzzzzzzzzzzzzzzzz
zzzzzzzzzzzzzzzzzzzzzz000000000000000000000000000000000000000000

Then the swapOutputToken is _swapData[length - 41:length - 21].

But if we have more tokens in path:

    const basicSwapParams = buildUniswapSwapAdapterData(
      ["0xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", "0xyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy", "0xzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"],
      [3000, 3000],
      testDepositAmount,
      expectedReturnAmount,
      0,
    ).swapData;
000000000000000000000000000000000000000000000000000000024f49cbca
0000000000000000000000000000000000000000000000056bc75e2d63100000
0000000000000000000000000000000000000000000000055de6a779bbac0000
0000000000000000000000000000000000000000000000000000000000000080
0000000000000000000000000000000000000000000000000000000000000042
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx000bb8yyyyyyyyyyyyyyyyyy
yyyyyyyyyyyyyyyyyyyyyy000bb8zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
zzzz000000000000000000000000000000000000000000000000000000000000

swapOutputToken is _swapData[length - 50:length - 30], the swap() function will be reverted.

Impact

The keepers will not be able to successfully call SwapHandler.swapForTau(). Someone will get a reverted transaction if they misuse UniswapSwapAdapter.

Code Snippet

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

Tool used

Manual Review

Recommendation

Limit the swap pools, or check if the balance of _outputToken should exceed _amountOutMinimum.

Sierraescape commented 1 year ago

https://github.com/protokol/taurus-contracts/pull/82

MLON33 commented 1 year ago

https://github.com/protokol/taurus-contracts/pull/82

Fixed here

IAm0x52 commented 1 year ago

Fix looks good. Byte position of outputToken is now calculated dynamically based on path length rather than a hardcoded 21 bytes which only worked for single hops