Lines of code

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

Vulnerability details

Impact

lock can be blocked, and users will not be able to lock more funds or add up to their locked funds.

Proof of Concept

Users will start calling lock method to lock the funds in LockManager contract, the first time for a user to lock the funds it will trigger the condition L349:

if (_lockDuration == 0) {
            _lockDuration = lockdrop.minLockDuration; //could be 24 hours
        }

_lockDuration here will be the minimum value as defined in lockdrop.minLockDuration, the issue is not here and this will work fine, the issue occur when some users has already locked funds and now they like to lock more, let's check this code below L356:

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

The code above will achieve a revert if _lockDuration < lockdrop.minLockDuration which it seems fine, until the onlyAdmin call configureLockdrop to set minLockDuration without any awareness or any attention to users' old _lockDuration.

Meaning, users who has already locked funds and try to call lock again with thier old lockDuration playerSettings[_lockRecipient].lockDuration, possibly reverting incase the onlyAdmin updated the minLockDuration and this new value is less than player old lockDuration value, according to the sponsor, people should still be able to lock at any point regardless if there is a lockdrop event.

Tools Used

Manual Review

Recommended Mitigation Steps

Find a way that the admin can't decrease minLockDuration if users already locked their funds.

Assessed type

DoS

code-423n4 / 2024-05-munchables-validation

InvalidLockDurationError revert can cause a block for users when use lock method #553