Caviar contest - Tricko's results

Lines of code

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

Vulnerability details

Impact

The first minter can manipulate the supply of LP tokens and baseToken-fractional ratio, hindering small liquidity providers from interacting with the pair.

A malicious actor can mint 1wei of LP token from a new pair, then proceed to transfer baseToken to the Pair contract, artificially increasing the baseToken reserves. Therefore 1 wei of LP token can be worth arbitrarily large amounts of baseToken and fractional token (depending on how much baseToken is deposited to the Pair)

Proof of Concept

On a empty pool with baseToken USD (generic USD stablecoin with 18 decimals), the attacker can do the following steps:

1. Call add() with 1wei of USD and 1wei of fractional Token, minting 1wei of LP Token.
2. Transfer 100$ worth of USD (10**20 of USD) to the pair contract.

Then the next user minting would have to add more than 100$ worth of USD to be able to mint 1wei of LP. The attacker can transfer arbitrarily large amounts of tokens to the contract, making infeasible for small liquidity providers to provide any liquidity.

contract MinimumManipulation is Fixture {
    function setUp() public {
        uint256 baseTokenAmount = 1e30;
        uint256 fractionalTokenAmount = 1e30;

        deal(address(usd), address(1), baseTokenAmount, true);
        deal(address(p), address(1), fractionalTokenAmount, true);

        deal(address(usd), address(2), baseTokenAmount, true);
        deal(address(p), address(2), fractionalTokenAmount, true);
    }

    function testManipulation() public {
        vm.startPrank(address(1));
        usd.approve(address(p), type(uint256).max);

        p.add(1, 1, 0);
        usd.transfer(address(p), 100*1e18);
        vm.stopPrank();

        vm.startPrank(address(2));
        usd.approve(address(p), type(uint256).max);
        uint256 lp = p.add(101*1e18, 1, 0);

        //User has to add more than 100$ to the pool to be able to mint 1 token
        assert(lp == 1);
    }
}

Recommended Mitigation Steps

Please consider minting a minimal amount of LP tokens during the first mint and sending them to zero address, this increases the cost of the attack. Uniswap V2 uses the value 1000 as it is small enough to don't hurt the first minter, while still increasing the cost of this attack by 1000x.

