Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Munchables smart contract system written in Solidity. The audit took place between July 23 — July 29, 2024.

Wardens

97 Wardens contributed reports to Munchables:

1. merlinboii
2. 0xCiphky
3. MrPotatoMagic
4. dimulski
5. BugPull (IlIlHunterlIlI and rzizah)
6. shaflow2
7. Bozho
8. typicalHuman
9. dhank
10. Abdessamed
11. dontonka
12. 0xrex
13. santipu_
14. Rhaydden
15. artilugio0
16. ke1caM
17. vinica_boy
18. turvy_fuzz
19. matejdb
20. gajiknownnothing
21. forgebyola
22. prapandey031
23. araj
24. Utsav
25. DemoreX
26. 0x3b
27. franfran20
28. 0rpse
29. 0x_6a70
30. DPS (yvuchev and 0xJaeger)
31. peanuts
32. tedox
33. stanchev
34. joaovwfreire
35. phoenixV110
36. ironside
37. rudhra
38. Phoenix_x
39. Tomas0707
40. Heaven
41. Drynooo
42. McToady
43. nnez
44. gd
45. stuart_the_minion
46. iam_emptyset
47. enckrish
48. slvDev
49. KaligoAudits (0xShiki, owliie and weedeus)
50. Chad0
51. waydou
52. PENGUN
53. jesjupyter
54. synackrst
55. LeFy
56. JanuaryPersimmon2024
57. Topmark
58. cheatc0d3
59. K42
60. Trooper
61. Nexarion
62. robertodf99
63. King_
64. Maroutis
65. yaioxy
66. TPSec (PetarTolev and Pataroff)
67. Flare
68. 0xb0k0
69. Tamer
70. ihtishamsudo
71. TECHFUND-inc
72. EPSec (petarP1998 and 1337web3)
73. pipidu83
74. bearonbike
75. Joshuajee
76. eta
77. 0x04bytes
78. Shubham
79. sandy
80. Fon
81. AvantGard
82. Gosho
83. gkrastenov
84. radev_sw
85. m4ttm
86. Fitro
87. corporalking
88. 0xjarix
89. mojito_auditor
90. biakia
91. d4r3d3v1l

This audit was judged by 0xsomeone.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 6 unique vulnerabilities. Of these vulnerabilities, 5 received a risk rating in the category of HIGH severity and 1 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 3 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Munchables repository, and is composed of 1 smart contract written in the Solidity programming language and includes 277 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (5)

[H-01] Single plot can be occupied by multiple renters

Submitted by Bozho, also found by Bozho, Rhaydden (1, 2), ihtishamsudo, tedox, stuart_the_minion, dontonka, TECHFUND-inc, EPSec, araj, 0x3b, pipidu83, bearonbike, ironside, waydou, Joshuajee, Chad0, peanuts, iam_emptyset, gd, eta, Tomas0707, 0x04bytes, Shubham, stanchev, Flare, sandy, Fon, franfran20, dhank, vinica_boy, AvantGard, MrPotatoMagic, enckrish, prapandey031, Gosho, gkrastenov, Heaven, radev_sw, m4ttm, Fitro, corporalking, 0xjarix, phoenixV110, mojito_auditor, 0xCiphky, 0xrex, merlinboii, santipu_, dimulski, ke1caM (1, 2), biakia, Drynooo, Utsav, and d4r3d3v1l

The LandManager contract allows users to stake their munchables to a other user’s plots to earn schnibbles. There’s a functionality to transfer a staked token from one plot to another.

This functionality is flawed as it allows a user to transfer his token to another plot but doesn’t update the plotId field in the ToilerState struct. This means that the user can move his token but the contract still thinks it’s in the original plot.

    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; // @audit plotId isn't update inside toilerState
        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 missing toilerState[tokenId].plotId = plotId;
        plotOccupied[_toiler.landlord][oldPlotId] = Plot({
            occupied: false,
            tokenId: 0
        });
        ...

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

Impact

If the amount of landlord’s plots is decreased and user’s token is inside a plot outside of the new range, the token won’t be set as dirty.

This can lead to a situation where a user can stake initially on the first plots but after that move his token to a plot that is in the end of the range. When the plots are decreased, the contract will still think the token is in the first plot and won’t set it as dirty and another user can stake his token in the same plot which breaks the intended functionality for LandManager.

Proof of Concept

Starting state:

    \=> `_getNumPlots(landlord)` = 4
    \=> `toilerState[tokenId].plotId` = 1
    \=> `plotOccupied[landlord][1].occupied` = true
    \=> `plotOccupied[landlord][3].occupied` = false

Bob transfers his token from plot 1 to plot 3:

    \=> `toilerState[tokenId].plotId` remains = 1
    \=> `plotOccupied[landlord][1].occupied` = false
    \=> `plotOccupied[landlord][3].occupied` = true

Landlord’s plots are decreased to 2:

    \=> `_getNumPlots(landlord)` = 2
    \=> `toilerState[tokenId].plotId` = 1
    \=> `plotOccupied[landlord][1].occupied` is still = false
    \=> `plotOccupied[landlord][3].occupied` is still = true

farmPlots() is called by Bob:

    \=> `toilerState[tokenId].plotId` = 1
    \=> The contract still thinks Bob's token is in plot 1, but it's actually in plot 3
    \=> Bob's token won't be set as dirty as `toilerState[tokenId].plotId` < `_getNumPlots(landlord)`
    \=> `plotOccupied[landlord][1].occupied` = false
    \=> Another user can stake his token in plot 1 as well

Coded proof of concept

Add the following test inside transferToUnoccupiedPlot.test.ts and run with pnpm test:typescript:

it("successful path with logging plotId", async () => {
  //@audit-note Log plotId before transfer
  const beforeState =
    await testContracts.landManagerProxy.contract.read.getToilerState([1]);
  console.log("PlotId before transfer: " + beforeState.plotId);

  const { request } =
    await testContracts.landManagerProxy.contract.simulate.transferToUnoccupiedPlot(
      [1, 10],
      {
        account: bob,
      },
    );
  const txHash = await testClient.writeContract(request);
  await assertTxSuccess({ txHash: txHash });

  //@audit-note Log plotId after transfer
  const afterState =
    await testContracts.landManagerProxy.contract.read.getToilerState([1]);
  console.log("PlotId after transfer: " + afterState.plotId);

  //@audit-note Assert plotId hasn't changed
  assert.equal(beforeState.plotId, afterState.plotId, "PlotId did not change");
});

Recommended Mitigation Steps

Update plotId inside toilerState when moving plots.

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

Assessed type

Context

0xinsanity (Munchables) confirmed via duplicate Issue #88

0xsomeone (judge) commented:

The Warden has correctly identified a flaw in the way data is maintained after moving into an unoccupied plot that would result in a plot remaining occupied after having been vacated, rewards to be attributed incorrectly, and unstaking operations to be processed improperly.

Given that rewards can be directly affected by the vulnerability and in line with the evaluation of #89 I believe this merits a high-risk severity rating.

[H-02] Invalid validation in _farmPlots function allowing a malicious user repeated farming without locked funds

