Assuming all ERC20 tokens have a similar amount of decimals

ERC20 tokens do not have the same amount of tokens, but tehre are some exceptions.

Some tokens have low decimals (e.g. USDC has 6). Even more extreme, some tokens like Gemini USD only have 2 decimals. This may result in larger than expected precision loss.

Some tokens have more than 18 decimals (e.g. YAM-V2 has 24). This may trigger unexpected reverts due to overflow, posing a liveness risk to the contract.

Vulnerability description

Private pools have a “change” fee setting that is used to charge fees when a change is executed in the pool (user swaps tokens for some tokens in the pool). This setting is controlled by the changeFee variable, which is intended to be defined using 4 decimals of precision:

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

In the case of an ERC20 this should be scaled accordingly based on the number of decimals of the token. The implementation is defined in the changeFeeQuote function:

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

As shown in the previous snippet, in case the baseToken is an ERC20, then the exponent is calculated as ERC20(baseToken).decimals() - 4 The main issue here is that if the token decimals are less than 4, then the subtraction will cause an underflow due to Solidity's default checked math, causing the whole transaction to be reverted.

One major example is GUSD, Gemini dollar, which has only two decimals. If any of these tokens are used as the base token of a pool, then any call to the change will be reverted, as the scaling of the charge fee will result in an underflow.

Proof of concept

In the following test we recreate the “Gemini dollar” token (GUSD) which has 2 decimals and create a Private Pool using it as the base token.

Any call to change or changeFeeQuote will be reverted due to an underflow error.

function test_PrivatePool_changeFeeQuote_LowDecimalToken() public {
    // Create a pool with GUSD which has 2 decimals
    ERC20 gusd = new GUSD();

    PrivatePool privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(gusd), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        500, // uint56 _changeFee,
        100, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );

    // The following will fail due an overflow. Calls to `change` function will always revert.
    vm.expectRevert();
    privatePool.changeFeeQuote(1e18);
}

Mitigation

The implementation of changeFeeQuote should check if the token decimals are less than 4. And handle this case by dividing by the exponent difference to correctly scale it

chargeFee / (10 ** (4 - decimals))

For example, in the case of GUSD with 2 decimals, a chargeFee value of 5000 should be treated as 0.50.

Sources

https://medium.com/coinmonks/learn-attack-vectors-and-explore-h-m-severity-issues-over-underflow-e331aa41d97b