Caviar Private Pools - AkshaySrivastav's results

Lines of code

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

Vulnerability details

Impact

The PrivatePool.buy function intends to distribute royalty amount whenever NFTs are bought. Its implementation looks like this:

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

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // transfer the NFT to the caller
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

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

                // add the royalty fee to the total royalty fee amount
                royaltyFeeAmount += royaltyFee;
            }
        }

        // add the royalty fee amount to the net input aount
        netInputAmount += royaltyFeeAmount;

        // ...

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

        // emit the buy event
        emit Buy(tokenIds, tokenWeights, netInputAmount, feeAmount, protocolFeeAmount, royaltyFeeAmount);
    }
    
    function _getRoyalty(uint256 tokenId, uint256 salePrice)
        internal
        view
        returns (uint256 royaltyFee, address recipient)
    {
        // get the royalty lookup address
        address lookupAddress = IRoyaltyRegistry(royaltyRegistry).getRoyaltyLookupAddress(nft);

        if (IERC2981(lookupAddress).supportsInterface(type(IERC2981).interfaceId)) {
            // get the royalty fee from the registry
            (recipient, royaltyFee) = IERC2981(lookupAddress).royaltyInfo(tokenId, salePrice);

            // revert if the royalty fee is greater than the sale price
            if (royaltyFee > salePrice) revert InvalidRoyaltyFee();
        }
    }

It should be noted that the collection of royalty amount from buyer (L237 - L269) and distribution of royalty amounts to royalty recipient (L271 - L285) happens separately. Also while distributing the royalty amount to recipient the royalty amount value is queried again.

This implementation can be misused to steal baseToken funds from the pool. In the case where the royalty amount returned in the second _getRoyalty call is greater than the amount return in the first _getRoyalty call, the sufficient royalty amount won't be collected from the buyer so the extra royalty amount needed will be sent from the PrivatePool's baseToken balance.

In the case where the royalty recipient has influence over the royaltyRegistry, lookupAddress or the royaltyInfo call, he can manipulate the result of second _getRoyalty call. If the result of second query is greater than the first one then the pool's own baseToken balance will transferred to the royalty recipient.

This bug will cause loss of funds to all the depositors of the pool as the originally deposited baseTokens and the pool's swap fee reserves can be stolen using this vulnerability.

It should be noted that Caviar protocol intends to support open creation of private pools with any configuration possible as desired by the user. Hence the likelyhood of this exploit is high.

Proof of Concept

Consider this scenario:

• Pool was created and 100 WETH were deposited by owner and other depositors.
• After some NFT swaps the pool accrued additional 10 WETH as the swap fee.
• A malicious user or someone with an influence over the royaltyRegistry, lookupAddress or the royaltyInfo call invokes the buy function.
• The PrivatePool.buy function queries and calculates the royalty amount, the royaltyFeeAmount comes out to be 10 WETH.
• These 10 WETH are collected from the buyer.
• The buy function makes the second call to the _getRoyalty function before the royalty amount transfer. But this time the amount returned comes out to be 20 WETH.
• The contract now proceeds with the royalty payment of 20 WETH out of which only 10 WETH were collected from the buyer and rest of 10 WETH are from pool's own 110 WETH balance.
• In this case the pool suffers a loss of 10 WETH.
• The attack cycle can be repeated again to steal more funds.

Note that the funds required to initiate the buy call and its recovery was omitted to make the scenario easier to understand. Any funds needed for the buy call can be recovered by performing further swaps.

Tools Used

Manual review

Recommended Mitigation Steps

Cosnider performing the royalty amount calculation, amount transfer from buyer to pool and amount transfer from pool to royalty recipient in a single for loop, just like how it is done in the sell function.

    function sell(
        ...
    ) public returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount) {
        // ...

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

        // ...
    }

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L236-L249 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#L335-L341

Vulnerability details

Impact

