Munchables - Myd's results

Lines of code

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

Vulnerability details

Impact

If a user calls setLockDuration multiple times without unlocking their tokens in between, the unlock time will be calculated incorrectly. This means that users may be able to unlock their tokens earlier or later than intended, violating the intended locking rules.

Potential Loss of Funds If the unlock time is calculated to be earlier than intended, users may be able to unlock and transfer their tokens before the intended lock period has expired. This could lead to potential loss of funds or assets that were meant to be locked for a specific duration.

The code responsible for updating the unlock time: LockManager.sol#L254-268

if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
    // check they are not setting lock time before current unlocktime
    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 problem is that the new unlock time is calculated based on the lastLockTime instead of the current block.timestamp. This means that if the user calls setLockDuration multiple times without unlocking their tokens in between, the unlock time will be calculated incorrectly.

For example, let's say the initial lock duration is set to 1 week, and the user calls setLockDuration again after 3 days to extend the lock duration to 2 weeks. The new unlock time should be block.timestamp + 2 weeks, but instead, it will be calculated as lastLockTime + 2 weeks, which is incorrect.

Proof of Concept

Alice calls setLockDuration(604800) (1 week) at block timestamp 1000. Her unlock time is set to 1000 + 604800 = 605800. Three days later, at block timestamp 1259200, Alice calls setLockDuration(1209600) (2 weeks) to extend her lock duration. According to the current implementation, the new unlock time is calculated as lastLockTime + 1209600 = 605800 + 1209600 = 1815400. However, the correct unlock time should be block.timestamp + 1209600 = 1259200 + 1209600 = 2468800.

As you can see, the calculated unlock time (1815400) is earlier than the intended unlock time (2468800), allowing Alice to unlock her tokens before the intended lock period has expired.

Tools Used

Manual Review

Recommended Mitigation Steps

The code should use the current block.timestamp instead of lastLockTime when calculating the new unlock time.

if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
    // check they are not setting lock time before current unlocktime
    if (
        uint32(block.timestamp) + uint32(_duration) <
        lockedTokens[msg.sender][tokenContract].unlockTime
    ) {
        revert LockDurationReducedError();
    }

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

Assessed type

Error