Submitted by gd, also found by tedox, slvDev, iam_emptyset, Bozho, stanchev, joaovwfreire, enckrish, BugPull, 0xCiphky, dimulski, shaflow2, stuart_the_minion, dontonka, Abdessamed, araj, phoenixV110, Chad0, Utsav, PENGUN, jesjupyter, dhank, synackrst, LeFy, MrPotatoMagic, prapandey031, DemoreX, 0rpse, merlinboii, and JanuaryPersimmon2024

This vulnerability allows for repeated farming and reward collection from a plot even after the landlord has unlocked funds, leading to an imbalance in the game economy and unfair advantage to certain players.

Proof of Concept

The _farmPlots function has a vulnerability where a plot with a plotId 0 can continue to be farmed and receive rewards even after the landlord has unlocked all funds.

The code snippet:

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

The _getNumPlots function should return zero when all funds are unlocked, invalidating the plot and setting the dirty flag.

However, the current implementation fails to do so, it only determines if NumPlots is less than the plotId and allows plot 0 to remain farmable.

The poc file:

import assert from "node:assert";
import { after, afterEach, before, beforeEach, describe, it } from "node:test";
import { parseEther, zeroAddress } from "viem";
import { DeployedContractsType } from "../../../deployments/actions/deploy-contracts";
import { assertContractFunctionRevertedError, assertTxSuccess } from "../../utils/asserters";
import { testClient } from "../../utils/consts";
import { getTestContracts, getTestRoleAddresses } from "../../utils/contracts";
import { MockNFTOverlordContractType, deployMockNFTOverlord } from "../../utils/mock-contracts";
import { registerPlayer } from "../../utils/players";

describe("LandManager: stakeMunchable", () => {
  let alice: `0x${string}`;
  let bob: `0x${string}`;
  let jirard: `0x${string}`;
  let beforeSnapshot: `0x${string}`;
  let beforeEachSnapshot: `0x${string}`;
  let testContracts: DeployedContractsType;
  let mockNFTOverlord: MockNFTOverlordContractType;

  before(async () => {
    testContracts = await getTestContracts();

    beforeSnapshot = await testClient.snapshot();

    const testRoleAddresses = await getTestRoleAddresses();
    mockNFTOverlord = await deployMockNFTOverlord({ testContracts });
    [alice, bob, jirard] = testRoleAddresses.users;
    await testClient.setBalance({
      address: alice,
      value: parseEther("10"),
    });
    await testClient.setBalance({
      address: bob,
      value: parseEther("10"),
    });
    await testClient.setBalance({
      address: jirard,
      value: parseEther("10"),
    });
  });

  beforeEach(async () => {
    beforeEachSnapshot = await testClient.snapshot();
  });

  afterEach(async () => {
    await testClient.revert({ id: beforeEachSnapshot });
  });

  after(async () => {
    await testClient.revert({ id: beforeSnapshot });
  });

  describe("all paths", () => {
    beforeEach(async () => {
      await registerPlayer({
        account: alice,
        testContracts,
      });
      await registerPlayer({
        account: bob,
        testContracts,
      });
      await registerPlayer({
        account: jirard,
        testContracts,
      });

      const { request } = await testContracts.lockManager.contract.simulate.lock(
        [zeroAddress, parseEther("1")],
        { account: alice, value: parseEther("1") }
      );
      const txHash = await testClient.writeContract(request);
      await assertTxSuccess({ txHash: txHash });

      await mockNFTOverlord.write.addReveal([alice, 100], { account: alice });
      await mockNFTOverlord.write.addReveal([bob, 100], { account: bob });
      await mockNFTOverlord.write.addReveal([bob, 100], { account: bob });
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 1
      await mockNFTOverlord.write.reveal([bob, 4, 12], { account: bob }); // 1
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 2
      await mockNFTOverlord.write.reveal([bob, 0, 13], { account: bob }); // 2
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 3
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 3
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 4
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 4
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 5
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 5
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 6
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 6
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 7
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 7
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 1
      await mockNFTOverlord.write.reveal([bob, 4, 12], { account: bob }); // 1
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 2
      await mockNFTOverlord.write.reveal([bob, 0, 13], { account: bob }); // 2
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 3
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 3
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 4
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 4
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 5
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 5
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 6
      await mockNFTOverlord.write.reveal([bob, 1, 14], { account: bob }); // 6
      await mockNFTOverlord.write.startReveal([alice], { account: alice }); // 7
      await mockNFTOverlord.write.reveal([alice, 1, 14], { account: alice }); // 7
      await mockNFTOverlord.write.startReveal([jirard], { account: jirard }); // 8
      await mockNFTOverlord.write.reveal([jirard, 0, 22], { account: jirard }); // 8
      await mockNFTOverlord.write.startReveal([jirard], { account: jirard }); // 9
      await mockNFTOverlord.write.reveal([jirard, 0, 23], { account: jirard }); // 9
    });
    it("unlock and repeat farm", async () => {
      const txHash = await testContracts.munchNFT.contract.write.approve(
        [testContracts.landManagerProxy.contract.address, 1n],
        { account: bob }
      );
      await assertTxSuccess({ txHash });
      const txHash2 = await testContracts.munchNFT.contract.write.approve(
        [testContracts.landManagerProxy.contract.address, 2n],
        { account: bob }
      );
      await assertTxSuccess({ txHash: txHash2 });
      const stakeMunchableTxHash =
        await testContracts.landManagerProxy.contract.write.stakeMunchable([alice, 1, 0], {
          account: bob,
        });
      await assertTxSuccess({ txHash: stakeMunchableTxHash });

      const { request } = await testContracts.lockManager.contract.simulate.unlock(
        [zeroAddress, parseEther("1")],
        { account: alice }
      );
      const txHash3 = await testClient.writeContract(request);
      await assertTxSuccess({ txHash: txHash3 });
      console.log("the landlord unlocked");

      console.log("farm immediately after unlocking");
      const { request: farmRequest1 } =
      await testContracts.landManagerProxy.contract.simulate.farmPlots([], {
        account: bob,
      });
      const farmHash1 = await testClient.writeContract(farmRequest1);

      const registeredPlayerAlice1 = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [alice]
      );
      console.log("the landlord:");
      console.log(registeredPlayerAlice1);
      const registeredPlayerBob1 = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [bob]
      );
      console.log("the user:");
      console.log(registeredPlayerBob1);

      // pass time
      let unlockTime: number;
      const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]);
      assert(lockedTokens instanceof Array);
      const lockedToken = lockedTokens.find((t) => t.tokenContract === zeroAddress);
      assert.ok(lockedToken);
      unlockTime = lockedToken.lockedToken.unlockTime;
      // console.log(unlockTime);

      // Fast-forward the chain to put unlock time in past
      await testClient.setNextBlockTimestamp({
        timestamp: BigInt(unlockTime + 24 * 3600 * 30),
      });
      await testClient.mine({ blocks: 1 });

      console.log("farm after waiting time");
      const { request: farmRequest2 } =
      await testContracts.landManagerProxy.contract.simulate.farmPlots([], {
        account: bob,
      });
      const farmHash2 = await testClient.writeContract(farmRequest2);

      const registeredPlayerAlice2 = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [alice]
      );
      console.log("the landlord:");
      console.log(registeredPlayerAlice2);
      const registeredPlayerBob2 = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [bob]
      );
      console.log("the user:");
      console.log(registeredPlayerBob2);
    });
  });
});

