Lines of code

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

Vulnerability details

Impact

If a player staked a munchable in one of the last plots available of a landlord and the price of ETH/USD drops, the owner of the munchable will not be able to perform any staking related actions for any of their munchables. Staking, unstaking, transfer to another plot, farm plots will revert due to an underflow in the _farmPlots function. Leaving all the owner's munchables that were transferred to LandManager stuck.

Additionally, the landlord will be partially impacted because he will have at least one plot occupied that will not collect schnibbles from taxes.

Proof of Concept

An underflow can happen inside the function _farmPlots which is executed before any staking related action.

The issue happens because when the price of ETH/USD drops according to the price reported by LockManager, the amount of plots available for the landlord decreases.

This can make the condition in the if statement on line 258 true if one of the biggest plotId available for the landlord was in use. In that case, the variable timestamp gets assigned a value of plotMetadata[landlord].lastUpdated which corresponds to the timestamp of the last time the plot metadata was updated by the AccountManager.

The execution then follows until line 281, where a subtraction occurs that ends up in an underflow. In the expression (timestamp - _toiler.lastToilDate), under the circumstances described, most of the time _toiler.lastToilDate will be greater than plotMetadata[landlord].lastUpdated.

The reason why _toiler.lastToilDate will most of the time be greater than plotMetadata[landlord].lastUpdated is because lastUpdated is only updated with a new timestamp on very special occasions, such as when the landlord locks or unlock funds, which will not happen as frequently as farming plots.

Additionally, since the munchables stuck are owned by the staker and not the landlord, the landlord might not be aware that the issue was triggered, so they might not have an incentive to lock / unlock to solve the issue.

The following test demonstrates how the scenario could happen by emulating a player staking a munchable in one of the landlords last plots and then making the price of ETH/USD drop 5%. After the price drops, staking, unstaking, transfer and farm plots calls revert.

