Caviar Private Pools - carlitox477's results

Lines of code

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

Vulnerability details

Factory.create(address,address,uint128,uint128,uint56,uint16,bytes32,bool,bool,bytes32,uint256[] memory,uint256): DoS attack on new private pools creation by sending the same salt

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

Description

As the _salt parameter is included in the transaction, it is possible for a bot to frontrun the creation of a new pool with the same salt, resulting in the reversal of the intended transaction.

Impact

DoS attack on new private pools creation by sending the same salt.

POC

As per the documentation provided by OpenZeppelin, the address of a newly created contract using the create2 bytecode can be computed by the following formula: new_address = hash(0xFF, sender, salt, bytecode).

The bytecode is the one used by LibClone when cloneDeterministic is call:

// LibClone.cloneDeterministic(address,bytes memory, bytes32)
// Create the instance.
instance := create2(0, sub(data, 0x4c), add(extraLength, 0x6c), salt)

This line of code has 3 possible outputs:

• Address zero: This means that the address has code equivalent to the bytecode sent
• Error: In case the address already has code. This can happen in case of hash collision, however the chance is remote.
• Non zero address: There was no hash collision and new code has been deployed.

cloneDeterministic is call by Factory in function create:

privatePool = PrivatePool(
    payable(privatePoolImplementation.cloneDeterministic(_salt))
);

From quoted OpenZeppelin article, it can be deducted that the deterministic address privatePool relays on 3 values:

• sender: The sender is always the Factory contract since it is the one that calls the cloneDeterministic function.cloneDeterministic.
• bytecode: It corresponds to privatePoolImplementation's code.
• _salt: provided by the caller of the transaction.

Given that sender and bytecode cannot be manipulated by Factory.create(address,address,uint128,uint128,uint56,uint16,bytes32,bool,bool,bytes32,uint256[] memory,uint256) msg.sender, but _salt can be set at willing, nothing forbid frontrunning a transaction by sending the same _salt, forcing the frontrun transaction to revert given LibCode code:

// LibClone.cloneDeterministic(address,bytes memory, bytes32)
// If `instance` is zero, revert.
    if iszero(instance) {
        // Store the function selector of `DeploymentFailed()`.
        mstore(0x00, 0x30116425)
        // Revert with (offset, size).
        revert(0x1c, 0x04)
    }

To summarize, an example of a DOS attack can be:

1. Alice want to create a pool for her NFT and call function Factory.create(address,address,uint128,uint128,uint56,uint16,bytes32,bool,bool,bytes32,uint256[] memory,uint256) with _salt = bytes32(1), and send her transaction
2. Bob sees Alice transaction and decide to frontrun her by sending a transaction which calls Factory.create(address,address,uint128,uint128,uint56,uint16,bytes32,bool,bool,bytes32,uint256[] memory,uint256) with _salt = bytes32(1) (the rest of the values can be different).
3. Alice transaction reverts with error DeploymentFailed()

Mitigation steps

To mitigate the potential risk of a hash collision that could trigger a DeploymentFailed() error and the possibility of front-running vector attacks, the following measures are recommended:

1. Add a new uint256 storage variable to serve as a salt value for each new private pool that is created. After each new private pool is created, this value should be incremented.
2. Implement a new function that allows authorized addresses to increase the salt value.

By adding a unique salt value for each new private pool created, the risk of hash collisions can be greatly reduced, and by providing a function to increase the salt value, authorized users can ensure that the salt value is changed frequently enough to further reduce the risk of front-running vector attacks.

All this means:

    // Factory.sol

+   // New storage value
+   uint256 _salt;
    //...
    constructor() ERC721("Caviar Private Pools", "POOL") Owned(msg.sender) {
+       _salt = 1;
    }

    //...
+   function increaseSalt() external onlyAuthToIncreaseSalt{
+       salt++;
+   }

    //..
    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
    ) public payable returns (PrivatePool privatePool) {
        // check that the msg.value is equal to the base token amount if the base token is ETH or the msg.value is equal
        // to zero if the base token is not ETH
        // require((_baseToken == address(0) && msg.value == baseTokenAmount) || (_baseToken != address(0) && msg.value == 0))
        if (
            (_baseToken == address(0) && msg.value != baseTokenAmount) ||
            (_baseToken != address(0) && msg.value > 0)
        ) {
            revert PrivatePool.InvalidEthAmount();
        }

        // deploy a minimal proxy clone of the private pool implementation
        privatePool = PrivatePool(
-           payable(privatePoolImplementation.cloneDeterministic(_salt))
+           payable(privatePoolImplementation.cloneDeterministic(bytes32(_salt)))
        //..
+       _salt++
    }