The buy and sell functions of PrivatePool intends to pay royalty amounts to royalty recipients on every token swap. The calculation of royalty amount is done as follows:

    function buy(...)
        public
        payable
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // ...

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
            if (payRoyalties) {
                (uint256 royaltyFee,) = _getRoyalty(tokenIds[i], salePrice);
                royaltyFeeAmount += royaltyFee;
            }
        }
        
        // ...
        
        if (payRoyalties) {
            for (uint256 i = 0; i < tokenIds.length; i++) {
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

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

It can be seen that the a common salePrice value is taken for all NFTs. The comment also mentions that

calculate the sale price (assume it's the same for each NFT even if weights differ)

This assumption is wrong and results in incorrect royalty amounts sent to royalty recipient. It should never be assumed that the royalty percentage of all NFTs will be equal. The profit/loss of royalty recipient is counter balanced with the funds of the trader.

A detailed example is provided in the PoC section.

The bug results in direct loss of funds to the users interacting with the buy and sell functions, or the royalty recipient. As the calculations are similar in both buy and sell functions, both of them contain this bug.

Proof of Concept

Consider the scenario in which two NFts (NFT1 and NFT2) are present in the pool. Assumptions:

• The weights ratio of the two NFTs is 5:1
• For simplicity assume the worth of NFT1 is 10 ETH and NFT2 is 2 ETH
• Royalty fee of NFT1 is 10% and NFT2 is 20%.
• All other fee are assumed to be 0

Now suppose a buy transaction for these two NFTs happens in the pool.

In the ideal case, the royalty amounts for each NFT should be:

• NFT1 - 10% of 10 ETH = 1 ETH
• NFT2 - 20% of 2 ETH = 0.4 ETH So a total of 1.4 ETH gets transferred to the royalty recipient.

But this is what happens with the current implementation of PrivatePool.

• The salePrice is calculated as: (10 ETH + 2 ETH) / 2 = 6 ETH
• The royalty amounts comes out to be:
• NFT1 - 10% of 6 ETH = 0.6 ETH
• NFT2 - 20% of 6 ETH = 1.2 ETH So a total of 1.8 ETH gets transferred to the royalty recipient (0.4 ETH more than the ideal amount).

This royalty amount including the incorrect & extra 0.4 ETH is collected from the buyer of the NFTs. Hence this results in loss of funds to the buyer. This lost amount gets transferred to the royalty recipient.

If the royalty percentages of the two NFTs are switched, then that will result in profit to trader and loss to royalty recipient.

Tools Used

Manual review

Recommended Mitigation Steps

Consider calculating the royalty amounts with respect to the individual price of each NFTs like _getRoyalty(tokenIds[i], salePrice of tokenIds[i]).

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L237-L281 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L328-L355

Vulnerability details

Impact

The PrivatePool.buy and PrivatePool.sell functions intend to distribute royalty amount whenever NFTs are traded. The implementation of buy and sell looks like this:

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

        // calculate the sale price (assume it's the same for each NFT even if weights differ)
        uint256 salePrice = (netInputAmount - feeAmount - protocolFeeAmount) / tokenIds.length;
        uint256 royaltyFeeAmount = 0;
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // transfer the NFT to the caller
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

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

                // add the royalty fee to the total royalty fee amount
                royaltyFeeAmount += royaltyFee;
            }
        }

        // add the royalty fee amount to the net input aount
        netInputAmount += royaltyFeeAmount;

        // ...

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

        // emit the buy event
        emit Buy(tokenIds, tokenWeights, netInputAmount, feeAmount, protocolFeeAmount, royaltyFeeAmount);
    }

    function sell(
        ...
    ) public returns (...) {
        // ...

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

        // ...
    }

It should be noted that while calculating royaltyFeeAmount the the recipient address returned from _getRoyalty function is ignored and the returned royaltyFee is added to the royaltyFeeAmount. This cumulative royalty amount is then collected from the trader.

However while performing the actual royalty transfer to the royalty recipient the returned recipient address is validated to not be equal to 0. The royalty is only paid when the recipient address is non-zero.

This inconsistency between royalty collection and royalty distribution can cause loss of funds to the traders. In the cases when royaltyFee is non-zero but recipient address is zero, the fee will be collected from traders but won't be distributed to royalty recipient. Hence causing loss of funds to the traders.

As the creation of private pools is open to everyone, the likelyhood of this vulnerability is high.

Proof of Concept

Consider this scenario:

• A buyer initiates the buy call for an NFT.
• The PrivatePool.buy function queries the _getRoyalty function which returns 10 WETH as the royaltyFee and 0x00 address as the royalty recipient.
• This 10 WETH value will be added to the royaltyFeeAmount amount and will be collected from the buyer.
• But since the recipient address is 0x00, the 10 WETH royalty amount will not be distributed.
• The 10 WETH amount won't be returned to the buyer either. It just simply stays inside the pool contract.
• The buyer here suffered loss of 10 WETH.

