Mitigation Confirmed for M-05

[code423n4]

Mitigation of M-05: Issue mitigated

Issue

M-05: EthRouter.sell, EthRouter.deposit, and EthRouter.change functions can be DOS'ed for some ERC721 tokens

Mitigation

https://github.com/outdoteth/caviar-private-pools/pull/7

Assessment of Mitigation

Issue mitigated

Comments

The mitigation adds the following EthRouter._approveNfts function, which executes if (ERC721(nft).isApprovedForAll(address(this), target)) return;. If the EthRouter contract has already approved target to spend any of the corresponding nft received by itself, calling the EthRouter._approveNfts function will not execute ERC721(nft).setApprovalForAll(target, true). This ensures that the EthRouter contract can only approve target to spend any of the corresponding nft received by itself once.

https://github.com/outdoteth/caviar-private-pools/pull/7/files#diff-b86dfa43afd0f9b51e32c536db6fa63577398b8cc9817b18be4f5863f8dbf227R324-R330

    function _approveNfts(address nft, address target) internal {
        // check if the router is already approved to transfer NFTs from the caller
        if (ERC721(nft).isApprovedForAll(address(this), target)) return;

        // approve the target to transfer NFTs from the router
        ERC721(nft).setApprovalForAll(target, true);
    }

Then, the following EthRouter.sell, EthRouter.deposit, and EthRouter.change functions are also updated to execute _approveNfts(nft, sells[i].pool), _approveNfts(nft, privatePool), and _approveNfts(nft, _change.pool) respectively instead of directly calling ERC721(nft).setApprovalForAll(...). These changes make sure that the EthRouter contract can only approve the corresponding pool to spend any of the corresponding ERC721 tokens received by itself once. Hence, the corresponding issue is mitigated.

https://github.com/outdoteth/caviar-private-pools/pull/7/files#diff-b86dfa43afd0f9b51e32c536db6fa63577398b8cc9817b18be4f5863f8dbf227R152-R212

    function sell(Sell[] calldata sells, uint256 minOutputAmount, uint256 deadline, bool payRoyalties) public {
        ...
        // loop through and execute the sells
        for (uint256 i = 0; i < sells.length; i++) {
            // fetch the nft address (PrivatePool and Pair both have an nft() method)
            address nft = PrivatePool(sells[i].pool).nft();
            ...

            // approve the pair to transfer NFTs from the router
            _approveNfts(nft, sells[i].pool);

            ...
        }
        ...
    }

https://github.com/outdoteth/caviar-private-pools/pull/7/files#diff-b86dfa43afd0f9b51e32c536db6fa63577398b8cc9817b18be4f5863f8dbf227R222-R251

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

        // approve the pair to transfer NFTs from the router
        _approveNfts(nft, privatePool);

        ...
    }

https://github.com/outdoteth/caviar-private-pools/pull/7/files#diff-b86dfa43afd0f9b51e32c536db6fa63577398b8cc9817b18be4f5863f8dbf227R257-R299

    function change(Change[] calldata changes, uint256 deadline) public payable {
        // check deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }
        // loop through and execute the changes
        for (uint256 i = 0; i < changes.length; i++) {
            Change memory _change = changes[i];
            // fetch the nft address (PrivatePool and Pair both have an nft() method)
            address nft = PrivatePool(_change.pool).nft();
            ...

            // approve the pair to transfer NFTs from the router
            _approveNfts(nft, _change.pool);

            ...
        }
        ...
    }

[c4-judge]

GalloDaSballo marked the issue as satisfactory