Caviar Private Pools - Josiah's results

Lines of code

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

Vulnerability details

Impact

changeFee that has been scaled with 4 decimals of of basis points is being adopted by flashloan(). This could make the function behave in an unexpected manner than intended.

Proof of Concept

The fee is calculated as:

PrivatePool.sol#L632

        uint256 fee = flashFee(token, tokenId);

The returned changeFee is a very smaller integer. For example, if it is 1 USDC, that will mean 10000 USDC equivalent to USD 0.01 because USDC is associated with 6 decimals. It could have been worse if the baseToken has 18 decimals, which is as good as nothing.

PrivatePool.sol#L750-L752

    function flashFee(address, uint256) public view returns (uint256) {
        return changeFee;
    }

Additionally, it is going to get all flash loan easily pass and execute because of an easy bypass here (baseToken is ETH in this case):

PrivatePool.sol#L635

        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

Recommended Mitigation Steps

It is recommended scaling changeFee like it has been done so in changeFeeQuote() before having it integrated with flashLoan().

Lines of code

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

Vulnerability details

Impact

There is an incorrect use of variable in the arithmetic calculation pertaining to assigning protocolFeeAmount in changeFeeQuote(). Although it does not currently affect the protocol, it will when the Factory owner decides to start collecting protocol fees.

Proof of Concept

feeAmount is first correctly computed after adjusting changeFee to the correct exponent. However, instead of using inputAmount, the function logic uses the diminished feeAmount to multiply with Factory(factory).protocolFeeRate(). This will make protocolFeeAmount very much smaller than expected possibly as good as nothing, defeating the purpose of introducing it to the system.

PrivatePool.sol#L731-L738

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // 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;

        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
    }

Recommended Mitigation Steps

It is recommended replacing feeAmount with inputAmount like it has been done so when calculating for feeAmount.