No checks for return value of token transfer

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

In the unlock() and _lock() functions in LockManager.sol, the user-specified token is transfered to/from the user. This token.transfer/token.transferFrom call's return value is not checked. In the Q&A section on the code4rena description page the following is stated: ERC20 used by the protocol: USDB, WETH, (assume we can add more to the future for use in LockManager). Assuming any token might be added to the protocol, also tokens like ZRX might be added. Since the returnvalue of the transfer/tranferFrom calls is not checked as mentioned above, this will lead to problems if such tokens are used.

Impact

Since some tokens like ZRX do not revert on failure when transfering them, users would be able to steal tokens from other users and users' tokens may be locked in the protocol. Assuming the protocol supports ZRX in the future, if a user locks ZRX in the contract by calling the lock function, the user could not approve these tokens. In that case the token.transferFrom call will fail but not revert. Therefore the user's lockedToken.quantity will be increased but no tokens are transfered to the contract. If now another user transfers tokens into the protocol, the first malicious user can call unlock and since now tokens are in the protocol, they will receive those tokens without ever having depsosited any.

Other scenario: If a user tries to unlock their tokens and the token.transfer call fails, this will not be noticed. As the lockedToken.quantity is decreased, and the tokens are not transfered to the user but the functioncall ends successfully, these tokens will be locked in the protocol, causing the user to lose their tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Add another if-statement like the following, ensuring that the transfer was successful: (also add a check for the call in _lock)

[...]
} else {
    IERC20 token = IERC20(_tokenContract);
    if (!token.transfer(msg.sender, _quantity))
    {
      revert TransferFailed(...);
    }
}
[...]

## Assessed type

ERC20

[c4-judge]

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