Munchables - PetarTolev's results

alex-ppg marked the issue as not a duplicate

alex-ppg marked the issue as primary issue

The Warden and its duplicates outline a behavior whereby the migration manager will overwrite the remainder of a lock entry even if it is within the lock drop period. This is invalid behavior and will result in unaccounted remainders that will not carry over to the next lock operation properly.

I believe a medium-severity rating is acceptable given that it affects how NFTs are awarded in the lock drop system and I have selected this submission as the best due to describing the vulnerability in great and accurate detail.

alex-ppg marked the issue as selected for report

alex-ppg marked the issue as satisfactory

If its coming from the migration manager, we don't want to count that quantity towards the remainder calculations since we are choosing to migrate over old NFTs, not mint new ones.

Thanks for judging @alex-ppg , This is a great finding but sadly, it is out of scope https://code4rena.com/audits/2024-05-munchables#top:~:text=/src/managers/LockManager.sol. Lockmanager is the only contract in scope and this bug is only possible because Migration manager fails to implement the remainder, which is out of scope. Lock manager works as it should https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L344 and no remainder is lost

Hello @alex-ppg,

I disagree with the comments above.

The issue mentioned in the submission is about a reset reminder, not about the quantity.

The audit scope is limited to LockManager, and the root cause of this submission lies there. The _lock function, when called from MigrationManager, is intended to migrate the V1 NFTs to V2. However, resetting the reminder is an invalid behavior that needs to be fixed. The root cause and impact of the issue lie within the LockManager. The mitigation also covers only LockManager, so I believe the submission is in scope.

To cap it here @alex-ppg , in Migration manager QUANTITY is reduced to an acceptable level before calling locked manager https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L334

Therefore the Protocol added this to Lock Manager because quantity has been checked to align with the nft cost and there is no need to recheck because there will be no remainder.

if (msg.sender != address(migrationManager)) { // calculate number of nfts 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));
        }

Hi @Tomiwasa0,

The issue doesn't lie with the quantity sent from the migrationManager to the lockManager, but with the user's remaining balance (reminder). If the user sends slightly more than the nftPrice, and the remainder isn't used to mint an NFT, this remainder should be used in the next lock. The problem arises when there is such a remainder (e.g., 0.5 ETH if the nftPrice is 1 ETH and the user is locking 10.5 ETH) and the migrationManager calls _lock, the remaining balance will always be set to 0.

According to what was reported in #96, the fact the remainder is not held over different lockdrop periods is an acceptable behavior. So, if we assume the migrationManager will be called outside the lockdrop timeframes (that is an obvious assertion), this issue becomes a low issue at best. The issue described here is valid and has a huge impact just on users who have the remainder>0 and if migrationManager calls _lock in the same lockdrop timeframe.

Please @alex-ppg consider this: if the issue above is valid, so #12, #96 and my issue #452 will be valid. The only case this is not true is if you consider the possibility that migrationManager would migrate inside the lockdrop timeframe. But why should developers do that? We could suggest inserting a check inside the migrationManager to allow it to call the _lock method only outside the lockdrop timeframe. However, it will be out of scope.

If its coming from the migration manager, we don't want to count that quantity towards the remainder calculations since we are choosing to migrate over old NFTs, not mint new ones.

Even so, the issue leads to a concerning side-effect as it takes a previous lock remainder that may be non-zero and then sets it to zero. In the worst-case scenario, a user would have a legitimate lock of configuredToken.nftCost - 1 token units become worthless after the migration manager does a lock on its behalf.

I will provide an example scenario: Alice doesn't own enough tokens to addReveal 1 NFT. She owns 50 tokens while the price is 100 units. Alice then opts to lock 50 tokens, have its remainder stored, await the lock period to be done, unlock the tokens then lock once again to addReveal 1 NFT. Between both lock calls, the migrationManager does a lock on Alice's behalf. This means Alice would do the second lock without receiving the NFT.

As stated in issue #463 , the core impact is at the lock's remainder, not necessarily in NFT minting.

Hi @alex-ppg

Thanks a lot for taking the time to judge this contest.

I would like to add more context to the issue of remainder reset. I hope this context helps all the stakeholders here.

