code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

_create() function doesn't check if party variable is equal to 0. The parameter passed to the function could be erronously empty, even if it's unlikely. If a malicious actor succeds in triggering the function setting a party = 0, it whould lead to a distributor that works apparently correctly but doesn't let claim. Only DAO can with emergency function, but if it's emergency transfer is disable, funds are stuck.

Proof of Concept

Modify TokenDistributorUnit.t.sol/contract TokenDistributorUnitTest: //add party0 variable without initializing Globals globals; TokenDistributor distributor; TestParty party; TestParty party0; DummyERC20 erc20 = new DummyERC20(); DummyERC1155 erc1155 = new DummyERC1155();

constructor() {
    globals = new Globals(address(this));
    distributor = new TokenDistributor(globals);
    party = new TestParty();
}

//then add this test function

function test_claimNative_multiMemberWithFeesWithParty0() external { (address payable[] memory members, uint256[] memory memberTokenIds, uint256[] memory shares) = _mintRandomShares(_randomUint256() % 8 + 1); uint256 supply = _randomUint256() % 1e18; vm.deal(address(distributor), supply); vm.prank(address(party)); (ITokenDistributor.DistributionInfo memory di) = distributor.createNativeDistribution(party0, DEFAULT_FEE_RECIPIENT, DEFAULT_FEE_BPS); uint256 memberIdx = _randomUint256() % members.length; uint256 claimAmount = _computeMemberShare( _computeLessFees(supply, DEFAULT_FEE_BPS), shares[memberIdx] ); //_expectEmit2(); emit DistributionClaimedByPartyToken( party0, memberTokenIds[memberIdx], members[memberIdx], ITokenDistributor.TokenType.Native, ETH_TOKEN_ADDRESS, claimAmount );

//to test DAO emergencyWithdraw comment the following 3 lines and UNCOMMENT the following 3
    vm.prank(members[memberIdx]);
    distributor.claim(di, memberTokenIds[memberIdx]);
    assertEq(members[memberIdx].balance, claimAmount);

//vm.prank(DAO_ADDRESS);
//distributor.emergencyWithdraw(ITokenDistributor.TokenType.Native,ETH_TOKEN_ADDRESS,members[memberIdx],claimAmount);
//assertEq(members[memberIdx].balance, claimAmount);
}

Tools Used

Forge on Visual Studio

Recommended Mitigation Steps

Check if party = 0 and revert if succed

merklejerk commented 2 years ago

Won't happen with normal usage, since parties call this function directly. Checking a 0 address doesn't prevent you from accepting a dead address, or an EOA.

HardlyDifficult commented 2 years ago

This rec may help to prevent user error. Downgrading and merging with #223

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