c4-bot-7 commented 1 month ago

Lines of code

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

Vulnerability details

Impact

The function setLockDuration can be called to change the duration of locks that the user wishes to have applied to their locks by default. This function should make sure that users cannot reduce the lock times of their locks retroactively, so puts in a safeguard for the same.

Hypothetically, if users were able to reduce their lock times, that would be problematic. The system gives out rewards/incentives based on the duration of the locks, so users can game the system by creating long locks, collecting the benefits, and then reducing the lock time. This should not be allowed by the system.

In this contract, such a safeguard is implemented, but it is not implemented correctly.

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

The lock time is set to the lastLockTime + duration This can actually reduce the lock time.

Proof of Concept

Lets say a lock exists. It was created on Jan 1, and had a duration of 10 days. Thus, the unlock time is Jan 11.

On Jan 7, Alice calls setLockDuration with a duration of 5 days.

block.timestamp + duration = Jan 7 + 5 days = Jan 12. This is higher than the unlockTime, which is Jan 11. So no revert happens.

lastLockTime is set to the previous lock time, of Jan 1.

unlockTime is set to lastLockTime + duration = Jan 1 + 5 days = Jan 6.

Alice's lock, which was about to expire on Jan 11, is now immediately available!

Tools Used

Manual analysis

Recommended Mitigation Steps

The new lock time should be block.timestamp, not the old lock time. This will basically create a fresh lock from today. In the above POC, if the lastLockTime was set to block.timestamp, the new unlock time will be Jan 7 + 5 days = Jan 12 (since block.timestamp is Jan 7). This will ensure that the unlock time never decreases.

Change the code to:

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

Assessed type

Invalid Validation

c4-judge commented 1 month ago

alex-ppg marked the issue as duplicate of #89