DoS on buy() and change() in EthRouter.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L99-L144
https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L254-L293

Vulnerability details

Impact

Calling buy() or change() from EthRouter.sol could be denied if the transaction is frontrun such that one of the NFTs in the pool is bought or changed. This could happen when a nasty actor deliberately watches the mempool and frontruns a large transaction that involves multiple pools. It could also happen in a high volume trade where the transaction is inadvertently preceded by another user.

Proof of Concept

Here is a typical scenario:

1. Bob wants to buy token Agreements & Disclosures #1, QA Report #2, and Gas Optimizations #3. Token Agreements & Disclosures #1 belongs to pool A. Tokens QA Report #2, and Gas Optimizations #3 belong to pool B.
2. Bob submits an array of buys to the EthRouter to execute a buy from both pool A and pool B in one transaction.
3. Alex, upon seeing this in the mempool, frontruns Bob's transaction to buy token Gas Optimizations #3 from pool B.
4. Bob's transaction is denied because buy() in the PrivatePool where the NFT has been bought by Alex reverts when it is attempting to transfer the NFT to the caller, i.e. EthRouter:

File: PrivatePool.sol#L239-L240

            // transfer the NFT to the caller
            ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

Similarly, calling change() could also end up reverting in the following code lines if one of the NFTs no longer sits in the pool.

File: PrivatePool.sol#L445-L448

         // transfer the output nfts to the caller
        for (uint256 i = 0; i < outputTokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(address(this), msg.sender, outputTokenIds[i]);
        }

Tools Used

Manual

Recommended Mitigation Steps

Consider implementing try catch in buy() and change() of EthRouter that will gracefully fail but not revert in situations like these and continue to the next iteration. That way, the requests get partially filled to constitute a win-win situation to every party.

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #280

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)