Previous user rewards can be affected by the various changes in the token configs

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/AccountManager.sol#L372-L373 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/BonusManager.sol#L128 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L461-L487 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L115-L127 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L506-L525

Vulnerability details

Impact

Changes in configuredTokens parameters (usdPrice, active) take immediate effect on accumulated user rewards.

Proof of Concept

The protocol includes a reward program where users can harvest schnibbles based on the locked value and duration.

    function _harvest(address _caller) private returns (uint256 _harvested) {
        (uint256 dailySchnibbles, uint256 bonus) = getDailySchnibbles(_caller);
        dailySchnibbles += ((dailySchnibbles * bonus) / 1e18);

        uint256 secondsToClaim = block.timestamp -
            players[_caller].lastHarvestDate;
        uint256 harvestedSchnibbles = (dailySchnibbles * secondsToClaim) /
            1 days;

        players[_caller].unfedSchnibbles += harvestedSchnibbles;
        players[_caller].lastHarvestDate = uint32(block.timestamp);

>>      _harvested = harvestedSchnibbles;

        emit Harvested(_caller, harvestedSchnibbles);
    }

    function getDailySchnibbles(
        address _caller
    ) public view returns (uint256 _dailySchnibbles, uint256 _bonus) {
>>      uint256 weightedValue = lockManager.getLockedWeightedValue(_caller);
        // Arbitrary division here... If we remove it, we just need to make sure we modify level thresholds, & social/pet bonuses
        _dailySchnibbles = (weightedValue / 10);
        _bonus = bonusManager.getHarvestBonus(_caller);
    }

The lockManager.getLockedWeightedValue function is used to calculate the weighted value of the locked tokens. It iterates through the configured tokens and considers only the active tokens in the calculation.

    function getLockedWeightedValue(
        address _player
    ) external view returns (uint256 _lockedWeightedValue) {
        uint256 lockedWeighted = 0;
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            if (
                lockedTokens[_player][configuredTokenContracts[i]].quantity >
                0 &&
>>              configuredTokens[configuredTokenContracts[i]].active
            ) {
                // We are assuming all tokens have a maximum of 18 decimals and that USD Price is denoted in 1e18
                uint256 deltaDecimal = 10 **
                    (18 -
                        configuredTokens[configuredTokenContracts[i]].decimals);
                lockedWeighted +=
                    (deltaDecimal *
                        lockedTokens[_player][configuredTokenContracts[i]]
                            .quantity *
>>                      configuredTokens[configuredTokenContracts[i]]
                            .usdPrice) /
                    1e18;
            }
        }

        _lockedWeightedValue = lockedWeighted;
    }

Two things to notice here:

1. Only active tokens are used in calculations
2. Current usdPrice is used

Rewards are collected every time user locks or unlocks tokens. However, the issue arises when admin or price feeds change the parameters in the configuredTokens. Since the protocol uses the current parameters to calculate the Total Value Locked (TVL), it does not store the TVL calculated prior to the parameter change. This can lead to inconsistencies in the rewards received by users.

Two scenarios highlight the impact:

1. Price Parameter Change: If the usdPrice for a token changes while the tokens are locked, users may receive rewards based on the updated TVL, like this price existed through the whole lock time.
2. Deactivated Token: If a token that a user has locked becomes inactive before they unlock their tokens, they may receive zero rewards since their tokens are no longer considered in the TVL calculation.

Coded POC for SpeedRun.t.sol

    function test_NoReward() public {
        uint256 lockAmount = 100e18;

        console.log("Beginning run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);
        vm.warp(86401);

        ILockManager.ConfiguredToken memory ethToken;
        ethToken.active = false;
        ethToken.nftCost = 1e18;
        ethToken.usdPrice = 35000000;
        lm.configureToken(address(0), ethToken);

        lm.unlock(address(0), lockAmount);
        (, MunchablesCommonLib.Player memory player) = amp.getPlayer(address(this));
        console.log("SHCHNIBBLES: ", player.unfedSchnibbles);
    }

Tools Used

Foundry

Recommended Mitigation Steps

To address this issue, changes should be made to the protocol's architecture to accumulate user rewards before any token parameter changes are made. Storing and updating reward rate separately from user's locked amounts would allow for more flexibility and accuracy during token price and status updates.

Assessed type

Other

[CloudEllie]

Reopening for sponsor review

[0xinsanity]

Because of the long cadence by which the price will update, we are not concerned about variations in schnibble accrual based on price adjustments.

[alex-ppg]

The submission details that immediate changes to a token's configuration, such as the USD price reported, should not be consumed immediately which contradicts the intended purpose of the data point. I consider the submission invalid, as all changes should be immediately reflected as the Sponsor confirms.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[k4zanmalay]

I noticed a comment from a sponsor in https://github.com/code-423n4/2024-05-munchables-findings/issues/100 that losing staking rewards for deactivated token is the intended behavior. I believe this is unfair to the user since schnibbles rewards are accumulated over time, the staker should be able to claim rewards that were accumulated before the token was deactivated.

Let's say Alice locked tokens for 10 days, every day she receives 100 schnibbles, on the 5-th day the token she locked was deactivated. On the 10-th day when she unlocks she should be able to claim 5X100 = 500 schnibbles instead of 0.

[alex-ppg]

Hey @k4zanmalay, thanks for your feedback. As noted in my reply within #100 here, we can observe that a token being disabled will result in price measurements no longer reported for it and thus a security vulnerability arising from permitting claims while the token is deactivated.

Responsible deactivation procedures by the Munchables team will prevent the scenario you outlined (i.e. we plan to remove XYZ token from being supported in 3 days, permitting adequate time for users to claim rewards), and a token's de-activation should be immediate in case f.e. of a market crash of the token providing unreliable USD measurements for the oracles to reflect.

I appreciate the due diligence and will retain my original ruling on this submission.