If there is no active LockDrop has been started, yet, user can set their unlock time to 0 or very low values.

[c4-bot-6]

Lines of code

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

Vulnerability details

Vulnerability Detail

In _lock(), we set _lockDuration = lockdrop.minLockDuration;. If there hasn't been a LockDrop event yet, a user can take advantage of the fact that the unlockTime can be set very low or even 0.

Someone can set their unlockTime via setLockDuration to 0 or 1, or a very low value, so they will be able to unlock their tokens without waiting days and will still receive schnibbles.

Impact

This breaks the logic and allow a person to unlock their locked tokens anytime. As mentioned in the docs the Munchables team considers to add minimum lock time as 30 days.

Proof of Concept

Remove the

    lm.configureLockdrop(
        ILockManager.Lockdrop({
            start: uint32(block.timestamp),
            end: uint32(block.timestamp) + uint32(lockDuration * 3),
            minLockDuration: lockDuration
        })
    );

From MunchablesTest.sol

And in SpeedRun.t.sol add this test:

function test_LowTimeissue() public {
MunchablesCommonLib.Player memory _player;
uint256 lockAmount = 100e18;
deployContracts();
// register me
amp.register(MunchablesCommonLib.Realm(3), address(0));

// Logic for PROOF starts here:
(, _player) = amp.getPlayer(address(this));
uint256 initialSchnibbles = _player.unfedSchnibbles;

// Set duration to 0
lm.setLockDuration(0);
uint256 unlockTime = block.timestamp;
vm.warp(unlockTime);
lm.lock{value: lockAmount}(address(0), 100e18);
// Unlock just a day later
vm.warp(unlockTime + 1 days);
lm.unlock(address(0), 100e18);

// assert schnibbles were increased
(, _player) = amp.getPlayer(address(this));
console.log("Unfed Schnibbles:", _player.unfedSchnibbles);
assert(_player.unfedSchnibbles > initialSchnibbles);
}

then use forge test --match-test test_LowTimeissue -vvvv

Recommendation

A global minimum lock time is recommended as currently there is a minimum limit only when there has been a LockDrop event. There should be a check for that variable in the lock() function that ensures the minimum time is set.

Also:
Don't allow unlockTime to == the current block timestamp, by fixing this check in unlock():

if (lockedToken.unlockTime > uint32(block.timestamp)) {
revert TokenStillLockedError();
}

to be:

if (lockedToken.unlockTime >= uint32(block.timestamp))

Also make sure that in configureLockDrop the minLockDuration cannot be set to 0.

Assessed type

Other