Caviar Private Pools - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A compromised/malicious owner might repeatedly call execute->sell() to sell NFTs from PrivatePool to itself and steal royalty fees and drain the contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how a compromised/malicious owner steals funds from the contract.

1. Suppose there are three NFTs A, B, C in the PrivatePool contract and the royalty recipient is the owner's accomplice (or the owner).

2. The owner can use execute() as a bridge to call sell() to sell NFTs A, B, C from the PrivatePool contract to itself. This is accomplished by encoding the calldata for calling sell() in data of the input of execute(). The calldata contains both the sell() function signature and the arguments to sell A, B, and C as well as other other arguments such as weights.

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

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

3. While both NFTs and baseTokens will only transfer PrivatePool to itself, the royalty fee will be transferred to the royalty recipient, the owner or his accomplice.

4. This procedure can be repeated to drain/steal the funds from the contract.

Tools Used

VSCode

Recommended Mitigation Steps

We need to make sure target != address(this) so that execute() can not be used to call any method in the PrivatePool contract.

 function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
+       if(target == address(this) revert targetCannotBeThisContract();

        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(data);

        // if the call succeeded return the return data
        if (success) return returnData;

        // if we got an error bubble up the error message
        if (returnData.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let returnData_size := mload(returnData)
                revert(add(32, returnData), returnData_size)
            }
        } else {
            revert();
        }
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. change() sends msg.value instead of the remaining ETH balance to _change.pool in each iteration, as a result, the function might fail due to insufficient ETH.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The change() functions allows a user to executes a series of change operations against a private pool.

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

However, in each iteration, the function attempts to send msg.value to _change.pool for the payment of fees with a possible refund of unused fund for the next iteration.

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

However, except for the first iteration, there is no sufficient msg.value amount of ETH to send to _change.pool. The above line will revert as a result. The correct way to do is to send the remaining ETH balance instead of msg.value.

As a result, the change() function will always revert when there are multiple iterations.

Tools Used

VSCode

Recommended Mitigation Steps

Send the remaining ETH balance instead of msg.value:

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

        // loop through and execute the changes
        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);

+           uint256 remaining = address(this).balance;
            // execute change
-            PrivatePool(_change.pool).change{value: msg.value}(
+            PrivatePool(_change.pool).change{value: remaining}(
                _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]);
            }
        }

        // refund any surplus ETH to the caller
        if (address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. changeFeeQuote() will break when the number of decimals for baseToken is less than 4.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

changeFeeQuote() is used to calculate the fee required to change a given amount of NFTs.

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

However, when the number of decimals for baseToken is less than 4, the following line will have an underflow and revert.

  uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;

As a result, the protocol only break for baseToken that has less than 4 decimals.

Tools Used

VSCode

Recommended Mitigation Steps

Use division instead of subtraction.

 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 exponent = baseToken == address(0) ? 18: ERC20(baseToken).decimals();
-        uint256 feePerNft = changeFee * 10 ** exponent;
+        uint256 feePerNft = changeFee * 10 ** exponent / 10**4;
        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A seller of NFTs might lose the portion of royalty fees even though royalty fees have not been paid to the recipients. The main issue is that when recipient == address(0), no royalty fee is sent to the recipient, but the fee is still deducted from the user's share netOutputAmount.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how a seller of NFTs, Bob, might receive less funds than what he should have:

1. Suppose Bob is selling NFTs A, B, C with 3eth, 1eth for each by calling sell().

2. However, all the royalty recipient for A, B, and C has a zero address. As a result no royalty fee will be sent to royalty recipient. Suppose the royalty fee for each NFT is 0.03ETH (3%).

if (royaltyFee > 0 && recipient != address(0)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }

3. However, the function still deduct the royalty fees from the seller, Bob:

 netOutputAmount -= royaltyFeeAmount;

4. As a result, Bob will receive less funds from the sale. He lost some funds in the transaction.

Tools Used

VSCode

Recommended Mitigation Steps

deduct from netOutputAmount only those royalty fees when recipient != address(0); see below

function sell(
        uint256[] calldata tokenIds,
        uint256[] calldata tokenWeights,
        MerkleMultiProof calldata proof,
        IStolenNftOracle.Message[] memory stolenNftProofs // put in memory to avoid stack too deep error
    ) public returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount) {
        // ~~~ Checks ~~~ //

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

        // calculate the net output amount and fee amount
        (netOutputAmount, feeAmount, protocolFeeAmount) = sellQuote(weightSum);

        //  check the nfts are not stolen
        if (useStolenNftOracle) {
            IStolenNftOracle(stolenNftOracle).validateTokensAreNotStolen(nft, tokenIds, stolenNftProofs);
        }

        // ~~~ Effects ~~~ //

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

        // ~~~ Interactions ~~~ //

        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // transfer each nft from the caller
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);

            if (payRoyalties) {
                // calculate the sale price (assume it's the same for each NFT even if weights differ)
                uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;

                // get the royalty fee for the NFT
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                // tally the royalty fee amount
-                royaltyFeeAmount += royaltyFee;

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
+                   royaltyFeeAmount += royaltyFee;

                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee);
                    } else {
                        recipient.safeTransferETH(royaltyFee);
                    }
                }
            }
        }

        // subtract the royalty fee amount from the net output amount
        netOutputAmount -= royaltyFeeAmount;

        if (baseToken == address(0)) {
            // transfer ETH to the caller
            msg.sender.safeTransferETH(netOutputAmount);

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);
        } else {
            // transfer base tokens to the caller
            ERC20(baseToken).transfer(msg.sender, netOutputAmount);

            // if the protocol fee is set then pay the protocol fee
            if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);
        }

        // emit the sell event
        emit Sell(tokenIds, tokenWeights, netOutputAmount, feeAmount, protocolFeeAmount, royaltyFeeAmount);
    }

