Integer Overflow in `LockManager::setLockDuration` potentially leading to unexpected behavior.

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact:

Players can secure a quantity of tokens in LockManager::lock, which after a certain period of time can be released via LockManager::unlock. The time it takes to unlock it is set in LockManager::setLockDuration, however this function has an Integer overflow that could cause unexpected behavior in the game.

function setLockDuration(uint256 _duration) external notPaused {                           // ## [ 1 ]
    if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
        revert MaximumLockDurationError();

    playerSettings[msg.sender].lockDuration = uint32(_duration);                           // ## [ 2 ]
    // 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);                                              // ## [ 3 ]  
}

In [ 1 ] the LockManager::setLockDuration function receives the duration as a uint256 then in [ 2 ] take that uint256 and caste it to uint32, meaning it can represent a much larger range of values compared to uint32.

Then in [ 3 ] it sends that parameter as uint256 which may cause unexpected behaviors such as incorrect calculations, DoS or other vulnerabilities.

Recommended Mitigation:

A possible mitigation for this situation may be receive _duration as uint32 to avoid overflow

- function setLockDuration(uint256 _duration) external notPaused
+ function setLockDuration(uint32 _duration) external notPaused

Assessed type

ERC20