Run the poc file and get the output:

    the landlord unlocked
    farm immediately after unlocking
    the landlord:
    [
      '0x90F79bf6EB2c4f870365E785982E1f101E93b906',
      {
        registrationDate: 1713316696,
        lastPetMunchable: 1713316739,
        lastHarvestDate: 1713316738,
        snuggeryRealm: 0,
        maxSnuggerySize: 6,
        unfedSchnibbles: 169425833333333333n,
        referrer: '0x0000000000000000000000000000000000000000'
      }
    ]
    the user:
    [
      '0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65',
      {
        registrationDate: 1713316697,
        lastPetMunchable: 0,
        lastHarvestDate: 1713316708,
        snuggeryRealm: 0,
        maxSnuggerySize: 6,
        unfedSchnibbles: 465000000000000n,
        referrer: '0x0000000000000000000000000000000000000000'
      }
    ]
    farm after waiting time
    the landlord:
    [
      '0x90F79bf6EB2c4f870365E785982E1f101E93b906',
      {
        registrationDate: 1713316696,
        lastPetMunchable: 1715908700,
        lastHarvestDate: 1713316738,
        snuggeryRealm: 0,
        maxSnuggerySize: 6,
        unfedSchnibbles: 201046403333333333333n,
        referrer: '0x0000000000000000000000000000000000000000'
      }
    ]
    the user:
    [
      '0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65',
      {
        registrationDate: 1713316697,
        lastPetMunchable: 0,
        lastHarvestDate: 1713316708,
        snuggeryRealm: 0,
        maxSnuggerySize: 6,
        unfedSchnibbles: 602631397500000000000n,
        referrer: '0x0000000000000000000000000000000000000000'
      }
    ]
    ▶ LandManager: stakeMunchable
      ▶ all paths
        ✔ unlock and repeat farm (116.600875ms)
      ▶ all paths (116.832875ms)
    ▶ LandManager: stakeMunchable (1388.625417ms)

Recommended Mitigation Steps

Add the check for plotId is equal to numPolts.

diff -u src/managers/LandManager.sol src/managers/LandManager.sol.fix
@@ -255,7 +255,7 @@
             // the edge case where this doesn't work is if the user hasn't farmed in a while and the landlord
             // updates their plots multiple times. then the last updated time will be the last time they updated their plot details
             // instead of the first
