code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L230 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L323 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L689-L724

Vulnerability details

virtualBaseTokenReserves is recalculated every time a buy or sell operation is performed. The calculation is done incorrectly, so the next time a sale is made the price will be updated incorrectly.

Impact

buy and sell operations will be performed with incorrect prices, leading to private pools selling NFTs for less than expected.

Proof of Concept

virtualBaseTokenReserves should be calculated regarding the outcoming or incoming assets. That's not the case in neither buy, or sell, as they are using feeAmount instead of royaltyFee, and also omiting if payRoyalties is enabled.

virtualBaseTokenReserves in buy function:

230:    virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);

Link to code

virtualBaseTokenReserves in sell function:

323:    virtualBaseTokenReserves -= uint128(netOutputAmount + protocolFeeAmount + feeAmount);

Link to code

In neither case feeAmount should be used, but instead royaltyFee (buy) or royaltyFeeAmount (sell), as this is the value that will be transfered, and only if payRoyalties is enabled.

The price of the NFTs rely on buyQuote and sellQuote, which are directly dependant of virtualBaseTokenReserves:

    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
        uint256 inputAmount =
@>          FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

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

    function sellQuote(uint256 inputAmount)
        public
        view
        returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
    {
        // calculate the output amount based on xy=k invariant
@>      uint256 outputAmount = inputAmount * virtualBaseTokenReserves / (virtualNftReserves + inputAmount);

        protocolFeeAmount = outputAmount * Factory(factory).protocolFeeRate() / 10_000;
        feeAmount = outputAmount * feeRate / 10_000;
        netOutputAmount = outputAmount - feeAmount - protocolFeeAmount;
    }

Link to code

Tools Used

Manual review

Recommended Mitigation Steps

Update the virtualBaseTokenReserves on buy and sell considering royaltyFeeAmount and royaltyFee.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

outdoteth marked the issue as sponsor disputed

outdoteth commented 1 year ago

No exploit is given here. The reserves are updated in such a way that the xyk invariant holds. That is intentional.

GalloDaSballo commented 1 year ago

X * Y Math is applied Total is computed Invariants are updated Royalties are applied Transfers are done

I think the finding is invalid but will double check

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

0xJuancito commented 1 year ago

I'd like to provide some specific example on what the issue addresses as an incorrect calculation.

I'll show one example for buy() and ERC20 tokens. I'll try to point how virtualBaseTokenReserves differs from the result of the actual transfers when payRoyalties is false.

The same applies for sell(). The proof would be slightly different, but the root cause is the same.

Proof of Concept

virtualBaseTokenReserves will always be calculated considering feeAmount:

230:    virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);

Link to code

The corresponding transfers will only be for netInputAmount and protocolFeeAmount when payRoyalties == false.

256:    ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);
259:    if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);

Link to code

If payRoyalties == false, ERC20(baseToken).safeTransfer(recipient, royaltyFee); won't be called because of the if statement. So, it will never transfer any amount related to the feeAmount that was previously considered.

        if (payRoyalties) {
            for (uint256 i = 0; i < tokenIds.length; i++) {
                // get the royalty fee for the NFT
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee); // @audit this won't be called when `payRoyalties == false`
                    } else {
                        recipient.safeTransferETH(royaltyFee); // @audit this won't be called when `payRoyalties == false`
                    }
                }
            }
        }

Link to code

Point 1

So, for each buy or sell operation, it will miscalculate virtualBaseTokenReserves with an error of feeAmount, as its value won't match the results of the actual transfers.

Point 2

In addition, if payRoyalties == true, royaltyFee should be used to calculate virtualBaseTokenReserves instead of feeAmount, as that is the amount that will be transferred.

GalloDaSballo commented 1 year ago

I'd like to provide some specific example on what the issue addresses as an incorrect calculation.

I'll show one example for buy() and ERC20 tokens. I'll try to point how virtualBaseTokenReserves differs from the result of the actual transfers when payRoyalties is false.

The same applies for sell(). The proof would be slightly different, but the root cause is the same.

Proof of Concept

virtualBaseTokenReserves will always be calculated considering feeAmount:
230:    virtualBaseTokenReserves += uint128(netInputAmount - feeAmount - protocolFeeAmount);
Link to code

