User's lock remainder will be overwritten and lost between lockdrops

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

When a user makes a lock during a lockdrop, they receive NFTs for their lock. Currently the price of the NFT is expected to be 1e18 (according to test files and sponsor clarifications). Due to that, the protocol first calculates the remainder from quantity % configuredToken.nftCost and then calculates the amount of NFTs for the user from (quantity - remainder) / configuredToken.nftCost. Next time the user locks again, their new locked quantity is added with the remainder:

    uint256 quantity = _quantity + lockedToken.remainder;
    ...

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

This is a good idea so that if a user deposits an amount with a remainder, it is still added onto their next lock and used for minting NFTs. But, at the end of the function, the stored remainder is overwritten with the one calculated from the latest lock:

    lockedToken.remainder = remainder;

Since there will be multiple lock drops, what this means is that if a user locks any tokens between lockdrop 1 and lockdrop 2, any remainder they had leftover will be overwritten with 0. This is because when the temporary remainder is first declared it will be assigned a default value of 0. And that variable can only increase when we are in a lockdrop (block.timestamp >= lockdrop.start && block.timestamp <= lockdrop.end). The maximum amount of value that can be wasted is 0.99 eth which I believe is enough to warrant medium severity.

The whole idea of the remainder is for the users' capital to be efficient in the mints but in this case there is value lost for the user instead of what should be accrued.

Proof of Concept

Simple issue

Tools Used

Manual review

Recommended Mitigation Steps

Store the remainder between lock drops, overwriting with the default 0 value just loses value for the user in the end.

Assessed type

Error

[michaeljyeates]

This is by design

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid