Caviar contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

Liquidity providers may lose a portion of provided liquidity in either of the pair tokens. While the minLpTokenAmount protects from slippage when adding liquidity, it doesn't protect from providing liquidity at different K.

Proof of Concept

The Pair contract is designed to receive liquidity from liquidity providers (Pair.sol#L63). First liquidity provider in a pool may provide arbitrary token amounts and set the initial price (Pair.sol#L425-L426), but all other liquidity providers must provide liquidity proportionally to current pool reserves (Pair.sol#L420-L423). Since a pool is made of two tokens and liquidity is provided in both tokens, there's a possibility for a discrepancy: token amounts may be provided in different proportions. When this happens, the smaller of the proportions is chosen to calculate the amount of LP tokens minted (Pair.sol#L420-L423):

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

As a result, the difference in proportions will create an excess of tokens that won't be redeemable for the amount of LP tokens minted. The excess of tokens gets, basically, donated to the pool: it'll be shared among all liquidity providers of the pool. While the minLpTokenAmount argument of the add function (Pair.sol#L63) allows liquidity providers to set the minimal amount of LP tokens they want to receive, it doesn't allow them to minimize the disproportion of token amounts or avoid it at all.

// test/Pair/unit.Add.t.sol

function testLockOfFunds_AUDIT() public {
    address alice = address(0x31337);
    address bob = address(0x12345);
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    deal(address(usd), alice, 100e18, true);
    deal(address(usd), bob, 100e18, true);
    deal(address(p), alice, 100e18, true);
    deal(address(p), bob, 100e18, true);

    // Alice is the first liquidity provider.
    vm.startPrank(alice);
    usd.approve(address(p), type(uint256).max);
    p.add(10 ether, 10 ether, 0);
    vm.stopPrank();

    // Bob provides liquidity to the pool and sets the minimal LP amount.
    // The token amounts are deposited in different proportions, thus the smaller
    // one will be chosen to calculate the amount of LP tokens Bob will receive.
    vm.startPrank(bob);
    usd.approve(address(p), type(uint256).max);
    uint256 minLPAmount = 1e18;
    uint256 bobLPAmount = p.add(1.2 ether, 1 ether, minLPAmount);
    vm.stopPrank();

    // Bob has received the minimal LP amount he wanted.
    assertEq(bobLPAmount, minLPAmount);

    // However, after removing all his liquidity from the pool...
    (uint256 bobUSDBefore, uint256 bobFracBefore) = (usd.balanceOf(bob), p.balanceOf(bob));
    vm.prank(bob);
    p.remove(minLPAmount, 0, 0);
    (uint256 bobUSDAfter, uint256 bobFracAfter) = (usd.balanceOf(bob), p.balanceOf(bob));

    // ... Bob received less USD than he deposited.
    assertEq(bobUSDAfter - bobUSDBefore, 1.018181818181818181 ether);
    assertEq(bobFracAfter - bobFracBefore, 1.000000000000000000 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

In the add function, consider calculating optimal token amounts based on the amounts specified by user, current pool reserves, and the minimal LP tokens amount specified by user. As a reference, consider this piece from the Uniswap V2 Router: UniswapV2Router02.sol#L45-L60.

Lines of code

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

Vulnerability details

Impact

First liquidity provider of a pair may inflate the price of the LP token of the pair and steal liquidity provided by next liquidity providers.

Proof of Concept

The Pair contract allows liquidity providers to provide liquidity to the pool using the add function (Pair.sol#L63). To track liquidity provided by each provider and their share in the pool, an LP token is used (Pair.sol#L50). The amount of LP tokens minted is determined using the following rules:

1. if a pair is empty, the amount is calculated as sqrt(baseTokenAmount * fractionalAmount) (Pair.sol#L426);
2. if a pair has liquidity, the amount of LP tokens minted is proportional to the amount of tokens deposited (Pair.sol#L421-L423).

This opens up the following attack vector:

1. First liquidity provider of a pool may provide the minimal amount so that they get 1 wei of LP tokens.
2. First liquidity provider then sends some amount of tokens to the pool directly, to inflate the price of their LP tokens.
3. Other liquidity providers add liquidity to the pool and receive 0 LP tokens due to the rounding in the LP tokens amount calculation.
4. First liquidity provider withdraws their 1 wei of LP tokens and get the entire liquidity of the pool.

// test/Pair/unit/Add.t.sol
function testSharePriceManipulation() public {
    address alice = address(0x31337);
    address bob = address(0x12345);
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    deal(address(usd), alice, 100e18, true);
    deal(address(usd), bob, 100e18, true);
    deal(address(p), alice, 100e18, true);
    deal(address(p), bob, 100e18, true);

    // Alice deposits 1 wei of USD and the fractional token.
    // Alice gets 1 wei of LP token.
    // Alice them sends 10 USD to the pair to inflate the price of the 1 wei of LP token.
    vm.startPrank(alice);
    usd.approve(address(p), type(uint256).max);
    assertEq(p.add(1, 1, 0), 1);
    usd.transfer(address(p), 10 ether);
    vm.stopPrank();

    // Bob deposits 9 USD and 9 fraction tokens.
    // Bob gets 0 LP tokens due to rounding.
    vm.startPrank(bob);
    usd.approve(address(p), type(uint256).max);
    assertEq(p.add(9 ether, 9 ether, 0), 0);
    vm.stopPrank();

    // Alice removes 1 wei of LP tokens.
    (uint256 aliceUSDBefore, uint256 aliceFracBefore) = (usd.balanceOf(alice), p.balanceOf(alice));
    vm.prank(alice);
    p.remove(1, 0, 0);
    (uint256 aliceUSDAfter, uint256 aliceFracAfter) = (usd.balanceOf(alice), p.balanceOf(alice));

    // Alice got: the 10 USD she deposited + the 9 USD Bob deposited.
    assertEq(aliceUSDAfter - aliceUSDBefore, 19.000000000000000001 ether);
    // Alice also got: the 9 fractional tokens Bob deposits.
    assertEq(aliceFracAfter - aliceFracBefore, 9.000000000000000001 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

Multiple steps can mitigate the issue:

1. Consider reverting when 0 shares are minted. This will reduce the maximal price that can be set by an attacker since high prices will always cause a rounding that results in 0 shares minted.
2. Consider requiring that initial liquidity is provided upon pair deployment. This will also fix pool initialization front running attacks.
3. Consider setting up a minimal required LP tokens amount that is minted when first liquidity is provider. This will increase the cost of the attack since manipulating a price will require more funds for an attacker.

However, be aware that sending first 1000 wei of LP tokens to the zero address (similarly to how Uniswap V2 does it) will introduce a different problem: some fractional tokens won't ever be redeemable and thus at least one NFT in a pool won't ever be unwrappable.

Lines of code

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

Vulnerability details

Impact

Fractional tokens may be underpriced by 1 wei in majority of trades. The K constant invariant won't hold true for majority of trades.

Proof of Concept

The Pair contract allows users to buy fractional tokens for base tokens using the buy function (Pair.sol#L147). In the function, users provide the outputAmount argument, which is the number of fractional tokens they want to buy. The function calls buyQuote to calculate the amount of base tokens users have to pay to buy outputAmount. The buyQuote function calculates an amount using the current fractional token and base token reserves of a pair (Pair.sol#L398):

function buyQuote(uint256 outputAmount) public view returns (uint256) {
    return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
}

The division in the function may cause rounding errors–when this happens, fractional tokens will be underpriced by 1 wei. As a result, the K constant invariant won't hold true for such swaps. This will affect the majority of trades since the chance that the numerator divides by the denominator without a remainder is very low.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding 1 to the result of the calculation in buyQuote:

--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -396,7 +396,7 @@ contract Pair is ERC20, ERC721TokenReceiver {
     /// @param outputAmount The amount of fractional tokens to buy.
     /// @return inputAmount The amount of base tokens required.
     function buyQuote(uint256 outputAmount) public view returns (uint256) {
-        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
+        return 1 + (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
     }

Lines of code

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

Vulnerability details

Impact

An attacker may manipulate the price of a pair by transferring tokens directly to the pair. Since the Pair contract exposes the price function, it maybe be used as a price oracle in third-party integrations. Manipulating the price of a pair may allow an attacker to steal funds from such integrations.

Proof of Concept

The Pair contract is a pool of two tokens, a base token and a fractional token. Its main purpose is to allow users to swap the tokens at a fair price. Since the price is calculated based on the reserves of a pair, it can only be changed in two cases:

1. when initial liquidity is added: the first liquidity provider sets the price of a pool (Pair.sol#L85-L97); other liquidity providers cannot change the price (Pair.sol#L421-L423);
2. during trades: trading adds and removes tokens from a pool, ensuring the K constant invariant is respected (Pair.sol#L194-L204, Pair.sol#L161-L173).

However, the Pair contract calculates the price using the current token balances of the contract (Pair.sol#L379-L385, Pair.sol#L477-L481):

function baseTokenReserves() public view returns (uint256) {
    return _baseTokenReserves();
}

function _baseTokenReserves() internal view returns (uint256) {
    return baseToken == address(0)
        ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
        : ERC20(baseToken).balanceOf(address(this));
}

function fractionalTokenReserves() public view returns (uint256) {
    return balanceOf[address(this)];
}

This allows an attacker to change the price of a pool and skip the K constant invariant check that's enforced on new liquidity (Pair.sol#L421-L423).

Tools Used

Manual review

Recommended Mitigation Steps

Consider tracking pair's reserves internally, using state variables, similarly to how Uniswap V2 does that:

• UniswapV2Pair.sol#L22-L23:

uint112 private reserve0;           // uses single storage slot, accessible via getReserves
uint112 private reserve1;           // uses single storage slot, accessible via getReserves

• UniswapV2Pair.sol#L38-L42:

function getReserves() public view returns (uint112 _reserve0, uint112 _reserve1, uint32 _blockTimestampLast) {
    _reserve0 = reserve0;
    _reserve1 = reserve1;
    _blockTimestampLast = blockTimestampLast;
}

• UniswapV2Pair.sol#L38-L42:

// update reserves and, on the first call per block, price accumulators
function _update(uint balance0, uint balance1, uint112 _reserve0, uint112 _reserve1) private {
    require(balance0 <= uint112(-1) && balance1 <= uint112(-1), 'UniswapV2: OVERFLOW');
    uint32 blockTimestamp = uint32(block.timestamp % 2**32);
    uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
        // * never overflows, and + overflow is desired
        price0CumulativeLast += uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) * timeElapsed;
        price1CumulativeLast += uint(UQ112x112.encode(_reserve0).uqdiv(_reserve1)) * timeElapsed;
    }
    reserve0 = uint112(balance0);
    reserve1 = uint112(balance1);
    blockTimestampLast = blockTimestamp;
    emit Sync(reserve0, reserve1);
}