The corresponding transfers will only be for netInputAmount and protocolFeeAmount when payRoyalties == false.
256:    ERC20(baseToken).safeTransferFrom(msg.sender, address(this), netInputAmount);
259:    if (protocolFeeAmount > 0) ERC20(baseToken).safeTransfer(address(factory), protocolFeeAmount);
Link to code

If payRoyalties == false, ERC20(baseToken).safeTransfer(recipient, royaltyFee); won't be called because of the if statement. So, it will never transfer any amount related to the feeAmount that was previously considered.
        if (payRoyalties) {
            for (uint256 i = 0; i < tokenIds.length; i++) {
                // get the royalty fee for the NFT
                (uint256 royaltyFee, address recipient) = _getRoyalty(tokenIds[i], salePrice);

                // transfer the royalty fee to the recipient if it's greater than 0
                if (royaltyFee > 0 && recipient != address(0)) {
                    if (baseToken != address(0)) {
                        ERC20(baseToken).safeTransfer(recipient, royaltyFee); // @audit this won't be called when `payRoyalties == false`
                    } else {
                        recipient.safeTransferETH(royaltyFee); // @audit this won't be called when `payRoyalties == false`
                    }
                }
            }
        }
Link to code

Point 1

So, for each buy or sell operation, it will miscalculate virtualBaseTokenReserves with an error of feeAmount, as its value won't match the results of the actual transfers.

Point 2

In addition, if payRoyalties == true, royaltyFee should be used to calculate virtualBaseTokenReserves instead of feeAmount, as that is the amount that will be transferred.

Thank you for taking the time to clarify your report @0xJuancito

I don't quite get what you mean, both calculations are irrespective of Royalties, meaning they are idempotent and will result to:

Same math for storage values whether payRoyaties is true or false
Different value transferred (price + royalty true) This seems to be the intended behaviour

Can you please clarify which invariant is broken?

0xJuancito commented 1 year ago

Thanks for reviewing the comment.

My assumption is that the change in the tokens balance should be the same as the change in the virtual reserves after a buy operation.

If that should not hold, please disregard my comment. I appreciate your time invested.

In case the assumption is correct, here is a test that should pass, but fails.

The virtual reserves is always subtracting feeAmount, but it that does not perform any tokens transfer for that amount in this case.

Add to test/Buy.t.sol and run forge test -m "test_BuyBalance":

    function test_BuyBalance_WithProtocolFee() public {
        // based on `test_PaysProtocolFeeWithBaseToken()`

        // arrange
        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            address(shibaInu),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            4000, // @audit set `feeRate` to 4%
            merkleRoot,
            true,
            false // @audit `payRoyalties`
        );
        factory.setProtocolFeeRate(1000); // set to 1%

        // set up nfts
        for (uint256 i = 10; i < 13; i++) {
            tokenIds.push(i);
            milady.mint(address(privatePool), i);
        }

        // set up tokens
        (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount) =
            privatePool.buyQuote(tokenIds.length * 1e18);
        deal(address(shibaInu), address(this), netInputAmount);
        shibaInu.approve(address(privatePool), netInputAmount);

        // Track virtual reserves and token balance before `buy`
        uint256 shibaInuBalanceBefore = shibaInu.balanceOf(address(privatePool));
        uint256 virtualReservesBefore = privatePool.virtualBaseTokenReserves();

        // act
        privatePool.buy(tokenIds, tokenWeights, proofs);

        // Track virtual reserves and token balance after `buy`
        uint256 shibaInuBalanceAfter = shibaInu.balanceOf(address(privatePool));
        uint256 virtualReservesAfter = privatePool.virtualBaseTokenReserves();

        uint256 shibaInuBalanceDiff = shibaInuBalanceAfter - shibaInuBalanceBefore;
        uint256 virtualReservesDiff = virtualReservesAfter - virtualReservesBefore;

        // @audit This will fail
        // It is not true when `feeRate != 0` as in this case
        assertEq(shibaInuBalanceDiff, virtualReservesDiff);
    }

outdoteth commented 1 year ago

the change in the tokens balance should be the same as the change in the virtual reserves

That assumption is incorrect. The change in virtual reserves should be independent of protocolFees, royalties, and pool fees.

code-423n4 / 2023-04-caviar-findings

Incorrect calculation of virtualBaseTokenReserves leads to incorrect pricing of NFTs #964

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Proof of Concept

Proof of Concept