diff --git a/Pair.sol.orig b/Pair.sol
index 185d25c..a2e84f9 100644
--- a/Pair.sol.orig
+++ b/Pair.sol
@@ -1,51 +1,52 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.17;

 import "solmate/tokens/ERC20.sol";
 import "solmate/tokens/ERC721.sol";
 import "solmate/utils/MerkleProofLib.sol";
 import "solmate/utils/SafeTransferLib.sol";
 import "openzeppelin/utils/math/Math.sol";

 import "./LpToken.sol";
 import "./Caviar.sol";

 /// @title Pair
 /// @author out.eth (@outdoteth)
 /// @notice A pair of an NFT and a base token that can be used to create and trade fractionalized NFTs.
 contract Pair is ERC20, ERC721TokenReceiver {
     using SafeTransferLib for address;
     using SafeTransferLib for ERC20;

     uint256 public constant ONE = 1e18;
     uint256 public constant CLOSE_GRACE_PERIOD = 7 days;
+    uint256 public constant MINIMUM_LIQUIDITY = 1000;

     address public immutable nft;
     address public immutable baseToken; // address(0) for ETH
     bytes32 public immutable merkleRoot;
     LpToken public immutable lpToken;
     Caviar public immutable caviar;
     uint256 public closeTimestamp;

     event Add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
     event Remove(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 lpTokenAmount);
     event Buy(uint256 inputAmount, uint256 outputAmount);
     event Sell(uint256 inputAmount, uint256 outputAmount);
     event Wrap(uint256[] tokenIds);
     event Unwrap(uint256[] tokenIds);
     event Close(uint256 closeTimestamp);
     event Withdraw(uint256 tokenId);

     constructor(
         address _nft,
         address _baseToken,
         bytes32 _merkleRoot,
         string memory pairSymbol,
         string memory nftName,
         string memory nftSymbol
     ) ERC20(string.concat(nftName, " fractional token"), string.concat("f", nftSymbol), 18) {
         nft = _nft;
         baseToken = _baseToken; // use address(0) for native ETH
         merkleRoot = _merkleRoot;
         lpToken = new LpToken(pairSymbol);
         caviar = Caviar(msg.sender);
@@ -60,60 +61,63 @@ contract Pair is ERC20, ERC721TokenReceiver {
     /// @param fractionalTokenAmount 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)
         public
         payable
         returns (uint256 lpTokenAmount)
     {
         // *** Checks *** //

         // check the token amount inputs are not zero
         require(baseTokenAmount > 0 && fractionalTokenAmount > 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");

         // calculate the lp token shares to mint
         lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);

         // check that the amount of lp tokens outputted is greater than the min amount
         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

         // *** Effects *** //

         // transfer fractional tokens in
         _transferFrom(msg.sender, address(this), fractionalTokenAmount);

         // *** Interactions *** //

         // mint lp tokens to sender
+        if (lpToken.totalSupply() == 0) {
+            lpToken.mint(address(0), MINIMUM_LIQUIDITY);
+        }
         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);
         }

         emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
     }

     /// @notice Removes liquidity from the pair.
     /// @param lpTokenAmount The amount of LP tokens to burn.
     /// @param minBaseTokenOutputAmount The minimum amount of base tokens to receive.
     /// @param minFractionalTokenOutputAmount The minimum amount of fractional tokens to receive.
     /// @return baseTokenOutputAmount The amount of base tokens received.
     /// @return fractionalTokenOutputAmount The amount of fractional tokens received.
     function remove(uint256 lpTokenAmount, uint256 minBaseTokenOutputAmount, uint256 minFractionalTokenOutputAmount)
         public
         returns (uint256 baseTokenOutputAmount, uint256 fractionalTokenOutputAmount)
     {
         // *** Checks *** //

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

         // check that the base token output amount is greater than the min amount
         require(baseTokenOutputAmount >= minBaseTokenOutputAmount, "Slippage: base token amount out");

         // check that the fractional token output amount is greater than the min amount
@@ -396,61 +400,61 @@ 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);
     }

     /// @notice The amount of base tokens received for selling a given amount of fractional tokens.
     /// @dev Calculated using the xyk invariant and a 30bps fee.
     /// @param inputAmount The amount of fractional tokens to sell.
     /// @return outputAmount The amount of base tokens received.
     function sellQuote(uint256 inputAmount) public view returns (uint256) {
         uint256 inputAmountWithFee = inputAmount * 997;
         return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
     }

     /// @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.
     /// @return lpTokenAmount The amount of lp tokens received.
     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);
         } else {
             // if there is no liquidity then init
-            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+            return Math.sqrt(baseTokenAmount * fractionalTokenAmount) - MINIMUM_LIQUIDITY;
         }
     }

     /// @notice The amount of base tokens and fractional tokens received for burning a given amount of lp tokens.
     /// @dev Calculated as a share of existing deposits.
     /// @param lpTokenAmount The amount of lp tokens to burn.
     /// @return baseTokenAmount The amount of base tokens received.
     /// @return fractionalTokenAmount The amount of fractional tokens received.
     function removeQuote(uint256 lpTokenAmount) public view returns (uint256, uint256) {
         uint256 lpTokenSupply = lpToken.totalSupply();
         uint256 baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply;
         uint256 fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply;

         return (baseTokenOutputAmount, fractionalTokenOutputAmount);
     }

     // ************************ //
     //      Internal utils      //
     // ************************ //

     function _transferFrom(address from, address to, uint256 amount) internal returns (bool) {
         balanceOf[from] -= amount;

         // Cannot overflow because the sum of all user
         // balances can't exceed the max uint256 value.
         unchecked {
             balanceOf[to] += amount;
         }

         emit Transfer(from, to, amount);

Lines of code

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

Vulnerability details

Impact

Price calculations in price() are wrong and also does not take into account tokens with different amount of decimals, like USDC.

Proof of Concept

Run the following tests. In both cases we have a 1:1 pool between fractional tokens and baseToken. Therefore the expected price is 1 for both cases. But in both cases we get 0.

contract PriceTest is Test {
    using stdStorage for StdStorage;
    MockERC721 public bayc;
    MockERC20 public dai;
    MockERC20 public usdc;
    Caviar public c;
    Pair public p18;
    Pair public p6;


    function setUp() public {
        bayc = new MockERC721("yeet", "YEET");
        dai = new MockERC20("Dai", "DAI", 18);
        usdc = new MockERC20("USDC", "USDC", 6);

        c = new Caviar();
        p18 = c.create(address(bayc), address(dai), bytes32(0));
        p6 = c.create(address(bayc), address(usdc), bytes32(0));
    }

    function testItReturnsCorrectPrice6decimals() public {
        // arrange
        uint256 baseTokenReserves = 1e6;
        uint256 fractionalTokenReserves = 1e18;
        uint256 expectedPrice = 1;

        // forgefmt: disable-next-item
        stdstore
            .target(address(usdc))
            .sig("balanceOf(address)")
            .with_key(address(p6))
            .checked_write(baseTokenReserves);

        // forgefmt: disable-next-item
        stdstore
            .target(address(p6))
            .sig("balanceOf(address)")
            .with_key(address(p6))
            .checked_write(fractionalTokenReserves);

        // act
        uint256 price = p6.price();

        // assert
        assertEq(price, expectedPrice, "Price does not match");
        //Expected: 1, Actual: 0
    }

    function testItReturnsCorrectPrice18decimals() public {
        // arrange
        uint256 baseTokenReserves = 1e18;
        uint256 fractionalTokenReserves = 1e18;
        uint256 expectedPrice = 1;

        // forgefmt: disable-next-item
        stdstore
            .target(address(dai))
            .sig("balanceOf(address)")
            .with_key(address(p18))
            .checked_write(baseTokenReserves);

        // forgefmt: disable-next-item
        stdstore
            .target(address(p18))
            .sig("balanceOf(address)")
            .with_key(address(p18))
            .checked_write(fractionalTokenReserves);

        // act
        uint256 price = p18.price();

        // assert
        assertEq(price, expectedPrice, "Price does not match");
        //Expected: 1, Actual: 0
    }
}

Recommended Mitigation Steps

Check for number of decimals and calculate price accordingly.

diff --git a/Pair.sol.orig b/Pair.sol
index 185d25c..cb110f9 100644
--- a/Pair.sol.orig
+++ b/Pair.sol
@@ -388,7 +388,8 @@ contract Pair is ERC20, ERC721TokenReceiver {
     /// @dev Calculated by dividing the base token reserves by the fractional token reserves.
     /// @return price The price of one fractional token in base tokens * 1e18.
     function price() public view returns (uint256) {
-        return (_baseTokenReserves() * ONE) / fractionalTokenReserves();
+        uint256 decimals = baseToken == address(0) ? 18 : ERC20(baseToken).decimals();
+        return (_baseTokenReserves() * 10**(18 - decimals)) / fractionalTokenReserves();
     }

     /// @notice The amount of base tokens required to buy a given amount of fractional tokens.

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

Also modify tests

diff --git a/MockERC20.sol.orig b/MockERC20.sol
index ca837ee..a9bece5 100644
--- a/MockERC20.sol.orig
+++ b/MockERC20.sol
@@ -4,5 +4,5 @@ pragma solidity ^0.8.17;
 import "solmate/tokens/ERC20.sol";

 contract MockERC20 is ERC20 {
-    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_, 18) {}
+    constructor(string memory name_, string memory symbol_, uint8 decimals) ERC20(name_, symbol_, decimals) {}
 }

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/test/shared/mocks/MockERC20.sol

diff --git a/Price.t.sol.orig b/Price.t.sol
index f70513d..7377697 100644
--- a/Price.t.sol.orig
+++ b/Price.t.sol
@@ -6,31 +6,74 @@ import "forge-std/console.sol";

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

-contract PriceTest is Fixture {
+contract PriceTest is Test {
     using stdStorage for StdStorage;
+    MockERC721 public bayc;
+    MockERC20 public dai;
+    MockERC20 public usdc;
+    Caviar public c;
+    Pair public p18;
+    Pair public p6;
+
+    function setUp() public {
+        bayc = new MockERC721("yeet", "YEET");
+        dai = new MockERC20("Dai", "DAI", 18);
+        usdc = new MockERC20("USDC", "USDC", 6);

-    function testItReturnsCorrectPrice() public {
+        c = new Caviar();
+        p18 = c.create(address(bayc), address(dai), bytes32(0));
+        p6 = c.create(address(bayc), address(usdc), bytes32(0));
+    }
+
+        function testItReturnsCorrectPrice6decimals() public {
+        // arrange
+        uint256 baseTokenReserves = 1e6;
+        uint256 fractionalTokenReserves = 1e18;
+        uint256 expectedPrice = 1;
+
+        // forgefmt: disable-next-item
+        stdstore
+            .target(address(usdc))
+            .sig("balanceOf(address)")
+            .with_key(address(p6))
+            .checked_write(baseTokenReserves);
+
+        // forgefmt: disable-next-item
+        stdstore
+            .target(address(p6))
+            .sig("balanceOf(address)")
+            .with_key(address(p6))
+            .checked_write(fractionalTokenReserves);
+
+        // act
+        uint256 price = p6.price();
+
+        // assert
+        assertEq(price, expectedPrice, "Price does not match");
+    }
+
+    function testItReturnsCorrectPrice18decimals() public {
         // arrange
-        uint256 baseTokenReserves = 500;
-        uint256 fractionalTokenReserves = 1000;
-        uint256 expectedPrice = baseTokenReserves * 1e18 / fractionalTokenReserves;
+        uint256 baseTokenReserves = 1e18;
+        uint256 fractionalTokenReserves = 1e18;
+        uint256 expectedPrice = 1;

         // forgefmt: disable-next-item
         stdstore
-            .target(address(usd))
+            .target(address(dai))
             .sig("balanceOf(address)")
-            .with_key(address(p))
+            .with_key(address(p18))
             .checked_write(baseTokenReserves);

         // forgefmt: disable-next-item
         stdstore
-            .target(address(p))
+            .target(address(p18))
             .sig("balanceOf(address)")
-            .with_key(address(p))
+            .with_key(address(p18))
             .checked_write(fractionalTokenReserves);

         // act
-        uint256 price = p.price();
+        uint256 price = p18.price();

         // assert
         assertEq(price, expectedPrice, "Price does not match");

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/test/Pair/unit/Price.t.sol