Party DAO - ZanyBonzy's results

1. Improve the getCrowdFundLifecylce function. minContribution and minTotalContributions can be set to 0. Automatically marking the crowdfund as won, even if no single contribution has been made.

        if (block.timestamp >= expiry_) {
            if (totalContributions >= minTotalContributions) { //@note no contributions can be min
                return CrowdfundLifecycle.Won;
            } else {
                return CrowdfundLifecycle.Lost;
            }
        }

The creators can then wait till crowdfund expiry is reached, call the finalize function.

    function finalize() external {
        uint96 totalContributions_ = totalContributions; // 0

        // Check that the crowdfund is not already finalized.
        CrowdfundLifecycle lc = getCrowdfundLifecycle(); // won
        ...
           else {
            // Otherwise only allow finalization if the crowdfund has expired
            // and been won. Can be finalized by anyone.
            if (lc != CrowdfundLifecycle.Won) {
                revert WrongLifecycleError(lc);
            }
        }

        // Finalize the crowdfund.
        _finalize(totalContributions_);
    }

and mark the crowdfund as finalized.

    function _finalize(uint96 totalContributions_) internal {
        // Finalize the crowdfund.
        delete expiry;
        ...
        emit Finalized();
    }

Basically having the party created without any contribtions, albeit votingPower is 0.

2. The first contribute function is not needed, it is exactly the same as the second. A comment can be added informing users that using 0 as tokenId mints them a new party card

3. Consider limiting the cancel function to only hosts. It can be abused by users who are not in support of the proposal. For instance, in a party of a hundred members, 98 supports the proposal, 2 don't. The proposal is passed, executed and now in progress. Any of the 2 members can call the function and cancel the proposal which goes against the majority decision.

 function cancel(uint256 proposalId, Proposal calldata proposal) external {
        _assertActiveMember(); //@note not interested?
        ...

4. Party can be left hostless if all available hosts abdicate. Also consider using two step (abdicate and accept) in the case that a party has one host, and the host status is transferred to the wrong address. These causes the host functions to be inaccessible.

    function abdicateHost(address newPartyHost) external {
        _assertHost();
        // 0 is a special case burn address.
        if (newPartyHost != address(0)) {
            // Cannot transfer host status to an existing host.
            if (isHost[newPartyHost]) {
                revert InvalidNewHostError();
            }
            isHost[newPartyHost] = true;
        } else {
            // Burned the host status
            --numHosts;
        }
        isHost[msg.sender] = false;
        emit HostStatusTransferred(msg.sender, newPartyHost);
    }

5. Authorities can also be relinquished which can leave authority functions inaccessible. Consider limiting this to only added authorities.

    function abdicateAuthority() external {
        _assertAuthority();
        delete isAuthority[msg.sender];

        emit AuthorityRemoved(msg.sender);
    }

6. Precision may be lost when converting contributed amounts to voting power and vice versa. It can lead to users being refunded less than they contributed.

When a user contributes, they're given a votingPower which is calculated using the formula. votingPower = (amount * exchangeRateBps) / 1e4; After the crowdfund is lost, users have the options to get refunded their contributions. By doing this, their votingPower is reconverted back to contributions using the formula amount = (votingPower * 1e4) / exchangeRateBps;

Noting that solidity rounds down, the votingPower calclated from amount contribted will be slightly less than actual votingPower, which will then be used to calculate the amount to refund.

For example, A user contributes 50 eth, to a party with an exchangeRateBps of 9900 (99%). Barring no fees and funding split, he recieves (50 * 9900)/10000 = 49 votingPower, not 49,5 because solidity. When such user decides he wants a refund after crowdfund is lost. His contributed amount, is 49 * 10000 / 9900 = 49 eth, not the 50 he initially contributed.

He gets refunded 49 eth and loses 1 eth to the contract.

        // Check that the contribution amount is at or above the minimum. This
        // is done after `amount` is potentially reduced if refunding excess
        // contribution. There is a case where this prevents a crowdfunds from
        // reaching `maxTotalContributions` if the `minContribution` is greater
        // than the difference between `maxTotalContributions` and the current
        // `totalContributions`. In this scenario users will have to wait until
        // the crowdfund expires or a host finalizes after
        // `minTotalContribution` has been reached by calling `finalize()`.
        uint96 minContribution_ = minContribution;
        if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
        }

In a case where a definite goal is set i.e minTotalContributions = maxTotalContributions. I.e the party wants the immediately finalize as soon as needed contributions are met. The required goal will be unreachable, as amount left after refunding excess(i.e the amount needed to bring the crowdfund to full equilibrum) will be less than the mincontribution needed.

