unlock function also uses notpaused modifier thus it can prevent the user to withdraw their tokens.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

unlock function uses notpaused modifier which makes it too restricted as there is no trusted role so even a malicious admin can pause the contract and thus preventing the users to withdraw their tokens.

Proof of Concept

Following is unlock function

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

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Now it is perfectly fine to apply notPaused modifier on the lock function but incase of unlock function it can cause a scenario when a malicious admin can pause the contract and thus preventing users to get back their tokens. This wouldn't be an issue if the admin was trusted but here there is no trusted role.

Even if the admin was trusted then also unlock function should not have notPaused function because it would prevent the users to get back their tokens even when their lock time has passed.In emergency situation the users should not be denied of their tokens.

Tools Used

Manual review

Recommended Mitigation Steps

remove the notPaused modifier

Assessed type

Context