Stakers could harvest schnibbles based on wrong BASE_SCHNIBBLE_RATE

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Overview

The ConfigStorage could call the configUpdated() function to reconfigure all the important variables of the LandManager. The owner of the ConfigStorage contract initiates this transaction through one of the following functions:

    function manualNotify(uint8 _index, uint8 _length) public onlyOwner {
        for (uint i = _index; i < _index + _length; i++) {
            if (i >= notifiableAddresses.length) break;
            IConfigNotifiable(notifiableAddresses[i]).configUpdated();
        }
    }

    function manualNotifyAddress(address _contract) public onlyOwner {
        IConfigNotifiable(_contract).configUpdated();
    }

Now, let us suppose a user stakes his NFT. After 28 days the ConfigStorage decreases the BASE_SCHNIBBLE_RATE by half through the configUpdated() function. After 30 days, the user farms his staking rewards.

The user would get rewards based on the new halved BASE_SCHNIBBLE_RATE and not the previous rate, thereby losing almost half of his deserving schnibbles.

Proof of Concept

Below is a step-by-step PoC explaining the issue in detail.

Suppose on day 1 a user X stakes one of his Munchable through the stakeMunchable(address landlord, uint256 tokenId, uint256 plotId):

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

    function stakeMunchable(
        address landlord,
        uint256 tokenId,
        uint256 plotId
    ) external override forceFarmPlots(msg.sender) notPaused {
        (address mainAccount, ) = _getMainAccountRequireRegistered(msg.sender);
        if (landlord == mainAccount) revert CantStakeToSelfError();
        if (plotOccupied[landlord][plotId].occupied)
            revert OccupiedPlotError(landlord, plotId);
        if (munchablesStaked[mainAccount].length > 10)
            revert TooManyStakedMunchiesError();
        if (munchNFT.ownerOf(tokenId) != mainAccount)
            revert InvalidOwnerError();

        uint256 totalPlotsAvail = _getNumPlots(landlord);
        if (plotId >= totalPlotsAvail) revert PlotTooHighError();

        if (
            !munchNFT.isApprovedForAll(mainAccount, address(this)) &&
            munchNFT.getApproved(tokenId) != address(this)
        ) revert NotApprovedError();
        munchNFT.transferFrom(mainAccount, address(this), tokenId);

        plotOccupied[landlord][plotId] = Plot({
            occupied: true,
            tokenId: tokenId
        });

        munchablesStaked[mainAccount].push(tokenId);
        munchableOwner[tokenId] = mainAccount;

        toilerState[tokenId] = ToilerState({
            lastToilDate: block.timestamp,
            plotId: plotId,
            landlord: landlord,
            latestTaxRate: plotMetadata[landlord].currentTaxRate,
            dirty: false
        });

        emit FarmPlotTaken(toilerState[tokenId], tokenId);
    }

On day 58, the owner of the ConfigStorage decreases BASE_SCHNIBBLE_RATE by half through:

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

    function configUpdated() external override onlyConfigStorage {
        _reconfigure();
    }

On day 60, X farms his plot:

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

    function farmPlots() external override notPaused {
        _farmPlots(msg.sender);
    }

The farming would use the latest BASE_SCHNIBBLE_RATE:

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

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

Let us do a simple calculation. If the original BASE_SCHNIBBLE_RATE was m and it was not changed anytime, then after 60 days:

schnibblesTotal = 60 * 24 * 3600 * m; (basically, total time (in seconds) * m)

Now, since, the BASE_SCHNIBBLE_RATE is m/2, we have:

schnibblesTotal = (60 * 24 * 3600 * m) / 2;

Actually, the correct reward calculation should be:

schnibblesTotalBeforeChange = 58 * 24 * 3600 * m; (for 58 days the rate was m)
schnibblesTotalAfterChange = (2 * 24 * 3600 * m) / 2; (for 2 days the rate was m/2)
schnibblesTotal = schnibblesTotalBeforeChange + schnibblesTotalAfterChange;

Therefore, user X would face a loss of value in the schnibbles reward.

The Opposite would happen when the rate would change to 2m on day 58: user X would have gained unnecessary schnibbles reward.

