Caviar Private Pools - teddav's results

Lines of code

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

Vulnerability details

Royalties are computed 2 times: once before the transfer from the user, and again after.

This is a problem: the owner of the NFT contract can basically buy NFTs for free.

The only limitation is this line:

if (royaltyFee > salePrice) revert InvalidRoyaltyFee();

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

So you can only get the NFT for free, but you can’t empty the pool directly.

Flow of attack:

• set royalties to the minimum: 1 wei
• call buy() with all the NFTs in the pool, send a bit of extra ETH to make sure you’re reimbursed
• when msg.sender is called (with msg.sender.safeTransferETH): set royalties to the maximum: salePrice - 1 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L268 Note: this works if baseToken is ETH or if it’s an ERC777, in that case the exploiter’s contract will be called on this line: https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L259

Recommended Mitigation Steps

Don’t use 2 steps to compute and pay royalties, do everything in 1 loop.

Lines of code

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

Vulnerability details

It might not be an issue since it seems like this is the way it’s supposed to work. And as long as the user is aware of it, I guess it’s ok…

But owner of the PrivatePool can basically do whatever they want.

Multiple onlyOwner functions allow to manipulate the pool: withdraw, setVirtualReserves for example.

And execute() allows the owner to do whatever he wants with the pool