Caviar Private Pools - hihen's results

Lines of code

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

Vulnerability details

Impact

Malicious users can gain ownership of a PrivatePool and steal all assets deposited in the pool by front-running Factory.create().

Proof of Concept

Suppose a user creates his own PrivatePool and deposits assets using the following script:

1. Call Factory.predictPoolDeploymentAddress(salt_u) to get the address of the pool to create, assuming it is pool_u.
2. Send a transaction Factory.create(..., salt_u, ...) to deploy pool_u.
3. Send a transaction pool_u.deposit() to deposit NFT and base tokens.

An attacker can call Factory.create(..., salt_u, ...) before step 2 above to preemptively deploy pool_u, resulting in

• The attacker becomes the owner of pool_u.
• The user's tx - Factory.create() in step 2 will fail.
• The user's tx -pool_u.deposit() in step 3 will succeed.
• The attacker can then call pool_u.withdraw() to take both the NFTs and base tokens deposited by the user.

Tools Used

VS Code

Recommended Mitigation Steps

We can prevent this attack by including the creator address in the pool contract address generation algorithm.

Here is an implementation to fix this:

diff --git a/src/Factory.sol b/src/Factory.sol
index 09cbb4e..6e21ba6 100644
--- a/src/Factory.sol
+++ b/src/Factory.sol
@@ -89,7 +89,8 @@ contract Factory is ERC721, Owned {
         }

         // deploy a minimal proxy clone of the private pool implementation
-        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));
+        bytes32 safeSalt = keccak256(abi.encodePacked(_salt, msg.sender));
+        privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(safeSalt)));

         // mint the nft to the caller
         _safeMint(msg.sender, uint256(uint160(address(privatePool))));
@@ -165,7 +166,8 @@ contract Factory is ERC721, Owned {
     /// @notice Predicts the deployment address of a new private pool.
     /// @param salt The salt that will used on deployment.
     /// @return predictedAddress The predicted deployment address of the private pool.
-    function predictPoolDeploymentAddress(bytes32 salt) public view returns (address predictedAddress) {
-        predictedAddress = privatePoolImplementation.predictDeterministicAddress(salt, address(this));
+    function predictPoolDeploymentAddress(bytes32 salt, address creator) public view returns (address predictedAddress) {
+        bytes32 safeSalt = keccak256(abi.encodePacked(salt, creator));
+        predictedAddress = privatePoolImplementation.predictDeterministicAddress(safeSalt, address(this));
     }
 }

Lines of code

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

Vulnerability details

Impact

Any Factory NFTs deposited into a Factory-PrivatePool can have all assets in the corresponding PrivatePools stolen by malicious users.

Proof of Concept

Suppose there are two PrivatePools p1 and p2, p1.nft = address(Factory), and uint256(p1) and uint256(p2) are deposited into p1. Malicious users can use flashloan() to steal all the base tokens in p1 and p2:

1. Call p1.flashloan() to borrow the Factory NFT - uint256(p1) from p1.
2. In the flashloan callback, call p1.withdraw() to withdraw all the base tokens and the factory NFT - uint256(p2) from p1.
3. Return uint256(p1) to p1.

Suppose there are two PrivatePools p1 and p2, p1.nft = address(Factory), and uint256(p2) is deposited into p1. Malicious users can use flashloan() to steal all the base tokens and NFTs in p2:

1. Call p1.flashloan() to borrow factory NFT - uint256(p2) from p1.
2. In the flashloan callback, call p2.withdraw() to steal all the base tokens and NFTs in p2.
3. Return uint256(p2) to p1.

In addition, malicious users can also steal assets in p2 by:

1. p1.buy(uint256(p2))
2. p2.withdraw(...)
3. p1.sell(uint256(p2).

Tools Used

VS Code

Recommended Mitigation Steps

To prevent users from misusing the protocol and causing financial losses, we should prohibit the creation of PrivatePools with the Factory NFT:

diff --git a/src/PrivatePool.sol b/src/PrivatePool.sol
index 75991e1..14ec386 100644
--- a/src/PrivatePool.sol
+++ b/src/PrivatePool.sol
@@ -171,6 +171,8 @@ contract PrivatePool is ERC721TokenReceiver {
         // check that the fee rate is less than 50%
         if (_feeRate > 5_000) revert FeeRateTooHigh();

+        require(_nft != factory, "Unsupported NFT");
+
         // set the state variables
         baseToken = _baseToken;
         nft = _nft;