User will often overpay when adding liquidity

[code423n4]

Lines of code

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

Vulnerability details

Impact

One of the assets (either baseTokens or fractionalTokens) will likely be overpaid when the user calls Pair.add liquidity.

Proof of Concept

The Pair.add function takes baseTokenAmount, fractionalTokenAmount and minLpTokenAmount as inputs. The first two parameters are pulled in from the user (or in the case of Ether as baseToken have to be sent with the transaction), while the latter is used as slippage control:

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

The addQuote function returns the amount of lp tokens minted, as a minimum of the baseTokenShare and fractionalTokenShare. In the add function both amounts are pulled in full though:

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

This will likely cause overpayment for one of the assets as described under #Impact, unless the user choses 0% slippage, in which case his tx will fail, if there are any swaps or liquidity changes happening before the transaction passes.

This could also possibly used in frontrunning attacks, though this has not been checked in-depth for a viable scenario due to time constraints.

Tools Used

Manual Review

Recommended Mitigation Steps

• Option 1: calculate excess tokens and send them back to the user
• Option 2: Only pull the funds necessary (as UniswapV2 does)

[Shungy]

Seems valid.

Duplicate: #90

[iFrostizz]

Seems valid.

Duplicate: #90

The slippage is checked afterwards, in the add() function that is used to add liquidity. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L77
Additionally, it actually "pulls the funds necessary" because the pulled funds are those which are supplied by the user.

[Minh-Trng]

Seems valid.
Duplicate: #90

The slippage is checked afterwards, in the add() function that is used to add liquidity. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L77 Additionally, it actually "pulls the funds necessary" because the pulled funds are those which are supplied by the user.

It will pull more than necessary if baseTokenShare and fractionalTokenShare have different values. The slippage check does not mitigate that

[Shungy]

@iFrostizz , nope, this is not about slippage per se. Please read this report and report #90 carefully.

[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