code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L478-L507 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L219-L248

Vulnerability details

Impact

A wrapper contract is suggested for users prior to calling the function "deposit", but the wrapper contract doesn't actually handle ERC20 deposits to the private pools. Therefore users are still enforced to use the not safely deposit function in the pool.

Proof of Concept

The natspec on the deposit function in the private pool clearly states, that a user should only call this function unless he knows what's his doing. And the dev suggest that a wrapper contract is used instead which will check that the current price is within the desired bounds inputted by the user.

/// @dev DO NOT call this function directly unless you know what you are doing. Instead, use a wrapper contract that
/// will check the current price is within the desired bounds.

The problem is that the wrapper contract "EthRouter" only supports the native ETH for the base token. Therefore users can't actually use the safe wrapper contract which checks the desired bounds as depositing ERC20 tokens to the private pool doesn't work. As a result the only way for users to deposit ERC20 would be to call the not safely deposit function in the private pool.

    function deposit(
        address payable privatePool,
        address nft,
        uint256[] calldata tokenIds,
        uint256 minPrice,
        uint256 maxPrice,
        uint256 deadline
    ) public payable {
        // check deadline has not passed (if any)
        if (block.timestamp > deadline && deadline != 0) {
            revert DeadlinePassed();
        }

        // check pool price is in between min and max
        uint256 price = PrivatePool(privatePool).price();
        if (price > maxPrice || price < minPrice) {
            revert PriceOutOfRange();
        }

        // transfer NFTs from caller
        for (uint256 i = 0; i < tokenIds.length; i++) {
            ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);
        }

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

        // execute deposit
        PrivatePool(privatePool).deposit{value: msg.value}(tokenIds, msg.value);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a check ensuring that the current price is between the desired bounds as the wrapper contract doesn't handle deposits with ERC20 tokens.

    function deposit(
        uint256[] calldata tokenIds,
        uint256 baseTokenAmount
        uint256 minPrice,
        uint256 maxPrice
    ) public payable {
        // ~~~ Checks ~~~ //

        // ensure the caller sent a valid amount of ETH if base token is ETH or that the caller sent 0 ETH if base token
        // is not ETH
        if (
            (baseToken == address(0) && msg.value != baseTokenAmount) ||
            (msg.value > 0 && baseToken != address(0))
        ) {
            revert InvalidEthAmount();
        }

        uint256 price = price();
        if (price > maxPrice || price < minPrice) {
            revert PriceOutOfRange();
        }

        // ~~~ Interactions ~~~ //

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

        if (baseToken != address(0)) {
            // transfer the base tokens from the caller
            ERC20(baseToken).safeTransferFrom(
                msg.sender,
                address(this),
                baseTokenAmount
            );
        }

        // emit the deposit event
        emit Deposit(tokenIds, baseTokenAmount);
    }