Zero Address at Constructor

[code423n4]

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyFactory.sol#L21-L22
https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernanceNFT.sol#L44-L46
https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/party/PartyGovernance.sol#L266-L268
https://github.com/PartyDAO/party-contracts-c4/blob/main/contracts/proposals/FractionalizeProposal.sol#L34-L36

Vulnerability details

Impact

Zero address check for globals and vaultFactory should be implemented when deploying the contract. This is because there is no setter functions catered for the associated immutable variables. In the event a mistake was done, not only that all calls associated with them would be non-functional, the contract(s) would also have to be redeployed.

Proof of Concept

A zero address could accidentally be assigned to globals or vaultFactory, rendering the deployed contract obsolete.

Recommended Mitigation Steps

The constructor should typically be refactored as follows:

error AddressZeroEncountered();

constructor(IGlobals globals) {
        if (globals == address(0)) revert AddressZeroEncountered();
        GLOBALS = globals;
    }

[merklejerk]

Acceptable risk. Even with the check, it doesn't prevent us from configuring the wrong (nonzero) address. We verify deployments and migrations to mitigate the issue.

[HardlyDifficult]

Lowering risk and merging with the warden's QA report #59