From what this issue and others (for eg, #457, #96, #12, #69, #454, and others) have presented, we have the following 2 cases:

Case 1- Remainder reset within a single LockDrop period:

This is the core impact of the current issue. The remainder is reset when MigrationManager locks on behalf of the locker through the lockOnBehalf() function.

The sponsor has disputed this case above:

If its coming from the migration manager, we don't want to count that quantity towards the remainder calculations since we are choosing to migrate over old NFTs, not mint new ones.

Case 2- Remainder is reset outside of LockDrop periods, particularly between two LockDrop periods:

This is the core impact proposed by issues #457, #96, #12, #69, #454, and others. However, the sponsors have contradictory responses on different reports covering this case:- they accept #12 and reject #69. This has been observed by you as well:- from your comment on #96:

The sponsor appears to be somewhat contradictory in their responses to https://github.com/code-423n4/2024-05-munchables-findings/issues/69 and https://github.com/code-423n4/2024-05-munchables-findings/issues/12, however, in https://github.com/code-423n4/2024-05-munchables-findings/issues/12 they simply state that they discovered an issue with unlocking that was not identified by any other warden as well as an issue with locking on behalf of another user.

If I understand correctly, you have deemed case 2 as an invalid case. I make this assumption as per your comment on #96:

The remainder amount is solely applicable to lock drop period calculations and thus bears no effect outside of a lock drop's validity.

To add more context, I would like to quote another half of your comment on #96:

An issue I do envision, however, is the fact that a user who has a remainder in lock drop period A can carry it over in lock drop period B if they perform no interaction with the LockManager which might be undesirable, and I invite the Sponsor to evaluate this behavior.

To this, the sponsor replied:

That's fine. That is acceptable behavior.

Therefore, it seems to mean that the protocol has an intended behavior to carry over the remainder between subsequent LockDrop periods. However, this is the exact intended behavior that is broken by the issue in case 2. Allow me to illustrate this with the following example.

Let us assume a user has locked 1.9 eth tokens (nft cost being 1 eth, so the user gets 1 nft and 0.9 eth as the remainder) during LockDrop A. Now, after LockDrop A ends, the user decides to boost their Schnibbles harvest rate and locks 0.001 eth (this is obviously done in the non-LockDrop period). In the subsequent LockDrop period B, the user would not have their remainder of 0.9 eth just because of a minute investment of 0.001 eth in the previous non-LockDrop period.

Therefore, the intended behavior (remainder to be carried over across subsequent LockDrop periods) remains broken.

It seems plausible that remainder has no use when no nft is minted, that is, during the non-LockDrop period, thereby, making it unnecessary to account for the remainder when users lock their tokens during the non-LockDrop period. However, the remainder (that was calculated during the previous LockDrop period) would become important in the subsequent LockDrop period when the nft minting starts again.

To make things clear, it would be very helpful if the sponsors state the intended behavior for the 2 cases mentioned above. Specifically, the sponsors could answer whether:

• Case 1 is an intended behavior or an issue.
• Case 2 is an intended behavior or an issue.

I would be more than happy to provide more helpful information. I await the final decision that Alex would make.

Thanks

Thanks to everyone who contributed to this submission's discussion including the Sponsor @0xinsanity, and the Wardens @Tomiwasa0, @PetarTolev, @niser93, @0jovi0, and @Ys-Prakash!

I will address multiple concerns raised in as much of a concise manner as possible.

Addressing the Sponsor @0xinsanity and discussing from a security auditor perspective, I would like to note that the vulnerability does not relate to the remainder that should have been assigned for a migrated NFT, but rather the remainder a user has accumulated with previous locks. If they have accumulated a non-zero remainder with previous locks and migrated afterward, they would lose that remainder. As such, I implore you to revisit this submission.

Addressing the Wardens' concerns and discussing from a C4 perspective:

Scope Validity

The migrator-handling code is explicitly in scope as it is located in the LockManager. The LockManager cannot make any assumptions as to how the migrator invokes the contract given its out-of-scope nature, so a solution should focus on the LockManager contract itself directly. We cannot make an assumption that the migrator is owned by Munchables and that its code behaves in any way as it is out-of-scope.

Migrator Action Time

Similarly to the above, the migrator is out-of-scope. When evaluating this submission, we are meant to follow a zero-knowledge approach, and thus making an assumption that the LockManager will invoke the contract at a particular timestamp is invalid.

Distinction of Finding from Non-Lockdrop Remainder Erasures

Combining the above, we can assume that the migrator does not abide by any specific restraints and thus can invoke the LockManager migration when inside a lockdrop period making this a valid vulnerability that is distinct from the remainder resets outside of a lockdrop period as described in #96 and its duplicates.

Verdict

My original ruling for this submission will remain, as it demonstrates a scenario that is not described by many wardens and is the following:

• A remainder during a lockdrop for which it is used can be erased when the migrator interacts with the contract

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