-            if (_getNumPlots(landlord) < _toiler.plotId) {
+            if (_getNumPlots(landlord) <= _toiler.plotId) {
                 timestamp = plotMetadata[landlord].lastUpdated;
                 toilerState[tokenId].dirty = true;
             }

Assessed type

Invalid Validation

0xinsanity (Munchables) confirmed

0xsomeone (judge) commented:

The Warden and its duplicates have identified a significant issue with the code which will continue to attribute rewards for a plot with an ID of 0 regardless of whether the landlord has staked any tokens.

A high-risk severity is appropriate for this vulnerability as it effectively permits rewards to be minted without any staked funds on the landlord’s end. Submissions that detailed the problem with the equality case but failed to identify the 0 edge case (i.e. failed to argue a high-severity rating) will be awarded 75% of their intended reward.

[H-03] Miscalculation in _farmPlots function could lead to a user unable to unstake all NFTs

Submitted by KaligoAudits, also found by Topmark, Rhaydden, cheatc0d3, stuart_the_minion, K42, 0x3b, Trooper, rudhra, artilugio0, ironside, Nexarion, waydou, gd, robertodf99, franfran20, King_, Maroutis, yaioxy, TPSec, MrPotatoMagic, BugPull, prapandey031, 0xrex, merlinboii, santipu_, shaflow2, dimulski, 0xb0k0, Flare, and Tamer

The function _farmPlots() is used to calculate rewards farming rewards. This function is used in different scenarios, including like a modifier in the function unstakeMunchable() function when a user unstakes their NFT. Let’s have a look at how finalBonus is calculated in the function:

finalBonus =
    int16(
        REALM_BONUSES[
            (uint256(immutableAttributes.realm) * 5) +
                uint256(landlordMetadata.snuggeryRealm)
        ]
    ) +
    int16(
        int8(RARITY_BONUSES[uint256(immutableAttributes.rarity)])
    );

The finalBonus consists of bonus from the realm (REALM_BONUSES), as well as a rarity bonus (RARITY_BONUSES).

REALM_BONUSES can be either -10, -5, 0, 5 or 10, and RARITY_BONUSES can be either 0, 10, 20, 30 or 50.

finalBonus is meant to incentivize staking Munchables in a plot in a suitable realm. It can either be a positive bonus or reduction in the amount of schnibblesTotal. An example of a negative scenario is when REALM_BONUSES is -10 and RARITY_BONUSES is 0.

Let’s look at the calculation of schnibblesTotal:

schnibblesTotal =
        (timestamp - _toiler.lastToilDate) *
        BASE_SCHNIBBLE_RATE;
schnibblesTotal = uint256(
    (int256(schnibblesTotal) +
        (int256(schnibblesTotal) * finalBonus)) / 100
    );

schnibblesTotal is typed as uint256; therefore, it is meant as always positive. The current calculation, however, will result in a negative value of schnibblesTotal, when finalBonus < 0, the conversion to uint256 will cause the value to evaluate to something near type(uint256).max. In that case the next calculation of schnibblesLandlord will revert due to overflow:

schnibblesLandlord =
    (schnibblesTotal * _toiler.latestTaxRate) /
    1e18;

This is due to the fact that _toiler.latestTaxRate > 1e16.

Since unstakeMunchable() has a modifier forceFarmPlots(), the unstaking will be blocked if _farmPlots() reverts.

Scenario

• Alice has a Common or Primordial Munchable NFT from Everfrost.
• Bob has land (plots) in Drench.
• Alice stakes her NFT in Bob’s plot.
• After some time Alice unstakes her NFT.
• Since in the calculation of schnibblesTotal is used a negative finalBonus, the transaction will revert.
• Alice’s Munchable NFT and any other staked beforehand are blocked.

Proof of Concept

Add the following lines of code in tests/managers/LandManager/unstakeMunchable.test.ts:

.
.
.
await registerPlayer({
   account: alice,
+  realm: 1,
   testContracts,
});
await registerPlayer({
   account: bob,
   testContracts,
});
await registerPlayer({
   account: jirard,
   testContracts,
});
.
.
.
await mockNFTOverlord.write.addReveal([alice, 100], { account: alice });
   await mockNFTOverlord.write.addReveal([bob, 100], { account: bob });
   await mockNFTOverlord.write.addReveal([bob, 100], { account: bob });
   await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 1
-  await mockNFTOverlord.write.reveal([bob, 4, 12], { account: bob }); // 1
+  await mockNFTOverlord.write.reveal([bob, 1, 0], { account: bob }); // 1
   await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 2
   await mockNFTOverlord.write.reveal([bob, 0, 13], { account: bob }); // 2
   await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 3

When we run the command pnpm test:typescript the logs from the tests show that the successful path test reverts.

Impact

A user’s Munchable NFT with specific attributes can be stuck in the protocol.

Taking into account the rarity distribution across the different NFT-s and assuming that the distribution across realms is equal, the chance of this problem occurring is around 26% or 1 in every 4 staked munchables.

Further, this finding blocks unstaking every munchable that has previously been staked.

Recommended Mitigation

finalBonus is meant as a percentage change in the Schnibbles earned by a Munchable. Change the calculation of schnibblesTotal in _farmPlots() to reflect that by removing the brackets:

schnibblesTotal =
        (timestamp - _toiler.lastToilDate) *
        BASE_SCHNIBBLE_RATE;
-schnibblesTotal = uint256(
-    (int256(schnibblesTotal) +
-        (int256(schnibblesTotal) * finalBonus)) / 100
-    );
+schnibblesTotal = uint256(
+    int256(schnibblesTotal) + 
+       (int256(schnibblesTotal) * finalBonus) / 100
+    );

Assessed type

Math

0xinsanity (Munchables) confirmed

0xsomeone (judge) commented:

The Warden and its duplicates have outlined a significant issue in the way the finalBonus is added or subtracted from the total as the order of operations is incorrect.

I believe a high-risk severity rating is appropriate given that rewards are significantly impacted and it may ultimately be possible for a Munchable to be permanently locked in the system.

[H-04] in farmPlots() an underflow in edge case leading to freeze of funds (NFT)

Submitted by BugPull, also found by typicalHuman, artilugio0, Bozho, turvy_fuzz, vinica_boy, MrPotatoMagic, matejdb, 0xCiphky, merlinboii, shaflow2, 0x3b, gajiknownnothing, 0x_6a70, DPS, franfran20, 0rpse, forgebyola, and Phoenix_x

https://github.com/code-423n4/2024-07-munchables/blob/94cf468aaabf526b7a8319f7eba34014ccebe7b9/src/managers/LandManager.sol#L232-L310
https://github.com/code-423n4/2024-07-munchables/blob/94cf468aaabf526b7a8319f7eba34014ccebe7b9/src/managers/LandManager.sol#L162-L168

Impact

The user won’t be able to farm their plots or unstake due to the txn always panic reverting. Leading to loss of funds and DOS.

Proof of Concept

The problem arises whenever PRICE_PER_PLOT gets increased through configUpdated. Whenever it gets increased, then _getNumPlots will return less numbers for that LandLord, because its denominated by PRICE_PER_PLOT.

File: LandManager.sol
344:     function _getNumPlots(address _account) internal view returns (uint256) {
345:         return lockManager.getLockedWeightedValue(_account) / PRICE_PER_PLOT;
346:     }

For example:

• A user is staked in plotId 100, and the LandLord has 200 Plots.
• PRICE_PER_PLOT gets increased and makes the Plots of that Landlord to be 90.
• Now the user is staked to a Plot that is considered invalid and should be flagged as dirty of his toilerState[tokenId] struct.

Now, the problem here is that NumPlots is not decreased due to LandLord unlocking funds but instead due to PRICE_PER_PLOT getting increased -> meaning that plotMetadata[landlord].lastUpdated is too old.

In _farmPlots we see:

File: LandManager.sol
232:     function _farmPlots(address _sender) internal {
//////////.............OmmitCode
258:             if (_getNumPlots(landlord) < _toiler.plotId) {
259:                 timestamp = plotMetadata[landlord].lastUpdated;
260:                 toilerState[tokenId].dirty = true;
261:             }
//////////.............OmmitCode
280:             schnibblesTotal =
281:                 (timestamp - _toiler.lastToilDate) *
282:                 BASE_SCHNIBBLE_RATE;
//////////.............OmmitCode
309:         accountManager.updatePlayer(mainAccount, renterMetadata);
310:     }

In Line 258, we check for NumPlots if it’s lower than plotId of the user we then assign timeStamp variable to landlord.lastUpdated which is too old timeStamp (the last time the user locked funds).

Then in Line 281, we subtract timestamp (which is now landlord.lastUpdated which is too old timeStamp) from _toiler.lastToilDate which is larger than timestamp that will lead to panic revert.

Let’s see why lastToilDate is larger:

We set this variable in stakeMunchable to block.timestamp here:

File: LandManager.sol
162:         toilerState[tokenId] = ToilerState({
163:             lastToilDate: block.timestamp,
164:             plotId: plotId,
165:             landlord: landlord,
166:             latestTaxRate: plotMetadata[landlord].currentTaxRate,
167:             dirty: false
168:         });

The landlord.lastUpdated is only updated when he lock or unlock funds (which in our case, didn’t lock or unlock any funds before the user stake to him).

Recommended Mitigation Steps

To avoid this case from happening, when PRICE_PER_PLOT gets increased we should check if landlord.lastUpdated is > than _toiler.lastToilDate inside the block (of this case if (_getNumPlots(landlord) < _toiler.plotId)), so that we can differentiate from cases that LandLord unlocked funds from cases where PRICE_PER_PLOT got increased and the LandLord didn’t do anything with his funds for a while.

Assessed type

Under/Overflow

0xinsanity (Munchables) confirmed

0xsomeone (judge) commented:

The Warden and its duplicates have correctly identified that a plot becoming inactive due to natural price movements and/or configuration adjustments of the PRICE_PER_PLOT value may result in a timestamp-based underflow occurring as the renter of a plot might be more active than its landlord.

I believe that normally a medium severity rating would be appropriate due to the economic incentives that surround the situation. Specifically, it is in the best interest of a landlord to “unlock” the NFTs they hold to permit them to harvest rewards and thus pay rent. As such, it is highly likely that in a scenario such as the one described the landlord would promptly address the issue.

Given that a Munchable becoming deadlocked is a documented invariant of the audit, I am inclined to award this with a high-severity rating. A penalty of 75% has been applied to any submission that detailed an inadequate or incorrect alleviation.

[H-05] Failure to update dirty flag in transferToUnoccupiedPlot prevents reward accumulation on valid plot

Submitted by 0xCiphky, also found by merlinboii, Abdessamed, dhank, tedox, dontonka, gajiknownnothing, Drynooo, ironside, rudhra, typicalHuman, Tomas0707, stanchev, McToady, joaovwfreire, Heaven, phoenixV110, nnez, forgebyola, 0xrex, santipu_, shaflow2, ke1caM, dimulski, and Bozho

The transferToUnoccupiedPlot function allows a user to transfer their Munchable to another unoccupied plot from the same landlord. This can be used for example by users currently staked in an invalid plot marked as “dirty”, meaning they will not be earning any rewards, to transfer to a valid plot so they can start earning rewards again.

Since the function checks that the transfer is to a valid plot, it should mean users are now eligible to start earning rewards again. However, it doesn’t update the user’s dirty flag back to false, meaning the user will still not be earning any rewards even after they have moved to a valid plot.

    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;
        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 next time _farmPlots is called, since dirty is still true, it’ll be skipped, accumulating no rewards.

    function _farmPlots(address _sender) internal {
        ...
        for (uint8 i = 0; i < staked.length; i++) {
            ...
            if (_toiler.dirty) continue;
            ...
            );
        }
        accountManager.updatePlayer(mainAccount, renterMetadata);
    }

Impact

Since the function does not update the user’s dirty flag back to valid, users who transfer their Munchable to a valid plot and the landlord will still not earn any rewards. This results in a situation where users remain unfairly penalized even after moving to a valid plot

Recommendation

Update the transferToUnoccupiedPlot function to reset the dirty flag to false and update the lastToilDate if it was previously marked as dirty when a user successfully transfers to a valid plot. This will ensure that users start earning rewards again once they are on a valid plot.

    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();
	// ADD HERE
        if (_toiler.dirty) {
            toilerState[tokenId].lastToilDate = block.timestamp;
            toilerState[tokenId].dirty = false;
        }
	//
        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);
    }

0xinsanity (Munchables) confirmed via duplicate Issue #89

Medium Risk Findings (1)

[M-01] Users can farm on zero-tax land if the landlord locked tokens before the LandManager deployment

Submitted by merlinboii, also found by Abdessamed, dontonka, araj, Utsav, MrPotatoMagic, dhank, prapandey031, DemoreX, santipu_, 0xCiphky, 0xrex, ke1caM, and dimulski

The LandManager::stakeMunchable() does not verify whether the plotMetadata[landlord] is set or not.

Since the LockManager contract is already deployed on-chain (0xEA091311Fc07139d753A6BBfcA27aB0224854Bae (Blast)), accounts that have locked their tokens before the LandManager deployment will need to trigger LandManager::triggerPlotMetadata() to update their LandManager.plotMetadata after the LandManager is deployed.

The current AccountManager interacts with the LockManager on-chain but does not introduce interaction with the LandManager during AccountManager::forceHarvest().

• On-chain: AccountManager::forceHarvest()
• Planned upgraded version: AccountManager::forceHarvest()

As a result, users can stake their tokens on land with a 0% tax rate if the landlord has not updated their plotMetadata and farm schnibbles without paying tax to the landlord until the landlord’s plot metadata is initialized in the LandManager contract (via triggerPlotMetadata() and updatePlotMetadata()).

File: src/managers/LandManager.sol

131:     function stakeMunchable(
132:         address landlord,
133:         uint256 tokenId,
134:         uint256 plotId
135:     ) external override forceFarmPlots(msg.sender) notPaused {
136:         (address mainAccount, ) = _getMainAccountRequireRegistered(msg.sender);
137:         if (landlord == mainAccount) revert CantStakeToSelfError();
138:         if (plotOccupied[landlord][plotId].occupied)
139:             revert OccupiedPlotError(landlord, plotId);
140:         if (munchablesStaked[mainAccount].length > 10)
141:             revert TooManyStakedMunchiesError();
142:         if (munchNFT.ownerOf(tokenId) != mainAccount)
143:             revert InvalidOwnerError();
144: 
145:         uint256 totalPlotsAvail = _getNumPlots(landlord);
146:         if (plotId >= totalPlotsAvail) revert PlotTooHighError();
147: 
148:         if (
149:             !munchNFT.isApprovedForAll(mainAccount, address(this)) &&
150:             munchNFT.getApproved(tokenId) != address(this)
151:         ) revert NotApprovedError();
152:         munchNFT.transferFrom(mainAccount, address(this), tokenId);
153: 
154:         plotOccupied[landlord][plotId] = Plot({
155:             occupied: true,
156:             tokenId: tokenId
157:         });
158: 
159:         munchablesStaked[mainAccount].push(tokenId);
160:         munchableOwner[tokenId] = mainAccount;
161: 
162:         toilerState[tokenId] = ToilerState({
163:             lastToilDate: block.timestamp,
164:             plotId: plotId,
165:             landlord: landlord,
166:@>           latestTaxRate: plotMetadata[landlord].currentTaxRate,
167:             dirty: false
168:         });
169: 
170:         emit FarmPlotTaken(toilerState[tokenId], tokenId);
171:     }

File: src/managers/LandManager.sol

232:     function _farmPlots(address _sender) internal {
233:         (
234:             address mainAccount,
235:             MunchablesCommonLib.Player memory renterMetadata
236:         ) = _getMainAccountRequireRegistered(_sender);
237: 
                --- SNIPPED ---

247:         for (uint8 i = 0; i < staked.length; i++) {
                --- SNIPPED ---

280:             schnibblesTotal =
281:                 (timestamp - _toiler.lastToilDate) *
282:                 BASE_SCHNIBBLE_RATE;
283:             schnibblesTotal = uint256(
284:                 (int256(schnibblesTotal) +
285:                     (int256(schnibblesTotal) * finalBonus)) / 100
286:             );
287:             schnibblesLandlord =
288:@>           (schnibblesTotal * _toiler.latestTaxRate) / //@audit _toiler.latestTaxRate will be 0
289:                 1e18;
290: 
291:             toilerState[tokenId].lastToilDate = timestamp;
292:@>           toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord]    //@audit update toilerState's latestTaxRate
293:                 .currentTaxRate;
294: 
295:@>          renterMetadata.unfedSchnibbles += (schnibblesTotal -
296:                 schnibblesLandlord);
297: 
298:             landlordMetadata.unfedSchnibbles += schnibblesLandlord;
299:             landlordMetadata.lastPetMunchable = uint32(timestamp);
300:             accountManager.updatePlayer(landlord, landlordMetadata);
301:             emit FarmedSchnibbles(
302:                 _toiler.landlord,
303:                 tokenId,
304:                 _toiler.plotId,
305:                 schnibblesTotal - schnibblesLandlord,
306:                 schnibblesLandlord
307:             );
308:         }
309:@>       accountManager.updatePlayer(mainAccount, renterMetadata);
310:     }

Impact

Users can farm schnibbles on plots with a 0% tax rate without paying any tax to the landlord, which disrupts the platform’s revenue model and incentives.

Severity Clarification Aspects

• Likelihood: Can be considered Medium as there are many accounts that have locked tokens in the LandManager. This is evident from the fact that it was deployed over 55 days ago and the locked tokens are worth more than $1 million (On-chain: LandManager).
• Impact: Can be considered Medium as it does not directly affect the landlord’s assets but results in a loss of benefits for landlords. Renters can exploit the issue by paying no tax on farming if the landlord’s tax rate is not updated. Although landlords can eventually update their rates, renters can continue farming at the previous 0% tax rate until they farm again after the update.

Proof of Concept

Prerequisites:

• Bob has locked tokens at the LockManager, with enough tokens for 10 plots.
• The AccountManager contract is upgraded to support the LandManager when forceHarvest() is called.
• The LandManager is deployed at block-10.
• block-11: Alice stakes her MunchNFT on Bob’s land with a 0% tax rate, as Bob has not triggered LandManager::triggerPlotMetadata() or interacted with the LockManager to trigger forceHarvest() after the LandManager deployment.

    toilerState[Alice's tokenId: 10] = ToilerState({
            lastToilDate: `block-11`,
            plotId: 0,
            landlord: Bob Account,
            latestTaxRate: plotMetadata[landlord].currentTaxRate => 0,
            dirty: false
        });

2. block-12: Bob triggers an update to his plotMetadata.

    plotMetadata[Bob Account] = PlotMetadata({
            lastUpdated: block-12,
            currentTaxRate: DEFAULT_TAX_RATE
        });

This currentTaxRate will not apply to Alice’s toilerState until Alice farms.

3. block-31: Alice farms.

During the schnibblesLandlord calculation, Alice’s _toiler.latestTaxRate (which was 0) will be used, even though Bob updated the currentTaxRate at block-12.

File: src/managers/LandManager.sol

232:     function _farmPlots(address _sender) internal {
233:         (
234:             address mainAccount,
235:             MunchablesCommonLib.Player memory renterMetadata
236:         ) = _getMainAccountRequireRegistered(_sender);
237: 
                --- SNIPPED ---

247:         for (uint8 i = 0; i < staked.length; i++) {
                --- SNIPPED ---

280:@>           schnibblesTotal =
281:               (timestamp - _toiler.lastToilDate) *   //@audit `block-31` - `block-11`
282:                 BASE_SCHNIBBLE_RATE;
283:             schnibblesTotal = uint256(
284:                 (int256(schnibblesTotal) +
285:                     (int256(schnibblesTotal) * finalBonus)) / 100
286:             );
287:             schnibblesLandlord =
288:@>           (schnibblesTotal * _toiler.latestTaxRate) / //@audit _toiler.latestTaxRate will be 0
289:                 1e18;
290: 
291:             toilerState[tokenId].lastToilDate = timestamp;
292:@>           toilerState[tokenId].latestTaxRate = plotMetadata[_toiler.landlord]    //@audit update toilerState's latestTaxRate
293:                 .currentTaxRate;
294: 
295:@>          renterMetadata.unfedSchnibbles += (schnibblesTotal -
296:                 schnibblesLandlord);
297: 
298:             landlordMetadata.unfedSchnibbles += schnibblesLandlord;
299:             landlordMetadata.lastPetMunchable = uint32(timestamp);
300:             accountManager.updatePlayer(landlord, landlordMetadata);
301:             emit FarmedSchnibbles(
302:                 _toiler.landlord,
303:                 tokenId,
304:                 _toiler.plotId,
305:                 schnibblesTotal - schnibblesLandlord,
306:                 schnibblesLandlord
307:             );
308:         }
309:@>       accountManager.updatePlayer(mainAccount, renterMetadata);
310:     }

Result: Alice benefits by retrieving the full schnibblesTotal from block-11 until block-31, even though Bob updated his plotMetadata immediately after Alice staked.

Recommended Mitigation Steps

Apply unset plotMetadata.currentTaxRate checks in the LandManager::stakeMunchable():

File: src/managers/LandManager.sol

131:     function stakeMunchable(
132:         address landlord,
133:         uint256 tokenId,
134:         uint256 plotId
135:     ) external override forceFarmPlots(msg.sender) notPaused {
136:         (address mainAccount, ) = _getMainAccountRequireRegistered(msg.sender);
137:         if (landlord == mainAccount) revert CantStakeToSelfError();
+138:        if (plotMetadata[landlord].lastUpdated == 0)
+139              revert PlotMetadataNotUpdatedError();
            --- SNIPPED ---
171:     }

Assessed type

Invalid Validation

0xsomeone (judge) commented:

As with other tax-related submissions, I do not consider edge conditions around tax configurations to be valid submissions in line with the validator’s assessment.

merlinboii (warden) commented:

@0xsomeone - I agree with this statement in validation-289, which misses out on the crucial point of the issue. I also agree with the other duplicates.

To specify further from the validator’s comment in the CSV:

While Landlords’ first locking of funds, updatePlotMetadata() will be called to init tax rate, please refer to L154 and L341; otherwise, there will be no valid plots to stake due to zero value locked.

The validator pointed out that the code at AccountManager.sol#L154 updates the plotMetadata right away when AccountManager.forceHarvest() is triggered, which is incorrect.

Noted that the all deployment addresses are listed in deployment-2024-06-01T21-33-57.416Z.json

The LockManager is currently live here and has stakers (or landlords in terms of the LandManager) who locked their tokens before the LandManager was deployed.

The oversight is that the current version of the AccountManager in the audit repository is not the one being used on-chain. Instead, it is an upgraded version prepared for use once the LandManager goes live.

This can be proven by accessing the live AccountManager contract here:

• Proxy: 0xfB5cd7507A3Cb0932029Df2E78165667954C8286 (set in the live LockManager)
• Logic: 0x76ee08E2f8da84D17A9625a93327775715EA6113

Below is the actual code currently used for the LockManager that has not integrated with LandManager (that have not deployed):

function forceHarvest(
    address _player
)
    external
    onlyConfiguredContract2(
        StorageKey.LockManager,
        StorageKey.MunchadexManager
    )
{
    (, MunchablesCommonLib.Player memory player) = this.getPlayer(_player);
    if (player.registrationDate != 0) {
        _harvest(_player);
        if (
            msg.sender == configStorage.getAddress(StorageKey.LockManager)
        ) {
@>          //landManager.updatePlotMetadata(_player);
        }
    }
}

This on-chain contract can prove that there is a significant amount of tokens locked and stakers in the LockManager before the introduction of the LandManager, allowing them to become landlords.

Malicious actors can stake to the non-triggered metadata landlord who has staked in the LockManager so far, immediately after the LandManager is deployed and gain rewards without paying tax rates.

Even if the landlord sets their tax rate right after the staking occurs, stakers will farm on the current tax rate at the time they staked until they farm again (proof of concept provided in this issue).

Moreover, another proof that the missing update timestamp of the landlord can occur is the triggerPlotMetadata() that is truly intended for this case.

In conclusion, I believe this issue, including other duplicates, should be reconsidered as per the statement above. The severity is also clearly described in this report and others.

0xsomeone (judge) commented:

@merlinboii, after a closer re-examination of the vulnerability described, I believe that it represents a borderline Medium severity submission.

My initial train of thought was that a landlord can simply relock their tokens to a different account or transfer their lock which is impossible per the implementation, meaning that they cannot force users to re-stake and thus acquire the updated tax rate.

The maximum impact of the submission is a single reward claim at a 0% tax rate which we can argue can last until the end of the landlord’s lock time or the time the plot would become dirty due to a price decrease, so a medium severity is appropriate for this submission.

0xinsanity (Munchables) acknowledged and commented:

I would not consider this medium. If a user sees land that has zero tax, they just won’t go to it. We’re gonna make it extremely apparent in the UI what tax rates are at for a given farm. We’re also gonna run the landmanager deployer contract atomatically with the configstorage updates. At most that’s a low-risk.

Low Risk and Non-Critical Issues

For this audit, 3 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by Rhaydden received the top score from the judge.

The following wardens also submitted reports: dimulski and peanuts.

[01] Lingering token approvals in LandManager may lead to unauthorized staking of Munchables

The LandManager contract lacks a mechanism to revoke or reset token approvals after they are granted. This could potentially allow the contract to stake Munchables without the current intent of the token owner, if they forget to revoke approvals directly through the MunchNFT contract.

The impact is low, as it doesn’t result in direct fund loss but could lead to unexpected staking of valuable NFTs.

Proof of Concept

In the stakeMunchable function of the LandManager contract, there’s a check for token approval:

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

if (
    !munchNFT.isApprovedForAll(mainAccount, address(this)) &&
    munchNFT.getApproved(tokenId) != address(this)
) revert NotApprovedError();

This check allows the staking operation to proceed if either a blanket approval for all tokens or a specific token approval is in place. However, there are two key issues:

1. The contract doesn’t provide a way to revoke these approvals within its own functions.
2. There’s no mechanism to reset or check for revoked approvals before subsequent staking operations.

For example, if a user grants approval and then later wants to revoke it, they must do so through the MunchNFT contract directly. If they forget to do this, the LandManager contract will still consider itself approved for future staking operations.

Recommended Mitigation Steps

Consider implementing a function in the LandManager that allows users to revoke approvals for their tokens, which would interact with the MunchNFT contract to remove the approvals.

