Prohibition to create private pools with the factory NFT

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L157 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L623 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L514

Vulnerability details

Impact

Any Factory NFTs deposited into a Factory-PrivatePool can have all assets in the corresponding PrivatePools stolen by malicious users.

Proof of Concept

Suppose there are two PrivatePools p1 and p2, p1.nft = address(Factory), and uint256(p1) and uint256(p2) are deposited into p1. Malicious users can use flashloan() to steal all the base tokens in p1 and p2:

1. Call p1.flashloan() to borrow the Factory NFT - uint256(p1) from p1.
2. In the flashloan callback, call p1.withdraw() to withdraw all the base tokens and the factory NFT - uint256(p2) from p1.
3. Return uint256(p1) to p1.

Suppose there are two PrivatePools p1 and p2, p1.nft = address(Factory), and uint256(p2) is deposited into p1. Malicious users can use flashloan() to steal all the base tokens and NFTs in p2:

1. Call p1.flashloan() to borrow factory NFT - uint256(p2) from p1.
2. In the flashloan callback, call p2.withdraw() to steal all the base tokens and NFTs in p2.
3. Return uint256(p2) to p1.

In addition, malicious users can also steal assets in p2 by:

1. p1.buy(uint256(p2))
2. p2.withdraw(...)
3. p1.sell(uint256(p2).

Tools Used

VS Code

Recommended Mitigation Steps

To prevent users from misusing the protocol and causing financial losses, we should prohibit the creation of PrivatePools with the Factory NFT:

diff --git a/src/PrivatePool.sol b/src/PrivatePool.sol
index 75991e1..14ec386 100644
--- a/src/PrivatePool.sol
+++ b/src/PrivatePool.sol
@@ -171,6 +171,8 @@ contract PrivatePool is ERC721TokenReceiver {
         // check that the fee rate is less than 50%
         if (_feeRate > 5_000) revert FeeRateTooHigh();

+        require(_nft != factory, "Unsupported NFT");
+
         // set the state variables
         baseToken = _baseToken;
         nft = _nft;

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

outdoteth marked the issue as sponsor confirmed

[outdoteth]

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

Proposed fix is to add a check that ensures that the nft which is used in the private pool is not a private pool NFT from the factory contract.

// check that the nft is not a private pool NFT
if (_nft == factory) revert PrivatePoolNftNotSupported();

[GalloDaSballo]

Judging severity on this finding is contingent on determining if using the factory as NFT is an external requirement or not

[GalloDaSballo]

Will seek advice from another Judge, but saying "if they do it, they lose everything" doesn't sound like an external requirement

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

After further thinking, I believe the finding has shown a vulnerability in how the pool may be used for composability.

I believe that similar "vault like" NFTs may be subject to the same risks, there's another contest happening at this time that may also be subject to the same risk

Those instances, would mostly be categorized as Medium Severity, because the implementations are not known

After discussing with additional judges, given that there are a category of NFTs that should not be Flashloaned (e.g. UniV3, Factory, other factories, etc..) believe it is most appropriate to judge the finding as Medium Severity, with the additional warning that similar "vault like" NFTs should also be examined with care.

The risk doesn't apply to ordinary collections

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)