8. Users who have their their token balance i.e (the amount they're to withdraw) equal to minWithdrawAmount might not be able to rageQuit if there are fees involved. User wants to quit the party and calls the rageQuit function

    function rageQuit(
        uint256[] calldata tokenIds,
        IERC20[] calldata withdrawTokens,
        uint256[] calldata minWithdrawAmounts,
        address receiver
    ) external {

The required checks are done and states are updated. Then fee is deducted

       ...
        {
            uint16 feeBps_ = feeBps;
        ...
                // Take fee from amount.
                uint256 fee = (amount * feeBps_) / 1e4;

                if (fee > 0) {
                    amount -= fee;
        ...
                }

As fee is the deducted, the new amount is then compared to the minimum wihtdrawal amount

                if (amount > 0) {
                    uint256 minAmount = minWithdrawAmounts[i];

                    // Check amount is at least minimum.
                    if (amount < minAmount) {
                        revert BelowMinWithdrawAmountError(amount, minAmount);
                    }
        ...
        emit RageQuit(msg.sender, tokenIds, withdrawTokens, receiver);
    }

which the user no longer meets, and therfore unable to leave. Recommend switching the positions, checking for the minAmount first, before deducting fees.

9. // IDs of cards to credit the contributions to. When set to 0, it means Incomplete comment.
10. Consider bounding array length at L211 InitialEthCrowdfund for (uint256 i; i < numContributions; ++i) { and L260 for (uint256 i; i < args.recipients.length; ++i) {

1. Approach taken in evaluating the codebase

• We started by reading the readme and documentation to understand the protocol. We took notes on the key points and main invariants, and we identified any parts that were unclear. We then made a list of questions about these parts.
• After we gained an idea of how the protocol should work, we used a static analyzer to find any easy-to-detect problems in the contracts. We then compared the report to the bots report and selected what we deemed important issues and removed any false positives.
• Finally, we did a thorough and comprehensive manual code review, focusing on the most important and critical areas. We tried to come up with different ways to exploit the system and looked for common contract vulnerabilities. We then documented all of the concerns we found and compared our notes to previous audits of similar protocols.

2. Architecture and Codebase quality analysis

• The protocol allows users to create and join parties by contributing to a crowdfund. Once the crowdfund is successful, the party is created and members are minted NFTs that represent their voting power. Members can then create proposals and vote on their execution.

• In audit scope, the contracts comprises

  CrowdFund creation and modification contracts - InitialETHCrowdfund.sol, ETHCrowdfundBase.sol;

  Party setup, modifications and proposal creation contracts - PartyGovernance.sol, PartyGovernanceNFT.sol;

  Proposal execution,storage and parameter modification contracts - ProposalExecutionEngine.sol, ProposalStorage.sol;

  Two new proposal contract types - SetGovernanceParameterProposal.sol & SetSignatureValidatorProposal.sol;

  Signature validator contract - OffChainSignatureValidator.sol;

  and a utility contract - Implementation.sol.

• The protocol also makes use of solmate's ERC721 contract, OZ's IERC2981 interface as well as being designed to follow certain EIP standards.

• The codebase appears to be well-written and well structured. With a few exceptions, the function sizes were compact and easy to understand.

• The test coverage is around 60% which not bad, but can be improved. Consider introducing invariant tests for the larger codebases.

• Error handling is very good. Custom errors were mostly used instead of repetitive strings which helps to save a lot of gas. The assert error was used in one or two cases, this should also be removed to save gas.

• The ProposalExecutionEngine contract uses tx.origin which is a currently disputed authorization opcode. This should be kept in mind for future updates incase the it becomes disabled.

3. Protocol risks

• Centralization as the protocol relies on hosts, authorities, and the partyDAO to perform privileged functions. These entities must be trusted not to abuse their power. Additionally, as authorities are implemented as smart contracts, any vulnerabilities in them could be exploited;
• Malicious party members may try to influence party proposals, by gathering excess voting power to push a proposal that they support or cancelling proposals they're not interested in while it's in progress;
• Bugs in any of the smart contracts could also compromise the protocol;
• Any vulnerabilities in the imported OZ and solady contracts also pose a risk to the protocol's security.

4. Conclusion

• The Party protocol provides an open protocol for group coordination, formation and distribution with the goal of making Ethereum multiplayer. The codebase is solid, well commented and documented, so devs and users will find it quite to easy understand. We recommend taking into consideration discovered issues and mitigating them, implementing constant upgrades and audits to keep the protocol always secure.

Time spent:

24 hours