Caviar Private Pools - ElKu's results

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

NFT tokens sent to the EthRouter contract by mistake can be drained by pool contracts. When someone calls sell, deposit or change functions on EthRouter contract, the contract gives the particular pool full approval (with setApprovalForAll) for tokens from that particular nft. This approval is not revoked later. If someone sends a token from the approved nft, the pool owner can use the execute function to withdraw this token from EthRouter to his own pool.NFT tokens sent to the EthRouter contract by mistake can be drained by pool contracts. When someone calls sell, deposit or change functions on EthRouter contract, the contract gives the particular pool full approval (with setApprovalForAll) for tokens from that particular nft. This approval is not revoked later. If someone sends a token from the approved nft, the pool owner can use the execute function to withdraw this token from EthRouter to his own pool.

Lines of code

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

Vulnerability details

Impact

Fee for the flashLoan operation is set as changeFee directly which could be in most cases only few thousand's. So the pool receives only few thousand wei as fee for every flashLoan transaction. changeFee is also immutable which makes the whole situation even worse.

Proof of Concept

flashFee function is defined as follows:

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

which is called in the flashLoan function:

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

So the fee is same as changeFee. This is set when the pool is created by the factory. And the comment says:

The change/flash fee to 4 decimals of precision. For example, 0.0025 ETH = 25. 500 USDC = 5_000_000.

And in the changeFeeQuote function we can see that the variable changeFee is multiplied by 10 ** exponent.

But in the flashLoan function, the fee is set as changeFee directly and send to the pool. which is very very less compared to what is supposed to be actual fee. This causes loss to pool owner.

//fee here is changeFee.
if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee); 

Tools Used

Manual Analysis

Recommended Mitigation Steps

Do the necessary multiplication on changeFee before setting it to the fee.

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

Lines of code

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

Vulnerability details

Impact

If the base token of the pool has less than 4 decimals, then this line will revert in changeFeeQuote function. Which means the change function which uses changeFeeQuote will revert as well.

Proof of Concept

Looking at the changeFeeQuote function:

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

The statement ERC20(baseToken).decimals() - 4; will revert if the base token has decimals less than 4.

Tokens like Gemini USD only have 2 decimals.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Either change the precision of changeFee or block base tokens which has less than 4 decimals.

Lines of code

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

Vulnerability details

Impact

Each NFT token might have a different royalty fee percentage associated with it. Which means the royalty to be paid depends on the exact sale price. But in the buy and sell functions, the total sale price is divided by the number of bought or sold tokens and this average price is used to calculate royaltyFee.

Proof of Concept

In buy function:

uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;

In sell function:

uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;

In both functions, royalty fee is calculated as follows, with the average price:

(uint256 royaltyFee,) = _getRoyalty(tokenIds[i], salePrice);

And we send this calculated fee to recipients:

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

Note that, this mistake causes unfairness, only to individual recipients and the total calculated fee doesnt get affected.

Tools Used

Manual analysis

Recommended Mitigation Steps

Calculate the royaltyFee based on weights rather than the average price. For example the netInputAmount could be divided into fees based on the weight of each token.

Low Severity Findings

1. The doc says:

Initially the protocol fee rate will be set to be 0% however it may be increased in the future, with advanced notice.

But there is no timelock function implemented in the smart contracts to make sure that the promise of advanced notice will kept. 2. Though most of the pool parameters can be changed later, changeFee remains immutable. This option could be nice for the pool owners.

3. The royalties, if enabled, requires the NFT to be listed in manifold.xyz. If its not listed there, the royalty program will fail.
4. setVirtualReserves function doesnt check if the new values are zero or not. If virtualBaseTokenReserves is zero then, buyQuote and sellQuote will return zero. If virtualNftReserves is zero then, buyQuote will revert.
5. The extra msg.value sent when calling the flashLoan function doesnt get returned to the user. Though this refund is done when the buy and change functions are called.
6. Pool owner can change the merkle root after increasing the weights of existing tokens in the pool, so that buyers need to pay much higher than what sellers got it when they sold it.
7. The stolen NFT tokens are not not allowed to be sold into the pool. But there is no such restriction to use stolen tokens with flashLoan function. This seems counterintuitive.
8. flashLoan usage of a token only pays pool fee which goes to the pool owner. But royaltyFees are not paid. This seems not right.
9. NFT tokens sent to the EthRouter contract by mistake can be drained by pool contracts. When someone calls sell, deposit or change functions on EthRouter contract, the contract gives the particular pool full approval (with setApprovalForAll) for tokens from that particular nft. This approval is not revoked later. If someone sends a token from the approved nft, the pool owner can use the execute function to withdraw this token from EthRouter to his own pool.