howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L264 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L285 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L335-L347 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L293 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L345-L379

Vulnerability details

LockManager.lockOnBehalf is called when the user has MunchableNFT which should be migrated.

The LockManager._lock function called by the lockOnBehalf handles the actual locking process. This function does several important steps:

It verifies that the account is registered and that there is sufficient allowance for the tokens to be locked.
It calculates the total amount to lock, including any remainder from previous locks.
It calculates the number of NFTs the user will receive based on the locked amount, only if msg.sender != MigrationManager
It transfers the tokens to the contract.
It updates the locked amount and the remainder (here is the issue, when called from the MigrationManager)
It sets the unlock time based on the lock duration.

The remainder is an important part of this process. It represents any leftover amount that wasn't enough to mint an additional NFT. Normally, this remainder is added to the next lock amount, allowing users to efficiently utilize all their tokens over time.

However, when _lock is called from MigrationManager, it improperly resets the remainder to 0 (step 5). This happens because the function doesn't differentiate whether it's being called from the migration process or a standard lock request. As a result, users end up receiving fewer MunchableNFTs than they should because the small leftover amounts are discarded instead of being carried over to the next lock.

Impact

The user's funds won't be used efficiently, resulting in fewer received MunchableNFTs. Also, the user won't be able to unlock these unused funds and relock them to be used. This happens because the MigrationManager resets the unlock time, forcing the user to wait an extra lockDuration.

Proof of Concept

When MigrationManager.migrateAllNFTs or MigrationManager.migratePurchasedNFTs is called

it calls _migrateNFTs, which in turn calls LockManager.lockOnBehalf

function _migrateNFTs(
    address _user,
    address _tokenLocked,
    uint256[] memory tokenIds
) internal {
    // ... 

    uint256 quantity = (totalLockAmount * discountFactor) / 10e12;
    if (_tokenLocked == address(0)) {
        _lockManager.lockOnBehalf{value: quantity}(
            _tokenLocked,
            quantity,
            _user
        );
    } else if (_tokenLocked == address(WETH)) {
        WETH.approve(address(_lockManager), quantity);
        _lockManager.lockOnBehalf(_tokenLocked, quantity, _user);
    } else if (_tokenLocked == address(USDB)) {
        USDB.approve(address(_lockManager), quantity);
        _lockManager.lockOnBehalf(_tokenLocked, quantity, _user);
    }
    emit MigrationSucceeded(_user, migratedTokenIds, newTokenIds);
}

lockOnBehalf calls the _lock function
when _lock is called from MigrationManager, the remainder calculation is skipped and is effectively reset to 0, causing users to lose potential MunchableNFTs from their leftover token amounts

function _lock(
    address _tokenContract,
    uint256 _quantity,
    address _tokenOwner,
    address _lockRecipient
) private {
    // ...

    // add remainder from any previous lock
    uint256 quantity = _quantity + lockedToken.remainder;
@>  uint256 remainder;
    uint256 numberNFTs;
    uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    if (_lockDuration == 0) {
        _lockDuration = lockdrop.minLockDuration;
    }
    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)) {
            // 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));
        }
    }

    // ...

@>  lockedToken.remainder = remainder;
    lockedToken.quantity += _quantity;
    lockedToken.lastLockTime = uint32(block.timestamp);
    lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration);

    // ...
}

Coded POC

*Place the following test case in [src/test/SpeedRun.t.sol](https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/test/SpeedRun.t.sol) and run it with the command `forge test --mt test_reminderWillBeResetWhenLockMigrateNFTMigrationManager -vvv`.* ```solidity address alice = makeAddr("alice"); function test_reminderWillBeResetWhenLockMigrateNFTMigrationManager() public { deployContracts(); vm.prank(alice); amp.register(MunchablesCommonLib.Realm(3), address(0)); uint256 lockAmount = 1.9e18; deal(alice, 2e18); deal(address(migm), 2e18); uint256 totalLocked; vm.prank(alice); lm.lock{value: lockAmount}(address(0), lockAmount); totalLocked += lockAmount; // skip lockDuration skip(lm.getPlayerSettings(alice).lockDuration); // Alice can unlock here vm.prank(alice); lm.unlock(address(0), 0.1e18); totalLocked -= 0.1e18; vm.prank(address(migm)); lm.lockOnBehalf{value: 1e18}(address(0), 1e18, alice); // Alice cannot unlock here, because MigrationManager has reset the unlockTime vm.expectRevert(); vm.prank(alice); lm.unlock(address(0), 0.1e18); // But also, if lock the remaining amount to fill the remainder, won't receive an NFT. vm.prank(alice); lm.lock{value: 0.2e18}(address(0), 0.2e18); totalLocked += 0.2e18; console.log("totalLocked: ", totalLocked); console.log("unrevealedNFTs: ", nfto.getUnrevealedNFTs(alice)); } ``` Logs: ```solidity totalLocked: 2000000000000000000 unrevealedNFTs: 1 ```

Tools Used

Manual Review

Recommended Mitigation Steps

Do not reset the remainder storage variable when the _lock function is called from the MigrationManager. Instead, ensure the remainder is correctly accumulated and carried over, allowing users to receive the correct amount of MunchableNFTs based on their total locked amount. This can be done by adding a condition to check the caller and handle the remainder accordingly.

Assessed type

Context

c4-judge commented 1 month ago

alex-ppg marked the issue as not a duplicate

c4-judge commented 1 month ago

alex-ppg marked the issue as primary issue

alex-ppg commented 1 month ago

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.

c4-judge commented 1 month ago

alex-ppg marked the issue as selected for report

c4-judge commented 1 month ago

alex-ppg marked the issue as satisfactory

0xinsanity commented 1 month ago

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.

Tomiwasa0 commented 1 month ago

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

PetarTolev commented 1 month ago

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.

Tomiwasa0 commented 1 month ago

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

PetarTolev commented 1 month ago

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.

niser93 commented 1 month ago

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.

0jovi0 commented 1 month ago

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.

Ys-Prakash commented 1 month ago

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

alex-ppg commented 1 month ago

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

DecentralDisco commented 2 weeks ago

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

code-423n4 / 2024-05-munchables-findings

When `LockManager.lockOnBehalf` is called from `MigrationManager`, the user's `reminder` will be set to 0, resulting in fewer received `MunchableNFTs` #455

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

Scope Validity

Migrator Action Time

Distinction of Finding from Non-Lockdrop Remainder Erasures

Verdict