Caviar Private Pools - cryptonue's results

Lines of code

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

Vulnerability details

Impact

Private Pool failed to change Fee Quote changeFeeQuote if the baseToken decimals is under 4. (a case for GUSD (Gemini USD) )

Proof of Concept

The Caviar project doesn't mention it doesn't cover ERC20 which decimal is under 4. Also, there are no obligation for ERC20 token standard contains decimal > 4. In fact, there is a stable USD token which have 2 decimals, GUSD (Gemini USD) link. Gemini is one of exchange in U.S, which also have exposure and potential to become big in the future.

If we look at PrivatePool.sol changeFeeQuote function, the issue raised here is, when the PrivatePool is using baseToken < 4, it will underflow, thus revert the transaction. So, change of fee would not be successful.

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;
...
738:     }

If caviar ignore this issue (and recommendation steps), further potential token with decimals < 4 would be arise, limiting only supporting baseToken with decimal > 4 is not necessary if the solution is clear.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use the baseToken decimals as the fee accuracy amount, rather than a fixed 4 decimals.

Lines of code

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

Vulnerability details

Impact

Buyer or seller can still send the royalty fee, even if royalty recipient address is address(0). The royalty will be held in PrivatePool contract.

Proof of Concept

In PrivatePool, in condition where the PrivatePool set the payRoyalties to be true, the buy and sell function initially calculate the royaltyFeeAmount which later will be added (if buy) to netInputAmount or decreased (if sell) netOutputAmount.

The issue here is, there is no check if the _getRoyalty return a valid royalty recipient address (not empty address). Later, when this royalty need to be paid to royalty recipient address, it then checked if the address is not address(0).

Therefore, if the _getRoyalty return a non empty royaltyFee but a ZERO address of royalty recipient, the netInputAmount += royaltyFeeAmount will add royalty fee to the input for buyer to be send, but later, when contract want to send the royalty, it can not be send, because blocked in if (royaltyFee > 0 && recipient != address(0))

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add more check to the royaltyFee

242:             if (payRoyalties) {
243:                 // get the royalty fee for the NFT
+                 (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);
+.                if (royaltyFee > 0 && recipient != address(0)){
246:                 // add the royalty fee to the total royalty fee amount
247:                 royaltyFeeAmount += royaltyFee;
+                 }
248:             }