code423n4 commented 1 year ago

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/distribution/TokenDistributor.sol#L352-L353

Vulnerability details

Impact

In TokenDistributor._createDistribution() function, it did not check if feeRecipient != address(0) before excluding fee from member supply.

// Create a distribution.
uint128 fee = supply * args.feeBps / 1e4;
uint128 memberSupply = supply - fee;

In case feeRecipient == address(0), no one can claim this fee and it is locked in TokenDistributor contract. Instead this amount of fee should be distributed to members of Party.

Proof of Concept

Scenario

A Party created a Token Distribution with feeBps = 100 but forgot and set feeRecipient == address(0) and supply = 1e18
In _createDistribution() it did not check if feeRecipient == address(0) before substracting fee
```
fee = supply * feeBps / 1e4 = 1e17;
memberSupply = supply - fee = 9e17;
```
So only 9e17 token weis is distributed to members and 1e17 token weis is locked in TokenDistributor since no one can claim it.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider do not substract fee from memberSupply in case feeRecipient == address(0)

// Create a distribution.
uint128 fee = supply * args.feeBps / 1e4;
uint128 memberSupply = supply - fee;
if (feeRecipient == address(0)) {
    memberSupply = supply;
}

merklejerk commented 1 year ago

We're OK with this risk. Someone could also easily set the feeRecipient to a nonzero, dead address with the same effect.

HardlyDifficult commented 1 year ago

This can help to prevent user error - Low risk. Merging with #312

code-423n4 / 2022-09-party-findings