Closed code423n4 closed 1 year ago
We don't see this as a real problem. The passing threshold for governance is still based on total (minted and unminted) voting power, so getting voting power earlier than others doesn't offer any real advantage.
It's more a matter of the relative voting powers of the parties. In these terms splitRecipient
will have somewhat oversized voting power proportionally to the share of the unminted total.
Yeah, but governance still operates against total possible voting power (minted + unminted), so I don't get how it would matter? Passing a proposal only depends on getting some fixed % of the total possible voting power. Their ability to unilaterally pass a proposal does not change regardless of when they burn. There are no "no" votes in the system so votes do not directly compete.
It boils down to the expected behavior. Suppose split recipient has the cut equal to the passing threshold. In the current situation he can unilateraly pass anything, no contributors needed. He can do it instantly, say back-running the state changing transaction that enables the burning.
What is proposed updates the logic so the presence of all others is required. In the same situation the recipient can still go alone, but only after all contributors has checked in and had the opportunity to propose something on their own.
What is desired?
We actually do want this current behavior. In V1 we witnessed many people contributing to crowdfunds and never returning to claim their fractions (v1 just went straight to fractional). That's why we now ask contributors to choose a delegate upon contribute and allow anyone to burn a contributor's crowdfund token-- so governance isn't held up by absent members. The suggestion doesn't require the presence of other contributors either, since anyone can burn anyone's contribution token for voting power. So if we built up the split recipient's total VP piecemeal, as suggested, then the split recipient would just have to burn everyone's tokens on their behalf. It's the same end result but wasting much more gas.
Interesting discussion but it seems this is not an issue. Closing as invalid.
Lines of code
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L370-L375
Vulnerability details
When burn() computes the contributor's share via _getFinalContribution() and
contributor = splitRecipient
it transfers the whole share to thesplitRecipient
in advance, even when not all the voting power were allocated.I.e. some contributors might not burn and the total voting power can be less than getFinalPrice() dictates (there will not be any funds shortage as those who didn't burned still have their ETH in the contract). This way
splitRecipient
always receives at least `splitBps` of voting power and sometimes more, proportionally to the count of the contributors who didn't burned.As an example, suppose half of contributors in ETH terms forget to burn, so the half of the voting power isn't minted, but
splitRecipient
has burned and received twice voting power in bps terms.Proof of Concept
_getFinalContribution() gives the whole voting power to the
splitRecipient
at once, even if other participants didn't burned:https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/crowdfund/Crowdfund.sol#L370-L375
I.e. this allocation is done in advance, assuming that all the
totalEthUsed = _getFinalPrice()
be burned.Recommended Mitigation Steps
Consider allocating
splitBps_
of voting power actually minted to thesplitRecipient
on each burn of other contributors.It can be either in push (every contributor pays for
splitRecipient
allocation) or in pull (splitRecipient
calls in and receives current share; multiple burning calls must be allowed in this case forsplitRecipient
).