Critical states are not updated when transferrnig tokens to new plot

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The _farmPlots function is executed during every toiling operation on a plot (staking, unstaking, and transferring a token to a new plot). The function checks if the plotId associated with the ToilerState exceeds the number of available plots for the landlord. If so, it sets the dirty status to true:

function _farmPlots(address _sender) internal {
    // ...

    for (uint8 i = 0; i < staked.length; i++) {
        timestamp = block.timestamp;
        tokenId = staked[i];
        _toiler = toilerState[tokenId];
        if (_toiler.dirty) continue; // <------------ skip the reward if already dirty
        landlord = _toiler.landlord;

        if (_getNumPlots(landlord) < _toiler.plotId) { 
             timestamp = plotMetadata[landlord].lastUpdated;
            toilerState[tokenId].dirty = true; // <------------ set dirty to true if the plot becomes invalid
        }

        // ...
    }
}

If the dirty status is set to true, it means that the current plot is no longer valid (so, schnibbles will not be farmed) and the token should be moved to a new valid plot to start farming again. And to transfer the token to a new valid plot, transferToUnoccupiedPlot should be called. The issue is that the function does not update the dirty and lastToildDate states:

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);
    if (_toiler.landlord == address(0)) revert NotStakedError();
    if (munchableOwner[tokenId] != mainAccount) revert InvalidOwnerError();
    if (plotOccupied[_toiler.landlord][plotId].occupied)
        revert OccupiedPlotError(_toiler.landlord, plotId);
    if (plotId >= totalPlotsAvail) revert PlotTooHighError();

    toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord]
            .currentTaxRate;
     // @audit `toilerState[tokenId].lastToilDate` was not updated
    plotOccupied[_toiler.landlord][oldPlotId] = Plot({
        occupied: false,
        tokenId: 0
    });
    plotOccupied[_toiler.landlord][plotId] = Plot({
        occupied: true,
        tokenId: tokenId
    });

    // @audit `toilerState[tokenId].dirty` was not updated

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

We can see that the function does not update the dirty status back to false if it was set to true. Therefore, even if the user transfers his token to a new valid plot because of the dirty status, the token will still be considered dirty and will not be able to farm schnibbles rewards.lastToilDate was not updated either, and it still points to the previous toiling action.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider updating the lastToilDate state and the dirty status if it was true. Below is a suggestion for an updated code of transferToUnoccupiedPlot:

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);
    if (_toiler.landlord == address(0)) revert NotStakedError();
    if (munchableOwner[tokenId] != mainAccount) revert InvalidOwnerError();
    if (plotOccupied[_toiler.landlord][plotId].occupied)
        revert OccupiedPlotError(_toiler.landlord, plotId);
    if (plotId >= totalPlotsAvail) revert PlotTooHighError();

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

+    if (toilerState[tokenId].dirty) toilerState[tokenId].dirty = false;
    emit FarmPlotLeave(_toiler.landlord, tokenId, oldPlotId);
    emit FarmPlotTaken(toilerState[tokenId], tokenId);
}

Assessed type

Context

[c4-judge]

alex-ppg marked the issue as satisfactory

[Brivan-26]

Hi @alex-ppg

This report identifies two issues:

1. Not updating the dirty status
2. Not updating the lastToildDate

Missing each update has a different impact (as explained in the report). The primary issue #26 Mentioned only the first missing update

I believe this one should either be selected for the report and other issues that pointed only to dirty status should receive partial-50, or this report should be credited for two different findings

[dontonka]

Hey @Brivan-26 , I just wanted to double check one fact:

Not updating the lastToildDate: I spoke about this with the sponsor during the contest and lastToildDate is not mandaroty to be updated here as the modifier forceFarmPlots will actually udpate it. Does that make sense or you are claiming something different here?

[Brivan-26]

Thanks for pointing out @dontonka . I'm claiming something different here: When the token is already in the dirty state, the forceFarmPlots will skip the token update. So, if a token that had already been in a dirty state is transferred to a new plot, the lastToilDate will still reference an old time, and when toiling again on the new valid plot, the rewards will be farmed starting from the outdated lastToilDate rather then from when the transfer to valid plot happened.

[dontonka]

That's a good observation actually and indeed seems a different finding (maybe not as severe thought), I should not have fully trusted what the sponsor told me blindly, dang my bad. Be aware that the following duplicates are also suggesting to update the lastToilDate.

317, 152 and 42

[alex-ppg]

Hey @Brivan-26 and @dontonka, thank you for your PJQA contributions.

Per C4 guidelines all submissions are graded based on the state of the codebase at the time the contest was initiated. An absence of the lastToilDate update does not result in any impact on this version, and updating it independently will not result in rewards being accumulated. As such, we cannot evaluate it as a distinct finding.

Based on the above, we will have the absence of the lastToilDate update factor in as an incorrect remediation to the issue. Per the C4 guidelines around satisfactory reports, this would warrant a partial penalty as it represents an incorrect alleviation to this submission.

Based on the above, I have proceeded with downgrading the reward of all duplicate reports not mentioning the lastToilDate to partial-75, and re-selected the primary report of this duplicate group.