Missing `transferFrom` check leading to locking exploit

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

In the LockManager::_lock function, the transferFrom() is used to actually transfer the tokens from the owner to the contract. According to the ERC20 standard, the transfer function "returns a boolean value indicating whether the operation succeeded" (OZ's Docs). If the transfer fails and returns false, the function will proceed as if the transfer was successful. The impact is that for an arbitrary ERC20 token, this transferFrom() call may return false but the _lock() logic will miss that, and assume it was successfully transferred into the contract and updates the lockedToken.quantity += _quantity accounting accordingly. This will result in the user locking tokens for free.

Proof of Concept

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

1. Alice wants to lock 10000 ERC20 tokens and calls the lock() function.
2. The transferFrom inside _lock fails due to exceeding the gas limit and returns false.
3. However, the return value is not checked, and the execution succeeds, adding the quantity into the lockedToken lockedToken.quantity += _quantity.
4. Alice successfully locked 10000 token for free.

Tools Used

Manual Review

Recommended Mitigation Steps

Check the success boolean of the transferFrom() call. Alternatively, use OZ’s SafeERC20’s safeTransferFrom() function.

Assessed type

Token-Transfer

[c4-judge]

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