Munchables - fyamf's results

Lines of code

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

Vulnerability details

Impact

An attacker can prevent an user from unlocking just by locking a small amount (or zero) on behalf of the user to update its unlockTime. If the attacke does so just before the user is going to unlock, its unlockTime updates to a larger value, so the user should wait again.

Proof of Concept

The attacker calls the function lockOnBehalf by providing just small (or zero) quantity of the token that the user has already locked on.

    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
            lockRecipient = _onBehalfOf;
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

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

By doing so, the attacker does not transfer anything, just pays gas fee, but it will update the user's lastLockTime, unlockTime, and lockDuration.

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

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

So, for example, suppose the user's token unlockTime is passed, and now he is going to unlock it. The attacker locks on behalf of the user small (or zero) quantity. By doing so, the user's token unlockTime is delayed by _lockDuration (which is at least equal to minLockDuration). Thus, the user should wait for another _lockDuration to be able to unlock it.

Moreover, if the user has set his lockDuration to zero. By applying this attack, the attacker can update the user's lockDuration to minLockDuration.

Tools Used

Recommended Mitigation Steps

It should be enforced that the quantity be larger than a minimum amount. So, the attacker must pay the minimum amount to apply this attack. Thus, this attack will be more expensive for the attacker.

    struct ConfiguredToken {
        uint256 usdPrice;
        uint256 nftCost;
        uint8 decimals;
        bool active;
        uint256 minimum;
    }

    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
            lockRecipient = _onBehalfOf;
            require(_quantity >= configuredTokens[_tokenContract].minimum, "quantity is smaller than the minimum.");
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

A malicious user can reduce the unlockTime maliciously, due to the wrong implementation of the function setLockDuration.

Proof of Concept

Suppose after locking, we have the following timing. Where, unlockTime = lastLockTime + _lockDuration.

• lastLockTime = 86_400 = day 1
• _lockDuration = 7 * 24 * 60 * 60 = 604_800 = 7 days
• unlockTime = 86_400 + 604_800 = 691_200 = day 8

--------------|--------------------------------------------------|------------->
        lastLockTime                                        unlockTime

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        //.....
        uint32 _lockDuration = playerSettings[_lockRecipient].lockDuration;
        //.....
        lockedToken.lastLockTime = uint32(block.timestamp);
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);
        //.....
    }

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

The malicious user (lockRecipient) can reduce the unlock time by calling the function setLockDuration. In this function, the new unlockTime is incorrectly calculated as lastLockTime + uint32(_duration).

    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);
    }

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

Suppose the malicious user at day 3 (block.timestamp = 86_400 * 3) calls this function with _duration = 86_400 * 5 = 5 days as a parameter. By doing so, the following condition is passed, because day 3 + 5 days = day 8 = unlockTime.

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

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

But, the lockedTokens[msg.sender][tokenContract].unlockTime is set to lastLockTime + uint32(_duration) = day 1 + 5 days = day 6. While the unlockTime was scheduled at day 8 before. So, it is reduced by two days.

            day 1           day 3           day 6               day 8
--------------|--------------|-----------------|------------------|-------------> t
        lastLockTime        now         newUnlockTime       unlockTime

If the attacker repeats this many times, the unlockTime will reduce further, even less than minLockDuration or even equal to lastLockTime. For example, the attacker can apply this attack by calling the function reduceUnlockTime in the following complete example:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

contract Test {
    error LockDurationReducedError();

    struct LockedToken {
        uint256 quantity;
        uint256 remainder;
        uint32 lastLockTime;
        uint32 unlockTime;
    }
    mapping(address => mapping(address => LockedToken)) public lockedTokens;

    // simple lock function
    function lock() public {
        LockedToken memory lockedToken = LockedToken({
            quantity: 0,
            remainder: 0,
            lastLockTime: uint32(block.timestamp),
            unlockTime: uint32(block.timestamp) + 8 days
        });
        lockedTokens[msg.sender][address(1)] = lockedToken;
    }

    // simple set lock duration function
    function setLockDuration(uint256 _duration) public {
        if (
            uint32(block.timestamp) + uint32(_duration) <
            lockedTokens[msg.sender][address(1)].unlockTime
        ) {
            revert LockDurationReducedError();
        }

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

    // if the attacker calls this function, it will reduce the unlockTime maliciously
    function reduceUnlockTime() public {
        uint256 duration;
        while (
            lockedTokens[msg.sender][address(1)].unlockTime !=
            lockedTokens[msg.sender][address(1)].lastLockTime
        ) {
            if (
                lockedTokens[msg.sender][address(1)].unlockTime >
                block.timestamp
            ) {
                duration =
                    lockedTokens[msg.sender][address(1)].unlockTime -
                    block.timestamp;
            } else {
                duration = 0;
            }

            setLockDuration(duration); // this will set the unlockTime to lastLockTime + duration
        }
    }
}

Tools Used

Recommended Mitigation Steps

The code should be modified as:

    function setLockDuration(uint256 _duration) external notPaused {
        //......
        lockedTokens[msg.sender][tokenContract].unlockTime =
            uint32(block.timestamp) +
            uint32(_duration);
        //.....
    }

Assessed type

Context