Caviar Private Pools - sashik_eth's results

Lines of code

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

Vulnerability details

The execute() function, which is callable by pool owners, enables the execution of arbitrary contracts with arbitrary data. This creates a risk that a malicious pool owner could steal assets approved by users for trading by sending them to their own address.

While traders may have a reasonable level of trust in the owner of Factory.sol - which is Caviar protocol itself - this is not the case for private pool owners. These owners are arbitrary users who create their own pools using the factory contract without any permissions. As a result, users cannot fully trust that pool owners will not steal their funds.

EthRouter.sol introduces protection from such issues, but it works only with eth native pools, leaving users of pools with ERC20 tokens without protection.

Impact

Any asset approved for the pool by the traders could be stolen by a malicious pool owner with a front-running transaction before the users execute their own trade.

Proof of Concept

Adding the next test to test/PrivatePool/Execute.t.sol could show the problem scenario:

    function test_MaliciousPoolOwner() public {

        address user = address(0x123);
        // Mint NFT to victim address
        milady.mint(user, 1111);
        // Approving NFT before trading
        vm.prank(user);
        milady.setApprovalForAll(address(privatePool), true);
        // Malicious owner steals NFT before user send's trading transaction
        bytes memory data = abi.encodeCall(milady.transferFrom, (user, address(this), 1111));
        privatePool.execute(address(milady), data);

        assertEq(user, milady.ownerOf(1111), "NFT was stolen");
    }

Recommended Mitigation Steps

Consider removing the function execute() from the PrivatePool.sol or adding an external router for ERC20 private pools.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L230-L231 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L323-L324

Vulnerability details

The buy() and sell() functions update the virtualBaseTokenReserves and virtualNftReserves variables during each trade. However, these two variables are of type uint128, while the values that update them are of type uint256. This means that casting to a lower type is necessary, but this casting is performed without first checking that the values being cast can fit into the lower type. As a result, there is a risk of a silent overflow occurring during the casting process.

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

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

        // calculate the required net input amount and fee amount
        (netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);
        ...
        // update the virtual reserves
        virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount); 
        virtualNftReserves -= uint128(weightSum);
        ...

Impact

If the reserves variables are updated with a silent overflow, it can lead to a breakdown of the xy=k equation. This, in turn, would result in a totally incorrect price calculation, causing potential financial losses for users or pool owners.

Proof of Concept

Consider the scenario with a base token that has high decimals number described in the next test (add it to the test/PrivatePool/Buy.t.sol):

    function test_Overflow() public {
        // Setting up pool and base token HDT with high decimals number - 30
        // Initial balance of pool - 10 NFT and 100_000_000 HDT
        HighDecimalsToken baseToken = new HighDecimalsToken();
        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            address(baseToken),
            nft,
            100_000_000 * 1e30,
            10 * 1e18,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false
        );

        // Minting NFT on pool address
        for (uint256 i = 100; i < 110; i++) {
            milady.mint(address(privatePool), i);
        }
        // Adding 8 NFT ids into the buying array
        for (uint256 i = 100; i < 108; i++) {
            tokenIds.push(i);
        }
        // Saving K constant (xy) value before the trade
        uint256 kBefore = uint256(privatePool.virtualBaseTokenReserves()) * uint256(privatePool.virtualNftReserves());

        // Minting enough HDT tokens and approving them for pool address
        (uint256 netInputAmount,, uint256 protocolFeeAmount) = privatePool.buyQuote(8 * 1e18);
        deal(address(baseToken), address(this), netInputAmount);
        baseToken.approve(address(privatePool), netInputAmount);

        privatePool.buy(tokenIds, tokenWeights, proofs);

        // Saving K constant (xy) value after the trade
        uint256 kAfter = uint256(privatePool.virtualBaseTokenReserves()) * uint256(privatePool.virtualNftReserves());

        // Checking that K constant succesfully was changed due to silent overflow
        assertEq(kBefore, kAfter, "K constant was changed");
    }