Also, before each staking operation, add a fresh approval check by calling the MunchNFT contract directly, rather than relying on potentially outdated approval states. Or consider implementing an approval expiration mechanism, where approvals granted to the LandManager contract automatically expire after a certain period or after specific conditions are met (e.g., after unstaking).

[02] Tax rate updates bypass Timestamp refresh, distorting Schnibble calculations for invalid plots

The updateTaxRate function fails to update the lastUpdated timestamp when changing a landlord’s tax rate. This can lead to incorrect Schnibble calculations in the _farmPlots function. The impact is particularly significant when dealing with plots that are no longer valid, as the function relies on the outdated lastUpdated timestamp for calculations.

Proof of Concept

Take a look at updateTaxRate function:

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

function updateTaxRate(uint256 newTaxRate) external override notPaused {
    (address landlord, ) = _getMainAccountRequireRegistered(msg.sender);
    if (newTaxRate < MIN_TAX_RATE || newTaxRate > MAX_TAX_RATE)
        revert InvalidTaxRateError();
    if (plotMetadata[landlord].lastUpdated == 0)
        revert PlotMetadataNotUpdatedError();
    uint256 oldTaxRate = plotMetadata[landlord].currentTaxRate;
    plotMetadata[landlord].currentTaxRate = newTaxRate;
    emit TaxRateChanged(landlord, oldTaxRate, newTaxRate);
}

This function updates the currentTaxRate but doesn’t update the lastUpdated timestamp in plotMetadata.

The problem manifests in the _farmPlots function:

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

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

Here, for plots that are no longer valid, the function uses plotMetadata[landlord].lastUpdated as the timestamp. If tax rates have changed since the last update which is possible to lastUpdated, this could lead to incorrect Schnibble calculations.

Recommended Mitigation Steps

Update the updateTaxRate function to include the lastUpdated timestamp:

function updateTaxRate(uint256 newTaxRate) external override notPaused {
    (address landlord, ) = _getMainAccountRequireRegistered(msg.sender);
    if (newTaxRate < MIN_TAX_RATE || newTaxRate > MAX_TAX_RATE)
        revert InvalidTaxRateError();
    if (plotMetadata[landlord].lastUpdated == 0)
        revert PlotMetadataNotUpdatedError();
    uint256 oldTaxRate = plotMetadata[landlord].currentTaxRate;
    plotMetadata[landlord].currentTaxRate = newTaxRate;
+   plotMetadata[landlord].lastUpdated = block.timestamp;
    emit TaxRateChanged(landlord, oldTaxRate, newTaxRate);
}

[03] Improper initialization in LandManager allows partial state setup, risking inconsistent behavior

The current initialization pattern in the LandManager contract and its inherited contracts (although, out of scope for this audit) creates a issue where partial initialization of the contract state is possible. This can lead to an inconsistent contract state, potentially causing unexpected behavior.

Proof of Concept

The issue stems from the inheritance structure and initialization pattern used in the LandManager contract:

LandManager inherits from BaseBlastManagerUpgradeable:

contract LandManager is BaseBlastManagerUpgradeable, ILandManager {
    // ...
}

BaseBlastManagerUpgradeable in turn inherits from BaseConfigStorageUpgradeable:

abstract contract BaseBlastManagerUpgradeable is
    BaseBlastManager,
    BaseConfigStorageUpgradeable
{
    // ...
}

But both parent contracts have their own initialize functions with the initializer modifier:

// In BaseConfigStorageUpgradeable
function initialize(address _configStorage) public virtual initializer {
    __BaseConfigStorage_setConfigStorage(_configStorage);
}

// In BaseBlastManagerUpgradeable
function initialize(address _configStorage) public virtual override initializer {
    BaseConfigStorageUpgradeable.initialize(_configStorage);
}

However, the LandManager contract overrides the initialize function:

function initialize(address _configStorage) public override initializer {
    BaseBlastManagerUpgradeable.initialize(_configStorage);
    _reconfigure();
}

Now, the issue here is that while the initializer modifier in LandManager prevents multiple calls to its initialize function, it doesn’t prevent direct calls to the initialize functions of parent contracts. This could lead to partial initialization if, for example, BaseConfigStorageUpgradeable.initialize is called directly, bypassing the initializer check in LandManager.

Recommended Mitigation Steps

We could consider using the __init pattern for internal initializers in parent contracts. For example, we could rename initialize to __BaseConfigStorageUpgradeable_init in BaseConfigStorageUpgradeable and make it internal.

In the LandManager’s initialize function, call all necessary internal initializers from parent contracts explicitly. And ensure that the initializer modifier is only used in the most derived contract (LandManager in this case).

[04] Inaccurate timestamp handling in _farmPlots function leading to potential inaccurate Schnibbles calculation

First of all, I’d like to note that this issue is considered out of scope BUT the sponsors said they will accept alternative solutions to mitigate this issue as it is clearly stated in the “Known issues” in the ReadMe.

The _farmPlots function in the LandManager contract uses the lastUpdated timestamp from plotMetadata to calculate the schnibbles earned by a renter. However, if the plot ID is greater than the number of plots available to the landlord, the function uses the latest lastUpdated timestamp, which can lead to inaccurate calculations if the landlord has updated the plots multiple times.

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

function _farmPlots(address _sender) internal {
    (
        address mainAccount,
        MunchablesCommonLib.Player memory renterMetadata
    ) = _getMainAccountRequireRegistered(_sender);

    uint256[] memory staked = munchablesStaked[mainAccount];
    MunchablesCommonLib.NFTImmutableAttributes memory immutableAttributes;
    ToilerState memory _toiler;
    uint256 timestamp;
    address landlord;
    uint256 tokenId;
    int256 finalBonus;
    uint256 schnibblesTotal;
    uint256 schnibblesLandlord;
    for (uint8 i = 0; i < staked.length; i++) {
        timestamp = block.timestamp;
        tokenId = staked[i];
        _toiler = toilerState[tokenId];
        if (_toiler.dirty) continue;
        landlord = _toiler.landlord;
        // use last updated plot metadata time if the plot id doesn't fit
        // track a dirty bool to signify this was done once
        // the edge case where this doesn't work is if the user hasn't farmed in a while and the landlord
        // updates their plots multiple times. then the last updated time will be the last time they updated their plot details
        // instead of the first
        if (_getNumPlots(landlord) < _toiler.plotId) {
            timestamp = plotMetadata[landlord].lastUpdated;
            toilerState[tokenId].dirty = true;
        }
        // existing code...
    }
    // existing code...
}

Impact

The impact of this issue is that the schnibblesTotal calculation may be based on an incorrect timestamp, leading to either overestimating or underestimating the schnibbles earned by the renter and the landlord. This can result in unfair distribution of rewards and potential dissatisfaction among users.

Recommended Mitigation Steps

Here are 3 steps that could be taken to fix this:

Solution 1: Track Multiple Timestamps

1. Modify PlotMetadata to include an array of timestamps:

struct PlotMetadata {
    uint256[] updateTimestamps;
    uint256 currentTaxRate;
}

2. Update _reconfigure and other functions to handle multiple timestamps:

function _reconfigure() internal {
    // existing code...
    plotMetadata[landlord].updateTimestamps.push(block.timestamp);
    // existing code...
}

3. Modify _farmPlots to use the correct timestamp:

