Lines of code

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

Vulnerability details

When a user calls unlock() ERC-20 tokens get transferred to him but the case where the token transfer does not succeed is not handled. His funds can get locked because of this.

Impact

Loss of funds

Proof of Concept

Some ERC-20 tokens do not revert on failiure but instead return false. This means that if the transfer fails nothing will happen but lockedToken.quantity will still get updated. This variable determines how many tokens a user can withdraw.

        lockedToken.quantity -= _quantity;

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

Tools Used

Manual Review

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20

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

Assessed type

ERC20

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

Unchecked ERC20 transfers can cause lock up #522