Lines of code

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

Vulnerability details

Impact

The _lock function doesn't check the return value of transferFrom function when transferring ERC-20 tokens. Right after calling transferFrom, function updates the token quantity assuming that transferFrom returned true. However, for certain ERC20 tokens, if insufficient tokens are present, no revert occurs but a result of false is returned. So its important to check this. If you don't, you could mint NFTs without have received sufficient tokens to do so. So you could loose funds.

Please check the same finding from PoolTogether, Visor contests. https://code4rena.com/reports/2024-03-pooltogether#m-06-funds-locked-due-to-missing-transfer-check https://code4rena.com/reports/2021-05-visorfinance#m-01-unhandled-return-value-of-transferfrom-in-timelockerc20-could-lead-to-fund-loss-for-recipients

Proof of Concept

The issue is in the _lock function, where the transferFrom is called without checking its return value. Here is the relevant part of the code:

// Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

If the transferFrom function returns false, the contract will still proceed as if the transfer was successful.

            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;

Above you can see the next part of the function, added the _quantity to lockedToken.quantity without checking if the transfer was successful.

Tools Used

Manual Review

Recommended Mitigation Steps

use safeERC20, or add the following line as best practice. if (!success) revert TokenTransferFailedError();

Assessed type

ERC20

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

Missing return value check for transferFrom in _lock function #525