Incorrect Handling of ERC20 Transfers in Locking Mechanism

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The _lock function in the smart contract does not correctly handle the result of the transferFrom call when transferring ERC20 tokens. This oversight allows an attacker to lock tokens without actually transferring them to the contract, which can lead to unauthorized access and withdrawal of tokens from the protocol.

Proof of Concept

function _lock(
    address _tokenContract,
    uint256 _quantity,
    address _tokenOwner,
    address _lockRecipient
) private {
    // ... (other code)

    if (_tokenContract != address(0)) {
        IERC20 token = IERC20(_tokenContract);
@>      token.transferFrom(_tokenOwner, address(this), _quantity);
    }

    // ... (other code)
}

The function token.transferFrom(_tokenOwner, address(this), _quantity) is called without checking its return value. According to the ERC20 standard, transferFrom should return a boolean indicating the success of the transfer. If the transfer fails the function returns false, but the current implementation does not handle this case, and the _lock function proceeds to update the contract state as if the transfer succeeded.

Imagine this scenario:

1. An attacker can approve the contract to spend tokens from an account that does not hold sufficient tokens.
2. Call the lock function to lock the tokens.
3. The transferFrom call fails, but _lock function continues and incorrectly records the tokens as locked.
4. The attacker can later call the unlock function to withdraw tokens that were never actually transferred to the contract.

Tools Used

manual review

Recommended Mitigation Steps

Use the OpenZeppelin SafeERC20 library to ensure that all ERC20 token transfers are correctly handled. The SafeERC20 library provides wrappers around the ERC20 operations that handle cases where the token contract does not return a boolean or returns false on failure.

Assessed type

ERC20

[c4-judge]

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