Lines of code

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

Vulnerability details

Impact

The LockManager::unlock function ERC20 tokens transfer without checking its return value. According to the ERC20 standard, the transfer function "returns a boolean value indicating whether the operation succeeded" (OZ's Docs). If the transfer fails and returns false, the function will proceed as if the transfer was successful, leading to a discrepancy between the contract's recorded state and the actual token balances. This can result in users not receiving their tokens while the contract incorrectly assumes the transfer succeeded, effectively causing a loss of access to those tokens for the user.

Proof of Concept

Let's assume that Alice had 1000 tokens locked in.
She proceeds to unlock these 1000 tokens.
token.transfer(msg.sender, 1000) silently fails and returns false.
However, the contract had already decreased lockedToken.quantity -= _quantity by 1000 tokens and proceeds as if the transfer succeeded.
Alice does not receive her 1000 ERC20 tokens, but the contract's state reflects that the tokens were transferred successfully.

At this point, Alice's assets are locked in the LockManager contract. They cannot be unlocked at a later point, because the corresponding lockedToken quantity is already deducted.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement OpenZeppelin's SafeERC20 safeTransfer function.

function unlock(
    address _tokenContract,
    uint256 _quantity
) external notPaused nonReentrant {
...
    // send token
    if (_tokenContract == address(0)) {
        payable(msg.sender).transfer(_quantity);
    } else {
        IERC20 token = IERC20(_tokenContract);
-        token.transfer(msg.sender, _quantity);
+       token.safeTransfer(msg.sender, _quantity);
    }

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

Assessed type

Token-Transfer

code-423n4 / 2024-05-munchables-findings

Funds locked due to missing transfer check #524