Skip to content
This repository has been archived by the owner on Sep 17, 2023. It is now read-only.

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

Open
sherlock-admin opened this issue Mar 13, 2023 · 3 comments
Open

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

sherlock-admin opened this issue Mar 13, 2023 · 3 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

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.

@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Mar 21, 2023
@Sierraescape Sierraescape added Disagree With Severity The sponsor disputed the severity of this issue Will Fix The sponsor confirmed this issue will be fixed labels Mar 21, 2023
@Sierraescape Sierraescape added the Sponsor Confirmed The sponsor acknowledged this issue is valid label Mar 23, 2023
@Sierraescape
Copy link

@Evert0x Evert0x added Medium A valid Medium severity issue and removed High A valid High severity issue Disagree With Severity The sponsor disputed the severity of this issue labels Mar 28, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Apr 1, 2023
@MLON33
Copy link

MLON33 commented Apr 10, 2023

@IAm0x52
Copy link
Collaborator

IAm0x52 commented Apr 18, 2023

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants