Mismatch of NFT addresses between EthRouter and PrivatePool can lead to NFT theft

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L49-L50 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L59-L60 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L70-L71 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L219-L222

Vulnerability details

Impact

The functions buy(), sell(), deposit(), and change() in EthRouter take the pool and NFT address as input. However, in reality, PrivatePool(pool).nft() may not match the NFT address passed in the transaction parameters:

struct Buy {
    address payable pool;
    address nft;  // pool.nft may be different!
...

struct Sell {
    address payable pool;
    address nft;  // pool.nft may be different!
...

struct Change {
    address payable pool;
    address nft;  // pool.nft may be different!
...

function deposit(
    address payable privatePool,
    address nft,  // privatePool.nft may be different!
...

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L48-L50

This can result in ordinary users losing their NFTs.

Proof of Concept

Consider a scenario where there are two private pools:

• POOL_RICH, which trades an expensive NFT collection with address 0xRICH.
• POOL_POOR, which trades a cheap NFT collection with address 0xPOOR.

Both collections have an NFT with id=123, and in both pools, the merkleRoot is equal to zero (i.e., the weights of all NFTs are considered equal and the Merkle tree is not used):

function sumWeightsAndValidateProof
    ...
    // if the merkle root is not set then set the weight of each nft to be 1e18
    if (merkleRoot == bytes32(0)) {
        return tokenIds.length * 1e18;
    }

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

A trader wants to buy an expensive NFT from the 0xRICH collection with id=123 and calls the EthRouter.buy() function, but mistakenly passes the following structure:

Buy(
    pool: POOL_RICH,  // pool with the 0xRICH collection
    nft: 0xPOOR,      // accidentally passed the wrong collection
    tokenIds: [123],
    ...
)

The EthRouter successfully spends part of the client's money and buys the expensive NFT from the 0xRICH collection in the POOL_RICH pool:

PrivatePool(buys[i].pool).buy{value: buys[i].baseTokenAmount}(
    buys[i].tokenIds, buys[i].tokenWeights, buys[i].proof
);

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L129-L132

In a normal situation, without the involvement of a hacker, this transaction will be reverted because EthRouter will attempt to transfer not the expensive NFT with id=123, but the NFT with the same ID from the cheaper 0xPOOR collection accidentally passed by the trader in the arguments (buys[i].nft = 0xPOOR):

function buy
    ...
    ERC721(
        buys[i].nft         // 0xPOOR is used instead of 0xRICH
    ).safeTransferFrom(
        address(this), 
        msg.sender, 
        buys[i].tokenIds[j]
    );

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L134-L137

But, in our case, the hacker sandwiches the victim's transaction and buys the NFT with id=123 from the 0xPOOR collection for cheap and transfers it to the EthRouter.

Thus, at the moment of the victim's transaction execution, the EthRouter already holds the NFT 0xPOOR with the same ID. Therefore, the transfer of the NFT in the victim's transaction is successful, and the victim receives a cheap NFT from the 0xPOOR collection in their wallet, while the expensive NFT 0xRICH remains on the EthRouter.

Immediately after the user's transaction, the hacker withdraws the expensive NFT from the EthRouter to their own wallet. The hacker can do this in many different ways.

The first way to withdraw the NFT involves using the same vulnerability related to the mismatch of NFT addresses. The hacker creates POOL_FAKE, which trades the 0xFAKE NFT for a fake tokens, and calls buy():

Buy(
    pool: POOL_FAKE,
    nft: 0xRICH,     // instead of 0xFAKE, the hacker specifies 0xRICH
    tokenIds: [123],

As a result of this operation, the hacker receives a super expensive NFT 0xRICH with ID=123, on which the victim spent a lot of money, and a fake NFT from the hacker remains on EthRouter.

There is another way to remove NFT from EthRouter - to use the infinite approves that EthRouter gives out. The hacker creates an empty hackerPool with the 0xRICH collection, and calls deposit() with an empty list of NFTs from the 0xRICH collection:

EthRouter.deposit(hackerPool, 0xRICH, [], ...)

In this case, EthRouter sets the hackerPool as an operator for the 0xRICH NFT:

ERC721(nft).setApprovalForAll(privatePool, true);

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

Then the hacker calls hackerPool.execute() with arguments that invoke IERC721(0xRICH).transferFrom(ethRouter, hackerAddress, 123).

Result. A realistic scenario is described in which the victim loses a super valuable NFT and instead receives an unnecessary cheap NFT, so the vulnerability is assigned a HIGH status.

Recommended Mitigation Steps

It is recommended to take the NFT collection address from the parameters of the pool itself or to verify that they match.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[outdoteth]

Requires the user to make a mistake in their inputs.

[c4-sponsor]

outdoteth marked the issue as disagree with severity

[c4-sponsor]

outdoteth marked the issue as sponsor confirmed

[outdoteth]

Fixed in: https://github.com/outdoteth/caviar-private-pools/pull/3

Proposed fix is to remove the nft field from the Buy, Sell, and Change structs and instead replace it with a direct call to PrivatePool or Pair to fetch the nft using the nft() method.

// fetch the nft address (PrivatePool and Pair both have an nft() method)
address nft = PrivatePool(buys[i].pool).nft();

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Downgrading to QA because the user would have to order the wrong NFT from the incorrect Pool

I believe the finding is worth fixing, am awarding it 3 bonus points

L + 3

[c4-judge]

GalloDaSballo marked the issue as grade-b