levi - `ArrakisV2Router::addLiquidity` can transfer amounts higher than the maximum specified by the user

[sherlock-admin]

levi

medium

ArrakisV2Router::addLiquidity can transfer amounts higher than the maximum specified by the user

Summary

ArrakisV2Router::addLiquidity can transfer amounts higher than the maximum specified by the user

Vulnerability Detail

When a user calls ArrakisV2Router::addLiquidity, they specify amount0Max and amount1Max as part of the parameters. These values are however not checked and enforced before tokens are transfered from the user. Only the minimum amounts are enforced:

        require(
            amount0 >= params_.amount0Min &&
                amount1 >= params_.amount1Min &&
                sharesReceived >= params_.amountSharesMin,
            "below min amounts"
        );

In the case where getMintAmounts returns higher values, the user would have a higher amount of tokens transfered from them.

        (amount0, amount1, sharesReceived) = resolver.getMintAmounts(
            IArrakisV2(params_.vault),
            params_.amount0Max,
            params_.amount1Max
        );

This is possible because getMintAmounts and totalUnderlyingForMint round up the amounts required. It is also possible in times of high price volatility.

Impact

Users will have more amounts transfered from them than they intended.

Code Snippet

https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-periphery/contracts/ArrakisV2Router.sol#L79-L84

https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-core/contracts/ArrakisV2Resolver.sol#L181-L206

https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-core/contracts/libraries/Underlying.sol#L72-L89

Tool used

Manual Review

Recommendation

Enforce a check to ensure the amounts returned by getMintAmounts are less than the maximum amounts specified by the user.

[leviack3rman]

Escalate

I agree that this is not a duplicate of #8 and should be its own issue. This issue specifically deals with the fact that a user can have more than what they specified as amount0Max and amount1Max transferred from them.

To prove this, here is a poc

import { expect } from "chai";
import { deployments, ethers, network } from "hardhat";
import {
    RouterSwapExecutor,
    ArrakisV2Router,
    ERC20,
    RouterSwapResolver,
    IArrakisV2,
    IGauge,
} from "../typechain";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/dist/src/signer-with-address";
import { Addresses, getAddresses } from "../src/addresses";
import { BigNumber, Contract, Wallet } from "ethers";
import {
    getPeripheryContracts,
    deployArrakisV2,
    getFundsFromFaucet,
    createGauge,
    getArrakisResolver,
} from "../src/testEnvUtils";
import { swapAndAddTest } from "../src/swapAndAddTest";
import { ecsign } from "ethereumjs-util";
import { SignatureTransfer, MaxSigDeadline } from "@uniswap/permit2-sdk";
import { mockPayloads, OneInchDataType } from "../src/oneInchApiIntegration";

let addresses: Addresses;

const sign = (msgHash: string, privKey: string): any => {
    const hash = Buffer.alloc(32, msgHash.slice(2), "hex");
    const priv = Buffer.alloc(32, privKey.slice(2), "hex");
    return ecsign(hash, priv);
};

