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

A user can reach maxTotalContribution, finalize, and then withdraw via rageQuit in a single transaction, effectively making a just finalized party fundless. #534

Open
c4-submissions opened this issue Nov 10, 2023 · 23 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/InitialETHCrowdfund.sol#L164
https://github.com/code-423n4/2023-10-party/blob/main/contracts/crowdfund/ETHCrowdfundBase.sol#L317
https://github.com/code-423n4/2023-10-party/blob/main/contracts/party/PartyGovernanceNFT.sol#L344

Vulnerability details

Impact

Malicious user can use a flashloan to finalize a crowdfund, and withdraw flashloan in a transaction, causing Party to immediately go bankrupt just after finalizing crowdfund.

Proof of Concept

In InitialETHCrowdfund, once total contributions reach maxTotalContribution, crowdfund gets finalized, and the funds get transferred to the Party. After this, no more conttributions will be accepted.

A malicious user can in a single transaction:

  • use a flashloan to make totalContributions reach maxTotalContributions,
  • finalize the crowdfund, and then
  • withdraw the flashloan from the party via ragequit.

The effect of this is that the party will now be with very low or no funds just after it got finalized. This can be used to grief hosts as they will have to start another crowdfunding just after completing one.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing the following:

  • storage variable lastContributionTimestamp should be updated after every contribution.
  • There should be a limit to the amount that each account can contribute, and if block.timestamp==lastContributionTimestamp, call should revert.

Now, an account cannot contribute over maxContribution, and an attempt to contribute with multiple accounts in a single transaction will revert.

Assessed type

Other

@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 10, 2023
c4-submissions added a commit that referenced this issue Nov 10, 2023
@ydspa
Copy link

ydspa commented Nov 11, 2023

Known issue: code-423n4/2023-04-party-findings#25

Invalid: OOS

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Nov 11, 2023
@c4-pre-sort
Copy link

ydspa marked the issue as insufficient quality report

@c4-judge
Copy link
Contributor

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

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 19, 2023
@Emedudu
Copy link

Emedudu commented Nov 22, 2023

This issue is not the same as code-423n4/2023-04-party-findings#25

In summary, this issue describes a griefing attack how a malicious user can use a flashloan to finalize a crowdfund, and withdraw the flashloan via ragequit.
The result is that, a party will be left fundless just after getting finalized, and there could even be fund loss if the party had some funds before the crowdfund.

Consider this scenario:

  • minTotalContribution=10 eth, maxTotalContribution=20 eth
  • 5 users contribute 1 eth each
    • totalContributions=5 eth
  • attacker flashloans 15 eth, contribute to crowdfund, which finalizes the crowdfund and transfers 20 eth to Party
  • attacker ragequits from party, which makes him receive his 15 eth flashloan
    • Party balance is 5 eth which is less than minTotalContributions
    • Note that if Party had some funds apart from 20 eth, attacker will also be able to steal them(15/20 of them)
  • Now, if the intention of the hosts was to use the Party funds to buy NFTs worth minTotalContributions of 10 eth, they won't be able to.

This is not fair to the hosts as the crowdfund got finalized, but their Party funds is less than the minTotalContributions they had in mind.

Looking at the sponsor's comment in the previous competition: "Looking into this more, the issue can only occur if a party sets an executionDelay of 0...".
This issue however, is present whether or not executionDelay of 0 is set, which proves that they are different issues.

Please have a closer look.

@c4-judge c4-judge reopened this Nov 23, 2023
@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 removed the grade

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 23, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 changed the severity to QA (Quality Assurance)

@gzeon-c4
Copy link

Agree this is a slightly different issue due to the introduction of rage quit. However, the impact is also significantly lower as the attacker can only perform a DOS and unable to steal additional fund from the party. Also this only works when rage quit is enabled on party creation. Downgrading to Low/QA @0xble

@Emedudu
Copy link

Emedudu commented Nov 23, 2023

Hi @gzeon-c4

However, the impact is also significantly lower as the attacker can only perform a DOS

Isn't this DOS significant enough to be a medium severity issue?

  • Say the minTotalContribution is 400 eth, maxTotalContributions is 500 eth
  • 200 people have contributed total of 200 eth; totalContributions=200
  • Attacker performs the attack: flashloans 300 eth, contributes which finalizes crowdfund, ragequits
  • Party is left with 200 eth
  • For the minTotalContributions to be 400 eth, it is very likely that 200 eth won't be enough for what the party needs
    • All other 200 users will be forced to ragequit

