`unlockTime` gets set incorrectly in `setLockDuration`

[c4-bot-3]

Lines of code

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

Vulnerability details

In the setLockDuration function in LockManager.sol, the protocol tries to ensure, that the player does not reduce the locking time of their tokens with the following check:

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

The problem arises when the unlockTime is then later set by adding _duration to the lastLockedTime and not block.timestamp:

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

Impact

Since lastLockTime is almost always lower than block.timestamp, it is possible to reduce the unlockTime. This causes the player to be able to wait a bit and then reduce the unlockTime due to the discrepancy between lastLockTime and block.timestamp. On the code4rena audit page the following is stated: The most important thing is that funds cannot get locked forever, people cannot take other people's funds, and that people cannot reduce lockup times that are previously set. which gets clearly violated by this, breaking one of the very important parts of the protocol.

Proof of Concept

Please create a file LockManager.t.sol in src/test/ and add the following contract:

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

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

contract LManager is MunchablesTest {
    function test_LockDuration() public {
        deployContracts();

        address[] memory addresses = new address[](1);
        addresses[0] = address(this);
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        // mint an NFT for me
        IOldMunchNFT(address(oldnftp)).safeMint(address(this), "Q");

        lm.getConfiguredToken(address(usdb));

        IERC20(usdb).balanceOf(address(this));
        IERC20(usdb).approve(address(lm), 1000000);

        lm.lock(address(usdb), 1000000);

        uint lastLockedBefore = lm.getLocked(address(this))[1].lockedToken.lastLockTime;
        uint unlockTimeBefore = lm.getLocked(address(this))[1].lockedToken.unlockTime;
        console.logUint(unlockTimeBefore);

        vm.warp(unlockTimeBefore/2);

        lm.setLockDuration(unlockTimeBefore - unlockTimeBefore/2);

        uint unlockTimeAfter = lm.getLocked(address(this))[1].lockedToken.unlockTime;
        console.logUint(unlockTimeAfter);

        vm.warp(block.timestamp + 5);

        console.logUint(block.timestamp);

        lm.unlock(address(usdb), 1000000);
    }
}

You can then test this with forge test --match-test test_LockDuration -vv. This will show that we can unlock our tokens way before the unlockTime has passed. The output will look somehow like this:

Logs:
  Created config storage
  Created account manager 0x2e234DAe75C793f67A35089C9d99245E1C58470b
  .
  .
  86401
  43202
  43205

As it will not revert we can see, we are able to unlock our tokens at the timestamp 43205 while the unlockTime initially is set to 86401.

Tools Used

Manual review, foundry

Recommended Mitigation Steps

The if-statement should be checked based on lastLockTime:

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

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

Assessed type

Invalid Validation