Munchables - Stormreckson's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177-L207 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L210-L242

Vulnerability details

The USD price update proposal contract allows roles to vote both for and against a proposal, which can lead to manipulation of the approval and disapproval counts. This vulnerability undermines the integrity of the voting process.

The USD price update proposal contract allows each role to cast both an approval and a disapproval vote for the same proposal. This can result in inconsistent and inaccurate counts for approvals and disapprovals, leading to the following issues:

• Proposals may be incorrectly approved or disapproved based on manipulated vote counts.
• Roles can effectively neutralize their own votes, causing confusion and deletions of correct proposals.

Even if roles are to be trusted,this should still be prevented in case of unforeseen circumstances like the roles losing access to bad actors.

Impact

Roles can manipulate the approval and disapproval counts. Proposals might get approved or disapproved based on inaccurate counts. Also can lead to deletion of correct proposals.

Proof of Concept

Currently the threshold for approvals and disapprovals is 3 which can still change. https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L16-L18

    uint8 APPROVE_THRESHOLD = 3;
   
    uint8 DISAPPROVE_THRESHOLD = 3;

The proposer automatically gets one approval because they approve their own proposal, which indicates there's only 2 approvals left for the proposal to be executed. However Roles can first disapprove then approve the proposal which will lead to situations where legitimate proposals are deleted in _execUSDPriceUpdate function because of the check https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L507-L510

if (
            usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD &&
            usdUpdateProposal.disapprovalsCount < DISAPPROVE_THRESHOLD
        ) {

This indicates if the approval threshold is met and the disapproval threshold is also met the proposal will get deleted by skipping the price update block https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L506-L528

function _execUSDPriceUpdate() internal {
        if (
            usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD &&
            usdUpdateProposal.disapprovalsCount < DISAPPROVE_THRESHOLD
        ) {
            uint256 updateTokensLength = usdUpdateProposal.contracts.length;
            for (uint256 i; i < updateTokensLength; i++) {
                address tokenContract = usdUpdateProposal.contracts[i];
                if (configuredTokens[tokenContract].nftCost != 0) {
                    configuredTokens[tokenContract].usdPrice = usdUpdateProposal
                        .proposedPrice;

                    emit USDPriceUpdated(
                        tokenContract,
                        usdUpdateProposal.proposedPrice
                    );
                }
            }

            delete usdUpdateProposal;
        }
    }
}

The current implementation allows a role to disapprove a proposal and approve then it without proper handling, leading to inflated counts for both approvals and disapprovals. https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177-L207

function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L210-L242

function disapproveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyDisapprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.disapprovalsCount++;
        usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;

        emit DisapprovedUSDPrice(msg.sender);

        if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
            delete usdUpdateProposal;

            emit RemovedUSDProposal();
        }

First the Role will disapprove since there's a check restricting the role to disapprove if they've already approved

(usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();

The Role can now call approveUSDPrice as there's no restriction stopping them from approving after disapproving the proposal.

Recommended Mitigation Steps

To prevent this issue, ensure that each role can only vote once per proposal.

function approveUSDPrice(
        uint256 _price
    )
        external
        onlyOneOfRoles(
            [
                Role.PriceFeed_1,
                Role.PriceFeed_2,
                Role.PriceFeed_3,
                Role.PriceFeed_4,
                Role.PriceFeed_5
            ]
        )
    {
        if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
        if (usdUpdateProposal.proposer == msg.sender)
            revert ProposerCannotApproveError();
         //@audit- restrict roles from approving after disapproving 
          (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
            revert ProposalAlreadydisapprovedError();
          
        if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
            revert ProposalAlreadyApprovedError();
        if (usdUpdateProposal.proposedPrice != _price)
            revert ProposalPriceNotMatchedError();

        usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
        usdUpdateProposal.approvalsCount++;

        if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
            _execUSDPriceUpdate();
        }

        emit ApprovedUSDPrice(msg.sender);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L343-L398

Vulnerability details

The lockdrop allows users to lock USD/ETH for a period of time, while the lockdrop is active they earn bonuses and get the NFTs. During this process the _lock logic calculates for the remainder of the assets locked into the contract by getting the modulo between the _quantity and nftcost. Any reminder asset is saved in the players struct of lockedToken.remainder which will be added to the users new lock if they choose to lock again. However this process can be exploited. This occurs because the remainder from previous locks is not considered during the unlocking process, leading to financial loss.

Impact

Players can steal Nfts by unlocking all the quantities locked and re-locking them to increase the remainder which gets added to the remainder increasing the amount of NFt the lock is really worth.

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L343-L398

        // add remainder from any previous lock
        uint256 quantity = _quantity + lockedToken.remainder;
        uint256 remainder;
        uint256 numberNFTs;
        uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

        if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }
        if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp)
        ) {
            if (
                _lockDuration < lockdrop.minLockDuration ||
                _lockDuration >
                uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();
            if (msg.sender != address(migrationManager)) {
                // calculate number of nfts
                remainder = quantity % configuredToken.nftCost;
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

                if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

                // Tell nftOverlord that the player has new unopened Munchables
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }
       }

        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

        emit Locked(
            _lockRecipient,
            _tokenOwner,
            _tokenContract,
            _quantity,
            remainder,
            numberNFTs,
            _lockDuration
        );
    }

