M-12 mitigation error - M-12 is not fully mitigated because a private pool can still be created for a factory or vault-like NFT that is similar to but not Private Pool Factory NFT

[code423n4]

Lines of code

https://github.com/outdoteth/caviar-private-pools/blob/main/src/PrivatePool.sol#L163-L209

Vulnerability details

Impact

This mitigation for M-12: Prohibition to create private pools with the factory NFT adds the following PrivatePoolNftNotSupported error in the PrivatePool contract and updates the PrivatePool.initialize function to execute if (_nft == factory) revert PrivatePoolNftNotSupported(). This prevents the creation of private pools that uses factory as nft so no one can deposit or sell the Private Pool Factory NFT to another created private pool. Since malicious actors can no longer utilize functions like PrivatePool.flashLoan and PrivatePool.withdraw to transfer out the base tokens and NFTs owned by a Private Pool Factory NFT, the issue regarding the Private Pool Factory NFT in M-12: Prohibition to create private pools with the factory NFT is mitigated.

https://github.com/outdoteth/caviar-private-pools/pull/14/files#diff-6beea546a01e795e1365ca8a74b2bd952631f6cd228a5a869bfed82bf1f46a0fR82

error PrivatePoolNftNotSupported();

https://github.com/outdoteth/caviar-private-pools/pull/14/files#diff-6beea546a01e795e1365ca8a74b2bd952631f6cd228a5a869bfed82bf1f46a0fR160-R206

    function initialize(
        address _baseToken,
        address _nft,
        uint128 _virtualBaseTokenReserves,
        uint128 _virtualNftReserves,
        uint56 _changeFee,
        uint16 _feeRate,
        bytes32 _merkleRoot,
        bool _useStolenNftOracle,
        bool _payRoyalties
    ) public {
        ...

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

        ...
        nft = _nft;
        ...
    }

Yet, this issue still applies to other factory or vault-like NFTs, which are similar to the Private Pool Factory NFT and can control other assets. If a private pool is created with nft corresponding to such factory or vault-like NFT, users can still deposit or sell such NFT to that private pool. Then, a malicious actor can call the PrivatePool.flashLoan function to borrow such NFT and call the PrivatePool.withdraw function in the flashloan callback to withdraw all of the other assets controlled by such NFT. Afterwards, such NFT becomes worthless without controlling the other assets. If anyone still buys or borrows such NFT, it is a loss to that buyer or borrower; otherwise, it is a loss to the private pool owner if she or he is not the malicious actor.

Proof of Concept

The following steps can occur for the described scenario.

1. Alice creates a private pool for a factory NFT, which is similar to but not the Private Pool Factory NFT, that controls other assets.
2. Bob sells one of such factory NFT to Alice's private pool.
3. Using Alice's private pool, Charlie calls the PrivatePool.flashLoan function to borrow such NFT and calls the PrivatePool.withdraw function in the flashloan callback to withdraw all of the other assets controlled by such NFT. Then, such NFT, which no longer controls any of the other assets, is returned to Alice's private pool.
4. After some time, Bob calls the PrivatePool.buy function to buy back such NFT that he sold in step 2. However, because such NFT has become worthless, he essentially loses the other assets that were controlled by such NFT.

Tools Used

VSCode

Recommended Mitigation Steps

A blocklist, which should only be updatable by the trusted admin, can be added to include the factory or vault-like NFTs that are similar to but not the Private Pool Factory NFT. Then, in addition to executing if (_nft == factory) revert PrivatePoolNftNotSupported(), the PrivatePool.initialize function can be updated to also revert if _nft is found in this blocklist. Moreover, this protocol should clearly communicate with its users regarding this risk so they can be extra cautious when deciding on whether they should interact with a private pool that uses such factory or vault-like NFT that is to be added to this blocklist.

Assessed type

Other

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[outdoteth]

Technically true, but adding a blocklist adds a centralized dependency. The negatives of centralization outweigh the marginal benefit of fixing this imo.

[GalloDaSballo]

Given:

• The original finding
• The specific comment made as part of the original finding, which in my opinion makes the finding a known issue
• The fact that there is a list of specific types of tokens that should never be flashloaned, the security of which is not in-scope for the audit but would be for an audit of their own codebases / integrations with this codebase

Am considering a QA Low Severity

[rbserver]

Hi @GalloDaSballo, really appreciate you for judging the issues over the weekend! I understand your points on this and my other report (#26) and would respect your judgements. Thanks again for your work and time!

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Downgrading to QA considering the previous disclosure.

I highly recommend @outdoteth to warn via the UI around the risks of composability, but I believe that from C4's side, the finding was already shown in the previous contest

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Closing as OOS / Known