Caviar contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Given baseTokenAmount and fractionalTokenAmount, the Add() function calls the addQuote() funtion to determine lpTokenAmount. However, addQuote() choses the minimum of baseTokenShare and fractionalTokenShare as the final value of lpTokenAmount. A user will end up overpaying either the base tokens or the fractional tokens since Add() will send the whole amount of baseTokenAmount and fractionalTokenAmount to the Pair contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63 On one hand, the addQuote function determines the lpTokenAmount as

uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);

On the other hand, the Add() function takes the whole amount baseTokenAmount and fractionalTokenAmount, so one of them would be overpaid.

Tools Used

Remix

Recommended Mitigation Steps

We need to revise the addQuote() function to return not only lpTokenAmount, but also actual actualBaseTokenAmount and actutalFractionalTokenAmount.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view 
             returns (uint256 ltTokenAmount, 
                          uint256 actualBaseTokenAmount,
                          uint256 actualFractionalTokenAmount)
        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();
            if(baseTokenShare < fractionTokenShare) {     // @audit
                      ltTokenAmout = baseTokenShare;
                      actualBaseTokenAmount = baseTokenAmount;
                      actualFractionalTokenAmount = baseTokenShare*frationalTokenReserves()/lpTokenSupply;
            }
            else{
                      ltTokenAmout = fractionalTokenShare;
                      actualBaseTokenAmount = fractionalTokenShare*baseTokenReserves()/lpTokenSupply;
                      actualFractionalTokenAmount = fractionalTokenAmount;
            } 
            
        } else {
            // if there is no liquidity then init
                   ltTokenAmount =  Math.sqrt(baseTokenAmount * fractionalTokenAmount);
                   actualBaseTokenAmount = baseTokenAmount;
                   actualFractionalTokenAmount = fractionalTokenAmount;
        }
    }

Then, the add() function to use actualBaseTokenAmount and actualFractionalTokenAmount to transfer the right amount of tokens and if necessary, we need to refund the base token if it is ETH.

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

        uint256 actualBaseTokenAmount;                      // @audit
        uint256 actualFrationalTokenAmount;               // @audit
,
        // calculate the lp token shares to mint
        (lpTokenAmount, actualBaseTokenAmount, actualFractionalTokenAmount) = addQuote(baseTokenAmount, fractionalTokenAmount);        // @audit, need to calculate the actual amount for each token

        // 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), actualFractionalTokenAmount); // @audit: use the actualFractionalTokenAmount

        // *** 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), actualBaseTokenAmount); // @audit: use actualBaseTokenAmount instead
        }
        else{
             uint256 refundAmount = baseTokenAmount - actualBaseTokenAmount; // @audit: need to refund
             if(refundAmount > 0) msg.sender.safeTransferETH(refundAmount); // @audit: need to refund
        }

        emit Add(actualBaseTokenAmount, actualFractionalTokenAmount, lpTokenAmount);
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. An attack can front run the first Add() transaction and gain free baseTokens using a similar idea of sandwich attack.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63

1. User Bob attemps to run the first Add Transaction with bastTokenAmount = 10e18 and fractionalTokenAmount = 1e18.

2. The attacker Alice front run this transaction with baseTokenAmount = 1e18 and fractionTokenAmount = 1e18, and set the price as 1 baseToken/fractionToken first, Alice will get back 1e18 lpTokens.

3. Bob continues to run the Add transaction from step 1, but now he is the second to run Add, and has to add the liquidity with the price set by Alice, and will return around 1e18 lptokens instead with his 1e18 fractionalTokens and 10e18 base tokens.

4. After Bob's transaction, the reserves of baseToken and fractional tokens have changed: there are 11e18 baseTokens and 2e18 fractionalTokens, so the price is around 5.5 baseToken/fractionalToken.

5. Attacker Alice removes all her liquidity from the pair, she can get 1e18 fractionalTokens, and around 5.5e18 base tokens, she gains almost 4.5e18 base tokens for free!

Tools Used

Remix

Recommended Mitigation Steps

The addQuote() fuction needs a better design to prevent such attack.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The calculation of buyQuote seems to not quite right, we need to add 1 add the end to avoid edge case such as zero. See the standard implementation at Uniswap V2: https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/libraries/UniswapV2Library.sol#L58

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

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L398-L400

Tools Used

Remix

Recommended Mitigation Steps