Caviar Private Pools - anodaram's results

Lines of code

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

Vulnerability details

Impact

The function PrivatePool.flashFee, which is used for calculating fee in flashLoan(), returns changeFee directly. changeFee is before applying decimals. So this part is definitely wrong.

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

Proof of Concept

Comment description about changeFee: (which shows changeFee is before applying decimals)

File: PrivatePool.sol
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;

In function flashLoan, it used flashFee directly for transferring the baseToken.

File: PrivatePool.sol
623:     function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
624:         external
625:         payable
626:         returns (bool)
627:     {
...
632:         uint256 fee = flashFee(token, tokenId);
...
635:         if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();
...
651:         if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);
...
654:     }

Tools Used

VSCode

Recommended Mitigation Steps

Fixed code:

File: PrivatePool.sol
750:     function flashFee(address, uint256) public view returns (uint256) {
751:         return changeFee * 10 ** ERC20(baseToken).decimals() / 10 ** 4;
752:     }

Lines of code

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

Vulnerability details

Impact

In PrivatePool.changeFeeQuote, it assumes that baseToken.decimals is not smaller than 4. We should take care about some ERC20 tokens have decimals smaller than 4.

Proof of Concept

File: PrivatePool.sol
733:         uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;

Example of ERC20 tokens with decimals smaller than 4. - Hyperion Token (HYN) - HoloToken (HOT) - Moss Carbon Credit (MCO2) - DAOstack (GEN) If the baseToken is one of these, we can't call changeFeeQuote.

Tools Used

VSCode

Recommended Mitigation Steps

Need to check the decimals of baseToken if it is bigger enough when initialize. Another way is:

Original:

        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feePerNft = changeFee * 10 ** exponent;

Fixed:

        uint256 feePerNft = changeFee;
        if (baseToken == address(0)) feePerNft *= 10 ** 14;
        else if (ERC20(baseToken).decimals() >= 4) feePerNft *= 10 ** (ERC20(baseToken).decimals() - 4);
        else feePerNft /= 10 ** (4 - ERC20(baseToken).decimals());