Lines of code

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

Vulnerability details

The LockManager contract has a mechanism to retain the remaining funds after calculating the number of NFTs during a lockdrop event. This reminder is used to calculate the number of NFTs in the next lock, ensuring the maximum utilization of funds to generate Munchables. However, if a user locks some funds between two lockdrops, the reminder resets to 0, making it ineffective for creating Munchables in the next lockdrop event. Additionally, a malicious user can exploit this vulnerability to reset the reminders of other users through the lockOnBehalf() function.

Impact

This vulnerability allows an attacker to delete the reminders of any users after a lockdrop event, thereby preventing them from using their remaining funds effectively in the next lockdrop event.

Proof of Concept

The vulnerability lies in the _lock() function. At line 344, the function uses the reminder from the previous lock, and at line 345, it resets the reminder to 0. However, if there is no active lockdrop event at this moment, the quantity is not used even though lockedToken.remainder is reset to 0 at line 379.

343:    // add remainder from any previous lock
344:    uint256 quantity = _quantity + lockedToken.remainder;
345:    uint256 remainder;
346:    uint256 numberNFTs;
347:    uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;
348:
349:    if (_lockDuration == 0) {
350:        _lockDuration = lockdrop.minLockDuration;
351:    }
352:    if (
353:        lockdrop.start <= uint32(block.timestamp) &&
354:        lockdrop.end >= uint32(block.timestamp)
355:    ) {
.           // --- disabling code snippet ----
361:        if (msg.sender != address(migrationManager)) {
362:            // calculate number of nfts
363:            remainder = quantity % configuredToken.nftCost;
364:            numberNFTs = (quantity - remainder) / configuredToken.nftCost;
.               // --- disabling code snippet ----
370:        }
371:    }
.       // --- disabling code snippet ----
.
379:    lockedToken.remainder = remainder;

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

The previous reminder of a lock is intended to be used in the next lock, even between two distinct lockdrop events. However, if there is a lock between two lockdrops, the previous reminder is removed. This can be exploited by malicious users to remove reminders of other users after each lockdrop event.

Tools Used

Manual Review

Recommended Mitigation Steps

Update the lockedToken.remainder only if there is an active lockdrop event.

Sample Code Snippet:

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        (
            address _mainAccount,
            MunchablesCommonLib.Player memory _player
        ) = accountManager.getPlayer(_lockRecipient);
        if (_mainAccount != _lockRecipient) revert SubAccountCannotLockError();
        if (_player.registrationDate == 0) revert AccountNotRegisteredError();
        // check approvals and value of tx matches
        if (_tokenContract == address(0)) {
            if (msg.value != _quantity) revert ETHValueIncorrectError();
        } else {
            if (msg.value != 0) revert InvalidMessageValueError();
            IERC20 token = IERC20(_tokenContract);
            uint256 allowance = token.allowance(_tokenOwner, address(this));
            if (allowance < _quantity) revert InsufficientAllowanceError();
        }

        LockedToken storage lockedToken = lockedTokens[_lockRecipient][
            _tokenContract
        ];
        ConfiguredToken storage configuredToken = configuredTokens[
            _tokenContract
        ];

        // they will receive schnibbles at the new rate since last harvest if not for force harvest
        accountManager.forceHarvest(_lockRecipient);

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

        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

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

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

        emit Locked(
            _lockRecipient,
            _tokenOwner,
            _tokenContract,
            _quantity,
            remainder,
            numberNFTs,
            _lockDuration
        );
    }

Assessed type

Other

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

`lockedToken.reminder` of a Previous Lock Resets to 0 If User/Attacker Locks Funds Between Two Lockdrop Events #453