Party DAO - Arz's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L148

Vulnerability details

The executeProposal() function in ProposalExecutionEngine.sol is used to execute different types of proposal call. One of this type is ArbitraryCallsProposal and this type should be able to send ether from the Party's balance or the forwarded ETH attached to this call - msg.value. However, the problem is that the executeProposal() function is not marked as payable and all calls that are sending ether and using msg.value will fail because of that.

Note: Even though this function is delegatecalled from PartyGovernance.sol::execute() which is payable the msg.value is still preserved when delegatecall is used and the function called needs to be payable too.

Impact

All proposal calls that are using attached ether to it will fail and Party's wont be able to execute these types of proposals which can be important.

Proof of Concept

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ProposalExecutionEngine.sol#L146-L148

146:   function executeProposal(
147:     ExecuteProposalParams memory params
148:   ) external onlyDelegateCall returns (bytes memory nextProgressData) { 

As you can see the executeProposal() function is not payable however later it then calls _executeArbitraryCalls():

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/proposals/ArbitraryCallsProposal.sol#L72-L74

72:  // If we're not allowing arbitrary calls to spend the Party's ETH, only
73:  // allow forwarded ETH attached to the call to be spent.
74:  uint256 ethAvailable = allowArbCallsToSpendPartyEth ? address(this).balance : msg.value;

As you can see and as the comment suggests the call should be able to use the forwarded ether attached to the call and ethAvailable is then used in _executeSingleArbitraryCall() but because the function is not payable the call will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the executeProposal() function payable

Assessed type

Payable

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L256

Vulnerability details

Because of a finding in the previous contest, the minContribution check is done after the amount is potentially reduced if refunding excess contribution. However this can be a problem if the maxTotalContributions - minTotalContributions is smaller than the minContribution and in some cases the crowdfund can be lost because of this.

Example:

minTotalContributions = 100 ether
maxTotalContributions = 102 ether
The current totalContributions = 99 ether
minContribution amount = 4 ether

When the last contributor wants to contribute he calls the function with 4 ether however the reduced amount will then be 3 ether and the tx will revert because of the minContribution check and the crowdfund will be lost.

Impact

There will be no way to finalize the crowdfund and because of that it will be lost even though it was supposed to be finalized.

Proof of Concept

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L240

240:  amount -= refundAmount;

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L256

256:  if (amount < minContribution_) {
257:      revert BelowMinimumContributionsError(amount, minContribution_);
258:  }

As you can see the amount is reduced and the check is done after it, the problem arises when the maxTotalContributions - minTotalContributions is smaller than the minContribution which will cause the crowdfund to be lost.

Tools Used

Manual Review

Recommended Mitigation Steps

Because we cant move the check before the refund logic, the only way to prevent this is to check if the minContribution is bigger than the maxTotalContributions - minTotalContributions in the initialize function

Assessed type

Other