Incorrect timestamp updating for invalid plots due to USD price fluctuation

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LandManager.sol#L258-L282
https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LockManager.sol#L484-L492
https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LockManager.sol#L523-L524

Vulnerability details

Description

Varying by USD price of the locked tokens proposed by oracle can make the Plot smaller and dirty, but the landlord's plotMetadata.timestamp does not update when the price changes.

File: src/managers/LandManager.sol

344:     function _getNumPlots(address _account) internal view returns (uint256) {
345:@>       return lockManager.getLockedWeightedValue(_account) / PRICE_PER_PLOT;
346:     }

As the _getNumPlots() can vary due to two factors: lock size (landlord) and USD price of the locked token (proposed by a trusted oracle), the USD price could lower the return of lockManager::getLockedWeightedValue() for the same lock amount.

File: src/managers/LockManager.sol
On-chain deployed: 0xEA091311Fc07139d753A6BBfcA27aB0224854Bae (Blast)

468:     /// @inheritdoc ILockManager
469:     function getLockedWeightedValue(
470:         address _player
471:     ) external view returns (uint256 _lockedWeightedValue) {
                --- SNIPPED ---
481:                 uint256 deltaDecimal = 10 **
482:                     (18 -
483:                         configuredTokens[configuredTokenContracts[i]].decimals);
484:                 lockedWeighted +=
485:                     (deltaDecimal *
486:                         lockedTokens[_player][configuredTokenContracts[i]]
487:                             .quantity *
488:@>                       configuredTokens[configuredTokenContracts[i]]
489:                             .usdPrice) /
490:                     1e18;
491:             }
492:         }
493: 
494:         _lockedWeightedValue = lockedWeighted;
495:     }

        --- SNIPPED ---

510:     /*******************************************************
511:      ** INTERNAL FUNCTIONS
512:      ********************************************************/
513: 
514:     function _execUSDPriceUpdate() internal {
                --- SNIPPED ---
520:             for (uint256 i; i < updateTokensLength; i++) {
521:                 address tokenContract = usdUpdateProposal.contracts[i];
522:                 if (configuredTokens[tokenContract].nftCost != 0) {
523:@>                   configuredTokens[tokenContract].usdPrice = usdUpdateProposal
524:                         .proposedPrice;
525: 
                --- SNIPPED ---
534:         }
535:     }

As a result of not updating the landlord's plotMetadata.timestamp when the USD price changes, the plotMetadata.timestamp that reflects the timestamp for when the plots become dirty is incorrect. This causes the user to unfairly gain schnibbles form an incorrect time tracking.

File: src/managers/LandManager.sol

232:     function _farmPlots(address _sender) internal {
233:         (
234:             address mainAccount,
235:             MunchablesCommonLib.Player memory renterMetadata
236:         ) = _getMainAccountRequireRegistered(_sender);
237: 
                --- SNIPPED ---

247:         for (uint8 i = 0; i < staked.length; i++) {
                --- SNIPPED ---
                
258:             if (_getNumPlots(landlord) < _toiler.plotId) {
259:@>               timestamp = plotMetadata[landlord].lastUpdated;
260:                 toilerState[tokenId].dirty = true;
261:             }
                --- SNIPPED ---
280:@>           schnibblesTotal =
281:@>               (timestamp - _toiler.lastToilDate) *
282:@>               BASE_SCHNIBBLE_RATE;
                --- SNIPPED ---
310:     }

Impact

Users gain schnibbles based on an incorrect time period due to outdated timestamps, leading to potential unfair.

---

Severity Clarification Aspects:

• Likelihood: Can be considered Medium or High since the tokens that can be locked may have fluctuations in USD price. Although the locked assets listed on-chain include USDB (a stablecoin), they also allow fluctuating assets like WETH, BLAST, so price fluctuation is a concern.

• Impact: Can be considered High. The issue significantly impacts users by leading to incorrect schnibbles calculations and unfair rewards.

Proof of Concept

For the tokenId that stakes on the last available plotId of the landlord, this issue could potentially occur.

Revert on Farming due to Underflow in Use Lesser Timestamp

1. Landlord has 10 plots available. => plotMetadata.lastUpdated: block 9.

2. User stakes at plot 9 (the maximum available plotId).

3. User farms to gain schnibbles and pays tax to the landlord as expected. => lastToilDate: block 19.

4. Assuming the proposed USD price change occurs at block 30, and the USD price in the LockManager has decreased. The landlord still locks 10 plots and does not perform any activity to update their plotMetadata.

5. User attempts to farm again, but _getNumPlots() returns a smaller value, causing that tokenId to become dirty.

   • timestamp: plotMetadata.lastUpdated = block 9.
   • Reverts as schnibblesTotal = (timestamp(block 9) - lastToilDate (block 19)) * BASE_SCHNIBBLE_RATE.

Not Revert but Only Use Lesser Timestamp for Calculation

1. Landlord has 10 plots available. => plotMetadata.lastUpdated: block 9.

2. User stakes at plot 9 (the maximum available plotId).

3. User farms to gain schnibbles and pays tax to the landlord as expected. => lastToilDate: block 19.

4. Landlord calls LockManager::setLockDuration at block 20 (without changing any lock amount). => plotMetadata.lastUpdated: block 20.

5. Assuming the proposed USD price change occurs at block 30, and the USD price in the LockManager has decreased. The landlord still locks 10 plots and does not perform any activity to update their plotMetadata.

6. User attempts to farm again, but _getNumPlots() returns a smaller value, causing that tokenId to become dirty.

   • timestamp: plotMetadata.lastUpdated = block 20.
   • schnibblesTotal = (timestamp(block 20) - lastToilDate (block 19)) * BASE_SCHNIBBLE_RATE.

In both cases, the invalid plot seems to use the wrong timestamp retrieved from plotMetadata.lastUpdated, which should actually reflect the action that made them invalid.

Tools Used

• Manual Review

Recommended Mitigation Steps

Update plotMetadata.lastUpdated whenever there is a change in the USD price that impacts the number of plots (numPlots). This ensures that the timestamp reflects the correct state of the plots.

However, since the LockManager is already deployed, handling the configuration off-chain might be required. To propose the price, it requires an actor that holds the PriceFeed Role. When the new price is applied, they should update the related landlord.

Currently, there are three tokens available for locking, and the number of plots is retrieved from three different quantities of locked tokens, depending on each landlord. When updating the landlord's plotMetadata, ensure that the token price updates are related to the landlord's locked tokens that is used in calculated the number of plots.

Assessed type

Context

[0xinsanity]

Acknowledged, not a concern.

[alex-ppg]

This submission is effectively a duplicate of #76 as it arises from the same underflow operation and even the same calculation albeit due to a different condition. I do not agree with the Sponsor's statement that this is not a concern as an underflow would result in a Denial-of-Service.

The alleviation shared on #76 is actually not correct @0xinsanity so I would appreciate the link being fixed so we can ensure that this submission and #76 have been simultaneously addressed.

[c4-judge]

alex-ppg marked the issue as duplicate of #76

[c4-judge]

alex-ppg marked the issue as satisfactory

[0xinsanity]

You are correct, I misread the comment. I see, my comparison operator was reversed... Does this fix it? https://github.com/munchablesorg/munchables-common-core-latest/commit/75f700020455977396a6685210e811229425b498