`NFTBoostVault::updateNft` allows any **ERC1155** to be supplied; doing so will slash voting power; users may be phished into losing their voting power

[code423n4]

Lines of code

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

Vulnerability details

Issue details

The core community voting vault for governance enables token-weighted vote counting with delegation and an NFT "boost". NFT boost allows certain ERC1155 assets to receive "multipliers"; when users deposit those NFTs, the voting power of their deposited ERC20 tokens are boosted by multiplier. As such there is a restriction that when a users registers into the vault (with tokens and/or NFT) only specific allowed ERC1155 are allowed (NFTs that have been attributed multipliers by the team).

When first registering, via NFTBoostVault::addNftAndDelegate, the provided ERC1155 token for staking is checked as only specific NFTs are allowed:

    // confirm that the user is a holder of the tokenId and that a multiplier is set for this token
    if (_tokenAddress != address(0) && _tokenId != 0) {
        if (IERC1155(_tokenAddress).balanceOf(user, _tokenId) == 0) revert NBV_DoesNotOwn();

        multiplier = getMultiplier(_tokenAddress, _tokenId);

        if (multiplier == 0) revert NBV_NoMultiplierSet();
    }

with the observation that getMultiplier returns 0 multiplier for non-tracker NFTs. As such, an NBV_NoMultiplierSet is raised and transaction is reverted when an invalid ERC1155 is provided.

Users are also allowed to update their booster NFT with a new one or add one if they didn't have one via the NFTBoostVault::updateNft. It is in this function that the issues exists as the function does not check what ERC1155 is provided, only checks that it is owned by the user.

    function updateNft(uint128 newTokenId, address newTokenAddress) external override nonReentrant {
        if (newTokenAddress == address(0) || newTokenId == 0) revert NBV_InvalidNft(newTokenAddress, newTokenId);

        if (IERC1155(newTokenAddress).balanceOf(msg.sender, newTokenId) == 0) revert NBV_DoesNotOwn();

The NFT/token is then saved to the registration, transferred to the vault and the voting power is updated to take it into consideration.

        // set the new **ERC1155** values in the registration and lock the new ERC1155
        registration.tokenAddress = newTokenAddress;
        registration.tokenId = newTokenId;

        _lockNft(msg.sender, newTokenAddress, newTokenId, 1);

        // update the delegatee's voting power based on new **ERC1155** nft's multiplier
        _syncVotingPower(msg.sender, registration);
    }

This brings up the second issue that the new voting power will be slashed/set to 0, effectively user will lose any voting power up to this point.

Voting power is slashed because _syncVotingPower has a call chain of:

• _syncVotingPower -> sets last voting power to the new calculated voting power
  • _currentVotingPower -> returns 0 as the new calculate voting power
    • getMultiplier -> returns 0

Impact

• If a user mistakenly updates the registration with any random ERC1155 he will loose any power up to that point and will not be able te reclaim his tokens (if also staked) until withdraws are allowed.
• User can also be socially engineered by a malicious actor into thinking that he is indeed updating with a valid NFT while actually loosing his voting rights. This can be carried out as part an elaborate governance attack. The malicious actor can also block NFT withdrawal
• This also breaks protocol assumptions/invariant that users are allowed to have staked only approved NFTs (those who have been set a multiplier).

Proof of Concept

A theoretical POC to a governance a attack:

• bob wants to have a malicious proposal passed, so he creates and proposes one
• bob then creates special ERC1155 NFTs made to look like valid project tokens
• bob now has several scenarios he can play to have the community allow his proposal to pass:
  • bob pretends to represent the community and alerts users that a governance attack is underway and explains why every major holder has been given a special booster NFT airdrop to combat this attack. Users will update their NFTs effectively loosing their voting power
  • bob compromises the discord server (or any other phishing tactic) to "gift" the NFTs to users and incentives them to update them
