Arcade.xyz - oakcobalt's results

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L342-L347 https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/BaseVotingVault.sol#L96-L101

Vulnerability details

Impact

In NFTBoostVault.sol, delegates' voting power with NFT boosts is affected by a multiplier updated by the manager role. And the update of all delegates' voting power calculated based on the multiplier is dependent on a public function updateVotingPower() intended to be called by users or anyone.

While this setup seems reasonable, there are serious vulnerabilities in voting power accounting in the context of the actual voting process.

(1) Delegates' votes are most likely going to be mis-accounted when a multiplier is updated close to or right before a proposal creation. Because updateVotingPower() might not be settled early enough relative to the proposal creation timestamp, this allows voting powers to be queried during voting based on an older multiplier.

(2) In addition, allowing general users or anyone to call a crucial time-sensitive updateVotingPower() comes with its own risks. For example, the process of calling updateVotingPower() allows users to cherry-pick registrations. Users can easily choose the favorable registrations to update first if the new multiplier is favorable; Or they can choose the registrations from other users they consider a threat first when they see the new multiplier is against other users' interests.

(3) updaterVotingPower() also has its own limitation on intense gas cost, as well as restrictions on the maximum 50 registrations per call. High gas costs strongly disincentive users to act honestly to update all registrations voting power, and it forces users to be in a condition to cherry-pick who to update or not to update. Also, the maximum number of registrations makes it impossible to update all registrations in one call when there are more than 50 registrations. When updateVotingPower() has to be done in more than one transaction, this further increases the risks of untimely updates of voting power, or even no updates.

Proof of Concept

We can see that updateVotingPower() is a public function with 50 maximum number of array of registrations per call. And whenever it is called by a user, the delegates voting power will be synced with the current multiplier with current block.timestamp stored in storage along with their voting power, which is done under the hood in History.sol.

//NFTBoostVault.sol - updateVotingPower()
    function updateVotingPower(address[] calldata userAddresses) public override {
|>      if (userAddresses.length > 50) revert NBV_ArrayTooManyElements();

        for (uint256 i = 0; i < userAddresses.length; ++i) {
            NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[userAddresses[i]];
            _syncVotingPower(userAddresses[i], registration);
        }
    }

(https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L342-L347)

//NFTBoostVault.sol - _syncVotingPower()
...
       votingPower.push(registration.delegatee, delegateeVotes - uint256(change * -1));
...

//History.sol - push()
...
        uint256 blockNumber = block.number << 192;
        uint256 packedData = blockNumber | data;
...
        assembly {
            // Stores packed data in the equivalent of storageData[length]
            sstore(
                add(
                    // The start of the data slots
                    add(storageData.slot, 1),
                    // index where we store
                    index
                ),
                packedData
            )
        }
...

And in the voting process, when a delegate cast a vote, their voting power is queried through queryVotePower() from BaseVotingVault.sol. When qureyVotePower() is called by the core voting governance contract, the proposal's creation timestamp will be passed to query the historical voting power at or before the proposal's creation time.

//CoreVoting.sol - vote()
...
            votingPower += uint128(
                IVotingVault(votingVaults[i]).queryVotePower(
                    msg.sender,
                    proposals[proposalId].created,
                    extraVaultData[i]
                )
            );
...

//BaseVotingVault.sol - queryVotePower()
...
 return votingPower.findAndClear(user, blockNumber, block.number - staleBlockLag);
    }
...

(https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/BaseVotingVault.sol#L96-L101)

//History.sol - findAndClear()
...
 (uint256 staleIndex, uint256 loadedData) = _find(storageData, blocknumber, staleBlock, minIndex, length);
...
//History.sol - _find()
...
        // We load at the final index of the search
        (uint256 _pastBlock, uint256 _loadedData) = _loadAndUnpack(data, minIndex);
        // This will only be hit if a user has misconfigured the stale index and then
        // tried to load father into the past than has been preserved
        require(_pastBlock <= blocknumber, "Search Failure");
        return (staleIndex, _loadedData);

As seen above, the timestamp at which updateVotingPower() stores in storage is crucial in fetching correct voting power snapshots during the voting process. And as described above, there are many cases where this stored timestamp could be arbitrary depending on specific scenarios of user interactions with updateVotingPower().

See a test here as POC to show one scenario where even though all registrations are updated, the queried votes during the voting process are still incorrect: (1) signers[1] registers and delegate to signers[3]; (2) multiplierA changed to multiplierB at tx2.blockNumber; (3) signers[1]'s call to updateVotingPower() settles at tx2.blockNumber + 3; (4) singers[2] registers and delegate to signers[3]; (5) multiplierB changed to multiplierC at tx4.blockNumber; (6) signers[2] update voting power for signers[1]&[2] and call settles at tx4.blockNumber + 3; (7) Separately, Proposal 1 created at tx2.blockNumber +2 and Proposal 2 created at tx4.blockNumber + 2; (8) signers[3] vote for proposal 1 and proposal 2. But votes were queried with an older multiplier for both proposals.

Here's the test result.

  Governance Operations with NFT Boost Voting Vault
    Governance flow with NFT boost vault
voting power 1 after update VP:  1300000000000000000
voting power 2 after update VP:  8400000000000000000
voting power for  proposal 1 (after multiplier update 1):  1200000000000000000
voting power for  proposal 2 (after multiplier update 2):  7800000000000000000
      ✔ delegate votes mis-accounted when a multiplier is updated close to or right before a proposal creation (1854ms)


  1 passing (6s)

Tools Used

Manual Review Hardhat

Recommended Mitigation Steps

(1) It's not ideal to combine updateVotingPower() with setMultiplier() because manager has to bare the full gas cost, and it might also be too gas intensive to include in a single transaction. However, for the sake of fair vote count, the two logic should be combined in some way in the same transaction.

(2) To mitigate gas cost, instead of storing the scaled multiplied votes for each delegate and registration, Only store the historic and current multiplier with block.number, such that the only balances of delegates stored are the unscaled voting power without multipliers.

In addition, NFTBoostVault.sol needs to overwrite BaseVotingVault.sol - queryVotePower() to query both the token amount and a multiplier based on a blockNumber of a proposal creation. Then return the multiplied scaled voting power to the actual voting accounting process.

Assessed type

Governance

1. In ArcadeTreasury.sol, the cool-down period restrictions can be bypassed, by updating setThreshold with a thresholds.small value below current allowance.

In setThreshold(), when the new thresholds.small value is less than existing GSC allowance gscAllowance[token], gscAllowance will be reset. This bypass the mandatory cool-down period control in setGSCAllowance().

I consider this low severity because Admin role should be considered trusted in setting critical parameters. However, whenever setThreshold() is called with a smaller threshold, this side effect of updating gscAllowance before a cool-down period ending might be considered undesirable.

//ArcadeTreasury.sol - setThreshold()
...
        // if gscAllowance is greater than new small threshold, set it to the new small threshold
        if (thresholds.small < gscAllowance[token]) {
|>          gscAllowance[token] = thresholds.small;

            emit GSCAllowanceUpdated(token, thresholds.small);
        }
...

//ArcadeTreasury.sol - setGSCAllowance()
...
        // enforce cool down period
        if (uint48(block.timestamp) < lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN) {
            revert T_CoolDownPeriod(block.timestamp, lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN);
        }
...

Recommendation: Consider add a cool-down period check inside setThreshold() as well.

2. No minimal Multiplier value check in setMultiplier() function

In NFTBoostVault.sol, a manager can set a multiplier with a maximum of 1.5e3, and the minimum multiplier value should be 1e3. However, there are only checks for max. value is enforced in setMultiplier() and nothing to prevent a value lower than 1e3 to be set.

    function setMultiplier(address tokenAddress, uint128 tokenId, uint128 multiplierValue) public override onlyManager {
        if (multiplierValue > MAX_MULTIPLIER) revert NBV_MultiplierLimit();

        NFTBoostVaultStorage.AddressUintUint storage multiplierData = _getMultipliers()[tokenAddress][tokenId];
        // set multiplier value
        multiplierData.multiplier = multiplierValue;

        emit MultiplierSet(tokenAddress, tokenId, multiplierValue);
    }

Recommendation: Add a check for min value, in case a value lower than 1e3 is accidentally passed.

3. No checks for minimal spending threshold values enforced in spend or approve functions

In ArcadeTreasury.sol, by design there are three threshold of spending or funds approval for governance or GSC spending - small, medium and large. However, only the upper bound for each level of spending is enforced, no lower bound is enforced. This means that governance can use largeSpend() or mediumSpend() to execute small amount of spending.

This creates overlaps between multiple functions including smallSpend(), mediumSpend(), largeSpend(), approveSmallSpend(),approveMediumSpend(),approveLargeSpend(). Under some conditions, multiple functions can be interchangeable for users causing confusion.

    function mediumSpend(
        address token,
        uint256 amount,
        address destination
    ) external onlyRole(CORE_VOTING_ROLE) nonReentrant {
        if (destination == address(0)) revert T_ZeroAddress("destination");
        if (amount == 0) revert T_ZeroAmount();

        _spend(token, amount, destination, spendThresholds[token].medium);
    }

    function _spend(address token, uint256 amount, address destination, uint256 limit) internal {
        // check that after processing this we will not have spent more than the block limit
        uint256 spentThisBlock = blockExpenditure[block.number];
        if (amount + spentThisBlock > limit) revert T_BlockSpendLimit();
        blockExpenditure[block.number] = amount + spentThisBlock;
...

Recommendation: Consider adding minimal spending threshold checks in medium and large spending functions, to ensure clear distinctions in spending limit tiers.

Any comments for the judge to contextualize your findings

• Finding edge cases in vault contracts, with a special focus on NFTBoostVault.sol exploring scenarios where multiplier might be mis-accounted in vote counts.
• Finding edge cases in ArcadeTreasury.sol that lead to loss of funds.
• The particularity of core voting and GSC voting process and how the two processes might lead to unintended behaviors or loss of funds in ArcadeTreasury.sol.
• Finding general vulnerabilities that might lead to unintended behaviors or disruption in governance actions.
• Explore attack scenarios that might lead to DOS or loss of funds.

Approach taken in evaluating the codebase

• Manual code walkthrough
• Custom scripts

Architecture recommendations

Consider creating direct interactions between NFT contracts and NFTBoostVault.sol.

Current contract implementations in ReputationBadge.sol and NFTBoostVault.sol segregate each other. For example, an NFT can be minted in ReputationBadge.sol for a user. But if this NFT is qualified for voting power boost, a user has to interact with NFTBoostVault.sol to add the NFT and delegate.

Instead, consider allowing ReputationBadge.sol to interact with NFTBoostVault.sol to add NFT for users who have voting power but no linked NFT.

In addition, consider allowing multiple NFT boosts for one registration in NFTBoostVault.sol. This will allow multiple NFTs to be added to the vault, further increasing the interactions between NFT and Vault contracts, at the same time, increasing user experience.

Codebase quality analysis

• Sufficient contract and function documentation.
• Good test coverage.
• Occasional wasteful gas operation observed.
• Occasional unnecessary complexity in on-chain storage structure: In ArcadeTreasury.sol, blockExpenditure is a mapping that stores the amount of token spent and approved every time with current block.number as the key. This mapping variable stores historical block.number with a corresponding value in storage. However, there is currently no on-chain query on historical values, nor is there event emitted with block.number to allow off-chain accounting to take advantage of the mapping structure. Consider either removing the mapping variable and only storing the current block value, or if there is a need for historical values, re-write event to allow emmitting with block.number as event args so that proper off-chain accounting and queries can be done.

Centralization risks

Low

Mechanism review

In ArcadeTreasury.sol, some code might have overlapping functionalities under certain conditions. The distinction between smallSpend(), mediumSpend() and largeSpend() might not be as clear when governance can use largeSpend() to spend a small amount. See details in the reports.

Systemic risks

In ArcadeTreasury.sol, ERC20 approve() is used with every governance or GSC-related approval action. There are known attack scenarios with ERC20 approve function. See details in the reports.

Time spent:

25 hours