Munchables - bearonbike's results

Lines of code

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

Vulnerability details

Impact

When user lock their funds, funds will be caculated to add unopened Munchables, in special reminder will be used in the next lock, along with next lock's funds, to calculate how many unopened Munchables can be added.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
            // if msg.sender is migrationManager, this will be skiped.
            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));
            }
    }

If user migrate their NFTs, when _tokenLocked is ETH, WETH or USDB, migratioManager will call lockOnBehalf().

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

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

        ...
    }

After that, in _lock(), caculation of reminder will be skiped due to msg.sender is migrationManager. This a problem that lockedToken.reminder wii be reset to zero, because in _lock(), Only when the caller is not the migrationManager will the reminder be calculated and assigned to lockedToken.remainder, otherwise, the initial reminder variable (which is 0) will simply be assigned to lockedToken.reminder.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
>       uint256 remainder;

        ...
            // if msg.sender is migrationManager, this will be skiped.
            if (msg.sender != address(migrationManager)) {

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

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

                nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
            }

        ...

        // if msg.sender is migrationManager, lockedToken.remainder will reset to zero.
>       lockedToken.remainder = remainder;
    }

This problem will cause user lose unopened Munchables, taking nftCost equal to 30 as an example.:

                        lock1   lock2        migrate NFT       lock3       
locked quantity         100     100             100             100
reminder                10      20                0              10        
added Munchables        3       6                 9              12

User has locked a total of 400 Ether, should add 400/30=13 Munchables not 12 instead. Ultimately, users may lose the Munchables that originally belonged to them.

Proof of Concept

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L311-L398 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/MigrationManager.sol#L335-L348

Tools Used

Vscode

Recommended Mitigation Steps

When migratioManager call _lock(), remain the reminder

    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));
-   }
+   } else {
+       remainder = lockedToken.remainder
+   }

Assessed type

Error