Caviar Private Pools - indijanc's results

Lines of code

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

Vulnerability details

Impact

When ERC20 tokens with decimals less than 4 are set as the baseToken the change() functionality of PrivatePool will not work due to an underflow.

Proof of Concept

The changeFeeQuote() function uses the magic number 4 to subtract from the return value of baseToken.decimals() to calculate the changeFee.

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

This poses a problem for ERC20 tokens with decimals less than 4 as the function will always underflow and revert, hence breaking the change functionality of the contract.

A popular ERC20 token with only 2 decimals currently is GUSD which is #8 stablecoin by market capitalization.

Tools Used

Manual review.

Recommended Mitigation Steps

Here's some mitigation options:

1. Prevent creating a PrivatePool if baseToken.decimals() is less than 4.
2. Use maximum number of decimals supported for such tokens or 4 decimals precision otherwise for the changeFee. This would of course have to be well documented so Pool owners would be aware that changeFee has variable decimal precision based on the selected baseToken. Example feePerNft calculation:

        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 baseTokenDecimals = baseToken == address(0) ? 18 : ERC20(baseToken).decimals();
        uint256 exponent = baseTokenDecimals > 4 ? baseTokenDecimals - 4 : 0;
        uint256 feePerNft = changeFee * 10 ** exponent;