Caviar Private Pools - rbserver's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L87-L88 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L731-L738 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L750-L752 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

Vulnerability details

Impact

The examples in the changeFee's comment state that 25 changeFee would be equivalent to 0.0025 ETH and 5_000_000 changeFee would be equivalent to 500 USDC.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L87-L88

    /// @notice The change/flash fee to 4 decimals of precision. For example, 0.0025 ETH = 25. 500 USDC = 5_000_000.
    uint56 public changeFee;

These examples hold true in the following PrivatePool.changeFeeQuote function. feePerNft would be evaluated to 25 * (10 ** 14) = 0.0025e18, which is 0.0025 ETH, when changeFee is 25. Similarly, feePerNft would be evaluated to 5_000_000 * (10 ** 2) = 500e6, which is 500 USDC, when changeFee is 5_000_000.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L731-L738

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feePerNft = changeFee * 10 ** exponent;

        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
    }

However, this is not the case for the following PrivatePool.flashFee function because its returned flash fee is not adjusted for baseToken's and changeFee's decimals. For example, when changeFee is 25, this function would return a flash fee of 25 wei ETH that is much lower than 0.0025 ETH; when changeFee is 5_000_000, this function would return a flash fee of 5_000_000 wei USDC that is much lower than 500 USDC. Then, when uint256 fee = flashFee(token, tokenId) is executed in the PrivatePool.flashLoan function below, fee is much lower than it should be. As a result, the flash loan borrower pays a much lower flash fee, and the private pool owner loses a large portion of the flash fee that she or he should receive.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L750-L752

    function flashFee(address, uint256) public view returns (uint256) {
        return changeFee;
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

    function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(token, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

        // transfer the NFT to the borrower
        ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

        // call the borrower
        bool success =
            receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

        // check that flashloan was successful
        if (!success) revert FlashLoanFailed();

        // transfer the NFT from the borrower
        ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

        // transfer the fee from the borrower
        if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }

Proof of Concept

The following steps can occur for the described scenario.

1. As the private pool owner, Alice initializes the private pool with baseToken being ETH and changeFee being 25.
2. Alice then deposits some valuable NFTs in this private pool.
3. Bob calls the PrivatePool.flashLoan function to utilize one of these valuable NFTs to earn 0.005 ETH in another protocol but only pays 25 wei ETH as the fee for this flash loan.
4. Although she should earn 0.0025 ETH per flash loan, Alice only earns 25 wei ETH for this flash loan so she loses a large portion of the flash fee that she should receive.

Tools Used

VSCode

Recommended Mitigation Steps

The PrivatePool.flashFee function can be updated to adjust changeFee for baseToken's and changeFee's decimals like how the PrivatePool.changeFeeQuote function calculates feePerNft. This adjusted value can then be returned as the flash fee.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L152-L209 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L219-L248 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L254-L293 https://etherscan.io/address/0xf5b0a3efb8e8e4c201e2a935f110eaaf3ffecb8d#code#L672

Vulnerability details

Impact

The following EthRouter.sell, EthRouter.deposit, and EthRouter.change functions call the corresponding ERC721 tokens' setApprovalForAll functions.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L152-L209

    function sell(Sell[] calldata sells, uint256 minOutputAmount, uint256 deadline, bool payRoyalties) public {
        ...
        // loop through and execute the sells
        for (uint256 i = 0; i < sells.length; i++) {
            ...
            // approve the pair to transfer NFTs from the router
            ERC721(sells[i].nft).setApprovalForAll(sells[i].pool, true);
            ...
        }
        ...
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L219-L248

    function deposit(
        address payable privatePool,
        address nft,
        uint256[] calldata tokenIds,
        uint256 minPrice,
        uint256 maxPrice,
        uint256 deadline
    ) public payable {
        ...
        // approve pair to transfer NFTs from router
        ERC721(nft).setApprovalForAll(privatePool, true);
        ...
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L254-L293

    function change(Change[] calldata changes, uint256 deadline) public payable {
        ...
        // loop through and execute the changes
        for (uint256 i = 0; i < changes.length; i++) {
            Change memory _change = changes[i];
            ...
            // approve pair to transfer NFTs from router
            ERC721(_change.nft).setApprovalForAll(_change.pool, true);
            ...
        }
        ...
    }

For ERC721 tokens like Axie, which its setApprovalForAll function is shown below, calling their setApprovalForAll functions with the same msg.sender-_operator-_approved combination would revert because of requirements like require(_tokenOperator[msg.sender][_operator] != _approved). For these ERC721 tokens, calling the EthRouter.sell, EthRouter.deposit, and EthRouter.change functions for the first time, which call such tokens' setApprovalForAll functions for the first time, can succeed; however, calling the EthRouter.sell, EthRouter.deposit, and EthRouter.change functions again, which call such tokens' setApprovalForAll functions with the same pool as _operator and true as _approved again, will revert. In this case, the EthRouter.sell, EthRouter.deposit, and EthRouter.change functions are DOS'ed for such ERC721 tokens.

https://etherscan.io/address/0xf5b0a3efb8e8e4c201e2a935f110eaaf3ffecb8d#code#L672

  function setApprovalForAll(address _operator, bool _approved) external whenNotPaused {
    require(_tokenOperator[msg.sender][_operator] != _approved);
    _tokenOperator[msg.sender][_operator] = _approved;
    ApprovalForAll(msg.sender, _operator, _approved);
  }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the EthRouter.sell function to sell 1 Axie NFT to a private pool, which succeeds.
2. Alice calls the EthRouter.sell function again to sell another Axie NFT to the same private pool. However, this function call's execution of ERC721(sells[i].nft).setApprovalForAll(sells[i].pool, true) reverts because Axie's require(_tokenOperator[msg.sender][_operator] != _approved) reverts.
3. Bob tries to repeat Step 2 but his EthRouter.sell function call also reverts.
4. Hence, the EthRouter.sell function is DOS'ed for selling any Axie NFTs to the same private pool for any users.

Tools Used

VSCode

Recommended Mitigation Steps

The EthRouter.sell, EthRouter.deposit, and EthRouter.change functions can be respectively updated to check if the EthRouter contract has approved the corresponding pool to spend any of the corresponding ERC721 tokens received by itself. If not, the corresponding ERC721's setApprovalForAll function can be called; otherwise, the corresponding ERC721's setApprovalForAll function should not be called.

function _approveNfts(address nft, address target) internal {
    // check if the router is already approved to transfer NFTs from the caller
    if (ERC721(nft).isApprovedForAll(address(this), target)) return;

    // approve the target to transfer NFTs from the router
    ERC721(nft).setApprovalForAll(target, true);
}

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L99-L144

Vulnerability details

Impact

When calling the following PrivatePool.buy function, royaltyFee would only be paid to recipient if royaltyFee > 0 && recipient != address(0) is true.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289

    function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof)
        public
        payable
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        ...
        if (payRoyalties) {
            for (uint256 i = 0; i < tokenIds.length; i++) {
                // get the royalty fee for the NFT
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }
        }
        ...
    }

Yet, when calling the EthRouter.buy function to buy an NFT from a public pool, royaltyFee would be paid to royaltyRecipient if royaltyFee > 0 is true. In this case, when the royalty info registry returns royaltyRecipient that is address(0), such as due to misconfiguration, the user still pays royaltyFee to address(0). As a result, the user loses such royaltyFee ETH amount.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L99-L144

    function buy(Buy[] calldata buys, uint256 deadline, bool payRoyalties) public payable {
        ...
        for (uint256 i = 0; i < buys.length; i++) {
            if (buys[i].isPublicPool) {
                ...
                if (payRoyalties) {
                    uint256 salePrice = inputAmount / buys[i].tokenIds.length;
                    for (uint256 j = 0; j < buys[i].tokenIds.length; j++) {
                        // get the royalty fee and recipient
                        (uint256 royaltyFee, address royaltyRecipient) =
                            getRoyalty(buys[i].nft, buys[i].tokenIds[j], salePrice);

                        if (royaltyFee > 0) {
                            // transfer the royalty fee to the royalty recipient
                            royaltyRecipient.safeTransferETH(royaltyFee);
                        }
                    }
                }
            } else {
                ...
            }
            ...
        }
        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the EthRouter.buy function to buy 1 NFT from a public pool.
2. The calculated royaltyFee is 0.01 ETH but the royalty info registry returns royaltyRecipient that is address(0) due to misconfiguration.
3. In this case, Alice still sends 0.01 ETH to address(0).
4. Thus, Alice loses such 0.01 ETH.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L121 can be updated to if (royaltyFee > 0 && royaltyRecipient != address(0)) {.

QA REPORT

[01] FEE-ON-TRANSFER TOKENS ARE NOT SUPPORTED

For a fee-on-transfer token, calling the following PrivatePool.buy function can transfer an token amount that is less than netInputAmount to the PrivatePool contract because the transfer fee is deducted after ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount) is executed. However, virtualBaseTokenReserves is still increased by uint128(netInputAmount - feeAmount - protocolFeeAmount). The resulting virtualBaseTokenReserves will be larger than the actual token amount owned by the PrivatePool contract after transferring out feeAmount and protocolFeeAmount tokens. This will lead to incorrect calculations when calling functions like PrivatePool.buyQuote and PrivatePool.sellQuote that depend on virtualBaseTokenReserves. For example, when calling the PrivatePool.buy function, which calls PrivatePool.buyQuote function, again would calculate an incorrect netInputAmount for the user to pay since virtualBaseTokenReserves is already higher than it should be; as a result, the user can pay too much.

As a mitigation, the PrivatePool.buy function can be updated to record the PrivatePool contract's balance of such fee-on-transfer token before and after the token transfer, and the token balance difference can then be used to update virtualBaseTokenReserves.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289

    function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof)
        public
        payable
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // ~~~ Checks ~~~ //

        // calculate the sum of weights of the NFTs to buy
        uint256 weightSum = sumWeightsAndValidateProof(tokenIds, tokenWeights, proof);

        // calculate the required net input amount and fee amount
        (netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);
        ...
        // ~~~ Effects ~~~ //

        // update the virtual reserves
        virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);
        virtualNftReserves -= uint128(weightSum);

        // ~~~ Interactions ~~~ //

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        ...
        if (baseToken != address(0)) {
            // transfer the base token from the caller to the contract
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);
        } else {
            // check that the caller sent enough ETH to cover the net required input
            if (msg.value < netInputAmount) revert InvalidEthAmount();

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);

            // refund any excess ETH to the caller
            if (msg.value > netInputAmount) msg.sender.safeTransferETH(msg.value - netInputAmount);
        }
        ...
    }

