DOS on LandManager when `getLockedWeightedValue` suddenly changes

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

The LandManager contract has a modifier forceFarmPlots that calls _farmPlots() before stakeMunchable, unstakeMunchable or transferToUnoccupiedPlot. Plots can also be manually farmed by calling the farmPlots function.

Inside the _farmPlots function, if a staked token is out of range, the contract sets the timestamp variable to plotMetadata[landlord].lastUpdated instead of block.timestamp.

      ❌@> 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 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) {
      ❌@>     timestamp = plotMetadata[landlord].lastUpdated;
                toilerState[tokenId].dirty = true;
            }

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

Afterward, schnibblesTotal is calculated using the following equation:

            schnibblesTotal =
       ❌@>    (timestamp - _toiler.lastToilDate) *
                BASE_SCHNIBBLE_RATE;

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

There is a false assumption that timestamp is always greater than _toiler.lastToilDate which could lead to a Denial of Service (DoS) in the LandManager contract. When _getNumPlots(landlord) decreases due to a price change or a token being disabled, _toiler.lastToilDate can be greater than plotMetadata[landlord].lastUpdated, causing an underflow and a revert.

Impact

User's staked munchables in the landlord's plots will be stuck in the contract until the price goes up or the landlord locks more tokens in the LockManager contract, which can take a long time.

Even if a user has 5 munchables staked to landlord A's plots, and 1 munchable staked to landlord B's plots, all 6 munchables will be stuck in the contract as _farmPlots will revert due to underflow.

Proof of Concept

Scenario for this attack:

• Starting State @ block.timestamp = 100
  • PRICE_PER_PLOT = 30
  • lockManager.getLockedWeightedValue(landlord) = 90
  • ⇒ _getNumPlots(landlord) = 3
  • ⇒ Landlord has 3 plots empty
  • plotMetadata[landlord].lastUpdated ⇒ 50

---

1. Bob calls stakeMunchable(landlord, 3, 2) @ block.timestamp = 150

   • plotOccupied[landlord][2].occupied = true
   • toilerState[3].lastToilDate = 150

---

2. LockManager.sol USD Price changes or configuredToken.active gets set to false @ block.timestamp = 200

   • Due that getLockedWeightedValue(landlord) goes below 90 and _getNumPlots(landlord) becomes = 1

---

3. Bob tries to call stakeMunchable / unstakeMunchable / transferToUnoccupiedPlot / farmPlots @ block.timestamp = 250
   • forceFarmPlots modifier is called before execution
   • _farmPlots(bob) loops through it's staked tokens
   • When tokenId = 3 ⇒ _toiler.plotId = 2
   • Because of the check *if* (_getNumPlots(landlord) < _toiler.plotId) timestamp gets updated from 250 to plotMetadata[landlord].lastUpdated ⇒ 50
   • toiler.\_lastToilDate is still 150
   • schnibblesTotal is calculated using the following equation
     • (timestamp - toiler._lastToilDate) * _BASE_SCHNIBBLE_RATE
     • converted to the current state: (50 - 150) * BASE_SCHNIBBLE_RATE ⇒ this will revert due to underflow

Coded proof of concept

Inside stakeMunchable.test.ts do the following:

• Add import { Role } from "../../../deployments/utils/config-consts"; as import
• Add let admin: `0x${string}`; below describe("LandManager: stakeMunchable", () => {
• Add the following test:

Toggle

```typescript it("DOS price change", async () => { console.log( "🕐 Current time: " + (await testClient.getBlock()).timestamp.toString(), ); const txHash = await testContracts.munchNFT.contract.write.setApprovalForAll( [testContracts.landManagerProxy.contract.address, true], { account: bob }, ); await assertTxSuccess({ txHash }); const plots = await testContracts.landManagerProxy.contract.read._getNumPlots( [alice], ); console.log("Plots when staking: " + plots.toString()); const stakeMunchableTxHash = await testContracts.landManagerProxy.contract.write.stakeMunchable( [alice, 1, plots - 2n], { account: bob, }, ); await assertTxSuccess({ txHash: stakeMunchableTxHash }); console.log(`✅ Staked successfully at plot[${plots - 2n}]`); // Jump 50 seconds ahead await testClient.increaseTime({ seconds: 50 }); await testClient.mine({ blocks: 1 }); console.log("🕐 Jumped some time ahead..."); console.log( "🕐 Current time: " + (await testClient.getBlock()).timestamp.toString(), ); console.log(`Trying to stake at plot[${plots - 3n}] without price changing`); const stakeMunchableTxHash2 = await testContracts.landManagerProxy.contract.write.stakeMunchable( [alice, 2, plots - 3n], { account: bob, }, ); await assertTxSuccess({ txHash: stakeMunchableTxHash2 }); console.log(`✅ Staked successfully at plot[${plots - 3n}]`); // Jump 50 seconds ahead await testClient.increaseTime({ seconds: 50 }); await testClient.mine({ blocks: 1 }); console.log("🕐 Jumped some time ahead..."); console.log( "🕐 Current time: " + (await testClient.getBlock()).timestamp.toString(), ); // can be any token in the lockManager, price needs to drop just enough so landlord's plots decrease by 2 const tokenData = { usdPrice: 800000000000000000000n, nftCost: 69n, active: true, decimals: 18, }; const { request } = await testContracts.lockManager.contract.simulate.configureToken( [zeroAddress, tokenData], { account: admin, }, ); const txHashToken = await testClient.writeContract(request); await assertTxSuccess({ txHash: txHashToken }); console.log("💸 Price changed"); const plots2 = await testContracts.landManagerProxy.contract.read._getNumPlots([alice]); console.log("Plots after price change: " + plots2.toString()); console.log("🕐 Jumped some time ahead..."); console.log( "🕐 Current time: " + (await testClient.getBlock()).timestamp.toString(), ); console.log(`Trying to stake to plot[${plots2 - 2n}] AFTER price changing`); const stakeMunchableTxHash3 = await testContracts.landManagerProxy.contract.write.stakeMunchable( [alice, 3, plots2 - 2n], { account: bob, }, ); const txReceipt = await testClient.waitForTransactionReceipt({ hash: stakeMunchableTxHash3, }); console.log( "❌ Stake reverted, LandManager is DOS'ed until price goes up or lender increases his locked weighted value ❌", ); assert.equal(txReceipt.status, "reverted"); }); ```

Tools Used

Manual review

Recommended Mitigation Steps

Refactor _farmPlots to check if timestamp < _toiler.lastToilDate:

+  if (timestamp < _toiler.lastToilDate) {
+    // or plotMetadata[landlord].lastUpdated = _toiler.lastToilDate + 1
+    plotMetadata[landlord].lastUpdated = block.timestamp;
+    continue;
+  }
  schnibblesTotal =
      (timestamp - _toiler.lastToilDate) *
      BASE_SCHNIBBLE_RATE;

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

This ensures that the calculation for schnibblesTotal will not cause an underflow and revert.

Assessed type

Context

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #76

[c4-judge]

alex-ppg marked the issue as satisfactory