Incorrect state change in `_lock` function allows user to steal tokens.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L380

Vulnerability details

The lock function is responsible for locking the user's tokens for a particular _duration. However, the _lock function adds the previous remainder each time when a user locks the token again to his provided quantity.

uint256 quantity = _quantity + lockedToken.remainder;

And also adds whole provided quantity to user to unlock at a particular unlockTime.

lockedToken.quantity += _quantity;

So the problem is here that a user can unlock the entire provided quantity after first lock, along with the previous remainder amount, during their second locking action.

Impact

As much as configuredToken.nftCost grows user will be able to steal more tokens according to below statement:

The highest remainder can be calculated as: nftCost - 1

Proof of Concept

Scenario:

1) Let's say the user locks 199 tokens which nftCost = 100. 2) Now remainder will be 99 according to this:

   remainder = quantity % configuredToken.nftCost;

3) So, due to the below statements in the _lock function, that users are able to unlock all the provided tokens, after the unlockTime finishes.

   lockedToken.quantity += _quantity

4) User unlocks 199 when the unlockTime elapsed. 5) After that, the user again locks 1 more token, and since the remainder is now 99, the quantity will be
100 according to the below statement.

     uint256 quantity = _quantity + lockedToken.remainder;

6) He Waits until the unlockTime finishes, call the unlock function again, and gather 100 more tokens. 7) In this way, the user drains 99 tokens.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider implement this on unlock function.

+   lockedToken.quantity += _quantity - remainder;

Assessed type

Other