Pair.add function can lose users token due to lack of calculation of amount to deposit. Also open opportunities for MEV bots

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L77-L96
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#L285

Vulnerability details

Description

Current function assure as that we will at least receive minLpTokenAmount amount of LP. However, this function assumes that the input parameter baseTokenAmount and fractionalTokenAmount will adjust to the LP amount calculated to deposit.

Impact

A user whose input parameters baseTokenAmount or fractionalTokenAmount does not adjust to the corresponding lpTokenAmount calculated inside the function can make the user lose funds, or even open frontrunning opportunities for MEV bots.

POC

Add this file to test folder, then run forge test --match-test testUserLoseOfFunds -vvv to check the next output.

Logs:
  Current user 1 USD balance:  1000
  Current user 1 Fractional Token balance:  1000

User 1 deposits 100 USD and 150 fractional tokens
        User 1 USD balance after LP minting through add function:  900
        User 1 Fractional Token balance after LP minting through add function::  850
        User 1 LP balance:  100

User 1 tries to remove their LP to get their 100 USD and 150 fractional token
        User 1 USD balance after calling remove function with all their LPs:  1000
        User 1 Fractional Token balance calling remove function with all their LPs:  975
        User 1 LP balance:  0

This shows that a user calling this function can accidentally give tokens to current LP holders.

A derivate consequence is that it opens a windows to MEV bots, they can include in the same block a transaction to:

1. Mint an LP with correct amounts
2. Wait until the user who put wrong inputs and sent more token than they are supposed by accident
3. The bot now can remove the LP with more tokens.

This last scenario means that user funds are in risk.

Mitigation steps

Calculates amount of tokens/ETH to transfer to the Pair contract in fact and use these values to execute the transfer function and/or to return ETH. I would suggest to add this calculations inside the addQuote function. Then the return values can be even used in the front end to suggest user amount to send in order to the slippage they set.

-   function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
+   function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256, uint256, uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            //@audit-ok 3 simple rules and return min is ok
            // calculate amount of lp tokens as a fraction of existing reserves
            // EXPECTED_SHARE = AMOUNT SENT * CURRENT SHARES / CURRENT BALANCE
+           uint256 _baseTokenReserve = baseTokenReserves(); 
+           uint256 _fractionalTokenReserves = fractionalTokenReserves(); 
            uint256 baseTokenShare = 
                (baseTokenAmount * lpTokenSupply) /
-               baseTokenReserves();
+               _baseTokenReserve;

            uint256 fractionalTokenShare = 
                (fractionalTokenAmount * lpTokenSupply) /
-               fractionalTokenReserves();
+               _fractionalTokenReserves;

+           if(baseTokenShare < fractionalTokenShare){
+               uint256 baseTokenToSend = _baseTokenReserve * baseTokenShare / lpTokenSupply;
+               uint256 fractionalTokenToSend = _fractionalTokenReserves * baseTokenShare / lpTokenSupply;
+               return baseTokenShare, baseTokenToSend, fractionalTokenToSend;
+           }else{
+               uint256 baseTokenToSend = _baseTokenReserve * fractionalTokenShare / lpTokenSupply;
+               uint256 fractionalTokenToSend = _fractionalTokenReserves * fractionalTokenShare / lpTokenSupply;
+               return fractionalTokenShare, baseTokenToSend, fractionalTokenToSend;
            }
-           return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
-           return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+           return Math.sqrt(baseTokenAmount * fractionalTokenAmount), baseTokenAmount, fractionalTokenAmount;
        }
    }

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
+      nonReentrant // consider adding this lock giving that we are sending back ETH to msg.sender, evaluate possible re-entrancy attacks
        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);
+       lpTokenAmount,baseTokenToSend, fractionalTokenToSend = 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 *** //
-       _transferFrom(msg.sender, address(this), fractionalTokenAmount);
+       _transferFrom(msg.sender, address(this), fractionalTokenToSend);

        // *** 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);
+           ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenToSend);
-       }
+       }else{
+            // return extra ETH
+           payable(msg.sender).call{value: msg.value - baseTokenToSend}()            
+       }

-       emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
+       emit Add(baseTokenToSend, fractionalTokenToSend , lpTokenAmount);
    }

[c4-judge]

berndartmueller marked the issue as duplicate of #376

[c4-judge]

berndartmueller marked the issue as satisfactory