code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L694-L706 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211-L289 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L301-L373 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L385-L452

Vulnerability details

Impact

If an NFT fraction has a weight greater than the virtual liquidity, it may become locked until the pool owner increases the virtual reserves to a level that accommodates the weight.

Proof of Concept

Assuming that the virtual liquidity in the pool is as follows:

virtualBaseTokenReserves = 100e18

virtualNftReserves = 5e18

Moreover, in this scenario, the actual reserves match the virtual reserves.

Assuming that Bob chooses to exchange his NFT fraction with a weighting of 2 for another NFT fraction in the same pool with a weighting of 1, there will be no immediate abnormality. However, the change function will fail to record the alteration in reserves.

PrivatePool.sol

function change(
        uint256[] memory inputTokenIds,
        uint256[] memory inputTokenWeights,
        MerkleMultiProof memory inputProof,
        IStolenNftOracle.Message[] memory stolenNftProofs,
        uint256[] memory outputTokenIds,
        uint256[] memory outputTokenWeights,
        MerkleMultiProof memory outputProof
    ) public payable returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // ~~~ Checks ~~~ //

        // check that the caller sent 0 ETH if base token is not ETH
        if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();

        // check that NFTs are not stolen
        if (useStolenNftOracle) {
            IStolenNftOracle(stolenNftOracle).validateTokensAreNotStolen(nft, inputTokenIds, stolenNftProofs);
        }

        // fix stack too deep
        {
            // calculate the sum of weights for the input nfts
            uint256 inputWeightSum = sumWeightsAndValidateProof(inputTokenIds, inputTokenWeights, inputProof);

            // calculate the sum of weights for the output nfts
            uint256 outputWeightSum = sumWeightsAndValidateProof(outputTokenIds, outputTokenWeights, outputProof);

            // @audit The balances don't get updated if the input NFT is of a higher weight than the output one

            // check that the input weights are greater than or equal to the output weights
            if (inputWeightSum < outputWeightSum) revert InsufficientInputWeight();

            // If the input weight sum is not equal to the 

            // calculate the fee amount
            (feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);
        }

        // ~~~ Interactions ~~~ //

        if (baseToken != address(0)) {
            // transfer the fee amount of base tokens from the caller
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), feeAmount);

            // if the protocol fee is non-zero then transfer the protocol fee to the factory
            if (protocolFeeAmount > 0) ERC20(baseToken).safeTransferFrom(msg.sender, factory, protocolFeeAmount);
        } else {
            // check that the caller sent enough ETH to cover the fee amount and protocol fee
            if (msg.value < feeAmount + protocolFeeAmount) revert InvalidEthAmount();

            // if the protocol fee is non-zero then transfer the protocol fee to the factory
            if (protocolFeeAmount > 0) factory.safeTransferETH(protocolFeeAmount);

            // refund any excess ETH to the caller
            if (msg.value > feeAmount + protocolFeeAmount) {
                msg.sender.safeTransferETH(msg.value - feeAmount - protocolFeeAmount);
            }
        }

        // transfer the input nfts from the caller
        for (uint256 i = 0; i < inputTokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), inputTokenIds[i]);
        }

        // transfer the output nfts to the caller
        for (uint256 i = 0; i < outputTokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(address(this), msg.sender, outputTokenIds[i]);
        }

        // emit the change event
        emit Change(inputTokenIds, inputTokenWeights, outputTokenIds, outputTokenWeights, feeAmount, protocolFeeAmount);
    }

Following the update, the virtual reserves are now lower than the actual reserves.

If two arbitrary buyers purchase four NFTs with a weight of 1 each from the pool, the NFT that was swapped for one with a lower weight will become stuck in the protocol until the pool owner re-balances the virtual reserves.

PrivatePool.sol

function buyQuote(uint256 outputAmount)
        public
        view
        returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the input amount based on xy=k invariant and round up by 1 wei

        // @audit fails if the outputAmount is bigger than the virtualNftReserves or equal to it | can't divide by 0 or arithmetic underflow
        uint256 inputAmount =
            FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

        protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = inputAmount * feeRate / 10_000;
        netInputAmount = inputAmount + feeAmount + protocolFeeAmount;
    }

If Alice attempts to purchase an NFT with the same or greater weight than the virtual reserves, one of two events may occur: a division by zero error in the case of equivalent amounts, or an arithmetic underflow in the case of an amount greater than the virtual reserves.

A Foundry PoC that demonstrates the scenario of division by zero:

    // @audit-info These are the default circumstances used by most of the tests:
    PrivatePool public privatePool;

    address baseToken = address(0);
    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100e18;
    uint128 virtualNftReserves = 5e18;
    uint16 feeRate = 0;
    uint56 changeFee = 0;
    bytes32 merkleRoot = bytes32(0);
    address owner = address(this);

    uint256[] tokenIds;
    uint256[] tokenWeights;
    PrivatePool.MerkleMultiProof proofs;

    function setUp() public {
        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            baseToken, nft, virtualBaseTokenReserves, virtualNftReserves, changeFee, feeRate, merkleRoot, true, false
        );

        // @audit-info Giving the pool 100 ether to trade with
        (bool success, ) = address(privatePool).call{value: 100 ether}("");
        require(success, "failed to send ether to private pool");

        for (uint256 i = 0; i < 5; i++) {
            milady.mint(address(privatePool), i + 1);
        }

        assertEq(milady.balanceOf(address(privatePool)), 5, "Didn't mint 5 NFTs for some reason.");
    }

    function test_failBecauseOfDivisionBy0() public {
        // @audit-info Trying to buy the 5 tokens present in the pool
        for (uint256 i = 0; i < 5; i++) {
            tokenIds.push(i + 1);
        }

        // @audit-info This should fail with a generic "revert" message:
        privatePool.buy{value: 100 ether}(tokenIds, tokenWeights, proofs);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Consider modifying the change function by updating the virtualBaseTokenReserves variable to prevent the virtual reserves from becoming smaller than the actual reserves. If the new weight is greater than the previous weight, increment the virtualBaseTokenReserves variable.

if (inputWeightSum > outputWeightSum) virtualBaseTokenReserves += inputWeightSum - outputWeightSum;

Consider modifying the formula used in the buyQuote function:

// from:
uint256 inputAmount =
            FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

// to:
uint256 inputAmount =
            FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves > outputAmount ? virtualNftReserves - outputAmount : 1));

It will not allow for division by 0 and will still keep the integrity of the logic in the formula. This resolves a rare scenario that occurs when attempting to purchase the entire reserve of virtual NFTs at once.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as high quality report

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #881

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)