howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-munchables/blob/94cf468aaabf526b7a8319f7eba34014ccebe7b9/src/managers/LandManager.sol#L131-L171

Vulnerability details

Impact

LandManager::stakeMunchable allows users to stake up to 11 Munchables instead of the intended maximum of 10. This discrepancy could lead to some strange behavior in other parts of the protocol that assume a strict limit of 10 staked Munchables per user.

Proof of Concept

Take a look at the stakeMunchable function: https://github.com/code-423n4/2024-07-munchables/blob/94cf468aaabf526b7a8319f7eba34014ccebe7b9/src/managers/LandManager.sol#L131-L171

    function stakeMunchable(
            address landlord,
            uint256 tokenId,
            uint256 plotId
        ) external override forceFarmPlots(msg.sender) notPaused {
            (address mainAccount, ) = _getMainAccountRequireRegistered(msg.sender);
            if (landlord == mainAccount) revert CantStakeToSelfError();
            if (plotOccupied[landlord][plotId].occupied)
                revert OccupiedPlotError(landlord, plotId);
  ❌        if (munchablesStaked[mainAccount].length > 10)
                revert TooManyStakedMunchiesError();
            if (munchNFT.ownerOf(tokenId) != mainAccount)
                revert InvalidOwnerError();

            uint256 totalPlotsAvail = _getNumPlots(landlord);
            if (plotId >= totalPlotsAvail) revert PlotTooHighError();

            if (
                !munchNFT.isApprovedForAll(mainAccount, address(this)) &&
                munchNFT.getApproved(tokenId) != address(this)
            ) revert NotApprovedError();
            munchNFT.transferFrom(mainAccount, address(this), tokenId);

            plotOccupied[landlord][plotId] = Plot({
                occupied: true,
                tokenId: tokenId
            });

   ❌     munchablesStaked[mainAccount].push(tokenId);
            munchableOwner[tokenId] = mainAccount;

            toilerState[tokenId] = ToilerState({
                lastToilDate: block.timestamp,
                plotId: plotId,
                landlord: landlord,
                latestTaxRate: plotMetadata[landlord].currentTaxRate,
                dirty: false
            });

            emit FarmPlotTaken(toilerState[tokenId], tokenId);
        }

The issue is because the check munchablesStaked[mainAccount].length > 10 is performed before adding the new Munchable to the staked list. This allows a user to stake their 11th Munchable before the function reverts.

Consider:

A user with 10 staked Munchables calls stakeMunchable.
The check munchablesStaked[mainAccount].length > 10 passes (10 is not greater than 10).
The function continues and adds the 11th Munchable to munchablesStaked[mainAccount].
The user now has 11 staked Munchables, exceeding the intended limit.

Tools Used

Manual review

Recommended Mitigation Steps

Change this condition in the stakeMunchable function to use >= instead of >:

-   if (munchablesStaked[mainAccount].length > 10)
+   if (munchablesStaked[mainAccount].length >= 10)
        revert TooManyStakedMunchiesError();

Assessed type

Error

c4-judge commented 3 months ago

alex-ppg changed the severity to QA (Quality Assurance)