user funds loos because function add() doesn't return the extra amount of baseToken or fractionalToken to the user

[code423n4]

Lines of code

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

Vulnerability details

Impact

Function add() adds liquidity to the pair, it transfers base token and fractional token from user and mint lp token for user, but during the calculation of the lp token amount contract take minimum amount of lp token based on percentage of user supplied base and fractional token to contract balance, but contract won't return the extra amount of base token or fractional token to the user. users don't know exactly how much of each token is needed they may send extra tokens to make sure transaction won't get reverted because of low amount of tokens but this would cause to lose those extra tokens. and when a user makes mistake send very large amount of base or fractional tokens those all would be lost.

Proof of Concept

This is add() code:

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

As you can see code calls addQuote() and mint amount of lp tokens returned by that function for user and there is no logic to return extra base token or fractional token to the user.
This is addQuote() code:

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

As you can see to calculate the amount of lp token contract calculates amount of lp tokens as a fraction of existing reserves and then took the minimum values. so when this scenario happens:

1. contract has 100 * 1e18 base token balance and 50 * 1e18 fractional token balance and lpTokenSupply is 10 * 1e18.
2. user1 wants to add liquidity and calls add(11 * 1e18 base token, 5 * 1e18 fractional token, 1 * 1e18 minimum lp token).
3. contract would calculate lp token amount as baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves() = (11 * 1e18 * 10 *1e18) / (100 * 1e18) = 11 * 1e17 and fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves() = (5 * 1e18 * 10 * 1e18) / (50 * 1e18) = 1e18 and lpTokenAmount = min(11 * 1e17 , 1e18) = 1e18. so contract would mint 1e18 LP token for user1 and transfer 11 * 1e18 base token and 5 * 1e18 fractional token from user to contract address.
4. now contract has 111 * 1e18 base token balance and 55 * 1e18 fractional token balance and lpTokenSupply is 11 * 1e18.
5. now user has 1e18 lp token which if user wants to remove the liquidity user would receive baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply = (111 * 1e18 * 1e18) / (11 * 118) = 10*1e18 base token and fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply = (55 * 1e18 * 1e18) / (11 * 118) = 5 * 1e18 fractional token amount.
6. so user1 provided liquidity and then withdraw it immediately and lost 1e18 amount of base token in doing so.

the problem is that in the step #3 contract calculates lp token amount based on user supplied base token and fractional token and take the minimum between them but in the token that is not generating minimum lp tokens, user supplied more tokens (in the above example base token) so contract should return those extra tokens. in the above example the 1e18 lp token worth 10 * 1e18 base token and user supplied 11 * 1e18 amount. so contract should transfer that extra amount after calculating the lp amount.

This issue would happen in each call to add() function as users don't know exactly how much of each token they need to supply and to make sure transaction won't fail they would supply more tokens and it would make them to lose funds. in each call one of the base token amount or fractional token amount would generate lower lp token and user other tokens would have some extra amount.
And in cases where user makes a mistake and transfer a lot of tokens from one of the base token or fractional token those tokens would be lost.

Tools Used

VIM

Recommended Mitigation Steps

after calculating the amount of minting lp tokens, calculate the amount of base token and fractional token required based on lp token amount and transfer those exact required amount of tokens to contract address and return the extra amount if necessary. something like this:

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

       // calculate actual amount of baseToken and fractionalToken needed
       (actualBaseTokenAmount, actualFractionalTokenAmount) = removeQuote(lpTokenAmount);  // calculate it by upper bound

        // *** Effects *** //

        // transfer fractional tokens in
        _transferFrom(msg.sender, address(this), actualFractionalTokenAmount); // transfer actual needed amount

        // *** 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);  // transfer actual needed amount
        }else { // transfer back extra ETH
           (msg.sender).msg.sender.safeTransferETH(baseTokenAmount -actualBaseTokenAmount);
        }

       

        emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
    }

[c4-judge]

berndartmueller marked the issue as duplicate of #376

[c4-judge]

berndartmueller changed the severity to 3 (High Risk)

[c4-judge]

berndartmueller marked the issue as satisfactory