howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L361-L370

Vulnerability details

Impact

Users will lose locked funds.

Proof of Concept

If migrationManager calls the lock function, the logic in the if will not be executed. Therefore, the remainder amount will still be equal to 0, and subsequent updates to the remainder amount and quantity will cause the user to lose the previous amount of remainder. Indicates that the remainder deposited by the user cannot be divisible, and also belongs to the user's assets.

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

Tools Used

manual

Recommended Mitigation Steps

It is recommended that the remainder variable should be calculated correctly regardless of whether the addReveal function is executed or not.

Assessed type

Other

c4-judge commented 1 month ago

alex-ppg marked the issue as partial-50