• alice (normal users) sees the new information and want to stop the attack to happen so she updates her boosting NFT and loses her voting rights
• alice realizes what happened but can't get her voting power back. Se tries to withdraw her tokens in order to restake them and get the voting power back but she can't because withdraws are not active
• eve (the team) realizes this and initiates the withdraws activation procedure but since this is gated by a timelock it does not make it in time and bob's proposal passes
• alice further realizes that she can update her NFT again to regain her voting power but the malicious ERC1155 will now fail any safeTransferFrom and balanceOf operation, thus withdraw, withdrawNft,updateNft all revert, since they all will reach one of the 2 calls.

A coded POC follows, add it to test\NftBoostVault.ts plus the import

import { MockERC1155 } from "../src/types";

at the beginning of the file

        it("User can wrongly call updateNft() with any **ERC1155** and it will slash rewards", async () => {
            const { arcdToken } = ctxToken;
            const {
                signers,
                nftBoostVault,
                reputationNft,
                mintNfts,
                setMultipliers,
            } = ctxGovernance;

            // mint users some reputation nfts
            await mintNfts();

            // also mint a random **ERC1155** which is not set a multiplier to
            const randomERC1155 = <MockERC1155>await deploy("MockERC1155", signers[0], []);
            await randomERC1155.deployed();
            await randomERC1155.mint(`${signers[0].address}`, 1, 1);

            // manager sets the value of the reputation NFT multiplier
            const { MULTIPLIER_A } = await setMultipliers();

            // signers[0] approves tokens to NFT boost vault and approves reputation nft
            await arcdToken.approve(nftBoostVault.address, ONE);
            await reputationNft.setApprovalForAll(nftBoostVault.address, true);

            // signers[0] registers reputation NFT, deposits tokens and delegates to signers[1]
            const tx = await nftBoostVault.addNftAndDelegate(ONE, 1, reputationNft.address, signers[1].address);

            const votingPower = await nftBoostVault.queryVotePowerView(signers[1].address, tx.blockNumber);
            expect(votingPower).to.be.eq(ONE.mul(MULTIPLIER_A).div(MULTIPLIER_DENOMINATOR));

            // signers[0] approves reputation random **ERC1155** NFT to voting vault
            await randomERC1155.setApprovalForAll(nftBoostVault.address, true);

            // signers[0] updates their reputation nft to random **ERC1155** which has no multiplier
            const tx2 = await nftBoostVault.updateNft(1, randomERC1155.address);

            // they are now again holding the first reputation nft which they have replaced
            const vaultRandomErc1155Bal = await randomERC1155.balanceOf(nftBoostVault.address, 1);
            expect(vaultRandomErc1155Bal).to.be.eq(1);

            // their delegatee voting power is updated based on the multiplier value of their new **ERC1155** nft
            // which in this case is 0 since the delegatee has no other delegations to himself
            const votingPower2 = await nftBoostVault.queryVotePowerView(signers[1].address, tx2.blockNumber);
            expect(votingPower2).to.be.eq(0);
        });

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a check that when calling NFTBoostVault::updateNft the provided ERC1155 token has a multiplier attached to it. If the NFT is not tracked, raise an NBV_NoMultiplierSet error. Example implementation:

diff --git a/contracts/NFTBoostVault.sol b/contracts/NFTBoostVault.sol
index 5f907ee..254d9a8 100644
--- a/contracts/NFTBoostVault.sol
+++ b/contracts/NFTBoostVault.sol
@@ -307,6 +307,10 @@ contract NFTBoostVault is INFTBoostVault, BaseVotingVault {

         if (IERC1155(newTokenAddress).balanceOf(msg.sender, newTokenId) == 0) revert NBV_DoesNotOwn();

+        uint128 multiplier = getMultiplier(newTokenAddress, newTokenId);
+
+        if (multiplier == 0) revert NBV_NoMultiplierSet();
+
         NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];

         // If the registration does not have a delegatee, revert because the Registration

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #214

[c4-judge]

0xean changed the severity to QA (Quality Assurance)