Players can steal funds from the `lockManager` preventing other users form unlocking the full amount of their withdrawals

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Players can

• steal from the LockManager contract preventing other users from unlocking their complete locked amount
• mint more NFT in the nft reveal if they are locking during an active lockdrop event

Proof of Concept

During locking in a lockdrop event, the amount of tokens that do not contribute to the nft release owing to the nftCost of locking the token is saved in the lockedToken.remainder variable. For every time a player locks a token, the lockedToken.remainder is added to the amount the user intends to lock as shown in L344 below.

However, when the user is unlocking his tokens the lockedToken.remainder is not updated and the player can only withdraw his total locked amount per time leaving no crumbs. This leads to a situation where the previously cached lockedToken.remainder is added to the players balance when they intend to lock another time as shown in L341 below thereby over inflating the players locked token amount and by extension more NFTs as shown on L364 .

POC Summary

For simplicity, assume there are 3 tokenContract A, B and C, MaxLockDuration = 90days, MinLockDuration = 20 days and time t started at day 0

    struct ConfiguredToken {
        uint256 usdPrice; // 
        uint256 nftCost = 6000e18; 
        uint8 decimals; 
        bool active;
    }

    struct LockedToken {
        uint256 quantity = 20_000e18;
        uint256 remainder;
        uint32 lastLockTime;
        uint32 unlockTime;
    }

    uint256 quantity = _quantity + lockedToken.remainder;

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

File: LockManager.sol
311:     function _lock(
312:         address _tokenContract,
313:         uint256 _quantity,
314:         address _tokenOwner,
315:         address _lockRecipient
316:     ) private {

...

343:         // add remainder from any previous lock
344: @>      uint256 quantity = _quantity + lockedToken.remainder;
345:         uint256 remainder;
346:         uint256 numberNFTs;
347:         uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;
348: 
349:         if (_lockDuration == 0) {
350:             _lockDuration = lockdrop.minLockDuration;
351:         }
352:         if (
353:             lockdrop.start <= uint32(block.timestamp) &&
354:             lockdrop.end >= uint32(block.timestamp)
355:         ) {
...
361:             if (msg.sender != address(migrationManager)) {
362:                 // calculate number of nfts
363: @>              remainder = quantity % configuredToken.nftCost;
364: @>              numberNFTs = (quantity - remainder) / configuredToken.nftCost;
365: 
...
366:             }
367:         }
...
378: 
379: @>      lockedToken.remainder = remainder;
380: @>      lockedToken.quantity += _quantity;
381:         lockedToken.lastLockTime = uint32(block.timestamp);
382         lockedToken.unlockTime =
383:             uint32(block.timestamp) +
384:             uint32(_lockDuration);
385: 
...
398:     }

401:     function unlock(
402:         address _tokenContract,
403:         uint256 _quantity
404:     ) external notPaused nonReentrant {
405:         LockedToken storage lockedToken = lockedTokens[msg.sender][
406:             _tokenContract
407:         ];
408:         if (lockedToken.quantity < _quantity)
409:             revert InsufficientLockAmountError();
410:         if (lockedToken.unlockTime > uint32(block.timestamp))
411:             revert TokenStillLockedError();
412: 
413:         // force harvest to make sure that they get the schnibbles that they are entitled to
414:         accountManager.forceHarvest(msg.sender);
415: 
416:  @>     lockedToken.quantity -= _quantity;
417: 
418:         // send token
...
425: 
426:         emit Unlocked(msg.sender, _tokenContract, _quantity);
427:     }

• lockedToken.remainder before locking A = 0

• Alice token A balance 20_000e18

• Bob’s token A balance 20_000e18

• LockManager token A balance 0

• Alice and Bob both locks A on day 30 in a lockdrop event for a duration of 20 days each

  • nftCost = 6000e18
  • _quantity = 20_000e18 + previous remainder
  • _quantity = 20_000e18 + 0 = 20_000e18
  • lockedToken.remainder = 20_000e18 % 6000e18 = 2_000e18
  • lockedToken.quantity = 20_000e18
  • lockDuration = 20 days
  • unlockTime = day 50
  • All 20_000 A tokens are locked in the contract

• Alice and Bob’s token A balance 0

• LockManager token A balance 40_000e18

• On day 51 Alice calls unlock(...) with _quantity = 20_000e18, but

  • lockedToken.remainder = 2_000e18
  • lockedToken.quantity = 0

• Alice token A balance 20_000e18

• LockManager token A balance 20_000e18

• Alice enters the next lockdrop event on day 55 for a duration of 20 days

  • nftCost = 6000e18
  • _quantity = 20_000e18 + previous remainder
  • _quantity = 20_000e18 + 2_000e18 = 22_000e18
  • lockedToken.remainder = 22_000e18 % 6000e18 = 4_000e18
  • lockedToken.quantity = 20_000e18
  • lockDuration = 20 days
  • unlockTime = day 75
  • All 20_000 A tokens are locked in the contract

• Alice token A balance 0

• LockManager token A balance 40_000e18

• On day 76 Alice calls unlock(...) with _quantity = 22_000e18, but

  • lockedToken.remainder = 4_000e18
  • lockedToken.quantity = 0

• Alice token A balance 22_000e18

• LockManager token A balance 18_000e18

• Alice now has an extra 2_000e18 token A

• Bob calls unlock(...) and he may either revert, not get his full lock amount or his funds get stuck without a way to withdraw depending on whether or token A reverts on failure

The goal here was to show how step by step a player can steal from the LockManager preventing others from withdrawing,

As a matter of fact notice that if Alice reats this for the next lockdrop event with token A she would make an extra 4_000e18 token A bringing her total theft to 6_000e18 token A

Tools Used

Manual review

Recommended Mitigation Steps

Moidify the unlock(...) function to update the lockedToken.remainder for the player whenever he unlocks as shown below

401:     function unlock(
402:         address _tokenContract,
403:         uint256 _quantity
404:     ) external notPaused nonReentrant {
405:         LockedToken storage lockedToken = lockedTokens[msg.sender][
406:             _tokenContract
407:         ];
408:         if (lockedToken.quantity < _quantity)
409:             revert InsufficientLockAmountError();
410:         if (lockedToken.unlockTime > uint32(block.timestamp))
411:             revert TokenStillLockedError();
412: 
413:         // force harvest to make sure that they get the schnibbles that they are entitled to
414:         accountManager.forceHarvest(msg.sender);
415: 
416:  @>     lockedToken.quantity -= _quantity;

417:  +      lockedToken.remainder = 0 

418:         // send token
419:         if (_tokenContract == address(0)) {
420:             payable(msg.sender).transfer(_quantity);
421:         } else {
422:             IERC20 token = IERC20(_tokenContract);
423:             token.transfer(msg.sender, _quantity);
424:         }
425: 
426:         emit Unlocked(msg.sender, _tokenContract, _quantity);
427:     }

Assessed type

Math

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)