Caviar Private Pools - adriro's results

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

L-7 Potential overflow while updating reserves values in PrivatePool contract -

Lines of code

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

Vulnerability details

Impact

The change function present in the EthRouter contract can be used to execute multiple change actions from multiple private pools using a single transaction.

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

254:     function change(Change[] calldata changes, uint256 deadline) public payable {
255:         // check deadline has not passed (if any)
256:         if (block.timestamp > deadline && deadline != 0) {
257:             revert DeadlinePassed();
258:         }
259: 
260:         // loop through and execute the changes
261:         for (uint256 i = 0; i < changes.length; i++) {
262:             Change memory _change = changes[i];
263: 
264:             // transfer NFTs from caller
265:             for (uint256 j = 0; j < changes[i].inputTokenIds.length; j++) {
266:                 ERC721(_change.nft).safeTransferFrom(msg.sender, address(this), _change.inputTokenIds[j]);
267:             }
268: 
269:             // approve pair to transfer NFTs from router
270:             ERC721(_change.nft).setApprovalForAll(_change.pool, true);
271: 
272:             // execute change
273:             PrivatePool(_change.pool).change{value: msg.value}(
274:                 _change.inputTokenIds,
275:                 _change.inputTokenWeights,
276:                 _change.inputProof,
277:                 _change.stolenNftProofs,
278:                 _change.outputTokenIds,
279:                 _change.outputTokenWeights,
280:                 _change.outputProof
281:             );
282: 
283:             // transfer NFTs to caller
284:             for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
285:                 ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
286:             }
287:         }
288: 
289:         // refund any surplus ETH to the caller
290:         if (address(this).balance > 0) {
291:             msg.sender.safeTransferETH(address(this).balance);
292:         }
293:     }

Change operations are subject to change fees, private pools owners can configure a change fee and there may also be protocol fees. To cover these potential costs, the change function allows ETH payments that are forwarded to the change function in the Private Pool. However, as we can see in line 273, the implementation forwards the whole msg.value amount to the Private Pool change functions. This means that if any of the individual calls consumes some ETH for fees, then any subsequent call will fail because the implementation will try to send msg.value again and the contract will lack that amount as some of it has been already consumed.

For example, if the user is trying to execute two different change operations, and each one takes 0.1 ETH in fees, then the user will call change sending 0.2 ETH (see PoC for a detailed walkthrough):

1. User calls EthRouter change with 0.2 ETH.
2. EthRouter executes first change and sends 0.2 ETH to the first Private Pool. The pool returns 0.1 ETH.
3. EthRouter tries to execute the second change operation, it will try to send 0.2 ETH to the second Private Pool. The operation will fail as the current balance in the EthRouter contract is 0.1 ETH.

The issue will prevent users from using this feature to execute multiple change operations when at least one of the actions (different than the last one) contains change fees.

Proof of Concept

The following test illustrates the described issue. Alice tries to execute two different change operations using two private pools Each operation has a fee of 0.0025 ETH, so she sends 0.005 ETH. Even though everything looks correct, the operation will fail as the EthRouter contract will try to send 0.005 ETH again during the second change operation.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_EthRouter_change_IncorrectCallValue() public {
    // Setup pools
    PrivatePool privatePool1 = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool1.initialize(
        address(0), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        25, // uint56 _changeFee,
        0, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );
    PrivatePool privatePool2 = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool2.initialize(
        address(0), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        25, // uint56 _changeFee,
        0, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );
    // pool 1 has milady 1, pool 2 has milady 2
    uint256 tokenId1 = 1;
    uint256 tokenId2 = 2;
    milady.mint(address(privatePool1), tokenId1);
    milady.mint(address(privatePool2), tokenId2);
    
    // Now alice tries to change both tokens from both pools. Alice has milady 3 and 4
    vm.startPrank(alice);
    
    vm.deal(alice, 1 ether);
    
    uint256 tokenId3 = 3;
    uint256 tokenId4 = 4;
    milady.mint(address(alice), tokenId3);
    milady.mint(address(alice), tokenId4);
    
    milady.setApprovalForAll(address(ethRouter), true);
    
    EthRouter.Change[] memory changes = new EthRouter.Change[](2);
    
    changes[0].pool = payable(privatePool1);
    changes[0].nft = address(milady);
    changes[0].inputTokenIds = new uint256[](1);
    changes[0].inputTokenIds[0] = tokenId3;
    changes[0].outputTokenIds = new uint256[](1);
    changes[0].outputTokenIds[0] = tokenId1;
    
    changes[1].pool = payable(privatePool2);
    changes[1].nft = address(milady);
    changes[1].inputTokenIds = new uint256[](1);
    changes[1].inputTokenIds[0] = tokenId4;
    changes[1].outputTokenIds = new uint256[](1);
    changes[1].outputTokenIds[0] = tokenId2;
    
    // Each change will take 0.0025 ETH
    uint256 value = 2* 0.0025 ether;
    
    // The following action will fail even though everything should be correct. The main issue is that the `change` funtion will try to forward `msg.value` to each call of the PrivatePool.change. The first call will take the fee (0.0025) and refund the excess (0.0025), but the second call will try to forward again 0.0050 ETH and will fail since the balance is only 0.0025 ETH.
    vm.expectRevert();
    ethRouter.change{value: value}(changes, 0);
    
    vm.stopPrank();
}

