howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Description

plotId is defined as uint256 and it can start from 0 index to n index. If a landlord have 3 max plot, basicly 0,1,2 indexes are available for plot. We can also see this behaviour from following lines:

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

It uses >= instead of > because index 0 is counted as available plot. In edge case, players total farm time calculated using lastUpdated time, but normally it's calculated with block.timestamp which generate more reward than edge case. But in following lines, while deciding the plotId is whether in edge case or not. It uses < operator and it should be <= for correct calculation:

            if (_getNumPlots(landlord) < _toiler.plotId) {
                timestamp = plotMetadata[landlord].lastUpdated;
                toilerState[tokenId].dirty = true;
            }

Impact

Main functionality problem
While calculation of edge case, plotId == maxPlot case calculated wrongly and player can farm more reward.

Proof of Concept

In order to illustrate this, we can use following scenario:

Alice is landlord and she have 4 plots ( _getNumPlots(landlord) )
Those plot ids are 0, 1, 2, 3
Bob has a plot whose id's 3
Then Alice make some changes and now she have 3 plots
Normally, bob's plot should be dirty right now because remainning plot ids are 0, 1, 2
Now, bob can farm his plot without being catched in if statement and it can farm his plot using block.timestamp variable.
So, it will generate more reward to him

Tools Used

Manual Review

Recommended Mitigation Steps

Changing < operator to <= will solve the problem

Assessed type

Other

c4-judge commented 1 month ago

alex-ppg marked the issue as partial-75