function _farmPlots(address _sender) internal {
    // existing code...
    for (uint8 i = 0; i < staked.length; i++) {
        // existing code...
        if (_getNumPlots(landlord) < _toiler.plotId) {
            uint256 correctTimestamp = _getCorrectTimestamp(landlord, _toiler.lastToilDate);
            timestamp = correctTimestamp;
            toilerState[tokenId].dirty = true;
        }
        // existing code...
    }
    // existing code...
}

function _getCorrectTimestamp(address landlord, uint256 lastToilDate) internal view returns (uint256) {
    uint256[] memory timestamps = plotMetadata[landlord].updateTimestamps;
    for (uint256 i = 0; i < timestamps.length; i++) {
        if (timestamps[i] > lastToilDate) {
            return timestamps[i];
        }
    }
    return block.timestamp; // fallback to current timestamp if no match found
}

Solution 2: Use a Mapping for Timestamps

1. Modify PlotMetadata to include a mapping of timestamps:

struct PlotMetadata {
    mapping(uint256 => uint256) updateTimestamps;
    uint256 currentTaxRate;
}

2. Update _reconfigure and other functions to handle the mapping:

function _reconfigure() internal {
    // existing code...
    plotMetadata[landlord].updateTimestamps[plotId] = block.timestamp;
    // existing code...
}

3. Modify _farmPlots to use the correct timestamp:

function _farmPlots(address _sender) internal {
    // existing code...
    for (uint8 i = 0; i < staked.length; i++) {
        // existing code...
        if (_getNumPlots(landlord) < _toiler.plotId) {
            uint256 correctTimestamp = plotMetadata[landlord].updateTimestamps[_toiler.plotId];
            timestamp = correctTimestamp;
            toilerState[tokenId].dirty = true;
        }
        // existing code...
    }
    // existing code...
}

Solution 3: Snapshot Mechanism

1. Create a Snapshot struct to store plot states:

struct Snapshot {
    uint256 timestamp;
    uint256 taxRate;
}

2. Modify PlotMetadata to include an array of snapshots:

struct PlotMetadata {
    Snapshot[] snapshots;
}

3. Update _reconfigure and other functions to handle snapshots:

function _reconfigure() internal {
    // existing code...
    plotMetadata[landlord].snapshots.push(Snapshot({
        timestamp: block.timestamp,
        taxRate: currentTaxRate
    }));
    // existing code...
}

4. Modify _farmPlots to use the correct snapshot:

function _farmPlots(address _sender) internal {
    // existing code...
    for (uint8 i = 0; i < staked.length; i++) {
        // existing code...
        if (_getNumPlots(landlord) < _toiler.plotId) {
            Snapshot memory correctSnapshot = _getCorrectSnapshot(landlord, _toiler.lastToilDate);
            timestamp = correctSnapshot.timestamp;
            toilerState[tokenId].dirty = true;
        }
        // existing code...
    }
    // existing code...
}

function _getCorrectSnapshot(address landlord, uint256 lastToilDate) internal view returns (Snapshot memory) {
    Snapshot[] memory snapshots = plotMetadata[landlord].snapshots;
    for (uint256 i = 0; i < snapshots.length; i++) {
        if (snapshots[i].timestamp > lastToilDate) {
            return snapshots[i];
        }
    }
    return Snapshot({timestamp: block.timestamp, taxRate: 0}); // fallback to current state if no match found
}

Any of the recommendations above should fix this issue.

[05] Repurposed StorageKeys in LandManager compromise code clarity

The use of unrelated StorageKey values for tax rate configurations in the LandManager reduces code readability, increases the risk of future maintenance errors, and could potentially lead to conflicts if these keys are needed for their original purposes in future updates. Although not a bug per se, this approach introduces subtle risks to the long-term maintainability and extensibility of the system.

Proof of Concept

In the LandManager contract, tax rates are configured using seemingly unrelated StorageKey values:

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

// LandManager.sol
function _reconfigure() internal {
    // ...
    MIN_TAX_RATE = IConfigStorage(configStorage).getUint(
        StorageKey.LockManager
    );
    MAX_TAX_RATE = IConfigStorage(configStorage).getUint(
        StorageKey.AccountManager
    );
    DEFAULT_TAX_RATE = IConfigStorage(configStorage).getUint(
        StorageKey.ClaimManager
    );
    // ...
}

However, in the IConfigStorage interface, these StorageKey values are defined for different purposes:

// IConfigStorage.sol
enum StorageKey {
    // ...
    LockManager,
    AccountManager,
    ClaimManager,
    // ...
}

While this approach solves the immediate problem of storing new configuration values without updating the StorageKey enum, it introduces subtle issues:

• The StorageKey names do not reflect their actual usage in LandManager, making the code less intuitive.
• Future developers might misinterpret the purpose of these keys, leading to potential errors.
• If these StorageKey values are needed for their original purposes in future updates, it could cause conflicts.

Recommended Mitigation Steps

Consider adding detailed comments in both LandManager and IConfigStorage contracts explaining the repurposing of these StorageKey values for LandManager’s tax rates. Also, implement additional checks or assertions in the configuration retrieval process to ensure these StorageKey values are used correctly in different contexts.

[06] Incorrect staking limit check allows users to stake 11 munchables instead of intended 10

The current implementation allows users to stake up to 11 Munchables, exceeding the intended maximum of 10. This could lead to an imbalance in the game protocol, potentially giving some users an unfair advantage by allowing them to earn more rewards than intended.

Proof of Concept

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

    function stakeMunchable(
        address landlord,
        uint256 tokenId,
        uint256 plotId
    ) external override forceFarmPlots(msg.sender) notPaused {
        // ... other checks ...
 @>     if (munchablesStaked[mainAccount].length > 10)
            revert TooManyStakedMunchiesError();
        // ... rest of the function
}

The condition munchablesStaked[mainAccount].length > 10 only reverts when trying to stake the 12th Munchable. This means:

1. A user can successfully stake their 1st to 10th Munchable.
2. They can also stake their 11th Munchable, as 11 is not greater than 10.
3. The error is only thrown when trying to stake the 12th Munchable.

As a result, users can stake 11 Munchables instead of the intended maximum of 10.

Recommended Mitigation Steps

Change the condition to use greater than or equal to (>=) instead of strictly greater than (>):

function stakeMunchable(
    address landlord,
    uint256 tokenId,
    uint256 plotId
) external override forceFarmPlots(msg.sender) notPaused {
    // ... other checks ...
-   if (munchablesStaked[mainAccount].length > 10)
+   if (munchablesStaked[mainAccount].length >= 10)
        revert TooManyStakedMunchiesError();
    // ... rest of the function
}

[07] Precision loss in landlord Schnibble calculation

The current implementation of the schnibble calculation for landlords can result in significant precision loss, potentially leading to landlords receiving fewer schnibbles than they should. In extreme cases, particularly with small schnibblesTotal values or low tax rates, landlords might receive no schnibbles at all when they should have received a small amount.

Proof of Concept

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

    schnibblesTotal =
        (timestamp - _toiler.lastToilDate) *
        BASE_SCHNIBBLE_RATE;
    schnibblesTotal = uint256(
        (int256(schnibblesTotal) +
            (int256(schnibblesTotal) * finalBonus)) / 100
    );
 ❌  schnibblesLandlord =
        (schnibblesTotal * _toiler.latestTaxRate) /
        1e18;