howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The transferToUnoccupiedPlot function in the LandManager contract fails to update the plotId in the toilerState mapping when a Munchable NFT is moved to a new plot resulting in a discrepancy between the actual plot a token is associated with and the plot recorded in the toilerState.

Proof of Concept

Look at the transferToUnoccupiedPlot function: https://github.com/code-423n4/2024-07-munchables/blob/94cf468aaabf526b7a8319f7eba34014ccebe7b9/src/managers/LandManager.sol#L199-L227

function transferToUnoccupiedPlot(
    uint256 tokenId,
    uint256 plotId
) external override forceFarmPlots(msg.sender) notPaused {
    (address mainAccount, ) = _getMainAccountRequireRegistered(msg.sender);
    ToilerState memory _toiler = toilerState[tokenId];
    uint256 oldPlotId = _toiler.plotId;
    uint256 totalPlotsAvail = _getNumPlots(_toiler.landlord);
    // ... (validity checks)

    toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord]
        .currentTaxRate;
    plotOccupied[_toiler.landlord][oldPlotId] = Plot({
        occupied: false,
        tokenId: 0
    });
    plotOccupied[_toiler.landlord][plotId] = Plot({
        occupied: true,
        tokenId: tokenId
    });

    emit FarmPlotLeave(_toiler.landlord, tokenId, oldPlotId);
    emit FarmPlotTaken(toilerState[tokenId], tokenId);
}

The function updates the plotOccupied mapping for both the old and new plots, correctly marking the old plot as unoccupied and the new plot as occupied. But, it fails to update the plotId in the toilerState mapping for the token.

This means that while plotOccupied correctly reflects the new state, toilerState[tokenId].plotId still contains the old plot ID. This could lead to issues in other parts of the protocol that rely on the toilerState mapping, such as the _farmPlots function:

function _farmPlots(address _sender) internal {
    // ...
    for (uint8 i = 0; i < staked.length; i++) {
        // ...
  @>      _toiler = toilerState[tokenId];
        // ...
        if (_getNumPlots(landlord) < _toiler.plotId) {
            timestamp = plotMetadata[landlord].lastUpdated;
            toilerState[tokenId].dirty = true;
        }
        // ... (farming calculations using _toiler.plotId)
    }
    // ...
}

In this function, the outdated plotId in toilerState could lead to incorrect farming calculations or unnecessary marking of plots as "dirty".

Tools Used

Manual review

Recommended Mitigation Steps

Update the transferToUnoccupiedPlot function to include an update to the toilerState mapping. Add the following line after updating the plotOccupied mappings and before emitting the events:

toilerState[tokenId].plotId = plotId;

This will make sure that the toilerState mapping is correctly updated with the new plot ID when a token is transferred to an unoccupied plot. Additionally, consider adding a similar update in any other functions that might move a Munchable to a different plot.

Assessed type

Other

c4-judge commented 3 months ago

alex-ppg changed the severity to 3 (High Risk)