Party DAO - adeolu's results

Lines of code

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

Vulnerability details

Impact

When a members contributes to a crowdfund via contribute(), the user donates a specified amount of eth to the crowdfund contract. The amount must be above the minContribution per user and lower than the maxContribution per user. If there are multiple members, the total amount of contributions must never exceed the maxTotalContributions and a crowdfund is considered sucessfull when the total amount of contributions is more than the minTotalContributions value.

When contributing, if a user donates an eth amount which takes the totalContributions above the maxTotalContributions, the user is refunded the eth differece and the amount donated is adjusted to an amount that just takes the totalContributions to be equal to the maxTotalContributions. It is possible that during the refund process, the adjusted contribution amount for the user will be below the minContribution per user. And this will cause a revert as seen here

        if (amount < minContribution_) {
            revert BelowMinimumContributionsError(amount, minContribution_);
        }

Lets have a scenario where minTotalContributions is 4.5 eth, maxTotalContributions is 5 eth, minContribution per user is 2 eth. If two members donate 2 eth each, the totalContributions becomes 4 eth. 0.5 eth is left to make the contributions up to minTotalContributions so that the crowdfund can be considered won and finalized. A third user proceeds to donate 2 eth, which is the minContribution per user. The system takes the 2 eth and because 2 eth will take the totalContributions up to 6 eth, the user gets refunded 1 eth so that totalContributions is eq to the maxTotalContributions of 5 eth. The third user contribution amount becomes 1 eth, but since 1 eth is not accepted because it is below minContribution (2 eth) per user the function reverts. This leaves the party crowdfund in a precarious situation as :

• no other member willing to donate can donate any amount less than 2 eth to take the totalContributions above minTotalContributions.
• any donatation of more than 2 eth will take the totalContributions above maxTotalContributions and cause a refund of excess eth while the user contribution amount will be adjusted to 1 eth, an amount which will be rejected by the function.
• minTotalContributions will never be reached.
• because of this the crowdfund will never be won, but instead will eventually become lost even though there have been members willing to contribute up to the minTotalContributions amount before expiry.

Note: the comments here says that it recognizes a similar scenario where "crowdfunds may not reach maxTotalContributions" and suggests that the members wait till the crowdfund is expired so that the host can finalize but what of the case where minTotalContributions is also never reached like my scenario describes? This will mean that the finalization will not be possible by the host and the crowdfund will be lost unfairly.

Proof of Concept

I wrote a test POC to illustrate this bug. See it below:

NOTE: paste code below into new file in the test/crowdfund folder and run with forge test --mt test_Case_02

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8;

import "forge-std/Test.sol";
import {Clones} from "openzeppelin/contracts/proxy/Clones.sol";

import "../../contracts/crowdfund/InitialETHCrowdfund.sol";
import "../../contracts/globals/Globals.sol";
import "../../contracts/party/PartyFactory.sol";
import "../../contracts/tokens/ERC721Receiver.sol";
import "../../contracts/renderers/PartyNFTRenderer.sol";
import "../../contracts/renderers/MetadataRegistry.sol";
import "../../contracts/renderers/RendererStorage.sol";
import "../../contracts/renderers/fonts/PixeldroidConsoleFont.sol";
import "../../contracts/distribution/TokenDistributor.sol";
import "../../contracts/gatekeepers/AllowListGateKeeper.sol";

import "../TestUtils.sol";
import {LintJSON} from "../utils/LintJSON.sol";

