Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refunds in contribute() may brick crowdfunds from becoming won/finalized and minTotalContributions will never be reached #135

Closed
c4-submissions opened this issue Nov 8, 2023 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-127 edited-by-warden insufficient quality report This report is not of sufficient quality satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 8, 2023

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

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 8, 2023
c4-submissions added a commit that referenced this issue Nov 8, 2023
@code4rena-admin code4rena-admin changed the title refunds in contribute() may brick crowdfunds from becoming won/finalized refunds in contribute() may brick crowdfunds from becoming won/finalized and minTotalContributions will never be reached Nov 8, 2023
@c4-pre-sort
Copy link

ydspa marked the issue as duplicate of #552

@c4-pre-sort
Copy link

ydspa marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 12, 2023
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 19, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as unsatisfactory:
Invalid

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

1 similar comment
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as unsatisfactory:
Out of scope

@c4-judge c4-judge removed the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 23, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards duplicate-127 and removed duplicate-552 labels Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-127 edited-by-warden insufficient quality report This report is not of sufficient quality satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants