The setLockDuration() function allows users to be able set new reduced lock up times, leading to false events being emitted

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Users can set new shorter duration times for their locked funds and emit false events.

Proof of Concept

The setLockDuration function has the following if-statement to make sure that no user can set a new lock duration that is before the current unlockTime:

    if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
             revert LockDurationReducedError();
}

Then the function updates the new unlockTime immediately after like so:

    uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
    lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);

The problem here is that we are using uint32(block.timestamp), instead of the previous lock time that was set, to check if a user is setting shorter durations. This will lead to shorter durations being set.

Let's imagine a user has already locked funds for a certain duration and wants to set a new duration. For example let's say the user currently has:

1) A previous last lock time at 500 2) An initial duration that was set at 1500 3) A current unlockTime at 2000 (500 + 1500 = 2000)

Now let's say the current block.timestamp is at 1900, and the user calls this function again, but with _duration being set to 150, lowered from 1500. Plugging in these numbers in the if-statement will not make it revert:

if(1900 + 150 < 2000) { revert LockDurationReducedError(); }

Now let's go ahead and update the unlockTime again by plugging in the new numbers and see what we get. The new unlock time is now:

lockedTokens[msg.sender][tokenContract].unlockTime = 500 + 150 = 650 650 < 2000

The event in this function will now emit a new duration of 150, and the new unlockTime is now reduced from 2000 to 650.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the uint32(block.timestamp) to lockedTokens[msg.sender][tokenContract].lastLockTime correctly check the user isn't setting shorter duration periods:

-    if ( uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
+    if ( lockedTokens[msg.sender][tokenContract].lastLockTime + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
             revert LockDurationReducedError();
}

Assessed type

Timing

[c4-judge]

alex-ppg marked the issue as duplicate of #89

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)