Use of .transfer() Instead of safeTransfer

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description: The LockManager contract uses the .transfer() method to send ERC20 tokens, specifically in the _lock and unlock functions. The .transfer() method is known to have limitations when dealing with tokens that do not strictly adhere to the ERC20 standard or when dealing with contracts that implement custom logic in their transfer or fallback functions.

Impact: Some ERC20 tokens implement custom logic that could cause transfers to fail if not handled properly, potentially leading to locked funds.

Proof of Concept: The vulnerability is present in the 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();

    accountManager.forceHarvest(msg.sender);

    lockedToken.quantity -= _quantity;

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

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

Recommended Mitigation: To ensure compatibility and safety, replace .transfer() with OpenZeppelin’s safeTransfer method for ERC20 tokens.

Assessed type

ERC20

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope