`lockedToken.remainder` can get corrupted depending on when `lock()` is called

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L344-L345 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L352-L355 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L379

Vulnerability details

Currently the value of lockedToken.remainder will be updated if lock() gets called during a lockdrop. However, the lockedToken.remainder will be reset to zero if lock() gets called during a non lockdrop period.

Proof of Concept

Assume a period of lockdrops and no lockdrops.

// ----------------------------------------------------------------------
//  lockdrop_1 | no_lockdrop_1 | lockdrop_2 | no_lockdrop_2 | lockdrop_3

Also assume

• _quantity = 7e18 (all calls)
• configuratedToken.nftCost = 8e18 (all calls)
• lockedToken.remainder = 0 (first call of each scenario, A.1 and B.1)

Scenario A

1. Bob calls lock() during lockdrop_1, resulting in quantity = 7e18, remainder = 7e18 and numberNFTs = 0.
2. Bob calls lock() during lockdrop_2, resulting in quantity = 15e18, remainder = 7e18 and numberNFTs = 1.
3. Bob calls lock() during lockdrop_3, resulting in quantity = 15e18, remainder = 7e18 and numberNFTs = 1.

Scenario B

1. Same as 1.a above.
2. Bob calls lock() during no_lockdrop_2, resulting in quantity = 15e18, remainder = 0 (branch for lockdrop not called).
3. Bob calls lock() during lockdrop_3, resulting in quantity = 7e18, remainder = 7e18 and numberNFTs = 0.

Effectively, the issue is that if it's not a lockdrop period, the state variable lockedToken.remainder will have the value of the local remainder variable, which will still be zero since the branch for lockdrop will not be executed.

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

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

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

Impact

If the poc example, we can see the value of numberNFTs will be different during lockdrop_3 depending if he called lock() during lockdrop_2 or no_lockdrop_2.

Therefore, the nftOverloard.addReveal() can have a smaller number of NFTs if the user previously called lock() during a non lockdrop period, since the remainder is not carried over.

It's also possible for a mallicious user to call lockOnBehalf during a non lockdrop period for another user with 1 wei to forcefully reset his remainder.

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

Tools Used

Manual review

Recommended Mitigation Steps

One approach can be to move lockedToken.remainder = remainder inside the lockdrop conditional branch so it only gets updated during lockdrop periods.

diff --git a/LockManager.sol b/LockManager.updated.sol
--- a/LockManager.sol
+++ b/LockManager.updated.sol
@@ -367,6 +367,8 @@ contract LockManager is BaseBlastManager, ILockManager, ReentrancyGuard {

                 // Tell nftOverlord that the player has new unopened Munchables
                 nftOverlord.addReveal(_lockRecipient, uint16(numberNFTs));
+
+                lockedToken.remainder = remainder;
             }
         }

@@ -376,7 +378,6 @@ contract LockManager is BaseBlastManager, ILockManager, ReentrancyGuard {
             token.transferFrom(_tokenOwner, address(this), _quantity);
         }

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

Assessed type

Timing