contract InitialETHCrowdfundTestBase is LintJSON, TestUtils, ERC721Receiver {
    using Clones for address;

    event Contributed(
        address indexed sender,
        address indexed contributor,
        uint256 amount,
        address delegate
    );
    event Refunded(
        address indexed contributor,
        uint256 indexed tokenId,
        uint256 amount
    );
    event PartyDelegateUpdated(address indexed owner, address indexed delegate);

    InitialETHCrowdfund initialETHCrowdfundImpl;
    Globals globals;
    Party partyImpl;
    PartyFactory partyFactory;

    PartyNFTRenderer nftRenderer;
    RendererStorage nftRendererStorage;
    TokenDistributor tokenDistributor;

    InitialETHCrowdfund.ETHPartyOptions partyOpts;
    InitialETHCrowdfund.InitialETHCrowdfundOptions crowdfundOpts;

    constructor() {
        globals = new Globals(address(this));
        partyImpl = new Party(globals);
        partyFactory = new PartyFactory(globals);

        initialETHCrowdfundImpl = new InitialETHCrowdfund(globals);

        MetadataRegistry metadataRegistry = new MetadataRegistry(
            globals,
            new address[](0)
        );

        // Upload font on-chain
        PixeldroidConsoleFont font = new PixeldroidConsoleFont();
        nftRendererStorage = new RendererStorage(address(this));
        nftRenderer = new PartyNFTRenderer(
            globals,
            nftRendererStorage,
            font,
            address(0),
            "https://party.app/party/"
        );
        tokenDistributor = new TokenDistributor(globals, 0);

        globals.setAddress(
            LibGlobals.GLOBAL_GOVERNANCE_NFT_RENDER_IMPL,
            address(nftRenderer)
        );
        globals.setAddress(
            LibGlobals.GLOBAL_RENDERER_STORAGE,
            address(nftRendererStorage)
        );
        globals.setAddress(
            LibGlobals.GLOBAL_TOKEN_DISTRIBUTOR,
            address(tokenDistributor)
        );
        globals.setAddress(
            LibGlobals.GLOBAL_METADATA_REGISTRY,
            address(metadataRegistry)
        );

        // Generate customization options.
        uint256 versionId = 1;
        uint256 numOfColors = uint8(type(Color).max) + 1;
        for (uint256 i; i < numOfColors; ++i) {
            // Generate customization options for all colors w/ each mode (light and dark).
            nftRendererStorage.createCustomizationPreset(
                // Preset ID 0 is reserved. It is used to indicates to party instances
                // to use the same customization preset as the crowdfund.
                i + 1,
                abi.encode(versionId, false, Color(i))
            );
            nftRendererStorage.createCustomizationPreset(
                i + 1 + numOfColors,
                abi.encode(versionId, true, Color(i))
            );
        }
    }

    struct CreateCrowdfundArgs {
        uint96 initialContribution;
        address payable initialContributor;
        address initialDelegate;
        uint96 minContributions;
        uint96 maxContributions;
        bool disableContributingForExistingCard;
        uint96 minTotalContributions;
        uint96 maxTotalContributions;
        uint40 duration;
        uint16 exchangeRateBps;
        uint16 fundingSplitBps;
        address payable fundingSplitRecipient;
        IGateKeeper gateKeeper;
        bytes12 gateKeeperId;
    }

    function _createCrowdfund(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args
            .disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        partyOpts.governanceOpts.passThresholdBps = 0.5e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);

        crowdfund = InitialETHCrowdfund(
            payable(address(initialETHCrowdfundImpl).clone())
        );
        if (initialize) {
            crowdfund.initialize{value: args.initialContribution}(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

    function _createCrowdfund(
        CreateCrowdfundArgs memory args
    ) internal returns (InitialETHCrowdfund) {
        return _createCrowdfund(args, true);
    }
}

contract InitialETHCrowdfundTest is InitialETHCrowdfundTestBase {
    using Clones for address;

    function test_Case_02() public {
        /** create new crowdfund with minContributions = 2 ether,  
   minTotalContributions = 4.5 ether, maxTotalContributions = 5 ether **/
        uint96 minContributuonPerUser = 2 ether;
        uint96 max_total_contributions = 5 ether;
        uint96 min_total_contributions = 4.5 ether;

        InitialETHCrowdfund crowdfund = _createCrowdfund(
            CreateCrowdfundArgs({
                initialContribution: 0,
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: minContributuonPerUser,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false,
                minTotalContributions: min_total_contributions,
                maxTotalContributions: max_total_contributions,
                duration: 7 days,
                exchangeRateBps: 1e4,
                fundingSplitBps: 100,
                fundingSplitRecipient: payable(_randomAddress()),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            })
        );

        //user 1 donates 2 eth
        address user_one = _randomAddress();
        uint96 user_one_contribution = 2 ether;
        vm.deal(user_one, user_one_contribution);

        vm.prank(user_one);
        crowdfund.contribute{value: user_one_contribution}(user_one, "");

        //user 2 donates 2 eth
        address user_two = _randomAddress();
        uint96 user_two_contribution = 2 ether;
        vm.deal(user_two, user_two_contribution);

        vm.prank(user_two);
        crowdfund.contribute{value: user_two_contribution}(user_two, "");

        //user 3 tries to donate 2 eth
        address user_three = _randomAddress();
        uint96 user_three_contribution = 2 ether;
        vm.deal(user_three, user_three_contribution);

        // since user 3 donated 2 eth, and previous users 1 and 2 donated 2 eth each, totalContributions is 4 eth, maxTotalContribution is 5.
        // user3 donation takes totalContributions to 6 but this is rejected by the system and the contribution of the user 3 is set to 1 eth
        // so that totalContributions will be eq to 5, the maxTotalContribution.
        // At this poinit, user 3 contribution amount is adjusted to 1 eth, and 1 eth is lower than the minimumContributionPerUser
        // so we expect a revert of the 1 ether contribution amount for user 3

        vm.expectRevert(
            abi.encodeWithSelector(
                ETHCrowdfundBase.BelowMinimumContributionsError.selector,
                1 ether,
                minContributuonPerUser
            )
        );
        vm.prank(user_three);
        crowdfund.contribute{value: user_three_contribution}(user_three, "");

        //now user 3 cannot contribute because 1 ether is needed to make it up to maxTotalContribution and 1 ether amount is below minContributionPer user so it always reverts.
        //totalContributions is 4 ether and  not up to minTotalContribution(4.5 ether) so this crowdfund can never be finalized.
        //It will be lost even though there are members willing to donate up to the minTotalContribution.
    }
}

Tools Used

manual review, foundry

Recommended Mitigation Steps

check that amount > minContribution before the refund. This way, the modified amount after refund is done will allow for maxTotalContributions === totalContributions.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

in the ETHCrowdfundBase contract, fundingSplitBps is a variable who's value denotes the the portion of contributions to send to the funding recipient. In practice it is set as basis points that represent percentages where 100 = 1% and 1e4/10_000 = 100%. See here

The function convertVotingPowerToContribution() calculate the contribution amount from the given voting power. If the fundingSplitBps is set to 1e4 (at deploy by party hosts) this means that 100% of the contributions should be sent to the funding receipient. convertVotingPowerToContribution() function will always revert because of its faulty math logic computation as seen below.

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

        if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
            amount = (amount * 1e4) / (1e4 - fundingSplitBps_);
        }

if fundingSplitBps_ is 100% or 1e4/10_000, the denominatior in the math computation above will be 0. Since divisions by zero is unallowed in solidity, the convertVotingPowerToContribution() will revert()/throw panic error.

Proof of Concept

if fundingSplitBps is set to be 100% (1e4) during initialization via _initialize(). In the convertVotingPowerToContribution() code logic below we can see that in the calculation of amount where (1e4 - fundingSplitBps_) this will be eq to 0.

    function convertVotingPowerToContribution(
        uint96 votingPower
    ) public view returns (uint96 amount) {
        amount = (votingPower * 1e4) / exchangeRateBps;

        // Add back funding split to contribution amount if applicable.
        address payable fundingSplitRecipient_ = fundingSplitRecipient;
        uint16 fundingSplitBps_ = fundingSplitBps;
        if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
            amount = (amount * 1e4) / (1e4 - fundingSplitBps_);
        }
    }