Also add this contract into the end of Buy.t.sol file for proper test work:

    contract HighDecimalsToken is ERC20 {
        constructor() ERC20("High Decimals Token", "HDT", 30) {}
    }

Recommended Mitigation Steps

Add checks that the casting value is not greater than the uint128 type max value:

File: PrivatePool.sol
229:         // update the virtual reserves
+            if (netInputAmount - feeAmount - protocolFeeAmount > type(uint128).max) revert Overflow();
230:         virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount); 
+            if (weightSum > type(uint128).max) revert Overflow();
231:         virtualNftReserves -= uint128(weightSum);

File: PrivatePool.sol
322:         // update the virtual reserves
+            if (netOutputAmount + protocolFeeAmount + feeAmount > type(uint128).max) revert Overflow();
323:         virtualBaseTokenReserves -= uint128(netOutputAmount + protocolFeeAmount + feeAmount);
+            if (weightSum > type(uint128).max) revert Overflow();
324:         virtualNftReserves += uint128(weightSum);

Lines of code

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

Vulnerability details

PrivatePool.sol functionality includes a few types of fees. One of them is changeFee, which is charged to users in two cases:

1. During exchange users' NFTs to NFTs deposited in the pool using change() function:

File: PrivatePool.sol
385:     function change(
386:         uint256[] memory inputTokenIds,
387:         uint256[] memory inputTokenWeights,
388:         MerkleMultiProof memory inputProof,
389:         IStolenNftOracle.Message[] memory stolenNftProofs,
390:         uint256[] memory outputTokenIds,
391:         uint256[] memory outputTokenWeights,
392:         MerkleMultiProof memory outputProof
393:     ) public payable returns (uint256 feeAmount, uint256 protocolFeeAmount) {
...
415:             // calculate the fee amount
416:             (feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);
...
421:         if (baseToken != address(0)) {
422:             // transfer the fee amount of base tokens from the caller
423:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), feeAmount);
...
427:         } else {
428:             // check that the caller sent enough ETH to cover the fee amount and protocol fee
429:             if (msg.value < feeAmount + protocolFeeAmount) revert InvalidEthAmount();
...
434:             // refund any excess ETH to the caller
435:             if (msg.value > feeAmount + protocolFeeAmount) {
436:                 msg.sender.safeTransferETH(msg.value - feeAmount - protocolFeeAmount);
437:             }
438:         }

2. During the execution of flashLoan() calls:

File: PrivatePool.sol
623:     function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data) 
624:         external
625:         payable
626:         returns (bool)
627:     {
...
631:         // calculate the fee
632:         uint256 fee = flashFee(token, tokenId);
633: 
634:         // if base token is ETH then check that caller sent enough for the fee
635:         if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();
...
650:         // transfer the fee from the borrower
651:         if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);
652: 
653:         return success;
654:     }

Calculation of those fees depends on the same variable changeFee (which can't be changed after pool deployment):

File: PrivatePool.sol
731:     function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) { 
732:         // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
733:         uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
734:         uint256 feePerNft = changeFee * 10 ** exponent;
735: 
736:         feeAmount = inputAmount * feePerNft / 1e18;
737:         protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
738:     }
...
750:     function flashFee(address, uint256) public view returns (uint256) {
751:         return changeFee;
752:     }

However, due to the logic of flashFee() and changeFeeQuote() functions size of those fees would be different dramatically for any of the popular ERC20 token and native eth pools, since changeFeeQuote() calculates fee in basis points depending on baseToken decimals, while flashFee() function return only value of changeFee variable. changeFee should be in basis points (due to the protocol docs) and has uint56 type with a max value of ~72e15, so it's likely that for most of the popular ERC20 tokens flashFee() would be dust value in any case. It's also opening the attack vector described in the previous Code4rena Caviar report M-04.

Impact