and unable to steal additional fund from the party

Please note that funds present in the Party before the finalization of the Crowdfund can also be stolen, because rageQuit transfers a pro rata amount of funds in the Party

@gzeon-c4
Copy link

The Low risk judgement is a combination of Med impact (DOS), and Low likelihood (can be easily avoided by not allowing rage quit)

@Emedudu
Copy link

Emedudu commented Nov 23, 2023

Hello @gzeon-c4 , please take another look at this:

#119 can be easily avoided by making maxTotalContributions-minTotalContribution greater than minContribution

#127 can also be easily avoided by making maxTotalContributions-minTotalContribution greater than minContribution

This issue can be easily avoided by disallowing ragequit, so I think they all have about same severity

@gzeon-c4
Copy link

Fair, although #119 and #127 have higher impact due to fund would be temporarily stuck where as in this case funds are immediately released. There does not seems to be anything in the doc or codebase to suggest rageQuitTimestamp should not start with a nonzero value before finalizing and thus a valid DOS. @0xble

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Nov 23, 2023
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by gzeon-c4

1 similar comment
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by gzeon-c4

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as selected for report

@c4-judge c4-judge added selected for report This submission will be included/highlighted in the audit report satisfactory satisfies C4 submission criteria; eligible for awards labels Nov 23, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as satisfactory

@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Nov 23, 2023
@stalinMacias
Copy link

stalinMacias commented Nov 23, 2023

@gzeon-c4
I believe this doesn't deserve to be medium, it's QA at max for multiple reasons.

Even if someone used a flash loan and rage quit leaving the party with funds less than the goal.

  1. There are no funds that can be stolen. There are no funds in the party before finalization. Only during finalization, the ETH/funds of the crowdfund is sent to the party.
    And even if we assume there can be funds prior to finalization, the totalVotingPower of the party would be zero, as the total voting power for a party is only set/allocated during the finalization process (finalize()). And since the totalVotingPower would be equal to zero, the attacker will not be able to get his funds back if he tries to rageQuit() because rageQuit() relies on the totalVotingPower to calculate how much to give the rage-quitter, check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L393

  2. There is no DoS here, all party members can just rage quit and get their funds back and/or create proposals and vote, normal and expected behavior.

  3. The attacker will certainly have to pay a flash loan fee, this ranges from a protocol to another, AAVE charges 0.09%, uniswap charges 0.3%. There is no incentive for the attacker to pay 0.3% or even 0.1% of the amount he borrowed to execute a useless attack. If he borrowed 200 ETH for example from AAVE, he'll have to pay 0.2 ETH / 412$ USD to execute this useless attack.

  4. When any user tries to rage quit, a fee is deducted from his withdrawings (check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L407)
    So if the attacker tries to rage quit to get his ETH back and repay the flash loan, a fee will be deducted from his ETH (flash loan). The fee deducted depends on the feeBps which is pre-set during the crowdfund creation. So the user will have to pay rage quit fee + flash loan fee to achieve absolutely nothing.

  5. This "attack" assumes that rage-quit will be enabled, which is not always the case

  6. Someone who is rich with a lot ETH can just execute this "attack", so the devs can't really fix the root cause. They can only fix the flash loan element (will be extra overhead though)

For the 6 reasons listed, I don't think there is any reason to keep this as a med.

@Emedudu
Copy link

Emedudu commented Nov 24, 2023

There are no funds that can be stolen. There are no funds in the party before finalization. Only during finalization, the ETH/funds of the crowdfund is sent to the party.
And even if we assume there can be funds prior to finalization, the totalVotingPower of the party would be zero, as the total voting power for a party is only set/allocated during the finalization process (finalize()). And since the totalVotingPower would be equal to zero, the attacker will not be able to get his funds back if he tries to rageQuit() because rageQuit() relies on the totalVotingPower to calculate how much to give the rage-quitter, check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L393

Attacker's flashloan contribution will cause crowdfund to be finalized, so totalVotingPower will be updated

There is no DoS here, all party members can just rage quit and get their funds back and/or create proposals and vote, normal and expected behavior.

It obviously is a griefing attack, and not an expected behaviour. See example above

