Unbounded delegating increases gas cost of transfer and can lock all funds

[code423n4]

Lines of code

https://github.com/fei-protocol/flywheel-v2/blob/main/src/token/ERC20MultiVotes.sol#L246

Vulnerability details

Impact

ERC20MultiVotes has variable maxDelegates that determines the maximum number of accounts any account can delegate to. This check is ignored for contracts where canContractExceedMaxDelegates[address] is set to true. These contracts can delegate to an uncapped number of accounts. This will in turn increase the gas cost of calling transfer transferFrom and _burn because they call _decrementVotesUntilFree which caches every delegated address when the contract does not have enough freeVotes to transfer the requested amount. Since the number of delegates is uncapped, it can increase to the point where transferring any amount of tokens will exceed the block gas limit, locking all funds in the delegating contract.

Proof of Concept

The purpose of allowing some contracts to ignore the maxDelegates check is to allow users to earn yield by depositing while maintaining their ability to vote on governance proposals. If the contract that is deposited into automatically delegates all votes back to the users, and exceeds a certain number of users, all funds will be locked in the contract. Some example scenarios:

If a contract delegates to ~4000 users, it will no longer be able to transfer all funds. This can be an issue if trying to rescue funds from a contract with a bug or move funds to a new implementation.

If a contract delegates to ~30000 users, calling delegates(contract) in a state changing function will exceed the block gas limit.

If a contract delegates to ~100000 users, transferring any amount of tokens will exceed the block gas limit.

This is a problem even if any of these limits aren't reached because the gas is paid for by users. Even at a fraction of the limit, UX will take a big hit since each user withdrawing will have to pay high fees.

Tools Used

Manual analysis. Foundry tests.

I added this test to ERC20MultiVotes.t.sol to come up with the numbers above.

function testGasBrickDelegateOverMaxDelegatesApproved() public {
    token.mint(address(this), 100e18);
    token.setMaxDelegates(8);

    token.setContractExceedMaxDelegates(address(this), true);

    uint160 i = 1; // address 0 causes reverts
    while (i < 100000) {
        token.incrementDelegation(address(i), token.freeVotes(address(this))/100 /* random-ish distribution of tokens */);
        i++;
    }
    token.incrementDelegation(address(250000), token.freeVotes(address(this))); // forces decrement
    startMeasuringGas("values view call");
    token.delegates(address(this));
    stopMeasuringGas();
    
    startMeasuringGas("transfer call");
    token.transfer(address(1000000), 1);
    stopMeasuringGas();
}

Recommended Mitigation Steps

To ensure the contract doesn't lock all funds, a global maxDelegates can be used. However, this issue is pretty complex and would more effectively be solved at the level of the contract that takes deposits. One solution would be to ensure the delegating contract has enough freeVotes to cover any single user withdrawing funds. This can be done by checking the state of the contract off-chain and depositing funds when necessary. It can also be somewhat mitigated by not delegating to every user that deposits and instead giving them the option of "requesting votes", however this can still lead to the same issue if everyone requests votes. A combination of these two methods would work well.

Another solution, which I think is the most optimal, would be to reimplement _decrementVotesUntilFree to not cache all addresses and instead just remove delegates starting at index 0 until free using .length() and .at() methods provided by EnumerableSet. This solution does not solve the problem with calling delegates(contract) nor does it allow transferring all tokens at once with > 4000 delegates. This second point can be dealt with by breaking up the transfer into smaller functions and bundling using flashbots. I have added an example implementation below.

function _decrementVotesUntilFree(address user, uint256 votes) internal {
    uint256 userFreeVotes = freeVotes(user);

    // early return if already free
    if (userFreeVotes >= votes) return;

    // cache total for batch updates
    uint256 totalFreed;

    // Loop through all delegates
    //address[] memory delegateList = _delegates[user].values();

    // Free delegates until through entire list or under votes amount
    uint256 size = _delegates[user].length();
    for (uint256 i = 0; i < size && (userFreeVotes + totalFreed) < votes; i++) {
        address delegatee = _delegates[user].at(i); // get each user as necessary, removes unnecessary SLOADs
        uint256 delegateVotes = _delegatesVotesCount[user][delegatee];
        if (delegateVotes != 0) {
            totalFreed += delegateVotes;

            require(_delegates[user].remove(delegatee)); // Remove from set. Should never fail.

            _delegatesVotesCount[user][delegatee] = 0;

            _writeCheckpoint(delegatee, _subtract, delegateVotes);
            emit Undelegation(user, delegatee, delegateVotes);
        }
    }

    userDelegatedVotes[user] -= totalFreed;
}

The sponsor mentioned they intend to solve this at the delegating contract level, nonetheless, this issue should at least be mentioned in the contract as it is open source and others might use it with an improper implementation.

[Joeysantoro]

This issue is invalid, the purpose of whitelisting contracts is so that contracts can aggregate user exposure to an unbounded set of users, but each user's balances are managed and limited to prevent dosing.

[Joeysantoro]

To further clarify, the owners should ONLY whitelist contracts which atomically free votes before transferring tokens, such that the unbounded array iteration never occurs.

That contract would need to take care to not be vulnerable to this issue described.

[0xean]

Based on the sponsors intended use of the whitelist I am going to mark this down to QA since the risk does exist, but the sponsor has a manual way to mitigate the issue in place.