Caviar Private Pools - GT_Blockchain's results

Judge has assessed an item in Issue #680 as 2 risk. The relevant finding follows:

D. FlashFee is a flat amount, meaning the cost to flash loan an NFT at floor price is the same as one worth 5x the floor price. FlashFee should instead be based on the weigh of the NFT being flashloaned, with more expensive NFTs charging a higher fee.

Judge has assessed an item in Issue #680 as 2 risk. The relevant finding follows:

A. Fee structure is inconsistent for buys/changes. In buy() and sell() function protocolFee is based on a percentage of the input/output amount but in change() the fee is based on the pool fee not the input amount. It should be based on the input/output amount for consistency

Lines of code

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

Vulnerability details

Impact

When a user is creating a pool they can specify a bytes32 salt that is passed to cloneDeterministic, which determines the address of the new contract. The salt can be copied by a front-running bot and submitted before the user. This will result in the pool being deployed at the same address but with either the same or different parameters. Since this contract at an address already exists the create2 will revert as well as the transaction. This can prevent a user from creating a pool successfully. If the MEV bot also copies the rest of the calldata parameters then the pool will also be created but ownership assigned to the bot not Alice like intended.

Proof of Concept

salt should not be decided by the user. privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

Tools Used

Visual Studio Code.

Recommended Mitigation Steps

Solution: The salt should be determined on chain as the keccak256 of the all the parameters and the msg.sender. Since the chance of a hash collision on salt would be virtually zero, the chance of the cloneDeterministic reverting are also extremely low. This ensures that the pool is always created as expected with the users' parameters and them as the owner.