Judge has assessed an item in Issue #215 as 2 risk. The relevant finding follows:

QA10 Both EthRouter#buy() and EthRouter#sell() do not check whether recipient == address(0), as a result, they might send royalty fees to the zero address - loss of funds.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. flashloan() will not work when changeFee == 0 for base tokens that revert on zero value transfer.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

flashloan() allows one to flashloan an NFT owned by a pool by paying the flashloan fee, changeFee.

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

However, the following line will revert when changeFee == 0 for base tokens that revert on zero value transfer (such as LEND, https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers).

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

This is because when changeFee == 0, fee == 0, and this line will revert since we are transferring zero amount of fee.

Tools Used

VSCode

Recommended Mitigation Steps

Make sure the amount to transfer is not zero:

 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);
+        if (baseToken != address(0) && fee != 0) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }   

QA1. Zero address check is recommended for the input:

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

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

QA2.A range check is recommended for the input: https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L141-L143

QA3. One should check whether tokenIds.length == tokenWeights.length.

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

QA4. Maybe bytes.concat is not needed here and the calculation can be simplified:

- leafs[i] = keccak256(bytes.concat(keccak256(abi.encode(tokenIds[i], tokenWeights[i]))));
+ leafs[i] = keccak256(abi.encode(tokenIds[i], tokenWeights[i]));

QA5. There is an error in the NatSpec of change():

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

Mitigation: the correction should be

- The sum of the caller's NFT weights must be less than or equal to the sum of the
    /// output pool NFTs weights.

+ The sum of the caller's NFT weights must be greater than or equal to the sum of the
    /// output pool NFTs weights.

QA6. The buy() function fails to verify that a private pool only supports ETH as base tokens. Similarly, the deposit() function does not check this either.

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

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

Mitigation: check whether the private pool only supports ETH as base tokens:

function buy(Buy[] calldata buys, uint256 deadline, bool payRoyalties) public payable {
        // check that the deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

        // loop through and execute the the buys
        for (uint256 i = 0; i < buys.length; i++) {
            if (buys[i].isPublicPool) {
                // execute the buy against a public pool
                uint256 inputAmount = Pair(buys[i].pool).nftBuy{value: buys[i].baseTokenAmount}(
                    buys[i].tokenIds, buys[i].baseTokenAmount, 0
                );

                // pay the royalties if buyer has opted-in
                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 {
                // execute the buy against a private pool
+               if(buys[i].pool.baseToken != address(0)) revert BaseTokenIsNotETH();
                PrivatePool(buys[i].pool).buy{value: buys[i].baseTokenAmount}(
                    buys[i].tokenIds, buys[i].tokenWeights, buys[i].proof
                );
            }

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

        // refund any surplus ETH to the caller
        if (address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }
    }

QA7. The execute() fails to send remaining ETH back to the user when there is any.

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

Mitigation: send any remaining ETH balance to the user

function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(data);

+        uint256 remaining = address(this).balance; 
+        msg.sender.safeTransferETH(remaining);

        // if the call succeeded return the return data
        if (success) return returnData;

        // if we got an error bubble up the error message
        if (returnData.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let returnData_size := mload(returnData)
                revert(add(32, returnData), returnData_size)
            }
        } else {
            revert();
        }
        
    }