In the amount calculation, this will mean that a 0 value will be the denominator i.e amount = (amount * 1e4) / (0). This division operation will be reverted because solidity doesnt allow divisions by 0. Thus convertVotingPowerToContribution() will be impacted.

Here is a test POC demonstrating the bug:

copy and paste into new file in the test/crowdfund folder. Run with forge test --mt test_Case_01

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8;

import "forge-std/Test.sol";
import { Clones } from "openzeppelin/contracts/proxy/Clones.sol";

import "../../contracts/crowdfund/InitialETHCrowdfund.sol";
import "../../contracts/globals/Globals.sol";
import "../../contracts/party/PartyFactory.sol";
import "../../contracts/tokens/ERC721Receiver.sol";
import "../../contracts/renderers/PartyNFTRenderer.sol";
import "../../contracts/renderers/MetadataRegistry.sol";
import "../../contracts/renderers/RendererStorage.sol";
import "../../contracts/renderers/fonts/PixeldroidConsoleFont.sol";
import "../../contracts/distribution/TokenDistributor.sol";
import "../../contracts/gatekeepers/AllowListGateKeeper.sol";

import "../TestUtils.sol";
import { LintJSON } from "../utils/LintJSON.sol";

contract InitialETHCrowdfundTestBase is LintJSON, TestUtils, ERC721Receiver {
    using Clones for address;

    event Contributed(
        address indexed sender,
        address indexed contributor,
        uint256 amount,
        address delegate
    );
    event Refunded(address indexed contributor, uint256 indexed tokenId, uint256 amount);
    event PartyDelegateUpdated(address indexed owner, address indexed delegate);

    InitialETHCrowdfund initialETHCrowdfundImpl;
    Globals globals;
    Party partyImpl;
    PartyFactory partyFactory;

    PartyNFTRenderer nftRenderer;
    RendererStorage nftRendererStorage;
    TokenDistributor tokenDistributor;

    InitialETHCrowdfund.ETHPartyOptions partyOpts;
    InitialETHCrowdfund.InitialETHCrowdfundOptions crowdfundOpts;

    constructor() {
        globals = new Globals(address(this));
        partyImpl = new Party(globals);
        partyFactory = new PartyFactory(globals);

        initialETHCrowdfundImpl = new InitialETHCrowdfund(globals);

        MetadataRegistry metadataRegistry = new MetadataRegistry(globals, new address[](0));

        // Upload font on-chain
        PixeldroidConsoleFont font = new PixeldroidConsoleFont();
        nftRendererStorage = new RendererStorage(address(this));
        nftRenderer = new PartyNFTRenderer(
            globals,
            nftRendererStorage,
            font,
            address(0),
            "https://party.app/party/"
        );
        tokenDistributor = new TokenDistributor(globals, 0);

        globals.setAddress(LibGlobals.GLOBAL_GOVERNANCE_NFT_RENDER_IMPL, address(nftRenderer));
        globals.setAddress(LibGlobals.GLOBAL_RENDERER_STORAGE, address(nftRendererStorage));
        globals.setAddress(LibGlobals.GLOBAL_TOKEN_DISTRIBUTOR, address(tokenDistributor));
        globals.setAddress(LibGlobals.GLOBAL_METADATA_REGISTRY, address(metadataRegistry));

        // Generate customization options.
        uint256 versionId = 1;
        uint256 numOfColors = uint8(type(Color).max) + 1;
        for (uint256 i; i < numOfColors; ++i) {
            // Generate customization options for all colors w/ each mode (light and dark).
            nftRendererStorage.createCustomizationPreset(
                // Preset ID 0 is reserved. It is used to indicates to party instances
                // to use the same customization preset as the crowdfund.
                i + 1,
                abi.encode(versionId, false, Color(i))
            );
            nftRendererStorage.createCustomizationPreset(
                i + 1 + numOfColors,
                abi.encode(versionId, true, Color(i))
            );
        }
    }

    struct CreateCrowdfundArgs {
        uint96 initialContribution;
        address payable initialContributor;
        address initialDelegate;
        uint96 minContributions;
        uint96 maxContributions;
        bool disableContributingForExistingCard;
        uint96 minTotalContributions;
        uint96 maxTotalContributions;
        uint40 duration;
        uint16 exchangeRateBps;
        uint16 fundingSplitBps;
        address payable fundingSplitRecipient;
        IGateKeeper gateKeeper;
        bytes12 gateKeeperId;
    }

    function _createCrowdfund(
        CreateCrowdfundArgs memory args,
        bool initialize
    ) internal returns (InitialETHCrowdfund crowdfund) {
        crowdfundOpts.initialContributor = args.initialContributor;
        crowdfundOpts.initialDelegate = args.initialDelegate;
        crowdfundOpts.minContribution = args.minContributions;
        crowdfundOpts.maxContribution = args.maxContributions;
        crowdfundOpts.disableContributingForExistingCard = args.disableContributingForExistingCard;
        crowdfundOpts.minTotalContributions = args.minTotalContributions;
        crowdfundOpts.maxTotalContributions = args.maxTotalContributions;
        crowdfundOpts.duration = args.duration;
        crowdfundOpts.exchangeRateBps = args.exchangeRateBps;
        crowdfundOpts.fundingSplitBps = args.fundingSplitBps;
        crowdfundOpts.fundingSplitRecipient = args.fundingSplitRecipient;
        crowdfundOpts.gateKeeper = args.gateKeeper;
        crowdfundOpts.gateKeeperId = args.gateKeeperId;

        partyOpts.name = "Test Party";
        partyOpts.symbol = "TEST";
        partyOpts.governanceOpts.partyImpl = partyImpl;
        partyOpts.governanceOpts.partyFactory = partyFactory;
        partyOpts.governanceOpts.voteDuration = 7 days;
        partyOpts.governanceOpts.executionDelay = 1 days;
        partyOpts.governanceOpts.passThresholdBps = 0.5e4;
        partyOpts.governanceOpts.hosts = new address[](1);
        partyOpts.governanceOpts.hosts[0] = address(this);

        crowdfund = InitialETHCrowdfund(payable(address(initialETHCrowdfundImpl).clone()));
        if (initialize) {
            crowdfund.initialize{ value: args.initialContribution }(
                crowdfundOpts,
                partyOpts,
                MetadataProvider(address(0)),
                ""
            );
        }
    }

    function _createCrowdfund(
        CreateCrowdfundArgs memory args
    ) internal returns (InitialETHCrowdfund) {
        return _createCrowdfund(args, true);
    }
}