A similar scenario is possible for the NFT sell flow.

Tools Used

Manual review

Recommended Mitigation Steps

Consider collecting royalty amount from traders only when the royalty recipient is non-zero.

    if (royaltyFee > 0 && recipient != address(0)) {
        royaltyFeeAmount += royaltyFee;
    }

Lines of code

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

Vulnerability details

Impact

The Factory.create function is responsible for creating new PrivatePools. It does this using the LibClone.cloneDeterministic function.

    function create(
        ...
        bytes32 _salt,
        ...
    ) public payable returns (PrivatePool privatePool) {
        if ((_baseToken == address(0) && msg.value != baseTokenAmount) || (_baseToken != address(0) && msg.value > 0)) {
            revert PrivatePool.InvalidEthAmount();
        }

        // deploy a minimal proxy clone of the private pool implementation
        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

        // ...
    }

The address of the new PrivatePool depends solely upon the _salt parameter provided by the user. Once the user's create transaction is broadcasted, the _salt parameter can be viewed by anyone watching the public mempool.

This public readability of _salt parameter creates two issues:

1. Stealing of user's deposit amount. If a user intends to create new pool and deposit some funds in it then an attacker can frontrun the user's txns and capture the deposit amounts. Here is how this can happen:

   • User broadcasts two txns, first one to create a pool with XXX as the salt and second one to deposit some ETH into the new pool.
   • The attacker views these pending txns and frontruns them to create a PrivatePool for himself with same XXX salt.
   • The new pool gets created for the attacker, the address of this pool will be same as what the user will be expecting for his pool.
   • The user's create pool txn gets reverted but deposit txn gets executed successfully. Hence the user deposited ETH in attacker's pool.
   • Being the owner of the pool the attacker simply withdraws the deposited ETH from the PrivatePool.

2. DoS for Factory.create. If a user intends to create a PrivatePool, his create txn can be forcefully reverted by an attacker by deploying a pool for himself using the user's salt. Here is how this can happen:

   • The user broadcasts the create pool txn with salt XXX.
   • The attacker frontruns the user's txn and creates a pool for hiself using the same XXX salt.
   • The user's original create txn gets reverted as attacker's pool already exist on the predetermined address.
   • This attack can be repeated again and again resulting in DoS for the Factory.create function.

Proof of Concept

These test cases were added to test/PrivatePool/Withdraw.t.sol file and were ran using forge test --ffi --mp test/PrivatePool/Withdraw.t.sol --mt test_audit

    function test_audit_create_stealDeposit() public {
        address user1 = makeAddr("user1");
        vm.deal(user1, 10 ether);
        vm.startPrank(user1);

        address predictedAddress = factory.predictPoolDeploymentAddress(bytes32(0));

        // tries to create pool and deposit funds
        // 1. factory.create(...)
        // 2. pool.deposit(...)

        // but user2 frontruns the txns

        address user2 = makeAddr("user2");
        changePrank(user2);

        uint baseTokenAmount = 0;

        PrivatePool pool = factory.create{value: baseTokenAmount}(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            bytes32(0),
            tokenIds,
            baseTokenAmount
        );
        assertEq(predictedAddress, address(pool));
        assertEq(factory.ownerOf(uint256(uint160(address(pool)))), address(user2));

        changePrank(user1);

        vm.expectRevert(LibClone.DeploymentFailed.selector);
        factory.create{value: baseTokenAmount}(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            bytes32(0),
            tokenIds,
            baseTokenAmount
        );

        pool.deposit{ value: 10 ether }(tokenIds, 10 ether);
        assertEq(address(pool).balance, 10 ether);

        changePrank(user2);
        pool.withdraw(address(0), tokenIds, address(0), 10 ether);
        assertEq(address(pool).balance, 0);
        assertEq(user2.balance, 10 ether);
    }

    function test_audit_create_DoS() public {
        address user1 = makeAddr("user1");
        vm.deal(user1, 10 ether);
        vm.startPrank(user1);

        address predictedAddress = factory.predictPoolDeploymentAddress(bytes32(0));

        // user1 tries to create pool
        // factory.create(...)

        // but user2 frontruns the txn

        address user2 = makeAddr("user2");
        changePrank(user2);

        uint baseTokenAmount = 0;

        PrivatePool pool = factory.create{value: baseTokenAmount}(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            bytes32(0),
            tokenIds,
            baseTokenAmount
        );
        assertEq(predictedAddress, address(pool));
        assertEq(factory.ownerOf(uint256(uint160(address(pool)))), address(user2));

        changePrank(user1);

        vm.expectRevert(LibClone.DeploymentFailed.selector);
        factory.create{value: baseTokenAmount}(
            baseToken,
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            bytes32(0),
            tokenIds,
            baseTokenAmount
        );
    }

