Caviar contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L93-L96

Vulnerability details

Impact

When the baseToken is an ERC777 token, when add() calls baseToken.safeTransferFrom, tokensToSend of msg.sender will be called. Since the baseToken in the Pair has not been increased at this time, this will make the user spend less baseToken when calling buy().

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

Consider the following scenario. Where there are 20,000 baseTokens and 200 fractionalTokens in Pair , and User A wants to buy 10 fractionalTokens, which costs 10 * 1000 * 20000 / 190 / 997 = 1055.8 baseTokens.

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

If User A calls add() to add 10,000 baseTokens and 100 fractionalTokens to the Pair, then to buy 10 fractionalTokens would cost 10 * 1000 * 30000 / 290 / 997 = 1037.6 baseTokens.

User A finds this vulnerability and calls buy() in the safeTransferFrom callback. Since 100 fractionalTokens have been added to the Pair and 10000 baseTokens have not been added to the contract, it costs 10 * 1000 * 20000 / 290 / 997 = 691.7 baseTokens to buy 10 fractionalTokens.

At this point there are 30691.7 baseTokens and 290 fractionalTokens in the contract. User A calls remove to get 30691.7/3 = 10230.6 baseTokens and 96.7 fractionalTokens.

    function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        uint256 baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply;
        uint256 fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply;

        return (baseTokenOutputAmount, fractionalTokenOutputAmount);
    }

User A gains 1055.8 - 691.7 + 230.6 = 594.7 baseToken and loses 3.3 fractionalTokens, at this point it costs 3.3 * 1000 * 30691.7 / 997 / (290-3.3) = 354.3 baseTokens to buy 3.3 fractionalTokens, the profit is 594.7 - 354.3 = 240.4 baseTokens

Of course, user A can choose to exchange when the exchange rate returns to 100:1, so the profit is 594.7 - 330 = 264.7 baseToken

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L93-L96 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L398-L400

Tools Used

None

Recommended Mitigation Steps

Consider adding reentrancy protection to add() and buy()

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L423

Vulnerability details

Impact

When a user calls the add function to add liquidity, the contract uses the addQuote function to calculate the amount of lpToken the user has received. When lpTokenSupply > 0, Math.min(baseTokenShare, fractionalTokenShare) will be the amount of lpTokens the user gets, but the amount of baseTokens or fractionalTokens the user needs to add is not reduced

    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);

Consider the following scenario, where the amount of baseToken in Pair is 10000, the amount of fractionalToken is 100, and lpTokenSupply is sqrt(10000 * 100) = 1000 User A is ready to provide 100 baseTokens and 1 fractionalToken. But before that User B calls the buy function and buys 10 fractionalTokens using 1115 baseTokens, so the amount of baseTokens in the contract is 11115 and the amount of fractionalTokens is 90. User A provides liquidity, and since Math.min(100 * 1000/11115 = 9, 1 * 1000/90 = 11) = 9, User A actually only needs to spend 100 baseTokens and 9*90/1000 = 0.8 fractionalTokens, but in the add function, User A is actually charged 100 baseTokens and 1 fractionalToken.

Note: If user A sets minLpTokenAmount to 10 in the above case, then user A will not be able to provide liquidity due to the very frequent swaps, and user A will have to reduce minLpTokenAmount to ensure that the add function call succeeds, but this also means that user A will have to bear the loss

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L423 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63-L85

Tools Used

None

Recommended Mitigation Steps

Consider using lpTokenAmount in the add function to calculate how much the user actually needs to spend to avoid overcharging tokens

    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);

+      uint256 lpTokenSupply = lpToken.totalSupply();
+      baseTokenAmount = lpTokenAmount * baseTokenReserves() / lpTokenSupply;
+      fractionalTokenAmount = lpTokenAmount * fractionalTokenReserves() / lpTokenSupply;

        // 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);
        }

Lines of code

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

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' provide because of the precision loss caused by the rather large value of price per share.

A malicious early user can add() with 1 wei of baseToken and fractionalToken as the first provider of the LP Token, and get 1 wei of lpToken.

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

Then the attacker can send 10000e18 - 1 of baseToken and fractionalToken to Pair, and inflate the price per LpToken from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

    function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        uint256 baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply;
        uint256 fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply;

        return (baseTokenOutputAmount, fractionalTokenOutputAmount);
    }

As a result, the future user who provides 9999e18 baseToken and fractionalToken will receive 0 LpToken (from 9999e18 * 1 / 10000e18)

They will immediately lose all of their assets.

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417-L428 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L435-L441 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L477-L481 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L379-L385

Tools Used

None

Recommended Mitigation Steps

Consider requiring a minimal amount of LpTokens to be minted for the first minter, and send part of the initial mints as a permanent reserve to the DAO/treasury/deployer so that the pricePerLpToken can be more resistant to manipulation. https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L121