Proposal vote power can be easily manipulated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeGSCCoreVoting.sol#L32 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L172-L181 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L234-L238

Vulnerability details

Impact

ArcadeGSCCoreVoting can be a target of vote manipulation: an attacker might be able to take a huge loan (even uncollateralized) for a single block before creating a proposal.

When voting, only this single block is checked when calculating the votingPower: this may lead to an attacker being able to execute arbitrary proposals with minimal risks involved.

Proof of Concept

When a proposal is created, the timestamp registered is the block before the creation. This mitigates flash loan attacks, but it's still possible to manipulate the vote with a normal loan:

proposals[proposalCount] = Proposal(
    proposalHash,
    // Note we use blocknumber - 1 here as a flash loan mitigation.
    uint128(block.number - 1), //@audit created
    uint128(block.number + lockDuration),
    uint128(block.number + lockDuration + extraVoteTime),
    uint128(quorum),
    proposals[proposalCount].votingPower,
    uint128(lastCall)
);

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L172-L181

During a vote, the msg.sender voting power is queried and it will use the previous created field:

for (uint256 i = 0; i < votingVaults.length; i++) {
    // ensure there are no voting vault duplicates
    for (uint256 j = i + 1; j < votingVaults.length; j++) {
        require(votingVaults[i] != votingVaults[j], "duplicate vault");
    }
    require(approvedVaults[votingVaults[i]], "unverified vault");
    votingPower += uint128(
        IVotingVault(votingVaults[i]).queryVotePower(
            msg.sender,
            proposals[proposalId].created,
            extraVaultData[i]
        )
    );
}

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L234-L238

If the attacker took a huge loan (even uncollateralized) for that single block, they would be able to manipulate the vote with minimal slippage, as it's only one block.

If this happens, they would be able to execute any arbitrary code, if queryVotePower depends on the amount of tokens held by the attacker.

Note about severity

By reading the comments in ArcadeGSCCoreVoting it seems that the voting vault used will be the ArcadeGSCVault:

 * The Arcade GSC Core Voting contract allows members of the GSC vault to vote on and execute proposals
 * in an instance of governance separate from general governance votes.

In this case, this issue can't occur, as the votingPower does not depend on the amount held by the attacker:

// If the address queried is the owner they get a huge number of votes
// This allows the primary governance timelock to take any action the GSC
// can make or block any action the GSC can make. But takes as many votes as
// a protocol upgrade.
if (who == owner) {
    return 100000;
}
// If the who has been in the GSC longer than idleDuration
// return 1 and otherwise return 0.
if (
    members[who].joined > 0 &&
    (members[who].joined + idleDuration) <= block.timestamp
) {
    return 1;
} else {
    return 0;
}    

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/vaults/GSCVault.sol#L145-L167

However, it's worth noting that this situation might change in the future, as ArcadeGSCCoreVoting approved vaults can be multiple, and they are not immutable:

/// @notice Updates the status of a voting vault.
/// @param vault Address of the voting vault.
/// @param isValid True to be valid, false otherwise.
function changeVaultStatus(address vault, bool isValid) external onlyOwner {
    approvedVaults[vault] = isValid;
}

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L333-L338

As there are other vaults that use tokens as voting power (e.g LockingVault), there is a real possibility that this might occur in the future, so I'm flagging it as high severity.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using a TWAP to check the voting power of a proposal, instead of checking only the block before the proposal was created.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

PowVT marked the issue as sponsor acknowledged

[c4-sponsor]

PowVT marked the issue as sponsor confirmed

[c4-sponsor]

PowVT marked the issue as disagree with severity

[PowVT]

Should make adding approved vaults to the ArcadeGSC contract revert. Will only use the GSCVault here and can eliminate any potential for the scenario described above. I would mark it as medium as this is not the intended usage of the GSCCoreVoting contract and there are high custom quorums for sensitive actions like approving a new vault.

[0xean]

Will only use the GSCVault here and can eliminate any potential for the scenario described above.

Based on this being a vote among the steering council I think M is the correct severity here.

• The Arcade GSC Core Voting contract allows members of the GSC vault to vote on and execute proposals
  • in an instance of governance separate from general governance votes.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[T1MOH593]

I would argue this issue is medium. Report says attacker can take loan in certain block to take most of the vote power. I argue that if attacker can lend this amount, he already owns this amount and TWAP won't save protocol, just requires this funds to be frozen in this governance token for several blocks to become available in voting. But it doesn't mitigate described attack.

Lending market especially in blockchain is overcollateralized, you can't lend more than deposited. I don't see difference between described attack vector and 51% attack which is obviously out of scope. Issue is more suitable in analysis report

[DadeKuma]

Lending market especially in blockchain is overcollateralized, you can't lend more than deposited

This is false, undercollateralized lending exists.

TWAP itself doesn't make the protocol immune to multi-block attacks, but it definitely helps mitigate the issue by making them exponentially difficult to perform in comparison to a single-block check.

The solution proposed by the sponsor mitigates this issue if multiple vaults are not required:

Should make adding approved vaults to the ArcadeGSC contract revert. Will only use the GSCVault here and can eliminate any potential for the scenario described above. I would mark it as medium as this is not the intended usage of the GSCCoreVoting contract and there are high custom quorums for sensitive actions like approving a new vault.

[T1MOH593]

1) From the economics view there is no undercollateralized lending. What you refer is more similar to real life lending, where you provide collateral not immediately, but undertake to repay it in future. In other words you can't lend more than you will have to the end of repay, no one will give you money for free. Otherwise it is just crowdfunding for 51% attack.

2) Compound-like governance framework and ERC20Votes is at least battle tested and they use similar approach in counting votes. Can it be medium vulnerability?

As I understand (but can be wrong) proposed solution by sponsor blocks usage of vaults with voting tokens. But I argue that these vaults are not vulnerable

[PowVT]

To add another note here, since there is no way to override the changeVaultStatus in CoreVoting because it is not virtual. The other easier fix would be to set the customQuorum for interacting with this function unobtainably high. This is more than likely the direction we will pursue since no contract changes needed.