The same issue does not seem to lie with the update of REALM_BONUSES and RARITY_BONUSES as they are not directly multiplied by a time difference; they act as a bonus (+ve or -ve) to increase or decrease the total schnibbles calculated.

Severity

Impact: The staker faces a loss of value if the BASE_SCHNIBBLE_RATE is decreased. The protocol faces a loss of value by providing unnecessary value to stakers if the BASE_SCHNIBBLE_RATE is increased. In any case, there is a loss of value. So, the impact seems to be High.

Likelihood: Since, the BASE_SCHNIBBLE_RATE is controlled by the owner of the ConfigStorage, and, it would not often be upgraded (to let the protocol grow smoothly), the likelihood seems Medium.

Therefore, the severity seems to be High.

Tools Used

VS Code Manual review

Recommended Mitigation Steps

The recommended mitigation for this issue is to have an array of all the stakers and iterate on this array to farm for all the owners before updating the BASE_SCHNIBBLE_RATE.

Basically, add an array in the LandManager:

uint256[] stakers;

Update this array whenever a staker stakes Munchables, or, unstakes all of his Munchables.

Now, add an internal function:

function _configBSR() internal {
    for (uint256 i = 0; i < stakers.length; i++) {
        _farmPlots(stakers[i]);
    }
    BASE_SCHNIBBLE_RATE = IConfigStorage(configStorage).getUint(StorageKey.MigrationManager);
}

Call this internal function from the _reconfigure() function:

    function _reconfigure() internal {
        // load config from the config storage contract and configure myself
        lockManager = ILockManager(
            IConfigStorage(configStorage).getAddress(StorageKey.LockManager)
        );
        accountManager = IAccountManager(
            IConfigStorage(configStorage).getAddress(StorageKey.AccountManager)
        );
        munchNFT = IERC721(configStorage.getAddress(StorageKey.MunchNFT));
        nftAttributesManager = INFTAttributesManager(
            IConfigStorage(configStorage).getAddress(
                StorageKey.NFTAttributesManager
            )
        );

        MIN_TAX_RATE = IConfigStorage(configStorage).getUint(
            StorageKey.LockManager
        );
        MAX_TAX_RATE = IConfigStorage(configStorage).getUint(
            StorageKey.AccountManager
        );
        DEFAULT_TAX_RATE = IConfigStorage(configStorage).getUint(
            StorageKey.ClaimManager
        );
-      BASE_SCHNIBBLE_RATE = IConfigStorage(configStorage).getUint(
-          StorageKey.MigrationManager
-      );
        PRICE_PER_PLOT = IConfigStorage(configStorage).getUint(
            StorageKey.NFTOverlord
        );
        REALM_BONUSES = configStorage.getSmallIntArray(StorageKey.RealmBonuses);
        RARITY_BONUSES = configStorage.getSmallUintArray(
            StorageKey.RarityBonuses
        );

+     _configBSR();

        __BaseBlastManagerUpgradeable_reconfigure();
    }

To optimize the loop process, we could restrict the total number of Munchables staked by all users, let's say to 200. This would lead to a maximum of 200 owners (if every user stakes 1 Munchable).

This mitigation optimization would have sustainable gas estimates given that the protocol would be deployed on Layer 2 (Blast, as per the Readme).

Assessed type

Invalid Validation

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c

[Ys-Prakash]

Hi @alex-ppg

Thanks for the quick judging.

I do not comprehend the idea of duplicating this issue with #91. Please allow me to explain below.

91 seems to provide:

1. A vague root cause: The report does not identify that the exact root cause is the update of BASE_SCHNIBBLE_RATE and not other config variables.
2. A vague impact: This is clear by your comment on the report:

The Warden outlines that a configurational update will retroactively apply to all pending rewards in the system. While we can criticize the practice, no actively exploitable vulnerability has been outlined rendering this observation to be a QA-level recommendation.

However, this report clearly identifies:

1. The root cause: The retroactive application of the updated BASE_SCHNIBBLE_RATE to all the pending rewards.
2. The impact along with a step-by-step day-wise PoC to demonstrate the loss of value for the stakers (or the unnecessary gain of value by them).