QA8. The buy() function fails to check if an NFT is stolen or not. This is important since when an NFT is sold, people might not have known it is stolen, and then after that, it is reported that an NFT A is stolen, which has been sold to the contract. It is important to not to allow people to buy stolen NFT as well.

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

Mitigation: check if an NFT is stolen as well during buying.

QA9. The EthRouter#buy() fails to ensure that msg.value is no less than all the buys[i].baseTokenAmount added together.

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

Mitigation: We need to check that msg.value is no less than all the buys[i].baseTokenAmount added together.

function buy(Buy[] calldata buys, uint256 deadline, bool payRoyalties) public payable {

        // check that the deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

+        uint num = buys.length;
+        for(uint256 i; i < num; i++)
+             inputAmount += buys[i].baseTokenAmount;
+        if(msg.value < inputAmount) revert NotSufficientETHProvided();
 
         
        // loop through and execute the the buys
        for (uint256 i = 0; i < buys.length; i++) {
            if (buys[i].isPublicPool) {
                // execute the buy against a public pool
                uint256 inputAmount = Pair(buys[i].pool).nftBuy{value: buys[i].baseTokenAmount}(
                    buys[i].tokenIds, buys[i].baseTokenAmount, 0
                );

                // pay the royalties if buyer has opted-in
                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 {
                // execute the buy against a private pool
                PrivatePool(buys[i].pool).buy{value: buys[i].baseTokenAmount}(
                    buys[i].tokenIds, buys[i].tokenWeights, buys[i].proof
                );
            }

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

        // refund any surplus ETH to the caller
        if (address(this).balance > 0) {
            msg.sender.safeTransferETH(address(this).balance);
        }
    }

QA10 Both EthRouter#buy() and EthRouter#sell() do not check whether recipient == address(0), as a result, they might send royalty fees to the zero address - loss of funds.

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

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

Mitigation: check and make sure recipient != address(0).

QA11. The PrivatePool#deposit() fails to keep track who the depositor is, not in the emitted event either.

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

Impact: there is no way a depositor can withdraw the deposited NFTs and ETH on his own.

Mitigation: keep track of all deposit information (NFTs and ETH) for each depositor and add a function to allow each depositor to withdraw his own NFTs and ETH, possibly with additional rewards. Minimum: change the event topics so that event will reflect the information of who deposits how much/which NFT at when.

QA12: The PrivatePool#buy() and PrivatePool#sell() functions lack slippage control for royalty fees. This is important since the owner might change the royalty payment mode by calling the setPayRoyalties() function.

Without this control: a user might expect not to pay the royalty but end up having to pay the royalty due to the change of the royalty payment mode by the owner's calling of the setPayRoyalties() function.

Mitigation: add one more argument called expectedPayRoyalties to the buy() and sell() and check whether it is equal to payRoyalties before proceeding with the rest of the flow.

QA13: The flashloan() does not check whether token is whitelisted or not (for example, whether it is equal to nft). As a result, a malicious user might create a malicious NFT contract, and (optionally but does not have to) send all the malicious NFTs to the PrivatePool contract and provides hooks for vulnerabilities:

1. The safeTransferFrom() can be malicious;
2. The ownerOf() can be malicious, as a result, the call of !availableForFlashLoan(token, tokenId) can be bypassed easily.
3. receiver.onFlashLoan() can be malicious.

Allowing so many hooks for malicious code in flashloan() is risky. For example, they can all be used for reentrancy attacks.

Mitigation: make sure that token == nft so that one can only flashloan NFTs from the nft contract.