Lines of code

https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LandManager.sol#L166

Vulnerability details

Description

stakeMunchable is not considering if the plot metadata exist or not during staking, which allow a player to stake on a plot with zero tax rate for the landlord, which is unexpected and seems to warrant Medium severity.

Impact

Player can stake on a plot with a zero tax rate which bypass the range [MIN_TAX_RATE, MAX_TAX_RATE]

PoC

Alice and Bob are the players. Alice has some ETH locked (so a potential landlord) but somehow didn't generate his plot metadata yet. 1) Bob's stake NFT 1 on Alice's plotId 0.

If we examine all the checks, nothing is verifying if the plot's metadata exist. _getNumPlots(landlord) is not generating nor doing any check either. So the toilerState will be created with a zero tax rate (plotMetadata[landlord].currentTaxRate == 0 as doesn't exist yet).

    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) //@audit: NC: remove this magic number
            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);
    }

2) Bob will be able to farm as normally, but the landlord will not earn any points.

By examining the code below, we can see that schnibblesLandlord will always be zero (as _toiler.latestTaxRate == 0), which is unexpected.

    ...

            schnibblesTotal = (timestamp - _toiler.lastToilDate) * BASE_SCHNIBBLE_RATE;
            schnibblesTotal = uint256((int256(schnibblesTotal) + (int256(schnibblesTotal) * finalBonus)) / 100); //@audit: is this math accurate?
            schnibblesLandlord = (schnibblesTotal * _toiler.latestTaxRate) / 1e18;

            toilerState[tokenId].lastToilDate = timestamp;
            toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord].currentTaxRate;

            renterMetadata.unfedSchnibbles += (schnibblesTotal - schnibblesLandlord);

            landlordMetadata.unfedSchnibbles += schnibblesLandlord;
            landlordMetadata.lastPetMunchable = uint32(timestamp);
            accountManager.updatePlayer(landlord, landlordMetadata);
            emit FarmedSchnibbles(
                _toiler.landlord,
                tokenId,
                _toiler.plotId,
                schnibblesTotal - schnibblesLandlord,
                schnibblesLandlord
            );
    ...

Tools Used

Manual review

Recommended Mitigation Steps

Apply the following patch to resolve this issue.

    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) //@audit: NC: remove this magic number
            revert TooManyStakedMunchiesError();
        if (munchNFT.ownerOf(tokenId) != mainAccount)
            revert InvalidOwnerError();
+       if (plotMetadata[landlord].lastUpdated == 0)
+           revert PlotMetadataNotUpdatedError();

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

Assessed type

Error

code-423n4 / 2024-07-munchables-findings

`stakeMunchable` is allowing to stake against `plot which doesn't have metadata` which will prevent the landlord from earning his tax #327