Recommendation

The change function could send the available contract balance to each call to the change function of the Private Pool contract. The pool will send the remainder ETH back to the EthRouter, and the EthRouter can continue sending the available amount in the contract to subsequent calls to the change function. Any ETH left at the end of the function is already refunded to the caller.

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

Lines of code

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

Vulnerability details

Impact

Private Pools support NFT borrowing using flash loans. Users that decide to use this feature have to pay a flash loan fee to the owner of the pool.

The contract has a changeFee variable that is used to configure the fee for changing NFTs, and this variable is also used to determine the fee for flash loans. In the case of a change operation, the value is interpreted as an amount with 4 decimals, and the token is the base token of the pool. This means that, for example, if the base token is ETH, a changeFee value of 25 should be interpreted as a fee of 0.0025 ETH for change operation.

However, as we can see in this following snippet, the flashFee function just returns the value of changeFee without any scaling or modification.

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

750:     function flashFee(address, uint256) public view returns (uint256) {
751:         return changeFee;
752:     }

This means that, following the previous example, a changeFee value of 25 will result in 0.0025 ETH for change operation, but just 25 wei for flash loans. The documentation hints that this value should also be scaled to 4 decimals in the case of the flash loan fee, but in any case this is clearly an incorrect setting of the flash loan fee.

Proof of Concept

In the following test, the pool is configured with a changeFee value of 25, and Alice is able to execute a flash loan by just paying 25 wei.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_PrivatePool_flashLoan_IncorrectFee() public {
    // Setup pool
    PrivatePool privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    uint56 changeFee = 25;
    privatePool.initialize(
        address(0), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        changeFee, // uint56 _changeFee,
        0, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );
    
    uint256 tokenId = 0;
    milady.mint(address(privatePool), tokenId);
    
    // Alice executes a flash loan
    vm.startPrank(alice);
    
    FlashLoanBorrower flashLoanBorrower = new FlashLoanBorrower();
    
    // Alice just sends 25 wei!
    vm.deal(alice, changeFee);
    privatePool.flashLoan{value: changeFee}(flashLoanBorrower, address(milady), tokenId, "");
    
    vm.stopPrank();
}

Recommendation

The flashFee function should properly scale the value of the changeFee variable, similar to how it is implemented in changeFeeQuote.

function flashFee(address, uint256) public view returns (uint256) {
    // 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;
    return feePerNft;
}

Lines of code

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

Vulnerability details

Impact

Private pools have a "change" fee setting that is used to charge fees when a change is executed in the pool (user swaps tokens for some tokens in the pool). This setting is controlled by the changeFee variable, which is intended to be defined using 4 decimals of precision:

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

87:     /// @notice The change/flash fee to 4 decimals of precision. For example, 0.0025 ETH = 25. 500 USDC = 5_000_000.
88:     uint56 public changeFee;

As the comment says, in the case of ETH a value of 25 should represent 0.0025 ETH. In the case of an ERC20 this should be scaled accordingly based on the number of decimals of the token. The implementation is defined in the changeFeeQuote function.

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

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

As we can see in the previous snippet, in case the baseToken is an ERC20, then the exponent is calculated as ERC20(baseToken).decimals() - 4. The main issue here is that if the token decimals are less than 4, then the subtraction will cause an underflow due to Solidity's default checked math, causing the whole transaction to be reverted.

Such tokens with low decimals exist, one major example is GUSD, Gemini dollar, which has only two decimals. If any of these tokens is used as the base token of a pool, then any call to the change will be reverted, as the scaling of the charge fee will result in an underflow.

Proof of Concept

In the following test we recreate the "Gemini dollar" token (GUSD) which has 2 decimals and create a Private Pool using it as the base token. Any call to change or changeFeeQuote will be reverted due to an underflow error.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_PrivatePool_changeFeeQuote_LowDecimalToken() public {
    // Create a pool with GUSD which has 2 decimals
    ERC20 gusd = new GUSD();

    PrivatePool privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(gusd), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        500, // uint56 _changeFee,
        100, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );

    // The following will fail due an overflow. Calls to `change` function will always revert.
    vm.expectRevert();
    privatePool.changeFeeQuote(1e18);
}

Recommendation

