Caviar Private Pools - bin2chen's results

Lines of code

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

Vulnerability details

Impact

EthRouter.change will revert, when changes.length > 1

Proof of Concept

EthRouter#change() can pass in multiple changes[] and loop execute pool.change() The code is as follows:

    function change(Change[] calldata changes, uint256 deadline) public payable {
        // check deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

..
        for (uint256 i = 0; i < changes.length; i++) {
            Change memory _change = changes[i];

            // transfer NFTs from caller
            for (uint256 j = 0; j < changes[i].inputTokenIds.length; j++) {
                ERC721(_change.nft).safeTransferFrom(msg.sender, address(this), _change.inputTokenIds[j]);
            }

            // approve pair to transfer NFTs from router
            ERC721(_change.nft).setApprovalForAll(_change.pool, true);

            // execute change
            PrivatePool(_change.pool).change{value: msg.value}( //<--------@audit  Wrong use 'msg.value'
                _change.inputTokenIds,
                _change.inputTokenWeights,
                _change.inputProof,
                _change.stolenNftProofs,
                _change.outputTokenIds,
                _change.outputTokenWeights,
                _change.outputProof
            );

            // transfer NFTs to caller
            for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
                ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
            }
        }     
...

The current implementation calls pool.change() in the loop, and the value passed in incorrectly uses msg.value. If there are multiple changes[], the second will fail because the contract balance will be insufficient, the first one consume feeAmount + protocolFeeAmount and the contract balance will be left msg.value - feeAmount -protocolFeeAmount

Here we should pass in address(this).balance

Tools Used

Recommended Mitigation Steps

    function change(Change[] calldata changes, uint256 deadline) public payable {
        // check deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

..

            ERC721(_change.nft).setApprovalForAll(_change.pool, true);

            // execute change
-           PrivatePool(_change.pool).change{value: msg.value}(
+           PrivatePool(_change.pool).change{value: address(this).balance}(
                _change.inputTokenIds,
                _change.inputTokenWeights,
                _change.inputProof,
                _change.stolenNftProofs,
                _change.outputTokenIds,
                _change.outputTokenWeights,
                _change.outputProof
            );

            // transfer NFTs to caller
            for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
                ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
            }
        }     
...

Lines of code

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

Vulnerability details

Impact

flashFee() missing multiplied by some exponent depending on the base token decimals. When execute flashLoan() fee will undercharged

Proof of Concept

When the changeFeeQuote() is calculated, it is performed multiplied by some exponent depending on the base token decimals.

    /// @notice Returns the fee required to change a given amount of NFTs. The fee is based on the current changeFee
    /// (which contains 4 decimals of precision) multiplied by some exponent depending on the base token decimals.
    /// @param inputAmount The amount of NFTs to change multiplied by 1e18.
    /// @return feeAmount The fee amount.
    /// @return protocolFeeAmount The protocol fee amount.
    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;
    }

but in flashFee() only return changeFee

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

missing multiplied by some exponent depending on the base token decimals.

Tools Used

Recommended Mitigation Steps

    function flashFee(address, uint256) public view returns (uint256) {
-       return changeFee;
+       uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
+       return changeFee * 10 ** exponent;
    }

L-01:EthRouter#buy/sell does not determine whether royaltyRecipient is address(0) If getRoyalty() returns royaltyFee>0, but royaltyRecipient==address(0) this will transfer the funds to address(0) Suggest adding judgment:

    function buy(Buy[] calldata buys, uint256 deadline, bool payRoyalties) public payable {
...
                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) {
+                       if (royaltyFee > 0 && royaltyRecipient!=address(0)) {
                            // transfer the royalty fee to the royalty recipient
                            royaltyRecipient.safeTransferETH(royaltyFee);
                        }
                    }
                }    

L-02:setProtocolFeeRate() does not limit the maximum value, there is a risk of malicious amplification of the fee It is recommended that this should not exceed, for example, 5%

contract Factory is ERC721, Owned {
...
    function setProtocolFeeRate(uint16 _protocolFeeRate) public onlyOwner {
+       if (_protocolFeeRate > 5_000) revert FeeRateTooHigh();  
        protocolFeeRate = _protocolFeeRate;
    }

Lines of code

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

Vulnerability details

Impact

Factory#create() Using the passed parameter salt to create contract may risk DOS attacks and replay attacks.

Proof of Concept

Anyone can call Factory#create() to create a pool The code is as follows:

    function create(
        address _baseToken,
        address _nft,
        uint128 _virtualBaseTokenReserves,
        uint128 _virtualNftReserves,
        uint56 _changeFee,
        uint16 _feeRate,
        bytes32 _merkleRoot,
        bool _useStolenNftOracle,
        bool _payRoyalties,
        bytes32 _salt,
        uint256[] memory tokenIds, // put in memory to avoid stack too deep error
        uint256 baseTokenAmount
    ) public payable returns (PrivatePool privatePool) {
...

        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
...

This function will call privatePoolImplementation.cloneDeterministic(_salt) ,and finally use CREATE2 to create the contract

An issue could be that an attacker can frontrun the creation TX with their own creation request, with the same salt. This would create the exact address created by the CREATE2 call, When the victim's transaction would be executed, the address is non-empty so the EVM would reject its creation. This would result in a bad UX for a user, who thinks the creation did not succeed. The result contract would still be usable, but would be hard to track as it was created in another TX. In more extreme cases users may deposit funds into this address

Another possible risk: cross-chain replay attack, malicious users in other chains can generate the same contract address, but the owner is not the same (the owner becomes the malicious user), and there is a risk of replay deposits.

So it is recommended to combine salt with msg.sender

Tools Used

Recommended Mitigation Steps

    function create(
        address _baseToken,
        address _nft,
        uint128 _virtualBaseTokenReserves,
        uint128 _virtualNftReserves,
        uint56 _changeFee,
        uint16 _feeRate,
        bytes32 _merkleRoot,
        bool _useStolenNftOracle,
        bool _payRoyalties,
        bytes32 _salt,
        uint256[] memory tokenIds, // put in memory to avoid stack too deep error
        uint256 baseTokenAmount
    ) public payable returns (PrivatePool privatePool) {
...

-        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));    
+        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(keccak256(abi.encode(msg.sender,_salt)))));

-   function predictPoolDeploymentAddress(bytes32 salt) public view returns (address predictedAddress) {
+   function predictPoolDeploymentAddress(address owner,bytes32 salt) public view returns (address predictedAddress) {    
        predictedAddress = privatePoolImplementation.predictDeterministicAddress(salt, address(this));
+       predictedAddress = privatePoolImplementation.predictDeterministicAddress(keccak256(abi.encode(owner,salt)), address(this));
    }