Lines of code

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

Vulnerability details

Impact

The LockManager contract maintains an internal accounting of the locked token balances for each user. If a token transfer fails silently during the unlock operation, the contract will update its internal accounting as if the transfer was successful, even though the tokens were not actually transferred back to the user's account. This will result in incorrect accounting of token balances within the contract, leading to discrepancies between the contract's internal state and the actual token balances.

The issue lies in the following lines: /LockManager.sol#L419-L423

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

The problem is that the transfer function of the IERC20 interface does not have a return value, which means that the success or failure of the token transfer is not checked.

If the token contract being used does not follow the ERC20 standard correctly and does not revert on failed transfers, the token transfer could fail silently, leading to potential loss of funds or incorrect accounting of token balances.

Proof of Concept

In the Ethereum ecosystem, the ERC20 token standard is widely adopted, and it defines a set of rules and functions that token contracts should implement. One of these functions is the transfer function, which is used to transfer tokens from the caller's account to another account.

According to the ERC20 standard, the transfer function should return a boolean value indicating whether the transfer was successful or not. However, some token contracts do not follow this standard correctly and do not return a value or revert the transaction in case of a failed transfer.

Example of a non-compliant ERC20 token contract that does not return a value from the transfer function:

pragma solidity ^0.8.0;

contract NonCompliantERC20 {
    mapping(address => uint256) public balances;

    function transfer(address _to, uint256 _value) public {
        // Perform some checks and update balances
        balances[msg.sender] -= _value;
        balances[_to] += _value;
        // No return value
    }
}

In the unlock function of the LockManager contract, the code assumes that the transfer function of the ERC20 token contract will either succeed or revert the transaction. However, with non-compliant token contracts like the one above, the transfer can fail silently without reverting the transaction.

Scenario that illustrates the potential impact:

Alice locks 100 tokens of a non-compliant ERC20 token in the LockManager contract. The LockManager contract updates its internal accounting, showing that Alice has 100 locked tokens. Later, Alice tries to unlock her tokens by calling the unlock function. The unlock function calls the transfer function of the non-compliant ERC20 token contract to transfer the tokens back to Alice's account. However, due to some internal error or condition in the token contract, the transfer fails silently without reverting the transaction. The LockManager contract updates its internal accounting, showing that Alice has 0 locked tokens, even though the tokens were not actually transferred back to her account.

In this scenario, Alice has effectively lost her tokens due to the silent failure of the token transfer. Additionally, the LockManager contract's internal accounting is now incorrect, as it shows that Alice has no locked tokens, even though the tokens are still locked in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the safeTransferand safeTransferFrom functions provided by OpenZeppelin's SafeERC20 library. These functions handle the return value checking and revert the transaction if the transfer fails.

Alternatively, you could manually check the return value of the transfer and transferFrom functions and handle the failure case appropriately.

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

// ...

function unlock(
    address _tokenContract,
    uint256 _quantity
) external notPaused nonReentrant {
    // ...

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

    // ...
}

Assessed type

Error

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

Silent Token Transfer Failure in LockManager Contract Leads to Incorrect Accounting and Potential Loss of Funds #528