[02] PrivatePool.flashLoan FUNCTION DOES NOT CHECK baseToken != address(0) && msg.value > 0

The following PrivatePool.flashLoan function executes if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount() so it is possible that the user sends some ETH while baseToken is an ERC20 token. This is different than functions like PrivatePool.change below, which executes if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount(). If the user sends some ETH while baseToken is an ERC20 token when calling the PrivatePool.flashLoan function, the user loses such sent ETH amount.

As a mitigation, the PrivatePool.flashLoan function can be updated to additionally check if baseToken != address(0) && msg.value > 0 is true. If so, calling this function should revert.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

    function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(token, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();
        ...
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L385-L452

    function change(
        uint256[] memory inputTokenIds,
        uint256[] memory inputTokenWeights,
        MerkleMultiProof memory inputProof,
        IStolenNftOracle.Message[] memory stolenNftProofs,
        uint256[] memory outputTokenIds,
        uint256[] memory outputTokenWeights,
        MerkleMultiProof memory outputProof
    ) public payable returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        ...
        // check that the caller sent 0 ETH if base token is not ETH
        if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();
        ...
    }

[03] PrivatePool.flashLoan FUNCTION DOES NOT REFUND UNUSED ETH AMOUNTS TO USERS

The following PrivatePool.flashLoan function does not refund the sent extra ETH amount that is unused, which is unlike functions like PrivatePool.buy below, which does refund the user in this situation. Users, who are familiar with the refund mechanism provided by functions like PrivatePool.buy, can send more than enough ETH when calling the PrivatePool.flashLoan function. However, since these unused ETH amounts sent by the users will not be refunded to them, these users lose such unused ETH amounts.

