Caviar Private Pools - jpserrat's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L632 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L651 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L748-L752 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L87-L88

Vulnerability details

Impact

If owner set feeAmount to zero, the change function may always revert in cases where the base token is of a type that revert

Proof of Concept

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

As it is said the change/flash fee should be 4 decimals of precision. But the way that is currently implemented for the flash fee the 4 decimals of precision are not working as it should.

    /// @notice Returns the fee required to flash swap a given NFT.
    /// @return feeAmount The fee amount.
    function flashFee(address, uint256) public view returns (uint256) {
        return changeFee;
    }

In this way if the owner sets the change fee to 25 (0.0025ETH), for the flashLoan function the owner would only receive 25 wei.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing the same logic that is currently being used for the change 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#L423

Vulnerability details

Impact

When user call change to trade nft, this function may not when work owner set the fee amount to zero.

Proof of Concept

Some tokens such as LEND revert when transfer with zero value. With the scneario where the owner add LEND as the base token and set feeAmount to zero this function would not work

Tools Used

Manual review

Recommended Mitigation Steps

Add a check if amount is bigger than zero

 if (baseToken != address(0)) {
    // transfer the fee amount of base tokens from the caller
    if (feeAmount > 0) ERC20(baseToken).safeTransferFrom(msg.sender, address(this), feeAmount);