code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

PrivatePool has a price function that is used to determine what the actual price of any NFT would be. This is needed and used for determining slippage. We can also see it being used in the EthRouter, where it operates with ETH only.

The formula used to determine price is incorrect, leading to a radically different price. This leads to a faulty slippage mechanism, that for any protocol integrator which wraps the PrivatePool and uses price for slippage validation will have distorted cases, ultimately leading to user attempting buys when the price is actually different.

There are 5 issues with the function that are detailed bellow.

It does not respect the xy=k invariant
Price doesn't include the protocolFeeRate and pool feeRate
Price is not round up, as when actually buying
There is a loss of precision present
If a token with more then 36 decimals appears, function will revert

Issues details

The price calculation is as follows:

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

    /// @notice Returns the price of the pool to 18 decimals of accuracy.
    /// @return price The price of the pool.
    function price() public view returns (uint256) {
        // ensure that the exponent is always to 18 decimals of accuracy
        uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());
        return (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;
    }

For 1, 2 and 3:

Calculating actual buying price, while keeping xy=k invariant is shown in buyQuote in PrivatePool

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

    // calculate the input amount based on *xy=k* invariant and round up by 1 wei
    uint256 inputAmount =
        FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

with outputAmount being the number of NFTs to be bought, multiplied by 1e18 and rounded up.

Since price() determines the price of a pool NFT, scaled to 18 decimals, a partial implementation would actually be similar to buyQuote:

    uint256 inputAmount = FixedPointMathLib.mulDivUp(1e18, virtualBaseTokenReserves, (virtualNftReserves - 1e18));

skipping the roundup and decimal adjustments for clarity we see that the founction sould be:

price = 1e18 * virtualBaseTokenReserves / (virtualNftReserves - 1e18) instead of (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;

This calculation of price will not reflect an actual price, as the buy will use buyQuote that maintains the xy=k invariant, while this does not.

It is also a very bad practice to not include fees in the price, users are misslead that the NFTs will cost less.

For point 4, the end division by virtualNftReserves (which is 1e18 multiplied) results in a lack of precision.

For point 5, because of the following subtraction, any token with more then 36 decimals will revert when price will be requested

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

        uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());

The protocol also clearly indicates to not call functions directly, but to use wrappers that use slippage (which is calculated using price()) in order to avoid losing funds on buy and change:

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L204-L211 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L293-L301

    /// @dev DO NOT call this function directly unless you know what you are doing. Instead, use a wrapper contract that
    /// will check the min output amount and revert if the slippage is too high.

Proof of Concept

Add the following test in Quotes.t.sol

    function test_price_ReturnsPriceTo18DecimalsIfERC20_fails() public {
        // arrange
        stdstore.target(address(privatePool)).sig(privatePool.baseToken.selector).checked_write(address(shibaInu));

        uint256 tokenDecimals = shibaInu.decimals(); // 6 decimals
        uint256 PRECISION = 1e18; // used to not lose tokens on rounding error
        uint256 price;

        (uint256 tokensFor1Nft, , ) = privatePool.buyQuote(1e18);
        if (tokenDecimals > 18) {
            price = tokensFor1Nft * PRECISION / 10 ** (tokenDecimals - 18) / PRECISION;
        }
        else if (18 > tokenDecimals) {
            price = tokensFor1Nft * 10 ** (18 - tokenDecimals); 
        }

        // act
        uint256 returnedPrice = privatePool.price();

        // assert
        assertEq(returnedPrice, price, "Should have returned price to 18 decimals");
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

Resolve the indicated issues, it would be simplest to modify the price function to work similar to buyQuote. Regardless of implementation it must include fees, expecially since it is used for slippage control.