The attacker will certainly have to pay a flash loan fee, this ranges from a protocol to another, AAVE charges 0.09%, uniswap charges 0.3%. There is no incentive for the attacker to pay 0.3% or even 0.1% of the amount he borrowed to execute a useless attack. If he borrowed 200 ETH for example from AAVE, he'll have to pay 0.2 ETH / 412$ USD to execute this useless attack.

Some protocols offer free flashloans like dYdX

When any user tries to rage quit, a fee is deducted from his withdrawings (check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L407)
So if the attacker tries to rage quit to get his ETH back and repay the flash loan, a fee will be deducted from his ETH (flash loan). The fee deducted depends on the feeBps which is pre-set during the crowdfund creation. So the user will have to pay rage quit fee + flash loan fee to achieve absolutely nothing.

Correct. Cost of Attack is expensive due to rageQuit fee, Result is unprofitable griefing. Hence, does not qualify for Medium.

This "attack" assumes that rage-quit will be enabled, which is not always the case

Enabling ragequit is a normal operation, so this alone is not enough to make this QA

Someone who is rich with a lot ETH can just execute this "attack", so the devs can't really fix the root cause. They can only fix the flash loan element (will be extra overhead though)

Just like most other flashloan attacks, rich people can easily execute them, so this is not a valid point

@Emedudu
Copy link

Emedudu commented Nov 24, 2023

Hello @gzeon-c4 ,

Based on the point raised by @stalinMacias that there is a rageQuit fee, which makes the attack expensive + Impact is unprofitable griefing , I don't think this issue qualifies for a MEDIUM(Probably QA).

I really appreciate your time and effort🙏

@gzeon-c4
Copy link

gzeon-c4 commented Nov 26, 2023

Thanks for the inputs! I do realize there is a fee, but if minTotalContribution==maxTotalContribution the attacker can frontrun the last contribution with a minimal size (equal to the buffer the Party have relative to their objective). The likelihood is definitely on the lower side since there are multiple mitigation from disabling rageQuit before finalization to an increased fundraising buffer.

Downgrading this to Low but bumping the warden's overall QA report grade.

@c4-judge
Copy link
Contributor

gzeon-c4 changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 26, 2023
@c4-judge
Copy link
Contributor

gzeon-c4 marked the issue as not selected for report

@c4-judge c4-judge removed the selected for report This submission will be included/highlighted in the audit report label Nov 26, 2023
@0xble
Copy link

0xble commented Nov 26, 2023

@gzeon-c4 I believe this doesn't deserve to be medium, it's QA at max for multiple reasons.

Even if someone used a flash loan and rage quit leaving the party with funds less than the goal.

  1. There are no funds that can be stolen. There are no funds in the party before finalization. Only during finalization, the ETH/funds of the crowdfund is sent to the party.
    And even if we assume there can be funds prior to finalization, the totalVotingPower of the party would be zero, as the total voting power for a party is only set/allocated during the finalization process (finalize()). And since the totalVotingPower would be equal to zero, the attacker will not be able to get his funds back if he tries to rageQuit() because rageQuit() relies on the totalVotingPower to calculate how much to give the rage-quitter, check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L393
  2. There is no DoS here, all party members can just rage quit and get their funds back and/or create proposals and vote, normal and expected behavior.
  3. The attacker will certainly have to pay a flash loan fee, this ranges from a protocol to another, AAVE charges 0.09%, uniswap charges 0.3%. There is no incentive for the attacker to pay 0.3% or even 0.1% of the amount he borrowed to execute a useless attack. If he borrowed 200 ETH for example from AAVE, he'll have to pay 0.2 ETH / 412$ USD to execute this useless attack.
  4. When any user tries to rage quit, a fee is deducted from his withdrawings (check https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernanceNFT.sol#L407)
    So if the attacker tries to rage quit to get his ETH back and repay the flash loan, a fee will be deducted from his ETH (flash loan). The fee deducted depends on the feeBps which is pre-set during the crowdfund creation. So the user will have to pay rage quit fee + flash loan fee to achieve absolutely nothing.
  5. This "attack" assumes that rage-quit will be enabled, which is not always the case
  6. Someone who is rich with a lot ETH can just execute this "attack", so the devs can't really fix the root cause. They can only fix the flash loan element (will be extra overhead though)

For the 6 reasons listed, I don't think there is any reason to keep this as a med.

Good points. Agree it should be a QA over Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a insufficient quality report This report is not of sufficient quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

9 participants