describe("MaxAmount test", function () {
    this.timeout(0);
    let wallet: SignerWithAddress;
    let walletAddress: string;

    let owner: SignerWithAddress;

    let token0: ERC20;
    let token1: ERC20;
    let rakisToken: ERC20;
    let stRakisToken: ERC20;

    let resolver: Contract;
    let swapExecutor: RouterSwapExecutor;
    let router: ArrakisV2Router;
    let swapResolver: RouterSwapResolver;

    let vault: IArrakisV2;

    let gauge: IGauge;
    let swapExecutorBalanceEth: BigNumber | undefined;
    let routerBalanceEth: BigNumber | undefined;
    let randomWallet: Wallet;

    before(async function () {
        await deployments.fixture();

        addresses = getAddresses(network.name);
        [wallet, , owner] = await ethers.getSigners();
        walletAddress = await wallet.getAddress();

        [swapResolver, swapExecutor, router] = await getPeripheryContracts(owner);

        resolver = await getArrakisResolver(owner);

        [vault] = await deployArrakisV2(
            wallet,
            addresses.USDC,
            addresses.WETH,
            500,
            resolver,
            walletAddress
        );

        token0 = (await ethers.getContractAt(
            "ERC20",
            await vault.token0()
        )) as ERC20;
        token1 = (await ethers.getContractAt(
            "ERC20",
            await vault.token1()
        )) as ERC20;

        rakisToken = (await ethers.getContractAt("ERC20", vault.address)) as ERC20;

        await getFundsFromFaucet(addresses.faucetUSDC, token0, walletAddress);
        await getFundsFromFaucet(addresses.faucetWeth, token1, walletAddress);

        [gauge, stRakisToken] = await createGauge(
            vault.address,
            wallet,
            owner.address
        );

        // await swapExecutor.connect(owner).whitelistRouter(router.address);

        swapExecutorBalanceEth = await wallet.provider?.getBalance(
            swapExecutor.address
        );
        routerBalanceEth = await wallet.provider?.getBalance(router.address);

        randomWallet = new ethers.Wallet(
            "0x36383cc9cfbf1dc87c78c2529ae2fcd4e3fc4e575e154b357ae3a8b2739113cf",
            wallet.provider
        );

        await wallet.sendTransaction({
            to: randomWallet.address,
            value: ethers.utils.parseEther("20"),
        });
    });

    it("#0 : should deposit funds with addLiquidity", async function () {
        const amount0In = ethers.BigNumber.from("10000").mul(
            ethers.BigNumber.from("10").pow("6")
        );
        const amount1In = ethers.utils.parseEther("10");

        await token0.connect(wallet).approve(router.address, amount0In);
        await token1.connect(wallet).approve(router.address, amount1In);

        const balance0Before = await token0.balanceOf(walletAddress);
        const balance1Before = await token1.balanceOf(walletAddress);
        const balanceArrakisV2Before = await rakisToken.balanceOf(walletAddress);

        await token0.allowance(wallet.address, router.address);
        await token1.allowance(wallet.address, router.address);

        const addLiquidityData = {
            amount0Max: amount0In,
            amount1Max: amount1In,
            amount0Min: 0,
            amount1Min: 0,
            amountSharesMin: 0,
            vault: vault.address,
            receiver: walletAddress,
            gauge: ethers.constants.AddressZero,
        };

        await router.addLiquidity(addLiquidityData);

        const balance0After = await token0.balanceOf(walletAddress);
        const balance1After = await token1.balanceOf(walletAddress);

        expect(balance0Before).to.be.gte(balance0After.add(amount0In));
        expect(balance1Before).to.be.gte(balance1After.add(amount1In));

    });
})

Add this test to the v2-periphery/test folder and name it MaxAmounts.test.ts then run

yarn run test --grep MaxAmount

The test should fail to show that the wallet balance of the user after adding liquidity, isn't greater or equal to the balance before plus the max mount they specified in the parameters.

This shows that an amount greater than the maximum amount the user specified is transferred from them.

[sherlock-admin]

Escalate

I agree that this is not a duplicate of #8 and should be its own issue. This issue specifically deals with the fact that a user can have more than what they specified as amount0Max and amount1Max transferred from them.

To prove this, here is a poc

import { expect } from "chai";
import { deployments, ethers, network } from "hardhat";
import {
    RouterSwapExecutor,
    ArrakisV2Router,
    ERC20,
    RouterSwapResolver,
    IArrakisV2,
    IGauge,
} from "../typechain";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/dist/src/signer-with-address";
import { Addresses, getAddresses } from "../src/addresses";
import { BigNumber, Contract, Wallet } from "ethers";
import {
    getPeripheryContracts,
    deployArrakisV2,
    getFundsFromFaucet,
    createGauge,
    getArrakisResolver,
} from "../src/testEnvUtils";
import { swapAndAddTest } from "../src/swapAndAddTest";
import { ecsign } from "ethereumjs-util";
import { SignatureTransfer, MaxSigDeadline } from "@uniswap/permit2-sdk";
import { mockPayloads, OneInchDataType } from "../src/oneInchApiIntegration";

let addresses: Addresses;

const sign = (msgHash: string, privKey: string): any => {
    const hash = Buffer.alloc(32, msgHash.slice(2), "hex");
    const priv = Buffer.alloc(32, privKey.slice(2), "hex");
    return ecsign(hash, priv);
};

