c4-bot-8 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

It is possible to brick PanopticFactory when it is deployed in a "permissionless" way if the factory was left unitialized. This issue could stay unnoticed for a while as the factory will work fine if left unitialized, and the owner set to address zero is a valid use case.

Given that they are now the owners no one but them will be able to call deployNewPool, impacting the availability of the protocol for other users. They will also monopolize the donorNFT as they will be the only user able to invoke issueNFT if they deploy a new pool.

Severity Considerations

I personally consider frontrunning the initialize function as low severity, as the protocol can simply deploy a new contract. However, this is not the issue I'm describing here.

The problem is that the factory will work fine even if left uninitialized and an escalation can occur after some time has already passed and users have already started using this contract. Given the implications regarding the NFTs issued and the pool tracking inside the factory, I consider this role escalation to be of Medium severity.

Proof of Concept

A zero-address owner is a valid use case as the factory can be used in a permissionless way when calling deployNewPool:

// restrict pool deployment to owner if contract has not been made permissionless
address _owner = s_owner;
if (_owner != address(0) && _owner != msg.sender) revert Errors.NotOwner();

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

By default, the owner will be the zero address unless initialize is called. Suppose that the function isn't called after deployment, and it goes unnoticed if the intention is to deploy a permissionless factory; it will work fine, and users will start using it and deploying new pools, while the factory will continue to issue donorNFTs and keep track of each pool in s_getPanopticPool.

After a while, an attacker may realize that the factory was never initialized. At that point, they can simply call initialize to take ownership of the contract:

function initialize(address _owner) public {
    if (!s_initialized) {
        s_owner = _owner;
        s_initialized = true;
    }
}

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider making deployNewPool fail if s_initialized is false. This way, the correct method to initialize the contract would be to call initialize with the zero address if the intention is to use a permissionless factory, preventing a takeover of the contract after the users start using it.

Assessed type

Invalid Validation

c4-judge commented 5 months ago

Picodes marked the issue as primary issue

Picodes commented 5 months ago

This issues is of higher quality than most of its duplicate who mainly insist on the fact that initialize is permissionless

dyedm1 commented 5 months ago

The intended deployment path is for the factory to be initialized in the same transaction it is deployed. We have a generic CREATE2 factory that deployes bytecode and passes through calldata once that contract is deployed.