Caviar contest - rjs's results

Lines of code

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

Vulnerability details

Impact

In the current Caviar protocol, anybody can supply initial liquidity to a newly created pool.

An LP who intends to create a new pool and add liqiduity could execute the following transactions:

1. LP transaction 1: pair = Caviar.create()
2. LP transaction 2: pair.add(..., uint256 minLpTokenAmount)

If the case where minLpTokenAmount allows for some slippage, an MEV searcher could force the LP to pay more for transaction 2 than is necessary by detecting LP transaction 1, and then sandwiching LP transaction 2 as follows:

1. LP transaction 1: pair = Caviar.create()
2. MEV searcher transaction 1: pair.add(...)
3. LP transaction 2: pair.add(..., uint256 minLpTokenAmount)
4. MEV searcher transaction 2: pair.remove(...)

The legitimate LP would be forced to pay the MEV searcher a slippage amount, or abandon their attempt to provide liquidity.

Proof of Concept

Anybody can call Pair.add() after Caviar.create().

In Pair.sol#L77, the lpTokenAmount that will be minted for the LP invoking Pair.add() is determined by addQuote.

In addQuote Pair.sol#L419 the contract checks if there is already liquidity in the pool:

419:         if (lpTokenSupply > 0) {

If there is no liquidity in the pool (i.e. lpTokenSupply == 0), Pair.sol#L424-L427 executes:

424:         } else {
425:             // if there is no liquidity then init
426:             return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
427:         }

Thus, the first party to call Pair.add() can control the initial value of k in the AMM k = x*y formula.

As a result, the legitimate LP provider is forced to either:

1. Provide zero slippage i.e. a minLpTokenAmount value equal to Math.sqrt(baseTokenAmount * fractionalTokenAmount)
2. Provide some slippage, effectively bribing the MEV searcher to allow their Pair.add() operation to proceed; the MEV searcher earns Math.sqrt(baseTokenAmount * fractionalTokenAmount) - minLpTokenAmount in this case.

The problem with option 1, is that an MEV searcher could effectively hold the contract 'hostage' by just executing pair.add() after Caviar.create(). While the legitimate LP won't be 'forced' to pay the 'bribe', in reality if they don't pay the MEV searcher what they are looking for, then they will be forced to abandon their attempt to provide liquidity. In my opinion, this forced loss of value qualifies as a high risk issue.

Note: Ordinary Slippage / Subsequent Liquidity Provisions

You could argue that this issue is also present in subsequent liquidity provisioning operations, however I think it's clear that the contract author's intention is to accept slippage during ordinary market operations as a known issue by virtue of the slippage parameter minLpTokenAmount.

The issue I am describing here differs from this ordinary course of operations because in the state immediately after pool creation there is 0 liquidity and therefore there cannot be slippage, and as a result an MEV searcher can hold the pool hostage until a 'bribe' is paid. After that point, ordinary market operations commence.

Recommended Mitigation Steps

I suggest making Caviar.create() an internal function, and providing a new public function e.g. Caviar.createWithLiquidity() that calls Caviar.create() and pair.add() in a single transaction.

An alternative would be to implement a more complex permission model for adding initial liquidity (e.g. limiting it to the pool creator), but then you run the risk of griefing (i.e. the pool creator never sends liquidity). To deal with that would introduce even more complexity, and I think that clearly goes against the current very clean, permisionless architecture of the contracts.

Summary

The Caviar codebase is very well written and follows a clean, permisionless model.

Low Risk Findings

Non-critical Findings

Low Risk Findings

[L-01] Anybody can create a pair without funding

Caviar.create() is not permissioned, and there is no cost other than a gas cost for invoking the operation.

As a result, it would be possible to pollute the Caviar contract with unfunded pairs.

This is a low-risk issue because front-ends could easily choose to filter out inactive or unfunded pairs.

Non-critical Findings

[N-01] Use WETH instead of native ETH

In the Pair contract there are various branches of logic to handle native ETH vs. ERC20 tokens. It's not clear why native ETH support is necessary, since WETH is available.

According to the authors own specification, Caviar is based on Uniswap V2. One of the big changes from Uniswap V1 to Uniswap V2 was to move from native ETH to WETH. While Uniswap offers much more functionality than Caviar, one of the benefits that still applies to Caviar is a simpler codebase: having extra branches to handle ETH increases the contract complexity, and therefore the risk of defects.