code423n4 commented 1 year ago

Lines of code

Vulnerability details

Impact

Party.initialize() , CollectionBuyCrowdfund.initialize() , BuyCrowdfund.initialize() , AuctionCrowdfund.initialize() are functions called with the onlyConstructor modifier . This allows these functions to only be called by a constructor function in any contract deployment.

With the modifier above, any arbitrary contract can be deployed with calls to the functions above to initialize the contracts and further child contracts. This would defeat the essence of access control that was attempted to control access to the initialize() call

Proof of Concept

Party.initialize() - >PartyGovernanceNFT._initialize() ---> PartyGovernance._initialize()

CollectionBuyCrowdfund.initialize() ---> BuyCrowdfundBase._initialize() ---> Crowdfund._initialize() ---> CrowdfundNFT._initialize()

BuyCrowdfund.initialize() ---> BuyCrowdfundBase._initialize() ---> Crowdfund._initialize() ---> CrowdfundNFT._initialize()

AuctionCrowdfund.initialize() ---> Crowdfund._initialize() ---> CrowdfundNFT._initialize()

Tools Used

Manual review

Recommended Mitigation Steps

It would be preferable to implement access control to the initialize() function assigned to a specific address

merklejerk commented 1 year ago

This works as intended. There is no intended access control for initializers since they can only be called from within the context of a constructor.

cryptphitraining commented 1 year ago

@merklejerk the second paragraph in my findings impact section highlighted the risk.

The snippet for the for the onlyConstructor modifier is below https://github.com/PartyDAO/party-contracts-c4/blob/d129d647796369a18e30b336e74e7d1bfc779597/contracts/utils/Implementation.sol#L22-L29

    // Reverts if the current function context is not inside of a constructor.
    modifier onlyConstructor() {
        uint256 codeSize;
        assembly { codeSize := extcodesize(address()) }
        if (codeSize != 0) {
            revert OnlyConstructorError();
        }
        _;
    }

This means that any attacker/user can deploy a contract with all the necessary information for initializing the contracts into their attacker contract's constructor. Since the calls to initialize the contracts, starting with Party.initialize() is put in the attacker contract's constructor, then it would pass the check.

merklejerk commented 1 year ago

@cryptphitraining What's the attack?

cryptphitraining commented 1 year ago

@merklejerk Since any user can initialize the contracts, it means the user/attacker has control of the inputs for the initialize() function arguments and they can set the state variable values to whichever they like.

One of such example is the user can set the Distributor feeRecipient address to their own address. It's all user controlled through calling the initialize function through any arbitrary contract's constructor and deploying that arbitrary contract to initialize the Party contracts.

To help drill down to how the feeRecipient's address can be set, the flow is below

Alice drafts out contractA.
ContractA's constructor contains the call to Party.initialize() with the necessary initData argument input.
Party.initialize() calls PartyGovernanceNFT._initialize() with initData.options.governance as part of the argument input
PartyGovernanceNFT._initialize() calls PartyGovernance._initialize() with governanceOpts as part of the argument input
When PartyGovernance._initialize() is called, the information from governanceOpts is used as opts argument input.
In PartyGovernance._initialize(), feeRecipient state variable is set by opts.feeRecipient input which is Alice's address
During distribution creation, the set feeRecipient is used
Alice can then call Distribution.claimFee() to claim all fees accrued in the Distribution contract.

Above is just one of the example, the user controlled input in Party.initialize() means Alice can use any values she wants and set state variables for any contract whose initialize() function is called starting from Party.initialize().

I hope this helps.

merklejerk commented 1 year ago

@cryptphitraining When you say "any user can initialize the contracts," exactly what are "the contracts?" Are you suggesting you can somehow initialize the Party implementation contract? How would you do that? Calling Party.initialize() without a delegatecall inside a constructor would revert because there will be code at its address thanks to onlyConstructor.

HardlyDifficult commented 1 year ago

onlyConstructor requires this.code.length == 0 - it seems address(this) was overlooked for this report, as opposed to msg.sender.code.length.

Closing as invalid.

code-423n4 / 2022-09-party-findings

Anyone can initialize contracts #278

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps