Arcade.xyz - 0x3b's results

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L363-L371

Vulnerability details

Impact

setMultiplier as the name suggests set a new multiplier for a token address of a given token ID, but it does not sync the voting power afterwards. This can be exploited by voters who after the update have received lower boost (and thus lower voting power), by them not updating their voting power.

Proof of Concept

setMultiplier is a function used by the manager of NFTBoostVault to set a new voting power for a NFT and it's corresponding token ID. However after this change no measure is done to enforce it, and voters and delegates are left with the old (before setMultiplier was called) voting power. This means if an owner deliberately want to lower to voting power of a certain NFT he can set the boost ration of that NFT to a lower amount, but users with that NFT will still receive the prev. boost, unless they are synced manually.

Example:

1. A given faction has 100 000 tokens and NFTs of type cow that gives 150% voting power => 150 000 vote tokens
2. Owner doesn't like cows (for some reason) so he lowers the voting power of NFT cow to 120%
3. The group of user does not care and can vote with the prev. voting power, since the onwer needs to sync their voting power manually, user by user, and thy may cost a fortune

Tools Used

Manual review

Recommended Mitigation Steps

Add a modifier that before every call updates the voting power of the msg.sender

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/token/ArcadeAirdrop.sol#L75-L79

Vulnerability details

Impact

User who have claimed before an update on the merkle tree in ArcadeAirdrop are not gonna be able to claim the extra rewards. The reward tokens will be left stuck in the contract, waiting for the manager to reclaim them.

Proof of Concept

Due to the nature of how ArcadeAirdrop is made and the expectation that there might be a change in the merkle tree in the near future, users will expect to be able to claim the extra rewards (if they are increase on merkle root change). However, because there is a problematic if stamen (that is inherited in ArcadeAirdrop by ArcadeMerkleRewards) users that have already claimed their rewards will have no share of the future ones.

Example:

1. Alice and Bob bolt have 100 tokens set to be claimed on the Merkle tree
2. Alice claims her 100 tokens
3. The owner sees that there is a huge activity on the platform and wants to incentivize users, so he increases rewards (by changing the merkle root) to 120 tokens
4. Bob claims his 120 tokens
5. Alice sees that and tries to claim the remainder of her 120 tokens (20), but the claim reverts because of this if

    function _validateWithdraw(uint256 totalGrant, bytes32[] memory merkleProof) internal {
        ...
        //@audit if the user has already claimed, he is not able to claim ever again
        if (claimed[msg.sender] != 0) revert AA_AlreadyClaimed();
        claimed[msg.sender] = totalGrant;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Change the claim implementation the way how mint in ReputationBadge is done. They have the total amount in the merkle tree and track claimed amount in a mapping.

Assessed type

DoS

Summary

Tho the given scope was not much ~1k, my allocated time was also not enough for a deep dive to be made in every concept used by Arcade. From my time spend on the code and the current understating of it I can say that is seems secure enough, and it's gonna be even more secure after the c4 audit. The protocol implements some interesting topics, like different vaults for keeping track/increasing voting power. It also featured a quite unique concept of ERC1155 NFTs used to boost the voting power in one of the vaults. Overall the code quality was good and there were some minor bugs and improvements detected by me, but so far nothing protocol breaking.

Time allocations

Findings

A diverse range of findings was presented for review, categories such as logical errors, code errors, and recommendations and even some optimizations and improvements that the devs can do. Nonetheless, the late entry that I had into the competition, ment that there was not enough time to delve deeply into any of the concepts.

Codebase quality

Code was interesting and of high quality, this may be because there was an audit before the one in c4, but nonetheless it had good and stable concepts, multiple variations of vaults (always good to have an option ), each one with it's own advantages and disadvantages. The code also featured a spending system that was very interesting, although the bad part is that the main voting mechanism were not in scope. It would have been a pleasure for me to dive into them, but we are gonna save it for the next audit hopefully.

Mechanism review

Vaults

The code featured many different implementations of vaults, each with it's own advantages and disadvantages . As the seeing the differences within the vaults it was presumed that NFTBoostVault was the most advanced part, so his allocated share of the time was greatest. Another interesting vault was ARCDVestingVault where grant were given to users. This system was not the most decentralized one, as one's manager needed to allow a user to have voting power, but it too seemed interesting. As always the base as BaseVotingVault was the most simple and secure one.

The one that got my eye was NFTBoostVault, not only because it was the biggest solc, but also because it featured quite unique boosting mechanism. The mechanism was interesting mainly because it used NFTs, the 1155 type, the idea was good, but there was one negative, and in was in the way that the owner needed manually to set the NTF address and it's own unique ID to a boost percentage. This may be gas costly as many NFT collections have 1k - 10k NFTs in them, but if the arcade collection has less than 500 I think it would not be that big of an issue.

Tokens

As there is not much code with the tokens there is not much to say about them. The distributor was interesting in a way that is was a single use contract. But it was quite simple, and simple things are generally secure, as there is not much to break within them. ArcadeToken was a nice edition to the system as it featured limited supply minting, as in a way not to inflate the token too much (as most projects do) and with an inflation rate of 2%. Finally ArcadeAirdrop was most out of scope, due to ArcadeMerkleRewards being out of scope, but nonetheless the contract is simple enough not to leak value.

NFTs

As the shortest of them all the NFT part it didn't offer much, as there were only a few function users can use (mint), although it used ERC1155 which was a nice improvement that make it different from the rest of projects that use NFTs (as they include them just to show off to the users that they have an NFT). One improvement in mint that i can suggest is to return the rest of msg.value to the users after the mint, as in the current implementation that is not done.

Architecture recommendations

There is not much to be added to what Arcade already has. As in the current scope that was given there was use of advanced NFTs (ERC1155), ERC20 tokens and a few different implementations of vaults. There is not much more needed for a stable system, as in solidity more is not always better, because more code and more thing introduces more risk, that is not linear, but exponential as complexity increases.

Centralization risk

From the perspective of the scope that we were given Arcade seemed quite centralized, due to the extensive use of onlyOwner/onlyManager, and close to 50% of the function being only accessible with one of these roles. Since I have not looked deeper into the out of scope system I cannot say for sure how centralized or decentralize the whole system is, however from seeing that some contracts (a brief look on out of scope code) implement a voting mechanism and out give scope if for the contracts that allocate the voting power, I can assume that Arcade is at least decentralized enough for any user to feel safe, and more decentralized than the normal protocols.

Systemic risks

For now the risk factor seems under control, as one audit was already done and currently a second one in c4 is near completion. Generally for systems with lack of intense complexity one, two or maximum three audits should be enough to cover the major faults.

Time spent:

17 hours