As a mitigation, the PrivatePool.flashLoan function can be updated to refund the unused ETH amounts to users.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

    function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(token, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

        // transfer the NFT to the borrower
        ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

        // call the borrower
        bool success =
            receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

        // check that flashloan was successful
        if (!success) revert FlashLoanFailed();

        // transfer the NFT from the borrower
        ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

        // transfer the fee from the borrower
        if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289

    function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof)
        public
        payable
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        ...
        if (baseToken != address(0)) {
            ...
        } else {
            ...
            // refund any excess ETH to the caller
            if (msg.value > netInputAmount) msg.sender.safeTransferETH(msg.value - netInputAmount);
        }
        ...
    }

[04] Factory.setProtocolFeeRate FUNCTION DOES NOT HAVE A LIMIT FOR SETTING protocolFeeRate

The following Factory.setProtocolFeeRate function can be called to set protocolFeeRate to be more than 10_000, which is unlike the PrivatePool.setFeeRate function that limits feeRate to be less or equal to 5_000. Setting protocolFeeRate without a limit can have unexpected consequences. For example, when protocolFeeRate is set to more than 10_000, the protocol fee can even exceed the total of all NFTs' salePrice when calling the PrivatePool.buy function in which the cost can be too high to the user unexpectedly.

As a mitigation, the Factory.setProtocolFeeRate function can be updated to only set protocolFeeRate to a value that cannot exceed certain reasonable limit, such as 5_000.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L141-L143

    function setProtocolFeeRate(uint16 _protocolFeeRate) public onlyOwner {
        protocolFeeRate = _protocolFeeRate;
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L562-L571

    function setFeeRate(uint16 newFeeRate) public onlyOwner {
        // check that the fee rate is less than 50%
        if (newFeeRate > 5_000) revert FeeRateTooHigh();

        // set the fee rate
        feeRate = newFeeRate;

        // emit the set fee rate event
        emit SetFeeRate(newFeeRate);
    }

[05] ETH AMOUNTS CAN BE SENT TO EthRouter AND Factory CONTRACTS ACCIDENTALLY

Because of the following EthRouter.receive and Factory.receive functions, ETH amounts can be accidentally sent to the EthRouter and Factory contracts. To prevent users accidentally send and lose such ETH amounts, the EthRouter.receive and Factory.receive functions can be updated to check if msg.sender is the PrivatePool contract; if not, calling these function should revert.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L88

    receive() external payable {}

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L55

    receive() external payable {}

[06] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, the critical address inputs should be checked against address(0).

The address(0) check is missing for the _royaltyRegistry input variable in the following constructor. Please consider checking it.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L90-L92

    constructor(address _royaltyRegistry) {
        royaltyRegistry = _royaltyRegistry;
    }

The address(0) checks are missing for the _factory, _royaltyRegistry, and _stolenNftOracle input variables in the following constructor. Please consider checking them.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L143-L147

    constructor(address _factory, address _royaltyRegistry, address _stolenNftOracle) {
        factory = payable(_factory);
        royaltyRegistry = _royaltyRegistry;
        stolenNftOracle = _stolenNftOracle;
    }

[07] CASTING factory TO address IS REDUNDANT

In the following code, factory is already address so it is redundant to cast it again to address. To make the code more efficient, please consider directly using factory instead of address(factory).

src\PrivatePool.sol
  259: if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);
  368: if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);

