howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Vulnerability Details:

The function _farmPlots() is used to calculate rewards farming rewards. This function is used in different scenarios, including like a modifier in the function unstakeMunchable() function when a user unstakes their NFT. Let’s have a look at how finalBonus is calculated in the function:

finalBonus =
    int16(
        REALM_BONUSES[
            (uint256(immutableAttributes.realm) * 5) +
                uint256(landlordMetadata.snuggeryRealm)
        ]
    ) +
    int16(
        int8(RARITY_BONUSES[uint256(immutableAttributes.rarity)])
    );

The finalBonus consists of bonus from the realm (REALM_BONUSES), as well as a rarity bonus (RARITY_BONUSES). REALM_BONUSES can be either -10, -5, 0, 5 or 10, and RARITY_BONUSES can be either 0, 10, 20, 30 or 50. finalBonus is meant to incentivize staking Munchables in a plot in a suitable realm. It can either be a positive bonus or reduction in the amount of schnibblesTotal. An example of a negative scenario is when REALM_BONUSES is -10 and RARITY_BONUSES is 0

Let's look at the calculation of schnibblesTotal:

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

schnibblesTotal is typed as uint256, therefore is meant as always positive. The current calculation, however, will result in a negative value of schnibblesTotal, when finalBonus < 0, the conversion to uint256 will cause the value to evaluate to something near type(uint256).max. In that case the next calculation of schnibblesLandlord will revert due to overflow:

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

This is due to the fact that _toiler.latestTaxRate > 1e16.

Since unstakeMunchable() has a modifier forceFarmPlots(), the unstaking will be blocked if _farmPlots() reverts.

Scenario

Alice has a Common or Primordial Munchable NFT from Everfrost.
Bob has land (plots) in Drench.
Alice stakes her NFT in Bob's plot.
After some time Alice unstakes her NFT.
Since in the calculation of schnibblesTotal is used a negative finalBonus, the transaction will revert.
Alice's Munchable NFT and any other staked beforehand are blocked.

Proof of Concept

Add the following lines of code in tests/managers/LandManager/unstakeMunchable.test.ts:

PoC

```diff . . . await registerPlayer({ account: alice, + realm: 1, testContracts, }); await registerPlayer({ account: bob, testContracts, }); await registerPlayer({ account: jirard, testContracts, }); . . . 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, 4, 12], { account: bob }); // 1 + await mockNFTOverlord.write.reveal([bob, 1, 0], { account: bob }); // 1 await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 2 await mockNFTOverlord.write.reveal([bob, 0, 13], { account: bob }); // 2 await mockNFTOverlord.write.startReveal([bob], { account: bob }); // 3 ```

When we run the command pnpm test:typescript the logs from the tests show that the successful path test reverts.

Impact

A user's Munchable NFT with specific attributes can be stuck in the protocol.

Taking into account the rarity distribution across the different NFT-s and assuming that the distribution across realms is equal, the chance of this problem occurring is around 26% or 1 in every 4 staked munchables.

Further, this finding blocks unstaking every munchable that has previously been staked.

Recommended Mitigation

finalBonus is meant as a percentage change in the Schnibbles earned by a Munchable. Change the calculation of schnibblesTotal in _farmPlots() to reflect that by removing the brackets:

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

Assessed type

Math

0xinsanity commented 2 months ago

fix: https://github.com/munchablesorg/munchables-common-core-latest/pull/55

alex-ppg commented 2 months ago

The Warden and its duplicates have outlined a significant issue in the way the finalBonus is added or subtracted from the total as the order of operations is incorrect.

I believe a high-risk severity rating is appropriate given that rewards are significantly impacted and it may ultimately be possible for a Munchable to be permanently locked in the system.

c4-judge commented 2 months ago

alex-ppg marked the issue as selected for report