Munchables - sandy's results

Lines of code

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

Vulnerability details

Impact

Malicious users can unlock token before minLockDuration by calling setLockDuration() function due to invalid unlock time calculation in the setLockDuration() function.

Proof of Concept

During the lockdrop period(period between lockdrop.start and lockdrop.end), unrevealed nfts are added for the player using the addReveal() function. During this period, the _lockDuration must be >= lockdrop.minLockDuration.

_lock():

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));
            }
        }

But, the user can call setLockDuration() function to bypass this minLockDuration check.

Let's assume, minLockDuration is set to 30 days. Now, if the _lockDuration value is set anything below 30 days, this check will revert in the _lock function:

if ( _lockDuration < lockdrop.minLockDuration || _lockDuration > uint32(configStorage.getUint(StorageKey.MaxLockDuration))) revert InvalidLockDurationError();

and if the _lockDuration hasn't been set previously by the user, it will be defaulted to minLockDuration.

_lock():

        if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration;
        }

Now, let's assume user hasn't set any lock duration using setLockDuration() function. Thus, the _lockDuration will be set to 30 days(suppose).

Theoretical PoC:

1. Lock is created with _lockDuration 30 days.
2. Let's assume block.timestamp is 0, thus the unlock time will be set at timestamp 0 + 30 * 86400 = 2592000(30 days timestamp) and last lock time will be set to 0.

_lock():
        
               lockedToken.lastLockTime = uint32(block.timestamp);
               lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration);

3. Now after 15 days, the malicious user calls setLockDuration() with _duration value 15 days. Now, inside the setLockDuration() function, there is a check to make sure _duration is not set before unlock time.

setLockDuration():
              
                if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
                    revert LockDurationReducedError();
                }

Here, block.timestamp is 15 days, _duration supplied is also 15 days and unlock time is 30 days. Thus, this check passes as 15 + 15 !< 30.

4. Finally the unlock time is set to lastLockTime + _duration supplied.

setLockDuration():

                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
                lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);

i.e in our case 0 + 15. Thus, unlock time is updated by attacker to 15 days, and now the attacker can withdraw at any time and 30 days minLockDuration is bypassed.

The root cause is setLockDuration() function can be called anytime and the _duration value supplied to setLockDuration() function is added with lastLockTime value instead of block.timestamp while setting new update time.

Tools Used

Manual Analysis

Recommended Mitigation Steps

In the setLockDuration() function, update the unlockTime as such:

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration)) {
            revert MaximumLockDurationError();
        }

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
                // check they are not setting lock time before current unlocktime
                if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
                    revert LockDurationReducedError();
                }

-                uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
-                lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);
+                lockedTokens[msg.sender][tokenContract].unlockTime = uint32(block.timestamp) + uint32(_duration);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

User can unlock all amount including remainder amount but remainder amount is not deleted from the lockedTokens mapping.

1. If user locks again after unlocking all amount, the previous remainder amount is added to the new locked amount. Thus, if user unlocks again, remainder amount is withdrawn twice from the contract and new remainder amount if exists will be cached again.
2. Though remainder amount is a small amount, as the user base increases and time passes, the small amount will accumulate to create a huge bad debt.
3. This bad debt whether small amount or large will prevent some of the users from unlocking and withdrawing all of their token amount.

Proof of Concept

When locking, in the _lock() function, remainder amount is added to supplied _quantity. But, if this the user first time locking, lockedToken.remainder amount will be 0.

lock():

    uint256 quantity = _quantity + lockedToken.remainder; 

Then, remainder amount is calculated as:

lock(): 

    remainder = quantity % configuredToken.nftCost; 

Now, remainder and _quantity are stored as:

lock():

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

Up until now, for a user first time locking,

1. lockedToken.remainder = 0
2. Let remainder = 100 wei
3. Let _quantity = 1e18 + 100 wei

Now, if the user unlocks all amount(1e18 + 100 wei) using unlock() function,

unlock():

    lockedToken.quantity -= _quantity; 

All the amount, i.e 1e18 + 100 wei including remainder is withdrawn but remainder is never deleted in the unlock() function.

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);
    }

Thus when the user locks again, the 100 wei remainder amount is again added to the supplied quantity:

_lock():

 uint256 quantity = _quantity + lockedToken.remainder;

And the cycle continues as long as the user locks and unlocks again and again and there exists remainder amount.

The root cause is calculating remainder from supplied amount, adding previous remainder and letting withdraw all amount including remainder amount.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Modify the unlock() function as such:

  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;

+       if (lockedToken.quantity == 0) {
+           delete lockedToken.remainder;
+       } else if (lockedToken.quantity <= lockedToken.remainder) {
+           lockedToken.remainder = lockedToken.remainder - lockedToken.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);
    }

The above modification will delete remainder amount if all the amount is withdrawn or set the remainder amount to remaining amount left. The else if condition is used in case if some malicious user leaves 1 wei in his account to re-claim the already withdrawn remainder amount.

Assessed type

Other