code423n4 commented 1 year ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L61

Vulnerability details

Impact

It's possible to render the governance and execution contract inoperable for a period of time and effectively kill some proposals due to execution failure, which will break the governance routine and function. This could be a mistake or on purpose.

Proof of Concept

Since in propose(), the arrays of proposal and proposalData are arbitrary input by the user, their sizes might be too big, resulting in a denial of service for the contract and breaking core functionality. It is possible that the gas required to execute all the tasks exceeds the block gas limit in the for loop, essentially making the contract inoperable, effectively fail the proposal.

The propose() function does not check the size of the proposal.

The unbounded calls array might DoS due to gas limit.

// contracts/proposals/ArbitraryCallsProposal.sol
    function _executeArbitraryCalls() {
61:       for (uint256 i = 0; i < calls.length; ++i) {
            // Execute an arbitrary call.
        // ...
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Set a maximum length for proposal array.

    if (numTargets > MAX_LENGTH) revert();

merklejerk commented 1 year ago

Fine with this. This only affects the execution of that single arbitrary call proposal and will not block other proposals from being executed. They can just try again with a smaller proposal.

HardlyDifficult commented 1 year ago

Even if the targets.length == 1, the call could exhaust gas limits. Since other proposals are not blocked closing as invalid.

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