Caviar Private Pools - hasmama's results

Lines of code

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

Vulnerability details

Impact

for example an attacker can call the create() function Adam NFT as input and these NFT will be transferred to the private pool without the Adam knowledge, the attacker can potentially steal NFT from Adam and deposit them into the private pool, result in Adam total loss of assets.

Proof of Concept

the function does not check factory to transfer the NFT that will be deposited to the pool here is the link where the vulnerability occure. https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c /src/Factory.sol#L71

for example let say jhon is an attacker and Adam is a victim, jhon create a new contract that inherits from Factory and implements a attack() function that takes the address of the Adam NFT and the uint256 salt as input.

	contract jhon is Factory {
	    function stillingadamnft(address adamNFT, uint256 salt) public {
	    }
	}

In the stillingadamnft() function, calculate the salt that will be used to put the minimal proxy clone of the PrivatePool contract. This salt needs to be unique to make ensure that the new private pool is a new contract.

	bytes32 salt = keccak256(abi.encodePacked(msg.sender, adamNFT, salt));

Call the create() function of the Factory contract, passing in the Adam NFT as one of the tokenIds and the calculated salt as the _salt parameter. Make sure to approve the Factory contract to transfer the Adam NFT before calling the create() function.

		function stillingadamnft(address adamNFT, uint256 salt) public {
		    
		    bytes32 poolSalt = keccak256(abi.encodePacked(msg.sender, adammNFT, salt));
		    
		    ERC721(adamNFT).approve(address(this), 1);

		    create(
			adamNFT,
			poolSalt,
			[1],
			0
		    );
		}

After the create() function returns, the new private pool contract will have been put with the Adam NFT as one of the deposited NFT, and the jhon will have control over the private pool. The Adam NFT will have been transferred to the private pool without the Adam knowledge.

Here is the modified create() function that includes the vulnerability:

		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,
		    uint256 baseTokenAmount
		) public payable returns (PrivatePool privatePool) {

		    if ((_baseToken == address(0) && msg.value != baseTokenAmount) || (_baseToken

Tools Used

VSCODE

Recommended Mitigation Steps

To mitigate this vulnerability, the Factory contract should check whether the caller has approved the factory to transfer the NFTs that will be deposited to the pool. This can be achieved by calling the ERC721.approve() function on the NFT contract with the Factory contract address and the token ID as inputs. The Factory contract should then check the approved status of the NFT before transferring it to the private pool. This will make ensure that only NFTs that have been approved by their owners are transferred to the private pool