Munchables - LinKenji's results

Lines of code

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

Vulnerability details

Impact

In the setLockDuration function, users could reduce the lock duration for their existing locks leading to funds being unlocked earlier than intended.

The issue is in the following code block: src/managers/LockManager.sol#L256-L267

                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 condition uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime is intended to prevent users from setting a lock duration that would result in an unlock time earlier than the current unlock time for that token.

However, this check is not sufficient because it only considers the new lock duration and the current unlock time. It does not take into account the lastLockTime of the existing lock.

If a user has an existing lock with a lastLockTime that is significantly in the past, setting a new lock duration, even if it is longer than the current lock duration, could still result in an earlier unlock time than the current unlock time.

For example, let's say a user has an existing lock with:

lastLockTime: 1000 (some time in the past) unlockTime: 2000 (current unlock time) lockDuration: 1000 (current lock duration)

If the user calls setLockDuration with a new duration of 1500, the condition uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime would pass because 1500 is greater than the current lock duration of 1000.

The crux of the problem lies in the fact that the lastLockTime is not updated when the lock duration is changed. This leads to the calculation of the new unlockTime being based on the old lastLockTime, which can result in an earlier unlock time than intended, effectively reducing the lock duration.

"So the issue lies in the fact that the lastLockTime is not updated when the lock duration is changed. This means that if a user has an existing lock with a lastLockTime of 1000 and an unlockTime of 2000, and they call setLockDuration with a new duration of 1500, the unlockTime will be updated to 1000 + 1500 = 2500, which is earlier than the original unlockTime of 2000."

"This is because the lastLockTime is not updated to the current timestamp, so the new unlockTime is calculated based on the old lastLockTime, effectively reducing the lock duration and allowing the user to unlock their funds earlier than intended."

Proof of Concept

Consider the following scenario:

A user locks tokens with a lock duration of 1 year (31536000 seconds). The lastLockTime is set to the current block timestamp, let's say 1000. The unlockTime is calculated as lastLockTime + lockDuration = 1000 + 31536000 = 31537000. After some time, let's say the current block timestamp is 10000, the user calls setLockDuration with a new duration of 2 years (63072000 seconds).

According to the current implementation, the condition uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime would pass because 10000 + 63072000 > 31537000.

However, the new unlockTime would be calculated as lastLockTime + _duration = 1000 + 63072000 = 63073000, which is earlier than the original unlockTime of 31537000.

This means that the user has effectively reduced the lock duration from 1 year to approximately 1.5 years (63073000 - 10000 = 63063000 seconds), allowing them to unlock their funds prematurely.

Violation of Invariants: This violates one of the main invariants stated in the README file: "people cannot reduce lockup times that are previously set." This invariant is crucial for maintaining the integrity of the lock mechanism and ensuring that funds are locked for the intended duration.

Tools Used

Manual Audit

Recommended Mitigation Steps

The check for reducing the lock duration should consider both the lastLockTime and the current unlockTime.

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

This condition ensures that the new unlockTime calculated as lastLockTime + _duration is not earlier than the current unlockTime, effectively preventing users from reducing the lock duration for their existing locks.

Assessed type

Error