Upgraded Q -> 3 from #314 [1722887703631]

[c4-judge]

Judge has assessed an item in Issue #314 as 3 risk. The relevant finding follows:

[QA-02] transferToUnoccupiedPlot fails to update toilerState.plotId, leading to incorrect plot tracking and could also unnecessarily mark plots as "dirty"

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".

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.

[c4-judge]

alex-ppg marked the issue as duplicate of #198

[c4-judge]

alex-ppg marked the issue as partial-75