howlbot-integration[bot] commented 3 months ago

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:

Lock is created with _lockDuration 30 days.
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);
```

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.
```solidity
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

c4-judge commented 3 months ago

alex-ppg marked the issue as duplicate of #89