When players lock their funds during an active lockdrop, the number of NFTs they receive is calculated, and any remainder from the quantity that doesn't convert into NFTs is stored in lockedToken.remainder. For instance, if a user locks 1000 tokens with an nftCost of 10.5, the calculation will be: solidity remainder = 1000 % 10.5; // remainder = 2.5 numberNFTs = (1000 - 2.5) / 10.5; // numberNFTs = 95 The players lockedToken.remainder = remainder; = 2.5ETH However the unlock function only processes the unlocking of the quantity but does not handle the remainder stored in lockedToken. https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401-L427 ```solidity function unlock( address _tokenContract, uint256 _quantity ) external notPaused nonReentrant { LockedToken storage lockedToken = lockedTokens[msg.sender][_tokenContract]; if (lockedToken.quantity < _quantity) revert InsufficientLockAmountError(); if (lockedToken.unlockTime > uint32(block.timestamp)) revert TokenStillLockedError();

      accountManager.forceHarvest(msg.sender);
      lockedToken.quantity -= _quantity;

      if (_tokenContract == address(0)) {
          payable(msg.sender).transfer(_quantity);
      } else {
          IERC20 token = IERC20(_tokenContract);
          token.transfer(msg.sender, _quantity);
      }

      emit Unlocked(msg.sender, _tokenContract, _quantity);
  }
 ```

The total quantity locked by the user can be withdrawn (1000) disregarding the remainder of 2.5ETH. A player can set the lock duration to a minimal time and when it expires before the lockdrop ends they can lock again this time the _quantity gets added tho the remainder increasing the quantity by 2.5 uint256 quantity = _quantity + lockedToken.remainder; Player locks 1000ETH again it will get added to 2.5ETH of the previous remainder, even if the player already withdrew the total quantity locked in the previous unlockTime, the user now gets 1002.5ETH earning another remainder of 5ETH the player can keep repeating this process to increase the remainder amount and finally use that to get the Nfts. Here's how: Initial lock and remainder

      remainder = 1000 % 10.5; // remainder = 2.5
      numberNFTs = (1000 - 2.5) / 10.5; // numberNFTs = 95

Player withdraws all quantity of 1000 and locks again 990ETH uint256 quantity = _quantity + lockedToken.remainder;

      remainder = 992.5 % 10.5; // remainder = 5.5
      numberNFTs = (992.5 - 5.5) / 10.5; // numberNFTs = 94

The player gets 94 nfts the player can keep increasing the remainder till it's enough to lock a little quantity that gets added to the remainder getting NFt for free: The player inflates the remainder to 9.5 then locks 100 uint256 quantity = _quantity + lockedToken.remainder;

      remainder = 109.5 % 10.5; // remainder = 4.5
      numberNFTs = (109.5 - 4.5) / 10.5; // numberNFTs = 10

Player locks 100 + remainder 9.5 = 109.5 Getting 10 Nfts gaining 1 free NFt. strategically using the lock and unlock mechanism with increasing nftCost allows thee player to maximize the NFT gains. Over multiple cycles, the accumulated remainder can yield free NFTs, particularly if the nftCostcontinues to rise.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider the remainder when unlocking and reset it to zero

Assessed type

Context