Caviar Private Pools - sces60107's results

Lines of code

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

Vulnerability details

Impact

EthRouter calls ERC721(nft).setApprovalForAll in many functions. It gives the full approval of NFT to the pools. An attacker can make EthRouter give the full approval of NFTs to a malicious pool address by passing the fake pool address to those functions.

Proof of Concept

Many functions in EthRouter call ERC721(nft).setApprovalForAll to set the full approval of NFT to the pool. deposit() is one of them. https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L244

    function deposit(
        address payable privatePool,
        address nft,
        uint256[] calldata tokenIds,
        uint256 minPrice,
        uint256 maxPrice,
        uint256 deadline
    ) public payable {
        …

        // approve pair to transfer NFTs from router
        ERC721(nft).setApprovalForAll(privatePool, true);

        …
    }

An attacker can call EthRouter.deposit() with a malicious pool address and any NFT address. Then the malicious pool has the full control of those NFTs on EthRouter

Tools Used

Manual Review

Recommended Mitigation Steps

Do not use setApprovalForAll if the private pool address isn’t fully-trusted. Use approve for each NFT token.