describe("MaxAmount test", function () {
    this.timeout(0);
    let wallet: SignerWithAddress;
    let walletAddress: string;

    let owner: SignerWithAddress;

    let token0: ERC20;
    let token1: ERC20;
    let rakisToken: ERC20;
    let stRakisToken: ERC20;

    let resolver: Contract;
    let swapExecutor: RouterSwapExecutor;
    let router: ArrakisV2Router;
    let swapResolver: RouterSwapResolver;

    let vault: IArrakisV2;

    let gauge: IGauge;
    let swapExecutorBalanceEth: BigNumber | undefined;
    let routerBalanceEth: BigNumber | undefined;
    let randomWallet: Wallet;

    before(async function () {
        await deployments.fixture();

        addresses = getAddresses(network.name);
        [wallet, , owner] = await ethers.getSigners();
        walletAddress = await wallet.getAddress();

        [swapResolver, swapExecutor, router] = await getPeripheryContracts(owner);

        resolver = await getArrakisResolver(owner);

        [vault] = await deployArrakisV2(
            wallet,
            addresses.USDC,
            addresses.WETH,
            500,
            resolver,
            walletAddress
        );

        token0 = (await ethers.getContractAt(
            "ERC20",
            await vault.token0()
        )) as ERC20;
        token1 = (await ethers.getContractAt(
            "ERC20",
            await vault.token1()
        )) as ERC20;

        rakisToken = (await ethers.getContractAt("ERC20", vault.address)) as ERC20;

        await getFundsFromFaucet(addresses.faucetUSDC, token0, walletAddress);
        await getFundsFromFaucet(addresses.faucetWeth, token1, walletAddress);

        [gauge, stRakisToken] = await createGauge(
            vault.address,
            wallet,
            owner.address
        );

        // await swapExecutor.connect(owner).whitelistRouter(router.address);

        swapExecutorBalanceEth = await wallet.provider?.getBalance(
            swapExecutor.address
        );
        routerBalanceEth = await wallet.provider?.getBalance(router.address);

        randomWallet = new ethers.Wallet(
            "0x36383cc9cfbf1dc87c78c2529ae2fcd4e3fc4e575e154b357ae3a8b2739113cf",
            wallet.provider
        );

        await wallet.sendTransaction({
            to: randomWallet.address,
            value: ethers.utils.parseEther("20"),
        });
    });

    it("#0 : should deposit funds with addLiquidity", async function () {
        const amount0In = ethers.BigNumber.from("10000").mul(
            ethers.BigNumber.from("10").pow("6")
        );
        const amount1In = ethers.utils.parseEther("10");

        await token0.connect(wallet).approve(router.address, amount0In);
        await token1.connect(wallet).approve(router.address, amount1In);

        const balance0Before = await token0.balanceOf(walletAddress);
        const balance1Before = await token1.balanceOf(walletAddress);
        const balanceArrakisV2Before = await rakisToken.balanceOf(walletAddress);

        await token0.allowance(wallet.address, router.address);
        await token1.allowance(wallet.address, router.address);

        const addLiquidityData = {
            amount0Max: amount0In,
            amount1Max: amount1In,
            amount0Min: 0,
            amount1Min: 0,
            amountSharesMin: 0,
            vault: vault.address,
            receiver: walletAddress,
            gauge: ethers.constants.AddressZero,
        };

        await router.addLiquidity(addLiquidityData);

        const balance0After = await token0.balanceOf(walletAddress);
        const balance1After = await token1.balanceOf(walletAddress);

        expect(balance0Before).to.be.gte(balance0After.add(amount0In));
        expect(balance1Before).to.be.gte(balance1After.add(amount1In));

    });
})

Add this test to the v2-periphery/test folder and name it MaxAmounts.test.ts then run

yarn run test --grep MaxAmount

The test should fail to show that the wallet balance of the user after adding liquidity, isn't greater or equal to the balance before plus the max mount they specified in the parameters.

This shows that an amount greater than the maximum amount the user specified is transferred from them.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

@ctf-sec @Gevarist

[ctf-sec]

Yeah this is not duplicate of #8

There is also a escalation in #8 saying:

Can use the issue 8 as main thread of discussion

[hrishibhat]

@ctf-sec is this a valid issue on its own?

[SergeKireev]

The POC and issue are invalid:

The poc

expect(balance0Before).to.be.gte(balance0After.add(amount0In)); expect(balance1Before).to.be.gte(balance1After.add(amount1In));

The condition is reversed compared to what the reporter wants to show: e.g the router is pulling in more funds

amount0In in the test is the maxAmount:

        const addLiquidityData = {
            amount0Max: amount0In,
            amount1Max: amount1In,
            amount0Min: 0,
            amount1Min: 0,
            amountSharesMin: 0,
            vault: vault.address,
            receiver: walletAddress,
            gauge: ethers.constants.AddressZero,
        };

so the condition in the POC reads: balanceBefore is greater than balanceAfter plus maxAmount, which should always be false for the protocol.

Now changing the condition to: balanceAfter is greater than balanceBefore sub maxAmount:

expect(balance0After).to.be.gte(balance0Before.sub(amount0In)); expect(balance1After).to.be.gte(balance1Before.sub(amount1In));

Which is the expected invariant for the router, makes the test pass.

The issue

Now regarding the issue itself, it is invalid, since even though the router rounds up when determining the amounts to pull in, it does so on an amount: mintAmount, which has been obtained by a rounded down division using amountXMax: https://github.com/sherlock-audit/2023-06-arrakis/blob/main/v2-core/contracts/libraries/Underlying.sol#L355-L387

[Gevarist]

yeah agree with @SergeKireev. It's invalid.

[hrishibhat]

Result: Invalid Unique Not a duplicate of #8 Considering this a non issue based on the above comments.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• leviack3rman: rejected