Arcade.xyz - 0xbranded's results

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L363 https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/external/council/CoreVoting.sol#L244-L248

Vulnerability details

Impact

As mentioned in the documentation, setting a new multiplier doesn't directly update users' voting power. Users can update the voting power of other users externally, but this is not ideal since it requires these users to burn gas and proposals may be voted on with an incorrect multiplier before the multiplier is updated.

If this happens, users will be unable to update any ballots that were cast when they voted with an old multiplier that was changed but not yet updated. Further, proposal votes will not reflect the true voting power of participants and won't be updated once cast, even if the voting power is later updated. While users have an incentive to update their own voting power before voting when the multiplier increases, when it decreases they may be able to vote on proposals with their previous (higher) voting power before it is set according to the new multiplier.

Proof of Concept

In the below code, a user calls addNftAndDelegate with his tokens and ERC1155 in the NFTBoostingVault. The multiplier is then updated, and he creates a proposal before someone calls updateVotingPower using his address. The result is the ballot still has the incorrect number of votes attached to it, and he can not update his ballot to reflect his new votingPower, or to change his vote.

He is unable to change his votes due to underflow in coreVoting resulting from his old votes being higher than his new ones.

            const ONE = ethers.utils.parseEther("1");
            const { arcdToken, arcdDst, deployer } = ctxToken;
            const {
                signers,
                coreVoting,
                increaseBlockNumber,
                nftBoostVault,
                reputationNft,
                reputationNft2, // other ERC1155 reputation NFT w/ different multiplier
                mintNfts,
                setMultipliers,
                feeController,
            } = ctxGovernance;

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

            // mint tokens take tokens from the distributor for use in tests
            await expect(await arcdDst.connect(deployer).toPartnerVesting(signers[0].address))
            .to.emit(arcdDst, "Distribute")
            .withArgs(arcdToken.address, signers[0].address, ethers.utils.parseEther("32700000"));
        
            expect(await arcdDst.vestingPartnerSent()).to.be.true;
        
            // transfer tokens to signers and approve locking vault to spend
            for (let i = 0; i < signers.length; i++) {
                await arcdToken.connect(signers[0]).transfer(signers[i].address, ONE.mul(100));
            }

            // manager sets the value of the reputation NFT multiplier
            const { MULTIPLIER_A, MULTIPLIER_B } = await setMultipliers();
            const MULTIPLIER_DENOMINATOR = 1e3;

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

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

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

            const INCREASED_MULTIPLIER = MULTIPLIER_A.add(ethers.BigNumber.from('300'))
            const DECREASED_MULTIPLIER = MULTIPLIER_A.sub(ethers.BigNumber.from('300'))

            // multiplier is set back to default value of 1e3
            await nftBoostVault.connect(signers[0]).setMultiplier(reputationNft.address, 1, INCREASED_MULTIPLIER);

            // junk call solely for the purposes of PoC of voting power
            let ABI = ["function junk(address who)"];
            let iface = new ethers.utils.Interface(ABI);
            const calldata = [iface.encodeFunctionData("junk", [signers[1].address])]
            
            // signers[0] votes on proposal after multiplier was updated, but before his voting power was
            await coreVoting.connect(signers[0]).proposal([nftBoostVault.address],[ethers.constants.HashZero], [signers[1].address], calldata,
                                                    Date.now() * 2, 0)
            const proposalId = (await coreVoting.connect(signers[0]).proposalCount()).sub(1);


            // voting power is updated and its value verified to ensure the base multiplier was used
            const tx2 = await nftBoostVault.updateVotingPower([signers[0].address]);
            const increasedVotingPower = await nftBoostVault.queryVotePowerView(signers[0].address, tx2.blockNumber);
            expect(increasedVotingPower).eq(ONE.mul(3).mul(INCREASED_MULTIPLIER).div(MULTIPLIER_DENOMINATOR));
            
            const votes = await coreVoting.connect(signers[0]).getProposalVotingPower(proposalId);

            // proposal's voting power is still set to its old value
            expect(votes[0]).eq(votingPower)

            // user can not update his vote or change it
            await expect(coreVoting.vote([nftBoostVault.address], [ethers.constants.HashZero], proposalId, 0)).reverted
            await expect(coreVoting.vote([nftBoostVault.address], [ethers.constants.HashZero], proposalId, 1)).reverted

            // voting power is now decreased
            await nftBoostVault.connect(signers[0]).setMultiplier(reputationNft.address, 1, DECREASED_MULTIPLIER);
            const tx3 = await nftBoostVault.updateVotingPower([signers[0].address]);
            const decreasedVotingPower = await nftBoostVault.queryVotePowerView(signers[0].address, tx3.blockNumber);
            expect(decreasedVotingPower).eq(ONE.mul(3).mul(DECREASED_MULTIPLIER).div(MULTIPLIER_DENOMINATOR));

            // user still cannot update his vote, as long as the multiplier was different
            await expect(coreVoting.vote([nftBoostVault.address], [ethers.constants.HashZero], proposalId, 0)).reverted
            await expect(coreVoting.vote([nftBoostVault.address], [ethers.constants.HashZero], proposalId, 1)).reverted

        });

Tools Used

Hardhat

Recommended Mitigation Steps

Although it was mentioned the voting power is not updated when voting due to gas costs, it is necessary in order to ensure the correctness of voting. A middle ground approach can be taken, where the votes are synchronized only if necessary. This can be done by overriding (and making virtual) the BaseVotingVault queryVotePower functions in NFTBoostVault.

A "multiplier" value can be added to the registration struct. Since multipliers are in base 1e3, a smaller uint size up to uint96 can be added to the end of the struct, in order to pack the struct and maintain the same word size. A check can now be added to queryVotePower whether the registration multiplier differs from _getMultipliers()[tokenAddress][tokenId].multiplier, and only if so call _syncVotingPower (which now updates the registration multiplier as well). This check will add a small gas cost to all votes and queries, but it will ensure all votes are done with their lastest, correct value according to the current multiplier.

Assessed type

Under/Overflow