code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L418-L427

Vulnerability details

Impact

User's votes will be nullified after updating the new NFT. This will be possible if that NFT is yet to get assigned with multiplier value.

Though the NFT can be updated with new multiplier value, but, at times it would be serious risk to the governance based mechanism. In certain situations, there might be malicious proposal gonna be happen, but it should be stopped by counter voting. Since some of users voting power is nullified temporarily, they can not oppose the malicious proposal.

Proof of Concept

NFTBoostVault has multiplier based vote allocation. By default the multiplier value is 1e3. This value would be more for NFT deposit. Incase of freshly adding the NFT , the function addNftAndDelegate is called and NFT is added with registration by calling the function..

When we look at the function

    function _registerAndDelegate(
        address user,
        uint128 _amount,
        uint128 _tokenId,
        address _tokenAddress,
        address _delegatee
    ) internal {
        uint128 multiplier = 1e3;

        // 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(); ----->> @@audit : revert happen for zero multiplier.

The above revert is done to ensure that user will get base voting power with respect to their deposited token amounts.

Now, lets look at the update NFT case.

Contract has the function updateNft function to update new NFT. the voting power is updated based on the NFT multiplier value.

Lets look at the code flow,

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();

    NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];

    // If the registration does not have a delegatee, revert because the Registration
    // is not initialized
    if (registration.delegatee == address(0)) revert NBV_NoRegistration();

    // if the user already has an ERC1155 registered, withdraw it
    if (registration.tokenAddress != address(0) && registration.tokenId != 0) {
        // withdraw the current ERC1155 from the registration
        _withdrawNft();
    }

    // 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); --------------------------------->> after adding new NFT, vote update will be done here.
}

at the end of the updateNft function, vote will be updated by calling the function _syncVotingPower. The _syncVotingPower function calls the _currentVotingPower function to call the current voting power in order to update the voting power of user and their delegatee.

function _syncVotingPower(address who, NFTBoostVaultStorage.Registration storage registration) internal {
    History.HistoricalBalances memory votingPower = _votingPower();
    uint256 delegateeVotes = votingPower.loadTop(registration.delegatee);

    uint256 newVotingPower = _currentVotingPower(registration);
    // get the change in voting power. Negative if the voting power is reduced

now lets check the _currentVotingPower function, it uses the getMultiplier function to get the multiplier value for the NFT. Here the issue comes, if the new NFT is not assigned with any multiplier value, this funtion would return the current voting power has zero.

    function _currentVotingPower(
        NFTBoostVaultStorage.Registration memory registration
    ) internal view virtual returns (uint256) {
        uint128 locked = registration.amount - registration.withdrawn;

        if (registration.tokenAddress != address(0) && registration.tokenId != 0) {
            return (locked * getMultiplier(registration.tokenAddress, registration.tokenId)) / MULTIPLIER_DENOMINATOR;
        }

        return locked;
    }

This way the updateNft would nullify the voting power of an user by calling the _syncVotingPower

Tools Used

Manual review.

Recommended Mitigation Steps

Update the getMultiplier functioin as shown below.


    function getMultiplier(address tokenAddress, uint128 tokenId) public view override returns (uint128) {
        NFTBoostVaultStorage.AddressUintUint storage multiplierData = _getMultipliers()[tokenAddress][tokenId];

        // if a user does not specify a ERC1155 nft, their multiplier is set to 1
        if (tokenAddress == address(0) || tokenId == 0) {
            return 1e3;
        }

        if(multiplierData.multiplier == 0) --------->> updated
           return 1e3;

        return multiplierData.multiplier;
    }

Assessed type

Governance

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #214

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)