contract InitialETHCrowdfundTest is InitialETHCrowdfundTestBase {
    using Clones for address;


    function test_Case_01() public {
        //set fundingSplitBps to max 100% and deploy crowdfund
        uint16 maxfundingSplitBps = 1e4;
         InitialETHCrowdfund crowdfund = _createCrowdfund(
            CreateCrowdfundArgs({
                initialContribution: 0,
                initialContributor: payable(address(0)),
                initialDelegate: address(0),
                minContributions: 0,
                maxContributions: type(uint96).max,
                disableContributingForExistingCard: false,
                minTotalContributions: 3 ether,
                maxTotalContributions: 5 ether,
                duration: 7 days,
                exchangeRateBps: 1e4,
                fundingSplitBps: maxfundingSplitBps,
                fundingSplitRecipient: payable(_randomAddress()),
                gateKeeper: IGateKeeper(address(0)),
                gateKeeperId: bytes12(0)
            })
        );


        // call convertVotingPowerToContribution() and expect it to revert because of improper division by 0
        vm.expectRevert();
        crowdfund.convertVotingPowerToContribution(100);

    }

}

Tools Used

manual review

Recommended Mitigation Steps

check that 1e4 - fundingSplitBps_ is not zero before the division and if zero, set amount to be zero.

Assessed type

Math