User cannot unlock all tokens.

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Users will be unable to unlock all their tokens due to a balance check which failed to take account of user remainders.

An honest attempt by user to withdraw all their locked tokens after lock duration elapse will revert.

Proof of Concept

When user call lock(), remainders are gotten from modular with nftcost.

// calculate number of nfts
                remainder = quantity % configuredToken.nftCost;
...
lockedToken.remainder = remainder;

However, during unlock(), remainder amount are not taken into consideration.

 if (lockedToken.quantity < _quantity) 
            revert InsufficientLockAmountError();

Tools Used

Manual review.

Recommended Mitigation Steps

--- if (lockedToken.quantity < _quantity) //@audit what about remainder?
            revert InsufficientLockAmountError();
+++ if (lockedToken.quantity + lockedToken.remainder < _quantity)
            revert InsufficientLockAmountError();

Assessed type

DoS

[Tomiwasa0]

I believe this should be reviewed has lockedtokens can be redeemed directly to the user but if a user locks 10 token from his 12. he cannot redeem the remaining 2. this will revert because the check doesn't include the remainder.

if (lockedToken.quantity < _quantity) revert InsufficientLockAmountError();

instead this should be checked

if (lockedToken.quantity + lockedToken.remainder < _quantity) revert InsufficientLockAmountError();

[lanrebayode]

This report shows that a user with remainder cannot unlock all tokens, as the function revert since the check fails to acknowledge remainder value.

Please take a second look at this.

[alex-ppg]

Hey @Tomiwasa0 and @lanrebayode, thanks for your input! The remainder is purely utilized for NFT drops and has no use outside of a lockdrop context. It does not indicate actual finds that are due for the user, as the quantity of the lock sufficiently depicts the amount that the user deposited.