FlashLoan fee sizes would be unreasonably low for any popular ERC20 tokens and native ETH. Even if the pool owner chooses a baseToken with a low decimal number (e.g. USDC) and sets a reasonable value for the changeFee (in terms of flash loan fee calculation), this would result in the fee for the change() function being too high. As a result, either the change() function would be too expensive for trades to use or the flashLoan() calls would not generate any real profit for the pool owner.

Proof of Concept

A short test added to test/PrivatePool/Flashloan.t.sol could demonstrate the difference between fees:

    function test_ChangeFee() public {
        // Getting amount of flash fee for 1 NFT
        uint256 flashFee = privatePool.flashFee(address(milady), 1);
        // Getting amount of change fee for 1 NFT 
        (uint256 changeFee, ) = privatePool.changeFeeQuote(1e18);
        // Failed log shows how huge is difference between these two fees
        assertEq(flashFee, changeFee, "Flash loan fee and change fee are different");
    }

Recommended Mitigation Steps

Consider updating flashFee() function to next:

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

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

Vulnerability details

The buy() and sell() functions calculate the average sale price of NFTs bought or sold during each trade to provide the royaltyInfo() call and send appropriate amounts of royalties to its receivers:

File: PrivatePool.sol
328:         uint256 royaltyFeeAmount = 0;
329:         for (uint256 i = 0; i < tokenIds.length; i++) {
330:             // transfer each nft from the caller
331:             ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
332: 
333:             if (payRoyalties) {
334:                 // calculate the sale price (assume it's the same for each NFT even if weights differ)
335:                 uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;
336: 
337:                 // get the royalty fee for the NFT
338:                 (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);
339: 
340:                 // tally the royalty fee amount
341:                 royaltyFeeAmount += royaltyFee;
342: 
343:                 // transfer the royalty fee to the recipient if it's greater than 0
344:                 if (royaltyFee > 0 && recipient != address(0)) {
345:                     if (baseToken != address(0)) {
346:                         ERC20(baseToken).safeTransfer(recipient, royaltyFee);
347:                     } else {
348:                         recipient.safeTransferETH(royaltyFee);
349:                     }
350:                 }
351:             }
352:         }

However, each NFT's price should be calculated based on its own weight, as different amounts may be received as royalties for NFTs traded in different buy() or sell() calls due to the individual royalty calculation logic. Also, the royalty recipient addresses may not necessarily be the same for all tokens in the collection. Therefore, different royalties receivers receive the same amount of royalties in case "their" tokens would be traded in the same call, but with different weights(=prices).

Impact

Royalty receivers could receive the wrong amount of royalties.

Proof of Concept

Royalty calculation due to EIP-2981 could have custom logic in terms of how and who would receive a royalty on which id from the collection. Consider two scenarios where royalty receivers get the wrong amounts due to describing the issue:

1.1 Alice creates an NFT collection with custom royaltyInfo logic that specifies a 10% royalty fee when the sale price is greater than 1e18 and 0% otherwise. 1.2 Alice creates a Caviar private pool where half of the collection has a weight of 1e18 and the other half has a weight of 2e18. 1.3 Bob purchases two NFTs (one with a weight of 1e18 and the other with a weight of 2e18) in a single buy() call for a total price of 2.5 ETH, paying 0.25 ETH as a royalty fee (0.125 ETH per NFT). In this case, the amount of royalties paid is incorrect, as Bob should only pay a royalty fee for the NFT with a weight of 2e18 as he would if he bought the same NFTs in different calls.

2.1 Alice creates an NFT collection with custom royaltyInfo logic that specifies that the royalty receiver for half of the collection ids is herself, while the other half - is Charlie. 2.2 Alice creates a Caviar private pool where half of the collection has a weight of 1e18 (those where she is a royalty receiver) and the other half has a weight of 2e18 (those where Charlie is a royalty receiver). 2.3 Bob purchases two NFTs (one with a weight of 1e18 and the other with a weight of 2e18) in a single buy() call for a total price of 2.5 ETH, paying 0.25 ETH as a royalty fee (0.125 ETH per NFT). In this case, both royalty receivers got the same amount of funds, while it would (and should) be different if Bob would buy the same NFTs in separate transactions.