To try this out you can add this test to the file tests/managers/LandManager/stakeMunchable.test.ts

    it("munchables stuck by underflow when ETH/USD drops", async () => {
      let blockData = await testClient.getBlock();
      let initialTimestamp = blockData.timestamp;

      // test address taken from anvil to be used as test account
      let landlord = "0xa0Ee7A142d267C1f36714E4a8F75612F20a79720";
      await testClient.setBalance({
        address: landlord,
        value: parseEther("10"),
      });
      const registerTxHash = await testContracts.accountManagerProxy.contract.write.register(
        [0, zeroAddress],
        {
          account: landlord,
        });
      await assertTxSuccess({ txHash: registerTxHash });

      // lock funds in account to have 1 plot available
      const lockTxHash = await testContracts.lockManager.contract.write.lock(
        [zeroAddress, parseEther("1")],
        { account: landlord, value: parseEther("1") }
      );
      await assertTxSuccess({ txHash: lockTxHash });

      // bob approves all tokens for land manager to be able to stake
      const txHash = await testContracts.munchNFT.contract.write.setApprovalForAll(
        [testContracts.landManagerProxy.contract.address, true],
        { account: bob }
      );
      await assertTxSuccess({ txHash });

      // bob stakes in the landlord's last plot available
      let lockedWeightedValue =
        await testContracts.lockManager.contract.read.getLockedWeightedValue(
          [landlord],
        );
      let PRICE_PER_PLOT = BigInt(30e18);
      let numPlots = lockedWeightedValue / PRICE_PER_PLOT;
      let lastPlot = numPlots - 1n;

      const stakeMunchableTxHash =
        await testContracts.landManagerProxy.contract.write.stakeMunchable([landlord, 1, lastPlot], {
          account: bob,
        });
      await assertTxSuccess({ txHash: stakeMunchableTxHash });

      // emulate that some time passes
      // this is not necessary, but makes the test more real. It can be commented to simplify
      await testClient.setNextBlockTimestamp({ 
        timestamp: initialTimestamp + 20n,
      })
      await testClient.mine({blocks: 1});

      // configure thresholds, propose and approve a new USD price
      // change USD price, emulate a 5% drop
      const etherUSDBefore = (await testContracts.lockManager.contract.read.configuredTokens([zeroAddress]));

      const testRoleAddresses = await getTestRoleAddresses();
      let admin = testRoleAddresses[0]; // admin role
      const setUSDThresholdsTxHash =
        await testContracts.lockManager.contract.write.setUSDThresholds([1, 1], { account: admin });
      await assertTxSuccess({ txHash: setUSDThresholdsTxHash });

      const price = etherUSDBefore[0] * 95n / 100n; // price dropped by 5%
      let priceFeed1 = testRoleAddresses[11]; // role Price Feed 1
      let priceFeed2 = testRoleAddresses[12]; // role Price Feed 2
      const proposeUSDPriceTx = await testContracts.lockManager.contract.write.proposeUSDPrice(
        [price, [zeroAddress]],
        {
          account: priceFeed1,
        }
      );
      await assertTxSuccess({ txHash: proposeUSDPriceTx });

      const approveUSDPriceTx = await testContracts.lockManager.contract.write.approveUSDPrice([price], {
        account: priceFeed2,
      });
      await assertTxSuccess({ txHash: approveUSDPriceTx });

      // From now on, all staking related actions that bob want to make will revert

      // bob should be able to transfer his munchable to an unoccupied plot,
      // but the call reverts
      const transferMunchableTxHash =
        await testContracts.landManagerProxy.contract.write.transferToUnoccupiedPlot([1, 0], {
          account: bob,
        });

      const transferMunchableTxReceipt = await testClient.waitForTransactionReceipt({
        hash: transferMunchableTxHash,
      });
      assert.equal(transferMunchableTxReceipt.status, 'reverted');

      // bob should be able to farm plots (including plots from other landlords),
      // but the call reverts
      const farmPlotsTxHash =
        await testContracts.landManagerProxy.contract.write.farmPlots([], {
          account: bob,
        });
      const farmPlotsTxReceipt = await testClient.waitForTransactionReceipt({
        hash: farmPlotsTxHash,
      });
      assert.equal(farmPlotsTxReceipt.status, 'reverted');

      // bob should be able to unstake his munchable,
      // but the call reverts
      const unstakeMunchableTxHash =
        await testContracts.landManagerProxy.contract.write.unstakeMunchable([1], {
          account: bob,
        });

      const unstakeMunchableTxReceipt = await testClient.waitForTransactionReceipt({
        hash: unstakeMunchableTxHash,
      });
      assert.equal(unstakeMunchableTxReceipt.status, 'reverted');

      // bob cannot stake a DIFFERENT munchable with a DIFFERENT landlord
      // this should work, but it reverts
      const stakeAnotherMunchableTxHash =
        await testContracts.landManagerProxy.contract.write.stakeMunchable([alice, 2, 0], {
          account: bob,
        });
      const stakeAnotherMunchableTxReceipt = await testClient.waitForTransactionReceipt({
        hash: stakeAnotherMunchableTxHash,
      });
      assert.equal(transferMunchableTxReceipt.status, 'reverted');
    });

Tools Used

Test suite

Recommended Mitigation Steps

A solution to the problem could be adding / replacing the contents of the if branch on line 258 by a continue, so that if a munchable is staked in an invalid plot it will not generate schnibbles.

Leaving the code like this:

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

Alternatively, if skipping the update for that munchable is not suitable, a check could be added before line 280 to ensure _toiler.lastToilDate is less than or equal to timestamp before computing schnibblesTotal.

            if (timestamp >= _toiler.lastToilDate) {
                schnibblesTotal =
                    (timestamp - _toiler.lastToilDate) *
                    BASE_SCHNIBBLE_RATE;
            } else {
                schnibblesTotal = 0;
            }

Assessed type

Under/Overflow

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

Underflow in _farmPlots leads to stuck munchables #238