User's schnibbles rewards are not harvested in the `setLockDuration` function

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/BonusManager.sol#L136 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/AccountManager.sol#L363-L387 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L249

Vulnerability details

Impact

User rewards are not harvested during lock duration updates, leading to inconsistencies in accrued rewards between users.

Proof of Concept

In the current LockManager implementation, schnibbles rewards are harvested during lock and unlock calls.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        ---SNIP---
        // they will receive schnibbles at the new rate since last harvest if not for force harvest
>>      accountManager.forceHarvest(_lockRecipient);

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        ---SNIP---
        // force harvest to make sure that they get the schnibbles that they are entitled to
>>      accountManager.forceHarvest(msg.sender);

Schnibbles reward rate depends on amount locked and the passed time.

    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);
    }

    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);
    }

There is also a bonus that depends on the user's lock duration setting.

    function getHarvestBonus(
        address _caller
    ) external view override returns (uint256) {
        uint256 weightedValue = _lockManager.getLockedWeightedValue(_caller);
        ILockManager.PlayerSettings memory _settings = _lockManager
            .getPlayerSettings(_caller);
        uint256 _migrationBonus;
        if (block.timestamp < migrationBonusEndTime) {
            _migrationBonus = _calculateMigrationBonus(_caller, weightedValue);
        }
        return
>>          _calculateLockBonus(_settings.lockDuration) +
            _migrationBonus +
            _calculateLevelBonus(_caller) +
            _calculateMunchadexBonus(_caller);
    }

Users have an option to change their lock duration during ongoing locks.

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

>>      playerSettings[msg.sender].lockDuration = uint32(_duration);

The issue arises when user rewards are not harvested before the update. This leads the protocol to assume that the user had the updated duration from the beginning of the lock, resulting in incorrect reward calculations.

Coded POC for SpeedRun.t.sol:

    function test_Reward() public {
        address alice = address(0xa11ce);
        vm.deal(alice, 100e18);

        uint256 lockAmount = 100e18;

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

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

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

        vm.warp(30 days);
        // Player 0 harvests while Player 1 does nothing
        amp.harvest();

        lm.setLockDuration(60 days);
        vm.prank(alice);
        lm.setLockDuration(60 days);
        vm.warp(60 days + 1);

        lm.unlock(address(0), lockAmount);
        vm.prank(alice);
        lm.unlock(address(0), lockAmount);
        (, MunchablesCommonLib.Player memory player0) = amp.getPlayer(address(this));
        (, MunchablesCommonLib.Player memory player1) = amp.getPlayer(alice);
        console.log("SHCHNIBBLES PLAYER0: ", player0.unfedSchnibbles);
        console.log("SHCHNIBBLES PLAYER1: ", player1.unfedSchnibbles);
    }

In the provided test case, two players lock a similar amount of tokens for the same duration and then update it. Player 0 harvests rewards before duration update, while Player 1 does not. This scenario leads to a discrepancy in rewards.

  SHCHNIBBLES PLAYER0:  22575000607638888888888888888
  SHCHNIBBLES PLAYER1:  24150000000000000000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Harvest user rewards before updating lock duration:

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();
+       accountManager.forceHarvest(msg.sender);
        playerSettings[msg.sender].lockDuration = uint32(_duration);

Assessed type

Other

[CloudEllie]

Reopening for sponsor review

[0xinsanity]

This has been fixed at: https://github.com/munchablesorg/munchables-common-core-latest/pull/14/files

[alex-ppg]

The submission outlines a discrepancy in the rewards a user will acquire due to the update of an account's lock duration, and thereby the update of all their existing locks, not claiming Schnibble rewards and thus not updating the relevant entries in the AccountManager.

The vulnerability effectively permits users to change their lock duration and acquire more rewards than they would normally receive as the update would retroactively apply to existingly-vested rewards. I believe a medium risk rating is fair as rewards would be distributed at a higher rate than expected and the problem can be capitalized maliciously.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[DecentralDisco]

The Munchables sponsor (0xinsanity) confirmed this issue outside of github. Label has been added by C4 staff for transparency.