Caviar Private Pools - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Any NFT or ERC20 token approved for the pool can be stolen by the owner of the pool. The pool owner is just a user. They are not part of the Caviar team and thus not trusted.

Proof of Concept

Every PrivatePool has a execute() function allowing the pool owner to execute arbitrary calls through the pool:

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

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

To buy & sell NFTs users have to approve the pool to access their NFTs and tokens. Most of the time, dApp developers approve the maximum amount of ERC20 tokens or all NFT tokens for any given address to save gas. Thus, we can assume that multiple users will have the pool approved to spend funds/NFTs at any given time. Through the execute() function the owner is able to steal all of it.

They simply call the baseToken or nft contract's transferFrom() function to send the approved tokens to their own address.

Tools Used

none

Recommended Mitigation Steps

Prohibit the pool from calling the underlying NFT and base token contract:

    function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
      require(target != nft && target != baseToken);
      // ...
    }

Lines of code

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

Vulnerability details

Impact

EthRouter can't exchange NFTs using multiple pools in a single tx.

Proof of Concept

When calling PrivatePool.change() the EthRouter passes msg.value:

            PrivatePool(_change.pool).change{value: msg.value}(
                _change.inputTokenIds,
                _change.inputTokenWeights,
                _change.inputProof,
                _change.stolenNftProofs,
                _change.outputTokenIds,
                _change.outputTokenWeights,
                _change.outputProof
            );

The pool will send back any surplus ETH, but the next iteration of the loop will try to send msg.value again. At that point, the router's balance will be less than msg.value. That will cause the tx to revert.

Tools Used

none

Recommended Mitigation Steps

Update struct Change to allow the caller to pass the ETH amount that will be sent to the pool. In EthRouter.change() send that to the pool instead of msg.value.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L274 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L236 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L338 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L335 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L778

Vulnerability details

Impact

The pool uses the wrong sale price to determine the NFT's royalty. Thus, NFT creators don't earn the correct amount of royalty when their token is sold through Caviar's private pools.

Since that can cause a loss of funds for the royalty recipient I rate the issue as HIGH.

Proof of Concept

The price of an NFT in a private pool depends on

• the pool's virtual reserves: virtualBaseTokenReserves & virtualNftReserves
• the weights of the NFTs that are bought/sold

When multiple tokens are bought from the pool, the buy() and sell() functions aggregate the total price and then divide it by the number of tokens to get the individual sale price. That is then used to get the royalty info for each token:

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

        // ...

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

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

But, that doesn't represent the actual sale price of each individual token. That depends on each token's weight. Given that:

• virtualBaseTokenReserves = 2e18 (2 ETH)
• virtualNftReserves = 4e18 (weights: 2x 1e18 & 1x 2e18) If Alice buys two NFTs (1e18 & 2e18) she will pay: 3e18 * 2e18 / 1e18 = 6e18. With the current implementation, 3e18 would be the sale price for both tokens. Although the one with weight 2e18 is worth twice as much. Instead, it should be totalSalePrice * tokenWeight / totalWeight: 6e18 * 2e18 / 3e18 = 4e18 and 6e18 * 1e18 / 3e18 = 2e18.

If the NFT contract distributes royalties to different addresses depending on the token ID, e.g. first 100 are super rare and go to address X while the rest of it goes to address Y, they won't receive the correct amounts. That possibility is even stated in EIP 2981:

The implementer MAY choose to change the percentage value based on other predictable variables that do not make assumptions about the unit of exchange. For example, the percentage value may drop linearly over time. An approach like this SHOULD NOT be based on variables that are unpredictable like block.timestamp, but instead on other more predictable state changes. One more reasonable approach MAY use the number of transfers of an NFT to decide which percentage value is used to calculate the royaltyAmount. The idea being that the percentage value could decrease after each transfer of the NFT. Another example could be using a different percentage value for each unique _tokenId.

Tools Used

none

Recommended Mitigation Steps

Determine the sale price according to their weight as described above.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L281 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L141 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L203

Vulnerability details

Impact

When a user buys or sells NFTs using the EthRouter, the recipient of the NFT royalties is able to reenter the EthRouter contract to use any of the remaining ETH. The damage is limited to any surplus ETH sent to EthRouter.buy() and any surplus ETH greater than minOutputAmount for EthRouter.sell().

Because of the limited amount of funds you can extract and the external dependency of having a malicious NFT royalty recipient, I rate the issue as MED.

Proof of Concept

When a user calls EthRouter.buy() they are allowed to send surplus ETH to the router to prevent the tx from failing if the price changes between simulation and execution. The surplus amount is refunded at the end of it.

    function buy(Buy[] calldata buys, uint256 deadline, bool payRoyalties) public payable {
        
        // loop through and execute the the buys
        // ...

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

Because the router has no reentrancy lock, anybody is able to reenter the contract. When buying an NFT from a pool the following entities are part of the transaction:

• pool contract (can't reenter)
• caviar factory contract (can't reenter)
• royalty recipient

If the royalty recipient is a malicious contract, they can reenter the EthRouter and use the remaining ETH in the contract to buy NFTs from other pools. The amount sent to Caviar pools doesn't depend on the amount of ETH sent to the router with the current call. There are no checks preventing you from using the router's current balance: https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L109

The same thing applies to EthRouter.sell() although you can only steal up to router balance - minOutAmount because of the following check: https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L203

Tools Used

none

Recommended Mitigation Steps

Add a reentrancy lock to the Router.

Lines of code

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

Vulnerability details

Impact

Instead of taking the fee from the receiver of the flashloan callback, it pulls it from msg.sender.

As specified in EIP-3156:

After the callback, the flashLoan function MUST take the amount + fee token from the receiver, or revert if this is not successful.

This will be an unexpected loss of funds for the caller if they have the pool pre-approved to spend funds (e.g. they previously bought NFTs) and are not the owner of the flashloan contract they use for the callback.

Additionally, for ETH pools, it expects the caller to pay the fee upfront. But, the fee is generally paid with the profits made using the flashloaned tokens.

Proof of Concept

If baseToken is ETH, it expects the fee to already be sent with the call to flashLoan(). If it's an ERC20 token, it will pull it from msg.sender instead of receiver:

    function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        // ...

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

        return success;
    }

Tools Used

none

Recommended Mitigation Steps

Change to:

        uint initialBalance = address(this).balance;
        // ... 

        if (baseToken != address(0)) ERC20(baseToken).transferFrom(receiver, address(this), fee);
        else require(address(this).balance - initialBalance == fee);