The implementation of changeFeeQuote should check if the token decimals are less than 4 and handle this case by dividing by the exponent difference to correctly scale it (i.e. chargeFee / (10 ** (4 - decimals))). For example, in the case of GUSD with 2 decimals, a chargeFee value of 5000 should be treated as 0.50.

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

L-2 Royalties are paid assuming all NFTs in the batch are equally priced -

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L238-L252 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L271-L285

Vulnerability details

Impact

In both buy and sell functions of the PrivatePool contract there may be royalty fees associated with the operations. These fees are paid by the caller of the function.

In both cases, royalty recipient and amount are fetched from a lookup address. In the case of buy, the fees for each token are accumulated in a royaltyFeeAmount which is then added to the netInputAmount.

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

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;

The function then transfers these fees to the recipients:

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

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

Note that here, fees are only sent if recipient != address(0), a condition that was not present in the first snippet of code. This means that if royaltyFee > 0 but recipient == address(0), then royalty fees will be accounted for in the netInputAmount that the caller must pay, but won't be distributed since the recipient is the zero address.

A similar case happens in the sell function. Royalty fees are accounted for in the total amount in line 341, but are only sent if the recipient is not the zero address (line 344).

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

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:         }
353: 
354:         // subtract the royalty fee amount from the net output amount
355:         netOutputAmount -= royaltyFeeAmount;

Recommendation

In both cases, the royalty fee should only be accounted for if the royalty recipient is not the zero address.

For the buy function:

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, address recipient) = _getRoyalty(tokenIds[i], salePrice);

        // add the royalty fee to the total royalty fee amount
        if (recipient != address(0)) {
          royaltyFeeAmount += royaltyFee;
        }
    }
}

For the sell function:

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

    // transfer the royalty fee to the recipient if it's greater than 0
    if (royaltyFee > 0 && recipient != address(0)) {
        // tally the royalty fee amount
        royaltyFeeAmount += royaltyFee;
    
        if (baseToken != address(0)) {
            ERC20(baseToken).safeTransfer(recipient, royaltyFee);
        } else {
            recipient.safeTransferETH(royaltyFee);
        }
    }
}

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

L-3 Potential loss of funds when paying royalties -

Lines of code

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

Vulnerability details

Impact

The Factory contract allows users to create and initialize Private Pools. New PrivatePool contracts are created and deployed using solady's LibClone library, which creates a minimal proxy, in a deterministic fashion:

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

privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

Under the hood, the library will deploy the proxy using create2 with the given salt value. This means that Private Pools deployed from the factory will have their addresses determined by the salt, which is a parameter of the create function. In fact, the contract provides a function named predictPoolDeploymentAddress which can be used to know beforehand the address of a Private Pool.

This can be used by a bad actor to front-run the creation of a pool. Given the address is determined by the salt parameter, an attacker can front-run the transaction with the salt value and end up having the same Private Pool address as the original transaction.

This implementation may also cause issues in case of a chain reorganization, or a rollback in case of a fraud dispute in rollup chains such as Optimism or Arbitrum. This might have a bigger impact, since the original owner of the pool may have made deposits into the pool in further transactions. In this case, the attacker can front-run the creation of the pool and eventually get the deposits from the next transaction of the user. The scenarios goes as follows:

1. User creates Private Pool using the Factory contract with a given salt.
2. User makes a deposit into the pool to transfer NTFs, funds, or both.
3. Chain reorganization or rollback happens.
4. Attacker front-runs the creation of the pool using the same salt as step 1.
5. The initial user transaction to create the pool will fail, but the deposit transaction will be executed, essentially depositing the assets in the now attacker's pool.
6. Attacker controls Private Pool along with its deposits.

See this excellent finding as a reference of the issue https://github.com/code-423n4/2023-01-rabbithole-findings/issues/661

This may also cause issues when the protocol is being deployed to multiple chains. The owner of a Private Pool may want to have the same address for their Private Pool across all different chains. A bad actor can front-run the user and deploy the Private Pool in other chains using the same salt value. Note that for this scenario to make sense the Factory contract would need to be deterministically deployed so it has the same address across all chains too.

Recommendation

One possible strategy would be to include the caller as part of the salt being used to create the Private Pool. This will prevent contracts from being created by unintended parties, while still being able to deploy contracts in a deterministic way.

As a reference implementation, the Metamorphic contracts library specifies that the first 20 bytes of the salt must match the caller's address.

https://github.com/0age/metamorphic/blob/master/contracts/ImmutableCreate2Factory.sol#L194-L203

modifier containsCaller(bytes32 salt) {
  // prevent contract submissions from being stolen from tx.pool by requiring
  // that the first 20 bytes of the submitted salt match msg.sender.
  require(
    (address(bytes20(salt)) == msg.sender) ||
    (bytes20(salt) == bytes20(0)),
    "Invalid salt - first 20 bytes of the salt must match calling address."
  );
  _;
}

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

