Wrong token amount is transferred when calling add() of contract Pair

[code423n4]

Lines of code

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

Vulnerability details

Impact

Users calling Pair.sol#add() will cost more tokens than needed.
It also makes Pair.sol#add() and Pair.sol#nftAdd() vulnerable to sandwich attacks.

Proof of Concept

It is incorrect that the token amounts transferred in function Pair.sol#add is derived from input parameters (baseTokenAmount, fractionalTokenAmount) rather than recalculate the amounts from the lpTokenAmount to mint:

  function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount) returns (uint256 lpTokenAmount) {
      // -- omit --
      lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
      // -- omit --
      _transferFrom(msg.sender, address(this), fractionalTokenAmount);
      // -- omit --
      lpToken.mint(msg.sender, lpTokenAmount);
      // -- omit --
      if (baseToken != address(0)) {
          ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
      }
      // -- omit --
  }

For example, if a pair contains 10 ETH and 1000 FT(fractional token), the following three users will get the same amount of lp tokens:

1. user1: add(1 ETH, 100 FT)
2. user2: add(1 ETH, 200 FT)
3. user3: add(11 ETH, 100 FT)

All of them will get the same number of lp tokens, which means user2 costs 100 FT more and user3 costs 10 ETH more.

Tools Used

Manual

Recommended Mitigation Steps

1. calculate the amount of fractional tokens to transferFrom based on the lpTokenAmount
2. if base token is ERC20: calculate the amount of base tokens to transferFrom based on the lpTokenAmount
3. if base token is ETH: calculate the amount of base tokens needed based on the lpTokenAmount and return the excess ETH to user.

[Shungy]

Dup #90

[c4-judge]

berndartmueller marked the issue as duplicate of #376

[c4-judge]

berndartmueller marked the issue as satisfactory