Users with expired locks who haven't withdrawn their tokens will receive the same benefits as if their tokens are locked

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L461-L487 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L349-L371 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L410-L411

Vulnerability details

Impact

Users whose lock has expired but they're still holding/keeping the tokens in the LockManager will receive the same benefits as if their tokens are locked.

This opens up the possibility for malicious users to take advantage of this mechanics and game the system by locking their tokens for the minimum amount of time, while collecting all of the rewards and being able to unlock/withdraw their tokens at any time.

Proof of Concept

There are two ways in which users are rewarded for locking their tokens inside the LockManager:

• The first one is whenever there is a lockdrop event, the amount of tokens which is being locked will equal a certain amount of NFTs:

 if (lockdrop.start <= uint32(block.timestamp) && lockdrop.end >= uint32(block.timestamp)) {
            if (
                _lockDuration < lockdrop.minLockDuration
                    || _lockDuration > uint32(configStorage.getUint(StorageKey.MaxLockDuration))
            ) revert InvalidLockDurationError();
            if (msg.sender != address(migrationManager)) {

                remainder = quantity % configuredToken.nftCost;
                numberNFTs = (quantity - remainder) / configuredToken.nftCost;

                if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

                // Tell nftOverlord that the player has new unopened Munchables
                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));

• The second one is related to collecting Schnibbles. The calculation on how many Schnibbles a user has collected is based on two things, the getLockedWeightedValue() function, and the the time between block.timestamp and the last harvest of said Schnibbles (either the user manually harvesting it by calling the function harvest() on the accounting manager, or automatically doing so when locking/unlocking tokens by forceHarvest() being called).

If there is a lockdrop event, the amount of NFTs that a user will receive is calculated immediately when the lock is made and addReveal() on the NFT Overlord is called.

During or outside of lock events, users can stake so that they accrue daily Schnibbles, which is another form of reward. As explained above this reward is dependent on the getLockedWeightedValue() and the time from the last harvest of said Schnibbles.

If we take a look at the getLockedWeightedValue():

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

If the token in-question is active, the weighted value is based solely on the quantity of tokens that the user has in the Lock Manager multiplied by the price of that token.

Let's examine the following scenario:

• A user has set their lockDuration as the minimum one (for example 10 hours), and if there's no lockdrop event currently this can be set to as little as 1 second, as there are no minimum duration checks outside of the lockdrop event block in _lock().

• Once _lock() is called, it continues to call forceHarvest() which will harvest any accumulated Schnibbles on behalf of the user and update the lastHarvestDate on the Accounting Manager, upon which the harvesting of Schnibbles is also based.

Since none of the two mechanisms is based upon the actual lock period, this makes it obsolete. The only thing which can be prevented by the lastHarvestDate is depositing and withdrawing in the same block, as forceHarvest() will be called on both lock and unlock, making this 0, subsequently the schnibbles to claim as well.

Taking this into consideration, there's no mechanism preventing users, to lock their tokens for 1 second (if no lockdrop period is active), and keeping them in the contract to accrue rewards, allowing them to withdraw/unlock them at any time they want and re-lock them again whenever they want for 1 second, continuing to accrue more Schnibbles.

If unlockTime passes the current block.timestamp and tokens can be unlocked, there's no mechanism which makes the rewards to stop accruing or forces the user to unlock them.

A coded PoC which can be used in the already existing SpeedRun.t.sol test suite:

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

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

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

        //set lock duration as 25 hours (this is due to the current minimum stake being set as 1 day due to a lockdrop event, but can be as little as 1 second outside of those events)
        lm.setLockDuration(25 hours);
        //lock 100 ETH for 11 hours.
        lm.lock{value: lockAmount}(address(0), lockAmount);

        //fast forward 26 hours
        vm.warp(26 hours);

        uint256 harvestedSchnibbles = amp.harvest();
        console.log("Harvested Schnibbles:", harvestedSchnibbles);

        //Unlock tokens
        lm.unlock(address(0), lockAmount);

        //lock 100 ETH again for 26 hours.
        lm.lock{value: lockAmount}(address(0), lockAmount);

        //Even though the tokens will be available to be unlocked after only 26 hours, they are kept for 7 days (and rewards are still accrued after the 26 hours):
        vm.warp(7 days);

        harvestedSchnibbles = amp.harvest();
        console.log("Harvested Schnibbles after 7 days:", harvestedSchnibbles);
    }

Results:

  Harvested Schnibbles:               379162615740740740740740740
  Harvested Schnibbles after 7 days: 2070833333333333333333333333

Tools Used

Manual Review

Recommended Mitigation Steps

Re-factor the getLockedWeightedValue to only accrue value if the unlock time of the user-in-question is less than the block.timestamp (i.e. meaning that the stake is still active).

Assessed type

Invalid Validation

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The submission specifies that locks do not expire beyond their lock time and that they are still eligible for daily rewards.

Based on my evaluation of the codebase, this appears to be desirable behavior as the daily rewards are immediately releasable (i.e. by simply relocking) and the actual time you acquire rewards for equates to the time you had them locked, regardless of whether they could be released or not. Longer lock times affect the "lockdrop" which is a separate rewarding mechanism that should not affect the "base" incentives of acquiring Schnibbles.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[iamandreiski]

Hi @alex-ppg thanks a lot for judging.

Expanding on your comment above there are a few things that should be taken into consideration as well:

• During lockdrop events the only upside is that the staker receives the NFTs, but the NFTs are revealed (count increased) immediately upon staking based on the amount you're staking, not the lock time (if the price is 1 ETH per NFT, you stake 5 ETH, you'll receive 5 NFTs no matter if you stake for 1 day or 90 days).
• Locking/staking is encouraged outside of lockdrop events as well, as per both documentation and the sponsor.
• For the locking/staking to be economically viable for the users, they'll need to get awards proportionate to their lock time (and/or only during their lock time).
• Since the Schnibbles are available to anyone who has funds in the system, the whole purpose of the lock mechanism is then defeated, the lock contract can basically be used as a "wallet", where users lock their tokens for the minimum time (if outside of lockdrop events, can be as little as 1 hour) and the funds are available to be removed any time and re-locked again, which again defeats the whole purpose of having a lock mechanism.
• With the above in context, this isn't just economically viable for the users to have staking/locking rewards while their funds are not locked but are readily available to be withdrawn at any time, this is also economically devastating for protocol mechanics as well, they'll have absolutely no control/predictability over how much funds can be removed in a given time.

I believe this should be valid, as without having awards based on only lock time, this defeats the whole purpose of having a locking manager as none of the incentives are based on lock time.

[alex-ppg]

Hey @iamandreiski, thanks for your feedback. You state that it is not economically viable to distribute rewards to users who can immediately withdraw their rewards, however, this is precisely the model of Synthetix, Sushiswap, and other well-known protocols. Effectively, the LockManager acts as a rudimentary staking system when no lockdrop is active and as a locking system whenever a lockdrop is active. Being able to configure the time you wish to lock your tokens for is imperative in case supplemental infrastructure is desired by the Munchables team (i.e. a contract that locks your token for 1 year and provides a boost on your Schnibble rewards).

I do not see any security vulnerability arising from the present behavior, and while we can criticize its structure it does not mean that such criticism would be a high risk or medium risk flaw.