L-8 Zero amount ERC20 token transfers may fail some implementations -

Report

Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (4):

File: src/EthRouter.sol

2: pragma solidity ^0.8.19;

File: src/Factory.sol

2: pragma solidity ^0.8.19;

File: src/PrivatePool.sol

2: pragma solidity ^0.8.19;

File: src/PrivatePoolMetadata.sol

2: pragma solidity ^0.8.19;

<a name="L-2"></a>[L-2] Royalties are paid assuming all NFTs in the batch are equally priced

• 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#L335
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L115
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L182

In the EthRouter and PrivatePool contracts, when buying or selling NFTs, royalty fees are calculated as if all tokens were equally priced, since the sale price is calculated by just taking the amount and dividing it by the numbers of tokens.

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

A more robust solution would be to factor the token weights in the calculation so each NFT is priced according to the weight it has in the pool.

<a name="L-3"></a>[L-3] Potential loss of funds when paying royalties

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

In order to pay royalties, the EthRouter contract queries a registry in order to fetch the recipient and the amount of the royalty fee. In both of these cases, the implementation checks that royaltyFee is greater than zero but doesn't verify that royaltyRecipient is not the empty address. If this is the case, then funds will be sent to the address(0).

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

The recommendation is to also check that royaltyFee != address(0).

(uint256 royaltyFee, address royaltyRecipient) =
    getRoyalty(buys[i].nft, buys[i].tokenIds[j], salePrice);

if (royaltyFee > 0 && royaltyRecipient != address(0)) {
    // transfer the royalty fee to the royalty recipient
    royaltyRecipient.safeTransferETH(royaltyFee);
}

<a name="L-4"></a>[L-4] NFT address is not validated in Factory create

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

The create function should validate that _nft != address(0).

<a name="L-5"></a>[L-5] Missing event for important parameter change

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Instances (3):

• https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L129
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L135
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L141

<a name="L-6"></a>[L-6] Factory setProtocolFeeRate should validate that the fee rate is within an acceptable range

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

The setProtocolFeeRate should validate that the _protocolFeeRate value is within an acceptable range for the protocol fee.

<a name="L-7"></a>[L-7] Potential overflow while updating reserves values in PrivatePool contract

• 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

In both the buy and sell functions of the PrivatePool contracts, reserves values are updated by downcasting an uint256 to a uint128, which may cause an overflow of the values.

virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);
virtualNftReserves -= uint128(weightSum);

The implementation should use a "safe" casting variant in order to prevent the overflow, such as solmate's SafeCastLib.

<a name="L-8"></a>[L-8] Zero amount ERC20 token transfers may fail some implementations

• https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L423
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L502
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L527
• https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L651

In the PrivatePool contract there are several occurrences of a potential ERC20 transfer of zero value. Some ERC20 token implementations revert the operation when transferring a zero amount, which will end up reverting the transaction.

In all of these cases, the implementation should check that the transferred value is greater than zero before calling the transfer function.

<a name="L-9"></a>[L-9] price function will revert for ERC20 token with high decimals

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

The price function present in the PrivatePool factory will revert if the base token is an ERC20 with more than 36 decimals, as the subtraction will cause an underflow.

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

EthRouter contract

• ERC721 transfers to the EthRouter contract can be executed using transferFrom instead of safeTransferFrom in order to save gas, as it is presumed that the EthRouter contract can handle ERC721 tokens and the callback isn't needed.
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L162
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L240
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L266

• The EthRouter contract always reapproves the ERC721 contract to the pool for each item in the sell, deposit and change functions. Consider using a lookup mapping to know which contracts have been already approved, if the contract has been already approved it will save a call to setApprovalForAll that does an SSTORE and LOG operation (taking OZ and solmate implementations as references).
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L166
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L244
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L270

• The whole Change struct is copied into memory in each iteration of the loop. Consider referencing it directly from calldata.
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L262

Factory contract

• ERC721 transfers to the Factory contract can be executed using transferFrom instead of safeTransferFrom in order to save gas, as it is presumed that the Factory contract can handle ERC721 tokens and the callback isn't needed.
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L120

PrivatePool contract

• Unchecked math can be safely used to save gas due to conditions already checked in the surrounding if statement. https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L268
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L436

• ERC721 transfers to the PrivatePool contract can be executed using transferFrom instead of safeTransferFrom in order to save gas, as it is presumed that the PrivatePool contract can handle ERC721 tokens and the callback isn't needed.
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L331 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L442 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L497 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L648

• setAllParameters will call all the config setters, and each of these will add a check for the owner. Consider extracting the functionality to avoid repeated access control checks.
  https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L602-L615

• In the sell function, the salePrice variable is calculated in each iteration of the loop. This isn't needed as the value doesn't depend on anything specific to the loop and remains constant across all iterations. Consider moving this calculation outside the loop to save gas. https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L335