Caviar contest - Junnon's results

Lines of code

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#L76-L80

Vulnerability details

Impact

The add function in Pair.sol allows users to add liquidity to the contract. One of the parameters of this function, called minLpTokenAmount, is used to calculate the necessary amount of liquidity to prevent slippage using the addQuote function. However, minLpTokenAmount does not prevent users from inputting the wrong amount of baseToken and fractionalToken, which may result in the loss of user funds.

Proof of Concept

the addQuote is used to calculate amount of LP Token will be received by user. the function calculate using Math.min between LP calculation of baseToken and fractionalToken. this is the code snippets

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

This function will return the minimum value between baseTokenShare and fractionalTokenShare. Consider the following scenario:

1. Alice initiates LP with 100 Base Tokens and 30 Fractional Tokens, resulting in 54 LP Tokens for Alice.
2. Bob adds LP with 100 Base Tokens and 100 Fractional Tokens, also resulting in 54 LP Tokens for Bob. This will not be reverted, as the addQuote function will return the same amount of LP Tokens regardless of whether Bob adds 100 Fractional Tokens or just 30 Fractional Tokens.
3. Since Bob and Alice have the same amount of LP Tokens, when they remove liquidity, they will receive the same amount of tokens, which is 100 Base Tokens and 65 Fractional Tokens.
4. This scenario results in Alice receiving 35 additional tokens and Bob receiving 35 fewer tokens.

for code POC add this following code to Add.t.sol:

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

        //Babe Balance Calculation 
        //Babe Base Token Amount = 100
        uint256 babeUsdAmount = baseTokenAmount; 
        // Babe Fractional Token Amount = 100
        uint256 babeFractionalAmount = baseTokenAmount;
        // Babe Receive the same amount of LP token as received by address(this) 
        uint256 expectedLpTokenAmount = lpTokenSupplyBefore;
        //According to sponsor this method use to calculate minLpTokenAmount in Offchain.
        minLpTokenAmount = p.addQuote(babeUsdAmount, babeFractionalAmount);

        //Give babe token for the transaction
        deal(address(usd), babe, babeUsdAmount, true);
        deal(address(p), babe, babeFractionalAmount, true);

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

        // assert
        assertEq(lpToken.balanceOf(address(this)), lpToken.balanceOf(babe),
            "Babe balance should same as address(this)"
        );
        assertEq(lpToken.balanceOf(babe), expectedLpTokenAmount,
            "Should have minted lp tokens"
        );
        assertEq(lpToken.totalSupply() - lpTokenSupplyBefore, expectedLpTokenAmount,
            "Should have increased lp supply"
        );

        // Babe removing LP
        vm.startPrank(babe);
        p.remove(lpTokenAmount,0,0);
        vm.stopPrank();

        // address(this) removing LP
        p.remove(lpTokenAmount,0,0);

        // these 2 user have same amount of base token
        assertEq(usd.balanceOf(address(this)), usd.balanceOf(babe),
            "Base Token Balance of Babe should same as balance of address(this)"
        );
        // these 2 users have same amount of fractional token
        assertEq(p.balanceOf(address(this)), p.balanceOf(babe),
            "Fractional Token Balance of Babe should same as balance of address(this)"
        );
    }

Recommended Mitigation Steps

The add function needs to verify that the composition of baseTokenAmount and fractionalTokenAmount is equal to the price of the token. There is already a price function within the contract to assist with this check. If the contract needs to allow for a certain amount of price fluctuation, a parameter can be added and compared with the current price.

example of mitigation

//let say there's toleration of 1% slippage
require(baseTokenAmount < fractionalTokenAmount * price() * 100 / 10000)