Tools Used

Foundry

Recommended Mitigation Steps

Consider making the upcoming pool address user specific by combining the salt value with user's address.

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

1. The Factory.create function is susceptible to re-entrancy as it performs a _safeMint before initializing the pool.

   function create(
       ...
   ) public payable returns (PrivatePool privatePool) {
       // ...
       privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
   
       // mint the nft to the caller
       _safeMint(msg.sender, uint256(uint160(address(privatePool))));
   
       // initialize the pool
       privatePool.initialize(...);
       // ...
   }

2. The Factory.create function performs plain transfer of funds instead of calling the deposit function. This way the Deposit event is not emitted.

   function create(
       ...
   ) public payable returns (PrivatePool privatePool) {
       // ...
       privatePool.initialize(...);
       
       if (_baseToken == address(0)) {
           // transfer eth into the pool if base token is ETH
           address(privatePool).safeTransferETH(baseTokenAmount);
       } else {
           // deposit the base tokens from the caller into the pool
           ERC20(_baseToken).transferFrom(msg.sender, address(privatePool), baseTokenAmount);
       }
   
       // deposit the nfts from the caller into the pool
       for (uint256 i = 0; i < tokenIds.length; i++) {
           ERC721(_nft).safeTransferFrom(msg.sender, address(privatePool), tokenIds[i]);
       }
   
       // emit create event
       emit Create(address(privatePool), tokenIds, baseTokenAmount);
   }

3. There is no fee cap on the Factory.setProtocolFeeRate function. A value greater then 10000 can break the fee calculations in private pool. Consider validating that the input is less than 10000.

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

4. Factory.tokenId does not validate the input id parameter. Consider validating that the id exist and the respective pool is created by the factory.

   function tokenURI(uint256 id) public view override returns (string memory) {
       return PrivatePoolMetadata(privatePoolMetadata).tokenURI(id);
   }

5. Once initialization is done the PrivatePool.feeRate variable can never be changed. Consider adding an owner restricted function to update feeRate.

6. In buy and sell functions consider validating that the length of all input arrays are equal (tokenIds & tokenWeights).

7. Consider adding a check in PrivatePool.sumWeightsAndValidateProof function to validate that every element of tokenWeights array is greater than or equal to 1e18.

   function sumWeightsAndValidateProof(
       uint256[] memory tokenIds,
       uint256[] memory tokenWeights,
       MerkleMultiProof memory proof
   ) public view returns (uint256) {
       // ...
       for (uint256 i = 0; i < tokenIds.length; i++) {
           require(tokenWeights[i] >= 1e18);       <------------
           // create the leaf for the merkle proof
           leafs[i] = keccak256(bytes.concat(keccak256(abi.encode(tokenIds[i], tokenWeights[i]))));
   
           // sum each token weight
           sum += tokenWeights[i];
       }
       // ...
   }

8. In the PrivatePoolMetadata.tokenURI function consider using Strings.toHexString(address(tokenId))) for the name field.

   function tokenURI(uint256 tokenId) public view returns (string memory) {
       // forgefmt: disable-next-item
       bytes memory metadata = abi.encodePacked(
           "{",
               '"name": "Private Pool ',Strings.toString(tokenId),'",',
               '"description": "Caviar private pool AMM position.",',
               '"image": ','"data:image/svg+xml;base64,', Base64.encode(svg(tokenId)),'",',
               '"attributes": [',
                   attributes(tokenId),
               "]",
           "}"
       );
   
       return string(abi.encodePacked("data:application/json;base64,", Base64.encode(metadata)));
   }