Security Snippets: #7

And another one bug that was found in Munchables.

Invalid validation in the setLockDuration function allows users to greatly reduce their unlock time or even unlock instantly.

When a user locks his tokens in the LockManager.sol contract, the unlockTime is calculated as block.timestamp + lockDuration.

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        ---SNIP---
        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
>>      lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

The lockDuration is a variable that can be configured by a user in the setLockDuration function anytime, even during the lock period.

The problem arises when existing locks are updated to the new duration.

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

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        // update any existing lock
        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);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

There is an invalid check performed, comparing block.timestamp + _duration with the current unlockTime. However, in the end, the new unlockTime is set as lastLockTime + _duration.

So any time you work with timelock functions be careful with unexpected unlock possibilities.

Read the full report here:

Link: https://code4rena.com/reports/2024-05-munchables#h-02-invalid-validation-allows-users-to-unlock-early