howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

In the setLockDuration function, the calculation of unlockTime using lastLockTime + uint32(_duration) can set the unlockTime to a value earlier than the current unlockTime if lastLockTime is in the past. This can unintentionally reduce the unlock time, violating the intended logic.

Proof of Concept

In the setLockDuration function, the unlockTime is calculated using lastLockTime + uint32(_duration), which can result in an earlier unlock time if lastLockTime is in the past:

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

Tools Used

Manual Review

Recommended Mitigation Steps

Use block.timestamp directly to ensure that the new unlockTime is always in the future relative to the current block time. This avoids issues with lastLockTime being in the past:

    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 =
                    uint32(block.timestamp) +
                    uint32(_duration);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

This ensures that the unlockTime is always set to a future time relative to the current block time, preventing any unintended reduction in the unlock time.

Assessed type

Context

c4-judge commented 1 month ago

alex-ppg marked the issue as duplicate of #89