Recommended Mitigation Steps

Consider the calculation of the royalty amount based on the token's individual weights:

+   uint256 tokenPrice = (netOutputAmount + feeAmount + protocolFeeAmount) * tokenWeights[i] / weightSum;
+   (uint256 royaltyFee,) = _getRoyalty(tokenIds[i], tokenPrice); 
-   uint256 salePrice = (netOutputAmount + feeAmount + protocolFeeAmount) / tokenIds.length;
-   (uint256 royaltyFee,) = _getRoyalty(tokenIds[i], salePrice); 

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L277 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L242-L248 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L341-L355

Vulnerability details

In case if pool supports paying royalties, buy() and sell() functions calculate the amounts of royalties for each NFT (line 244). These amounts are then charged to the buyer/seller as part of the purchase/sell price (line 252) for the benefit of the royalties recipients.

File: PrivatePool.sol
237:         uint256 royaltyFeeAmount = 0;
238:         for (uint256 i = 0; i < tokenIds.length; i++) {
239:             // transfer the NFT to the caller
240:             ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);
241: 
242:             if (payRoyalties) {
243:                 // get the royalty fee for the NFT
244:                 (uint256 royaltyFee,) = _getRoyalty(tokenIds[i], salePrice); 
245: 
246:                 // add the royalty fee to the total royalty fee amount
247:                 royaltyFeeAmount += royaltyFee;
248:             }
249:         }
250: 
251:         // add the royalty fee amount to the net input aount
252:         netInputAmount += royaltyFeeAmount;
...
256:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);
...
271:         if (payRoyalties) {
272:             for (uint256 i = 0; i < tokenIds.length; i++) {
273:                 // get the royalty fee for the NFT
274:                 (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);
275: 
276:                 // transfer the royalty fee to the recipient if it's greater than 0
277:                 if (royaltyFee > 0 && recipient != address(0)) {
278:                     if (baseToken != address(0)) {
279:                         ERC20(baseToken).safeTransfer(recipient, royaltyFee);
280:                     } else {
281:                         recipient.safeTransferETH(royaltyFee);
282:                     }
283:                 }
284:             }
285:         }

However, due to line 277 in case the recipient's address is 0x0 (either due to a mistake by the collection owner or an intentional desire to burn received royalties), royalties will still be collected from the buyer/seller but not sent anywhere and they will be held in the contract's balance on behalf of the pool owner.

In contrast to this behavior, in EthRouter.sol, royalties are sent regardless of the recipient's address when trading through a shared pool:

File: EthRouter.sol
113:                 // pay the royalties if buyer has opted-in
114:                 if (payRoyalties) {
115:                     uint256 salePrice = inputAmount / buys[i].tokenIds.length;
116:                     for (uint256 j = 0; j < buys[i].tokenIds.length; j++) {
117:                         // get the royalty fee and recipient
118:                         (uint256 royaltyFee, address royaltyRecipient) =
119:                             getRoyalty(buys[i].nft, buys[i].tokenIds[j], salePrice);
120: 
121:                         if (royaltyFee > 0) {
122:                             // transfer the royalty fee to the royalty recipient
123:                             royaltyRecipient.safeTransferETH(royaltyFee);
124:                         }
125:                     }
126:                 }

Impact

The pool will receive royalties that were not intended for it.

Proof of Concept

1.1 Alice creates an NFT collection, mistakenly setting the royalty receiver as address(0). 1.2 Bob creates a private pool with Alice's collection. 1.3 Charlie trades NFTs through Bob's pool. Royalties collected from Charlie but stayed on the pool address, allowing Bob to withdraw them anytime.

Recommended Mitigation Steps

Consider not collecting royalties from the seller/buyer in case if royalty recipient is zero address.

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

[L-07] Malicious collection owner could steal all base tokens by updating royalty during calls 1

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

[L-01] Reorg attack possibility in pool factory 1