The discrepancy in reward calculation is the impact. This example illustrates the discrepancy.

Let's say the BASE_SCHNIBBLE_RATE is about to get updated from its initial value to its double (from m to 2m).

Case 1: User X stakes his NFTs on day 1 → X calls farmPlots() to farm on day 28 → BASE_SCHNIBBLE_RATE update happens on day 28 after X farmed → X calls farmPlots() to farm on day 30. X would get rewards:

schnibblesTotalBeforeChange = 28 * 24 * 3600 * m; (for 28 days the rate was m)
schnibblesTotalAfterChange = 2 * 24 * 3600 * 2m ; (for 2 days the rate was 2m)
schnibblesTotal = schnibblesTotalBeforeChange + schnibblesTotalAfterChange; = 2764800 * m;

Assuming m = 1e15 (from https://github.com/code-423n4/2024-07-munchables/blob/main/deployments/utils/config-consts.ts#L353)

schnibblesTotal = 2764800 * 1e15 = 27648e17;

Case 2: User Y stakes his NFT on day 1 → BASE_SCHNIBBLE_RATE update happens on day 28 → Y calls farmPlots() to farm on day 30. Y would get rewards:

schnibblesTotal = 30 * 24 * 3600 * 2m = 5184000 * m;

Assuming m = 1e15

schnibblesTotal = 5184000 * 1e15 = 51840e17;

Y earns 87.5% more rewards than X when both of them staked for 30 days and were subjected to the same BASE_SCHNIBBLE_RATE update. This is the discrepancy.

In the PoC above, I have provided formulas for both cases (correct and wrong calculation) that showcase the discrepancy.

3. This report also states that only the update of BASE_SCHNIBBLE_RATE is a problem; REALM_BONUSES and RARITY_BONUSES do not have the same impact as they are percentages that get applied on the total schnibbles calculated and are not variables that are to be multiplied with a time difference. Mathematically, multiplication with a factor has a huge impact (80 - 100% discrepancy) than calculating percentages on the multiplied total (5-10% discrepancy).

Wardens spend a lot of time generating high-quality impact-driven PoC and reports. It would be unfair to duplicate higher quality reports containing full root cause, maximum achievable impact, and PoC with a vague report. Based on these factors this report should be de-duplicated with #91. This report represents a separate issue.

The sponsor, in #91, acknowledged the fact that the retroactive application of the updated BASE_SCHNIBBLE_RATE is an issue. This confirms the validity of the issue.

Moreover, we had a similar issue from the past Munchables contest where a call to accountManager.forceHarvest() was missing which led to a discrepancy in rewards:

https://github.com/code-423n4/2024-05-munchables-findings/issues/20

This is a very relevant statement from that issue's report:

Player 0 harvests rewards before duration update, while Player 1 does not. This scenario leads to a discrepancy in rewards.

That report proposed the same issue: An update of a daily reward rate and its retroactive application to the pending rewards. In this case, the daily reward is BASE_SCHNIBBLE_RATE. In that case, it was dailySchnibbles.

I believe #108 also states this issue to some extent. However, it does not show the discrepancy through formulas/calculations and has unclear mitigation.

The stakers face loss of rewards or gain of unnecessary rewards (as per the update of BASE_SCHNIBBLE_RATE). This makes the impact High. BASE_SCHNIBBLE_RATE update frequency would not be too much or too low but would be medium. So, the likelihood is Medium. This makes the severity High (severity analysis is also mentioned in the report).

Note

---

Using the notPaused modifier would not solve the problem. Basically, the solution is to force all the renters to farm their plots before BASE_SCHNIBBLE_RATE gets updated, and, there is no mechanism for it. Pausing the farm functionality would aggravate the issue as innocent users, who would want to farm before the update, would not be able to do so.

About mitigation

---

The mitigation in the report fixes the issue completely but might seem infeasible due to the high gas consumption. Alternate mitigation also exists. However, it would not completely solve the problem but would alleviate it. The alternate is as follows:

Add another arm in the struct:

    struct ToilerState {
        uint256 lastToilDate;
        uint256 plotId;
        address landlord;
        uint256 latestTaxRate;
        bool dirty;
+       uint256 BSR;
    }

Now, whenever a user farms plots, use their toiler's BSR instead of the global BSR:

            schnibblesTotal =
                (timestamp - _toiler.lastToilDate) *
-               BASE_SCHNIBBLE_RATE;
+               _toiler.BSR;               

After calculation, update the toiler's BSR in the _farmPlots(address _sender) function itself:

            toilerState[tokenId].BSR = BASE_SCHNIBBLE_RATE;

This mitigation is inspired by how the landlord tax system is maintained. A user maintains the tax rate that was agreed upon initially. The same thing could be done with BSR.

If this is applied in the case of X and Y above, the discrepancy would come down to 6.67% in the other direction (that is, X would earn more than Y). However, the mitigation in the bug report brings down the discrepancy to 0%.

It would be helpful if you could de-duplicate this issue from #91 and mark this report as a separate issue. I would be happy to provide more valuable information.

Thank you

[alex-ppg]

Hey @Ys-Prakash, thank you for your PJQA contribution. Your submission argues that an increase would be positive for stakers while a decrease would be negative. This is not indicative of a vulnerability and is instead a protocol preference. We can criticize the practice, but I do not envision any actual vulnerability or exploit arising from it. It is impossible to speculate on when rates will increase, so the retroactive application will affect a small subset of users as most would claim rewards as soon as profitable to capitalize on the active price of the reward token.

[Ys-Prakash]

Hi @alex-ppg

Thanks for the response.

I apologise for commenting after PJQA. However, I believe that the sponsor should share their views to make things more concrete and there are a few important points to make note of.

You said that:

Your submission argues that an increase would be positive for stakers while a decrease would be negative. This is not indicative of a vulnerability and is instead a protocol preference.

You judged an almost similar issue in the previous Munchables contest:

https://github.com/code-423n4/2024-05-munchables-findings/issues/20

It had the same impact: Discrepancy in reward calculation. The logic of the issue being a protocol preference could also have been applied in this judged issue. But it was not as the discrepancy was high (the sponsor decided to fix it). So, taking this as a reference, discrepancy in reward calculation is indeed a vulnerability, especially when the discrepancy is as huge as ~80%. The logic of the issue being a protocol preference could also have been applied in

You also said that:

It is impossible to speculate on when rates will increase

It is definitely difficult to speculate (due to lack of documentation and comments in the contest repo) but one can think of it as a reward rate: Reward rate increases to increase traction for the protocol, and it goes down when the protocol would want to stabilize the number of players. One more way to think about this: The game protocol is a harvesting related protocol. So, the devs could want different harvesting rates, that is BSR, for different seasons, that is, time of year. The sponsor could shed light on this part to make this more clear.

You said that:

so the retroactive application will affect a small subset of users as most would claim rewards as soon as profitable to capitalize on the active price of the reward token

This assumes that the active price of the reward token might not go up in the future. However, if it does so, particularly in a bull run, then the stakers would want to have as many rewards as possible, forcing them to wait during the increment update of BSR and collect the wrongly increased harvested rewards. This would hurt those users who would collect rewards before the BSR update (that is, collected using the correct calculation). So, the logic that you state could be applied both ways.

The provided mitigation in the report requires gas but only the time when the BSR gets updated, or, any config related change happens. Also, this gas would be paid by the admin (not the users) that too not frequently. The user experience is not hurt.

It would also be great if the sponsor could also share their views on whether they want this significant discrepancy vulnerability in their protocol. This would make things very clear.

Thanks

[Ys-Prakash]

Just to add to the previous comment, the logic of the issue being a protocol preference could be applied in the previous judged issue as a user would be getting wrongly calculated more rewards but at a cost of locking for more time.

However, the sponsor made it clear and fixed it as the discrepancy was high.

[alex-ppg]

Hey @Ys-Prakash, thank you for your follow-up feedback. The submission cited throughout your responses is dissimilar to this one as this submission revolves around an administrative / protocol level update in comparison to a user-initiated update that can be maliciously exploited.

I understand the concerns raised, however, the update cannot be initiated by the user and is initiated by the project itself. As such, it cannot constitute any grade beyond QA.