Munchables - shaflow2's results

Lines of code

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

Vulnerability details

Impact

When minLockDuration<lockdrop.end - lockdrop.start, it means that users can unlock funds during the lockdrop period and then relock them. Resulting in duplicate calculation of NFT for the same funds github:https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L352

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

Proof of Concept

Assuming lockdrop.start is 100, Lockdrop. end is 200, MinLockDuration is 10, ConfiguredToken. nftCost is 100, and User1 holds 99 tokens

1. At the beginning of lockdrop.start, User1 locks 99, now remainder is 99
2. Unlock after 10 blocks and lock again, _quantity + remainder = 198, An NFT will be added. Now remainder is 98.
3. Repeat the above operation, and at the end of lockdrop, User1 will add 9 nfts with a retain of 99

Tools Used

Manual Audit

Recommended Mitigation Steps

1. clear remainder when unlock
2. Store the amount of funds(totaluserused) that a user has already used to calculate NFT.Calculate nft only when _quantity+retain+quantity>totaluserused.

        if (
            lockdrop.start <= uint32(block.timestamp) &&
            lockdrop.end >= uint32(block.timestamp) &&
+            quantity + lockedToken.quantity > totaluserused
        ) {
            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));
+                totaluserused += quantity - remainder;
            }
        }

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }
+        if(lockedToken.quantity < lockedToken.remainder){
+            lockedToken.remander = lockedToken.quantity;
+        }
        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Assessed type

Other