New pool can deploy failed due to front-running

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L210-L259

Vulnerability details

Impact

When deploy a new pool by PanopticFactory#deployNewPool, it call POOL_REFERENCE.cloneDeterministic(salt) to calculate the pool address, then start the pool and record the pool's address to s_getPanopticPool mapping. The POOL_REFERENCE.cloneDeterministic finally call OpenZeppelin libraryClones#cloneDeterministic to calculate and create the pool address based on create2 opcode.

function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
    /// @solidity memory-safe-assembly
    assembly {
        // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
        // of the `implementation` address with the bytecode before the address.
        mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
        // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
        mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
        instance := create2(0, 0x09, 0x37, salt)
    }
    require(instance != address(0), "ERC1167: create2 failed");
}

As we can see, implementation and salt can be seen in memory, malicious users can monitor the two parameters and run with OpenZeppelin Clones#cloneDeterministic function directly to create the address, such that the PanopticFactory#deployNewPool tx would be failed because the pool's address already exist in ethereum account's db, so the later action newPoolContract#startPool can not be executed.

Tools Used

vscode

Recommended Mitigation Steps

Consider predicting the pool's address and use abi.encode(nonce, salt) as salt before deploy the pool, if the pool's address existed, increase the nonce and then deploy the pool again.

Assessed type

DoS

[Picodes]

What about https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L220

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[yuliyu123]

Hi, this issue front-running is just call Clones.cloneDeterministic(POOL_REFERENCE, salt)
to front-run any coming PanopticFactory#deployNewPool txs in memory, steps as following:

1. malicious users monitor coming PanopticFactory#deployNewPool tx in mainnet memory pool, and parse the salt parameter.
2. malicious users call Clones.cloneDeterministic(POOL_REFERENCE, salt) with the same salt parameter, this has no any permission validation so anyone can call it successfully.
3. following PanopticFactory#deployNewPool tx is executed reverted because the newPoolContract address already existed in ethereum db, such that the user deployNewPool tx can't success.

This issue is simliar to https://www.trust-security.xyz/post/permission-denied. When a single tx has A,B,C actions, if A/B action can be front-running, the whole tx can be reverted, so the C action can't be finished. In this issue is that the later actions can't be executed successfully.

[rbserver]

Hi @Picodes, thanks for judging! Since I happened to view this issue, I would like to share the following test for it.

    function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
        /// @solidity memory-safe-assembly
        assembly {
            // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
            // of the `implementation` address with the bytecode before the address.
            mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
            // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
            mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
            instance := create2(0, 0x09, 0x37, salt)
        }
        require(instance != address(0), "ERC1167: create2 failed");
    }

    function test_cloneDeterministic() public {
        bytes32 salt = bytes32(uint96(block.timestamp) + (uint256(uint160(address(this))) << 96));

        vm.startPrank(address(123));
        address instance1 = cloneDeterministic(address(this), salt);
        vm.stopPrank();

        vm.startPrank(address(321));
        address instance2 = cloneDeterministic(address(this), salt);
        vm.stopPrank();

        assert(instance1 != instance2);
    }

This test will pass to demonstrate that when different msg.sender call the cloneDeterministic with the same implementation and salt, the deployed addresses are different so no address collision occurs. Because the msg.sender who wants to DOS the new pool's deployment and the msg.sender who calls the PanopticFactory.deployNewPool function would be different, no address collision would occur for the new pool. Hence, such DOS does not occur, and this issue would be invalid.

Along the same line, I wonder how #482, which is judged as a medium risk at this moment, would be possible. #482 mentions that the attacker would use CREATE2 to deploy the controlled wallet contract that collides the potential pool's address. If CREATE2 does take into account the address of the sender invoking the CREATE2 opcode, since the attacker and the deployer have different addresses, how can such collision occur to prove the possibility of #482?

[yuliyu123]

Hi @Picodes, thanks for judging! Since I happened to view this issue, I would like to share the following test for it.

    function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
        /// @solidity memory-safe-assembly
        assembly {
            // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
            // of the `implementation` address with the bytecode before the address.
            mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
            // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
            mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
            instance := create2(0, 0x09, 0x37, salt)
        }
        require(instance != address(0), "ERC1167: create2 failed");
    }

    function test_cloneDeterministic() public {
        bytes32 salt = bytes32(uint96(block.timestamp) + (uint256(uint160(address(this))) << 96));

        vm.startPrank(address(123));
        address instance1 = cloneDeterministic(address(this), salt);
        vm.stopPrank();

        vm.startPrank(address(321));
        address instance2 = cloneDeterministic(address(this), salt);
        vm.stopPrank();

        assert(instance1 != instance2);
    }

This test will pass to demonstrate that when different msg.sender call the cloneDeterministic with the same implementation and salt, the deployed addresses are different so no address collision occurs. Because the msg.sender who wants to DOS the new pool's deployment and the msg.sender who calls the PanopticFactory.deployNewPool function would be different, no address collision would occur for the new pool. Hence, such DOS does not occur, and this issue would be invalid.

Along the same line, I wonder how #482, which is judged as a medium risk at this moment, would be possible. #482 mentions that the attacker would use CREATE2 to deploy the controlled wallet contract that collides the potential pool's address. If CREATE2 does take into account the address of the sender invoking the CREATE2 opcode, since the attacker and the deployer have different addresses, how can such collision occur to prove the possibility of #482?

Thanks for your test case, you are right that different msg.sender can create different instance, this issue indeed an invalid issue.

[Picodes]

@rbserver thanks for your test!
Regarding #482, I am not sure to see the link with this. In #482 the attacker and the deployer are the same person, and the goal is to set up a "honey pot" using address collision. So the goal is to find a collision, knowing with which addresses you'll deploy both contracts in advance.

[stalinMacias]

@Picodes I know QA ended yesterday, but, I would like to share my thoughts about this, hope this can be considered

So, if the attacker and the deployer are the same person, does not that mean that such an attack is possible only after Panoptic fully decentralizes the creation of the pools? In the beginning, only the owner can deploy new pools.

If so, at that moment, PanopticPools could be deployed for worthlessTokens/worhtTokens , tokens with reentrancy, a number of different combinations that could one way or the other cause damage to the users.
Then, as @dyedm1 mentioned, before they list one of the pools that was created by anybody, the protocol will do due diligence to validate if the pool is good so users can deposit in it. I think #482 offers the protocol one extra check for their checklist, check if the Pool's address has granted allowance to an address other than the CollateralTrackers and the SFPM

• All in all, the computational cost to set up an attack like that one is quite big, and the fact that first of all, that pool needs to be listed on the PanopticPool site and also users deposits in such a pool, makes even wider the breach between the cost to perform the attack and actually get any benefit from it.

The report can be a perfect QA because the team now is aware of one extra check when they do their due diligence when validating if a pool is good to be listed for users to deposit in it

[Picodes]

@stalinMacias thanks for your comment. Indeed this is only for the permissionless setup. I still think this attack path is expensive but credible and the cost is likely to decrease in the future, could be done for interesting pools where the expected extractable value is high, and could easily have been missed by Panoptic and users without this report, so is worth Medium severity

[rbserver]

@Picodes, thanks for judging and the clarification!

@yuliyu123 and @stalinMacias , thanks for the discussion!