[08] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 10_000, used in the following code with constants.

src\PrivatePool.sol
  172: if (_feeRate > 5_000) revert FeeRateTooHigh();  
  668: return tokenIds.length * 1e18;  
  703: protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;  
  704: feeAmount = inputAmount * feeRate / 10_000;  
  721: protocolFeeAmount = outputAmount * Factory(factory).protocolFeeRate() / 10_000;  
  722: feeAmount = outputAmount * feeRate / 10_000;  
  734: uint256 feePerNft = changeFee * 10 ** exponent; 
  736: feeAmount = inputAmount * feePerNft / 1e18; 
  737: protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000; 

[09] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

src\EthRouter.sol
  2: pragma solidity ^0.8.19;    

src\Factory.sol
  2: pragma solidity ^0.8.19;    

src\PrivatePool.sol
  2: pragma solidity ^0.8.19;    

src\PrivatePoolMetadata.sol
  2: pragma solidity ^0.8.19;    

[10] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss the @param or @return comments. Please consider completing the NatSpec comments for these functions.

src\EthRouter.sol
  301: function getRoyalty(address nft, uint256 tokenId, uint256 salePrice)    

src\Factory.sol
  71: function create(

src\PrivatePool.sol
  157: function initialize(    
  211: function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof)
  301: function sell(  
  385: function change(    
  694: function buyQuote(uint256 outputAmount) 
  713: function sellQuote(uint256 inputAmount) 

src\PrivatePoolMetadata.sol
  17: function tokenURI(uint256 tokenId) public view returns (string memory) {    
  35: function attributes(uint256 tokenId) public view returns (string memory) {    
  55: function svg(uint256 tokenId) public view returns (bytes memory) {  

[11] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following function miss NatSpec comments. Please consider adding NatSpec comments for this function.

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePoolMetadata.sol#L112-L119

    function trait(string memory traitType, string memory value) internal pure returns (string memory) {
        // forgefmt: disable-next-item
        return string(
            abi.encodePacked(
                '{ "trait_type": "', traitType, '",', '"value": "', value, '" }'
            )
        );
    }