Lines of code

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

Vulnerability details

Impact

Players call stakeMunchable to stake their Munchables on a plot. The function updates the toilerState of the NFT, and more particularly sets the latestTaxRate of the NFT's toiler state which affects the schnibbles allocation.

The issue is that the function does not check if the plot metadata was set for the landlord or not:

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, // @audit No check if the metadata was set
        dirty: false
});

If the plot metadata for the landlord was not set, the tax rate of the toiler state will be set to the default zero-value. As consequence, when the next toiling happens (which might take too long by the users's intention), the forceFarmPlots modifier will calculate the schnibbles rewards of the users with 0% tax:

function _farmPlots(address _sender) internal {
    (
        address mainAccount,
        MunchablesCommonLib.Player memory renterMetadata
    ) = _getMainAccountRequireRegistered(_sender);
    // ... 
        schnibblesTotal =
                (timestamp - _toiler.lastToilDate) *
                BASE_SCHNIBBLE_RATE;
        schnibblesTotal = uint256((int256(schnibblesTotal) +(int256(schnibblesTotal) * finalBonus)) / 100);
        schnibblesLandlord = (schnibblesTotal * 
                            _toiler.latestTaxRate)  // @audit `latestTaxRate` will be zero
                            / 1e18;

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

        renterMetadata.unfedSchnibbles += (schnibblesTotal -schnibblesLandlord);

        landlordMetadata.unfedSchnibbles += schnibblesLandlord;
        // ...
    }
    accountManager.updatePlayer(mainAccount, renterMetadata);
}

We can see that the calculation of the landlord schnibbles schnibblesLandlord will be evaluated to zero because the toiler's latestTaxRate was set to zero upon the stake, which makes the user earns schnibbles rewards with 0% tax.

Notice that it is not intended for the tax rate to be zero because it is bounded between MIN_TAX_RATE and MAX_TAX_RATE when calling updateTaxRate

Proof of Concept

Consider the following scenario that demonstrates the previously mentioned issue:

There is a landlord L with 5 available plots. The plot metadata was not set for the landlord L
Bob stakes his Munchables token on L at any given valid plot (let it be 1). The toilerState[Bob's token] will be updated accordingly and notice that latestTaxRate will be set to zero because plot metadata was not set for L
Time passes, and Bob toils his staked plot, for example he wants to unstake his Munchables, so he calls unstakeMunchable, the modifier forceFarmPlots will be executed to farm schnibbles points to Bob for the time passed
- The exeuction will come to calculate the schnibbles that will be allocated for landlore L: schnibblesLandlord = (schnibblesTotal * _toiler.latestTaxRate) / 1e18;, which will be evaluated to zero because _toiler.latestTaxRate was set to zero
- Bob's schnibbles rewards will be fully farmed with 0% tax: renterMetadata.unfedSchnibbles += (schnibblesTotal - schnibblesLandlord);
  Tools Used
  
  Manual Review

Recommended Mitigation Steps

Consider preventing users from staking if the plot metadata was not set for the landlord. Below is a suggestion for an updated code of stakeMunchable:

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();
+    if (plotMetadata[landlord].currentTaxRate == 0) revert PlotMetaDataNotSet();
    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

Invalid Validation

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

Munchables can be staked with 0% Tax rate #328