Unsafe ERC20MultiVotes logic allows attackers to use flashloans to manipulate governance proposals

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20MultiVotes.sol#L103

Vulnerability details

Impact

Unsafe logic to lookup past voting weight for accounts results in users being able to take flashloans to execute governance votes, exceeding any quorums.

Proof of Concept

GuildGovernor inherits OpenZeppelin's Governor contract to create and execute governance proposals. Additionally, it inherits GovernorVotes, used to retrieve the voting weight of accounts. When a vote is cast, Governor uses GovernorVotes._getVotes to retrieve the voting weight to apply for the voting account.

function _getVotes(
    address account,
    uint256 timepoint,
    bytes memory /*params*/
) internal view virtual override returns (uint256) {
    return token().getPastVotes(account, timepoint);
}

As we can see in _getVotes, we retrieve the voting weight by calling token().getPastVotes, which is implemented by ERC20MultiVotes for both CREDIT and GUILD.

function getPastVotes(
    address account,
    uint256 blockNumber
) public view virtual returns (uint256) {
    require(
        blockNumber < block.number,
        "ERC20MultiVotes: not a past block"
    );
    return _checkpointsLookup(_checkpoints[account], blockNumber);
}

/// @dev Lookup a value in a list of (sorted) checkpoints.
function _checkpointsLookup(
    Checkpoint[] storage ckpts,
    uint256 blockNumber
) private view returns (uint256) {
    // We run a binary search to look for the earliest checkpoint taken after `blockNumber`.
    uint256 high = ckpts.length;
    uint256 low = 0;
    while (low < high) {
        uint256 mid = average(low, high);
        if (ckpts[mid].fromBlock > blockNumber) {
            high = mid;
        } else {
            low = mid + 1;
        }
    }

    return high == 0 ? 0 : ckpts[high - 1].votes;
}

getPastVotes runs a binary search through the acccount's checkpoints to find the first checkpoint after the block number to retrieve votes for, and return the vote weight at that checkpoint.

The problem with this logic is that as long as we haven't already set a checkpoint after the block number to retrieve we can simply flashloan tokens and self delegate and that will be used as the voting weight since it's set as the first checkpoint after the blockNumber param. If an attacker can use flashloaned funds to vote on governance, they can simply flashloan enough tokens to exceed the quorum.

Tools Used

• Manual review

Recommended Mitigation Steps

To correctly implement the desired checkpointing logic, all checkpoints should track the balance from before the change in voting weight and use that as the voting weight retrieved from the binary search.

Assessed type

Invalid Validation

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

I think the described behavior cannot happen, because flashloans have to be closed in the same block, and the 2nd balance change within the block override the checkpoint returned by getPastVotes: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L375

[c4-sponsor]

eswak (sponsor) disputed

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[kadenzipfel]

@eswak is correct in that you can't use a flashloan to execute this attack because you'd have to close it within the same block which would then change the checkpoint returned. However, you can still execute the same attack with a one block loan. We can prove this by the fact that _checkpointsLookup retrieves the earliest checkpoint taken after blockNumber.

function _checkpointsLookup(
    Checkpoint[] storage ckpts,
    uint256 blockNumber
) private view returns (uint256) {
    // We run a binary search to look for the earliest checkpoint taken after `blockNumber`.
    uint256 high = ckpts.length;
    uint256 low = 0;
    while (low < high) {
        uint256 mid = average(low, high);
        if (ckpts[mid].fromBlock > blockNumber) {
            high = mid;
        } else {
            low = mid + 1;
        }
    }

    return high == 0 ? 0 : ckpts[high - 1].votes;
}

We can see by looking at the Governor contract that we _getVotes for the block that the proposal started at:

function _castVote(
        uint256 proposalId,
        address account,
        uint8 support,
        string memory reason,
        bytes memory params
    ) internal virtual returns (uint256) {
        _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Active));

        // @audit retrieves weight from _getVotes at proposalSnapshot blockNumber
        uint256 weight = _getVotes(account, proposalSnapshot(proposalId), params);
        _countVote(proposalId, account, support, weight, params);

        if (params.length == 0) {
            emit VoteCast(account, proposalId, support, weight, reason);
        } else {
            emit VoteCastWithParams(account, proposalId, support, weight, reason, params);
        }

        return weight;
    }

function proposalSnapshot(uint256 proposalId) public view virtual returns (uint256) {
        return _proposals[proposalId].voteStart;
    }

As long as the loan is not closed in the same block, that block checkpoint will remain and be used to retrieve voting weight for the proposal.

Therefore, this attack could proceed as follows:

• Proposal starts
• Attacker takes out a loan sufficient to exceed quorum at e.g. 20% interest rate (conservative)
• Attacker closes the loan in the next block
• Attacker votes for the proposal, exceeding quorum

We can see that even though it's not a flashloan, it still has the same impact for a very low cost which can be conservatively computed as follows:

let quorum = 1,000,000
let interestRate = 0.2
let blockTime = 12 seconds
let yearInSeconds = ~3.154e+7

cost = quorum * interestRate * blockTime / yearInSeconds
cost = 0.0760938491 tokens

With this cost, any governance proposal could be trivially attacked.

[Trumpero]

@kadenzipfel The protocol is intended to handle voting power by checkpoints of balances per block number, so it's legal to increase voting power in just 1 block. Although the cost of a 1-block loan is low, users still need collateral to loan, and they still need to hold this amount of Guild tokens during a block. It's similar with the case where a user buys a large amount of Guild tokens and sells them in the next block (with the cost being slippage).
Additionally, I believe one block loan scenario is very different from the initial report. Flashloan can't manipulate the voting power, so this issue should be invalid.

[kadenzipfel]

@Trumpero

The protocol is intended to handle voting power by checkpoints of balances per block number, so it's legal to increase voting power in just 1 block.

This is precisely why the protocol is vulnerable to governance attacks, as demonstrated above.

Although the cost of a 1-block loan is low, users still need collateral to loan, and they still need to hold this amount of Guild tokens during a block.

Whether or not an attacker has capital available prior to the attack does not affect the cost of the attack. Furthermore, we cannot simply say that because only well capitalized attackers can execute an attack that the risk is then nullified.

It's similar with the case where a user buys a large amount of Guild tokens and sells them in the next block (with the cost being slippage).

Yes, this is another example as to how someone can execute a governance attack on this insecure system.

Additionally, I believe one block loan scenario is very different from the initial report. Flashloan can't manipulate the voting power, so this issue should be invalid.

From the docs: "No new information should be introduced and considered in PJQA. Elaborations of the already introduced information can be considered (e.g. tweaking a POC), from either the Judge or the Warden, but they will only count towards the validity of the issue, not its quality score."

I would argue this falls within "elaborations of the already introduced information" since the core of the vulnerability remains the same: governance can be attacked via a loan. The type of loan is insignificant to the vulnerability, and the flashloan was simply used initially as an example as to how the attack could occur.

[Trumpero]

@kadenzipfel As I said, it's legal to increase voting power in just 1 block. It's allowed by the protocol; Guild tokens need to be held during that block. So it's very different from a flashloan attack.
No more discussion here.