Caviar contest - caventa's results

Lines of code

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

Vulnerability details

Impact

The user could call the addQuote function with baseTokenAmount and fractionalTokenAmount input values which results in DIFFERENT baseTokenShare and fractionalTokenShare. This is WRONG because both shares' values should be the same.

Proof of Concept

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

The returned lpTokenAmount should represent the SAME SHARE for both baseTokenShare and fractionalTokenShare.

Like how it was written in the removeQuote function. If the user provides the SAME lpTokenAmount, the system will return back baseTokenOutputAmount and fractionalTokenOutputAmount based on the same ratio => lpTokenAmount / lpTokenSupply

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

Add function is an outer function that will call addQuote function.

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

If the user calls the add functions with equal ratios like

add(_totalBaseTokenAmount / 3, _totalFractionalTokenAmount / 3, 0); add(_totalBaseTokenAmount / 3, _totalFractionalTokenAmount / 3, 0); add(_totalBaseTokenAmount / 3, _totalFractionalTokenAmount / 3, 0);

or

add(_totalBaseTokenAmount / 3, _totalFractionalTokenAmount / 3, 0); add(_totalBaseTokenAmount / 4, _totalFractionalTokenAmount / 4 0); add(_totalBaseTokenAmount / 5, _totalFractionalTokenAmount / 5, 0);

This is correct

However, the current code is unable to prevent users to call the add functions with different ratios like

add(_totalBaseTokenAmount / 3, _totalFractionalTokenAmount / 5, 0); add(_totalBaseTokenAmount / 4, _totalFractionalTokenAmount / 10 0); add(_totalBaseTokenAmount / 5, _totalFractionalTokenAmount / 15, 0);

Tools Used

Manual

Recommended Mitigation Steps

Add a new line of code. Allow the difference between baseTokenShare and fractionalTokenShare to be at most 9 (10-1) (Or maybe another smaller integer). There could be rounding issues that cause a VERY minor difference between the two values.

  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();
            
            +++    require(baseTokenShare > fractionalTokenShare? baseTokenShare - fractionalTokenShare < 10 : fractionalTokenShare - baseTokenShare < 10, "Share should be the same");

            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

And when both shares have VERY minor little differences, the min function will return the smallest value (See the next line).

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Caviar.sol#L33-L37

     string memory baseTokenSymbol = baseToken == address(0) ? "ETH" : baseToken.tokenSymbol();
     string memory nftSymbol = nft.tokenSymbol();
     string memory nftName = nft.tokenName();
     string memory pairSymbol = string.concat(nftSymbol, ":", baseTokenSymbol);
     pair = new Pair(nft, baseToken, merkleRoot, pairSymbol, nftName, nftSymbol);

"ETH" is used as an identifier for Pair to receive the native token Ether. However, If the ERC20 token has "ETH" as symbol, this will be confusing. Consider modifying the Pair constructor to accept a boolean flag to determine if the native token is used in the pair contract.

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L95 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L134 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L137 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L169 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L172 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L200 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L203 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L239 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L259 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L370

It is recommended that don't use solmate library to do transfers.

The safetransfer and safetransferfrom doesn’t check the existence of code at the token address. This is a known issue while using solmate’s libraries. Hence this may lead to miscalculation of funds and may lead to loss of funds , because if safetransfer() and safetransferfrom() are called on a token address that doesn’t have contract in it, it will always return success, bypassing the return value check.

We should use Use openzeppelin’s safeERC20 or implement a code existence check before every safeTransfer and safeTransferFrom.

3. Unwrap function should check the tokens exist in the merkle root like in the wrap function

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

   ---   function unwrap(uint256[] calldata tokenIds) public returns (uint256 fractionalTokenAmount) {
   +++ function unwrap(uint256[] calldata tokenIds, bytes32[][] calldata proofs) public returns (uint256 fractionalTokenAmount) {
   +++ _validateTokenIds(tokenIds, proofs); 

       // *** Effects *** //

        // burn fractional tokens from sender
        fractionalTokenAmount = tokenIds.length * ONE;
        _burn(msg.sender, fractionalTokenAmount);

        // *** Interactions *** //

        // transfer nfts to sender
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
        }

        emit Unwrap(tokenIds);
    }

4. We should ensure the length of tokenIds and proofs are the same.

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

    function _validateTokenIds(uint256[] calldata tokenIds, bytes32[][] calldata proofs) internal view {
    +++ require(tokenIds.length == proofs.length, "Length should be the same");
        // if merkle root is not set then all tokens are valid
        if (merkleRoot == bytes23(0)) return;

        // validate merkle proofs against merkle root
        for (uint256 i = 0; i < tokenIds.length; i++) {
            bool isValid = MerkleProofLib.verify(proofs[i], merkleRoot, keccak256(abi.encodePacked(tokenIds[i])));
            require(isValid, "Invalid merkle proof");
        }
    }