Players can gain more NFTs benefiting from that past remainder in subsequent locks

[c4-bot-5]

Lines of code

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

Vulnerability details

Description

The remaining locked tokens continue to accumulate after the player unlocks their tokens, whether partially or fully.

As a result, the player benefits by retrieving more NFTs when they lock their tokens again during the lock drop period or in the next lock drop period.

Location: LockManager::_lock()L344 - L380

    uint256 quantity = _quantity + lockedToken.remainder;
    uint256 remainder;
    uint256 numberNFTs;
    uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    // SNIPPED

    if (    
        lockdrop.start <= uint32(block.timestamp) &&
        lockdrop.end >= uint32(block.timestamp)
    ) {
        if (
            _lockDuration < lockdrop.minLockDuration ||
            _lockDuration >
            uint32(configStorage.getUint(StorageKey.MaxLockDuration))
        ) revert InvalidLockDurationError();
        if (msg.sender != address(migrationManager)) {
            // calculate number of nfts
-->         remainder = quantity % configuredToken.nftCost; 
            numberNFTs = (quantity - remainder) / configuredToken.nftCost;

            if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

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

  // SNIPPED 

->  lockedToken.remainder = remainder;
    lockedToken.quantity += _quantity;

Location: LockManager::unlock()L401 - L427

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
        if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Impact

During the lock drop period or in the subsequent lock drop period, the player retrieves more NFTs in proportion to the quantity locked. The remaining tokens from previous locks enhance the new locking quantity when relocked until the end of the lock period.

Moreover, it probably assumes that the lockDuration can either be equal or greater than the lock drop duration (lockDuration >= lockdrop.start - lockdrop.end), allowing players to reenter by each lock drop, or significantly less than the lock drop duration (lockDuration < lockdrop.start - lockdrop.end), enabling players to reenter during the same lockdrop.

Therefore, the likelihood of the issue is based on how offset players can reenter and take advantage of the remainders.

The following core functions take an effect of this issue:

• _lock()
• lockOnBehalf()
• lock()
• unlock()

Proof of Concept

Prerequisite

• The LockManager contract has zero balance.
• The actions taken during the any active lock drop period.
• Using the address(0) as a token contract represents locking the native token
  • _tokenContract = address(0)
• Assuming the configuration of nftCostof the address(0) is 3 ethers (3e18)

  • configuredTokens[_tokenContract: address(0)] = 3e18

    Partially Unlocking Case

    1. Lock 10e18 (_quantity) tokens as uint256 quantity = _quantity + lockedToken.remainder :: L344

The state updates as follows:

• quantity = 10e18(_quantity) + 0 (lockedToken.remainder) = 10e18
• remainder = 10e18 % 3e18 = 1e18
• numberNFTs = (10e18 - 1e18) / 3e18 = 3

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 1e18
• lockedToken.quantity += _quantity = 0 + 10e18 = 10e18
• address(LockManager).balance = 10e18

📍 This shows that locking of 10e18 tokens can retrieve 3 NFTs.

Location: LockManager.sol::_lock()

    // add remainder from any previous lock
--> L344:   uint256 quantity = _quantity + lockedToken.remainder;
            uint256 remainder;
            uint256 numberNFTs;
            uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;

    // SNIPPED

    if (    
        lockdrop.start <= uint32(block.timestamp) &&
        lockdrop.end >= uint32(block.timestamp)
    ) {
        if (
            _lockDuration < lockdrop.minLockDuration ||
            _lockDuration >
            uint32(configStorage.getUint(StorageKey.MaxLockDuration))
        ) revert InvalidLockDurationError();
        if (msg.sender != address(migrationManager)) {
            // calculate number of nfts
--> L363:   remainder = quantity % configuredToken.nftCost; 
            numberNFTs = (quantity - remainder) / configuredToken.nftCost;

            if (numberNFTs > type(uint16).max) revert TooManyNFTsError();

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

  // SNIPPED 

-> L379 lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;

🕔 After the lockDuration has passed, the player can unlock the locked tokens.

2. Unlock 5e18 tokens partially:

   • L416::lockedToken.quantity -= _quantity = 10e18 - 5e18 = 5e18
   • address(LockManager).balance = 5e18

3. Lock 5e18 tokens back again as uint256 quantity = _quantity + lockedToken.remainder (prev remainder#1);

   • quantity = 5e18(_quantity) + 1e18 (lockedToken.remainder) = 6e18
   • remainder = 6e18 % 3e18 = 0
   • numberNFTs = (6e18 - 0) / 3e18 = 2

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 0
• lockedToken.quantity += _quantity = 5e18 + 5e18 = 10e18
• address(LockManager).balance = 10e18

📍 In total, the player retrieves 5 NFTs (3 from the first locking + 2 from the second locking) by locking 10e18 tokens, benefiting from the remainder of the previous locking.

---

Fully Unlocking Case

---

1. Lock 13e18 (_quantity) tokens as uint256 quantity = _quantity + lockedToken.remainder :: L344

The state updates as follows:

• quantity = 13e18(_quantity) + 0 (lockedToken.remainder) = 10e18
• remainder = 13e18 % 3e18 = 1e18
• numberNFTs = (13e18 - 1e18) / 3e18 = 4 📍

Updates to lockedToken struct and LockManager contract balance:

• lockedToken.remainder = remainder = 1e18 📍
• lockedToken.quantity += _quantity = 0 + 13e18 = 13e18 📍
• address(LockManager).balance = 13e18 📍

📍 This shows that locking of 13e18 tokens can retrieve 4 NFTs.

🕔 After the lockDuration has passed, the player can unlock the locked tokens.

2. Unlock 13e18 tokens fully:

   • L416::lockedToken.quantity -= _quantity = 13e18 - 13e18 = 0 📍
   • address(LockManager).balance = 0 📍

3. Lock 11e18 tokens as uint256 quantity = _quantity + lockedToken.remainder (prev remainder#1);

   • quantity = 11e18(_quantity) + 1e18 (lockedToken.remainder) = 12e18
   • remainder = 12e18 % 3e18 = 0
   • numberNFTs = (12e18 - 0) / 3e18 = 4 📍

Updates to lockedToken and LockManager contract balance:

• lockedToken.remainder = remainder = 0 📍
• lockedToken.quantity += _quantity = 0 + 11e18 = 11e18 📍
• address(LockManager).balance = 11e18 📍

📍 In total, the player retrieves 4 new NFTs from locking 11e18 tokens. Compared to the first locking of 13e18 tokens, the second lock yields similar power to gain 4 NFTs, benefiting from the remainder of the previous locking round.

Tools Used

• Manual Review

Recommended Mitigation Steps

Manage the remaining tokens when the player unlocks locked tokens. Alternatively, use other methods to handling the total amount of NFTs and locked quantity during the lock drop.

Assessed type

Math

[0xinsanity]

Fixed here: https://github.com/munchablesorg/munchables-common-core-latest/pull/13/files

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden outlines a situation whereby the remainder of an account will not be properly maintained when unlocking a lock, permitting them to effectively carry over remainders that should otherwise not be accounted for.

While the vulnerability is present, its actual risk level is medium as the amounts involved would be insignificant and it would also imply that a user fully vested a minimum lock, and took advantage of the carried-over remainder in a second lock in the same "lockdrop".

I selected this submission as the best given that it outlines the issue using a consumable step-by-step case. I would like to note that the mitigation cited by the Sponsor above is insufficient and the mitigation followed in #41 is the correct one.

[c4-judge]

alex-ppg marked the issue as satisfactory

[adamidarrha]

I don't see why this is an issue. but intended behaviours. The Remainder represents the amount that is staked but not yet given for a nft . for example if the price of 1 nft is 1 eth. the person stakes 0.5 eth , the remainder would be 0.5 so next time he locks more than 0.5 eth he will be able to get the nft. It's not a problem if he stakes 0.5 eth , then unstakes , then stakes he should still be getting the nft because overall he deposited 1 eth to the contract . it's the same logic as stake -> stake and get minted nft.

[alex-ppg]

Hey @adamidarrha, thanks for your feedback! I believe that being able to acquire an NFT with a price of 1 ETH by using the same 0.5 ETH twice is invalid, as it means the TVL of the project (which is what the lockdrop wants to achieve) was not increased as much as it should have. The reason an amount-based requirement is set is to ensure that many high-value locks exist in the contract. Given that we can bypass this limitation by re-using the same funds, it means that a wider audience of token holders will participate in the NFT drop which in turn would be considered an inflationary action as it reduces the scarcity of the NFT.

I appreciate your contribution and will maintain my original ruling as the Sponsor also confirmed the vulnerability.