Insufficient validation allows to maliciously reduce lock duration

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

In the setLockDuration there is a check to make sure that the lock duration is not reduced while there are still tokens that are locked. The issue is that the validation is insufficient.

Proof of Concept

As we can see, this check makes sure that the new duration of the lock is not less than the previous one:

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

The problem is that the previous unlockTime is compared to the current time plus the new duration when it should be compared to the time of the last lock plus the new duration. This allows for the new unlockTime of the currently locked tokens to be less than the previous one. We understand from the validation above that this should not be permitted. Here is an example:

1/ The current lock duration of some locked tokens is 90 days 2/ We will assume, for simplicity, that the tokens were locked on block.timestamp equal to 0 days 3/ 40 days pass and the user decides to reduce the lock duration to 60 days 4/ The call does not revert in the check above as the current timestamp is at 40 days, which added to the new duration is more than the unlock time (100 days > 90 days) 5/ The unlock time is now set to 0 days (lastLockTime) plus 60 days, equal to 60 days 6/ Therefore, the lock duration was decreased successfully, even though from the validation shown above we can see that this should not be allowed.

Tools Used

Manual review

Recommended Mitigation Steps

There are two ways to solve this issue depending on the protocols intentions: a/ The check above should use: lastLockTime + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime OR b/ The new unlockTime should be set to block.timestamp + uint32(_duration)

Assessed type

Other

[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)