Caviar contest - wait's results

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.

Lines of code

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

Vulnerability details

Impact

Users might sell NFT or fractional tokens for 0 base token.

Proof of Concept

The return value of function Pair.sol#sellQuote may be 0.

function sellQuote(uint256 inputAmount) public view returns (uint256) {
    uint256 inputAmountWithFee = inputAmount * 997;
    return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
}

This will result in the user calling Pair.sol#sell lose the inputAmount of fractional tokens, because 0 base token will send to the user if minOutputAmount = 0.

Tools Used

Manual

Recommended Mitigation Steps

I recommend to revert the Pair.sol#sell if outputAmount is 0