howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Description

This has been reported by 4naly3er, but the impact would be more severe then Medium, which is why I still submit this report providing richer details about the issue.

Whenever a player wants to get their locked token back they will call LockManager::unlock. The current implementation present a flaw for players who locked native ETH token, as the contract will send those token back using payable.transfer function which restrict the operation with a limited amount of gas (Stipend currently at 2300).

Interacting with a smart contract is a little more expensive than an EOA, so if the locked token owner is a smart contract, there is always a chance in the future that if gas cost change on Blast which could make this operation to revert, preventing the player to unlock their ETH token which would be stuck forever (for all the players who locked ETH) in the contract, which seems to warrant High severity.

This is usuallly a Medium severity issue, but the fact that token would be stuck forever in the contract, which is a user funds loss, seems to justify the High severity.

Impacts

Player unable to unlock their ETH token if gas cost for opCall changes unfavorably

Recommended Mitigation Steps

Use call (and confirm the return flag) instead of transfer to send the ETH token back to the player. Reentrancy isn't a concern since the contract is already using nonReentrant modifier.

        if (_tokenContract == address(0)) {
-           payable(msg.sender).transfer(_quantity);
+           // solhint-disable-next-line avoid-low-level-calls
+           (bool sent, ) = msg.sender.call{value: _quantity}("");
+           require(sent, "Transfer failed");
        }

Assessed type

ETH-Transfer

c4-judge commented 3 months ago

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

dontonka commented 3 months ago

This submission and its duplicates outline an exhibit already reported by the static 4naly3er here: https://github.com/code-423n4/2024-05-munchables/blob/main/4naly3er-report.md#m-2-call-should-be-used-instead-of-transfer-on-an-address-payable

As such, I consider all relevant submissions to be out-of-scope as they are adequately labeled as HM by the analyzer.

Hello @alex-ppg, quoting your comment from #101 which my report is a duplicate of. My report clearly stated that this was reported by the bot but only as a Medium severity (not sure why you mention HM, it's only a M). As I understand the C4 rules, whenever you can proove a more severe impact then what the bot found, it is possible to be submitted and it should be. Is that not true anymore?

Here the impact of this issue are clearly more severe, which is why this was submitted. Granted it's a simple low hang fruit finding. Finally, if you change your judgement about this issue, please consider selecting my report as selected for report, as I think it's the more consice among all the dup.

alex-ppg commented 2 months ago

Hey @dontonka, thanks for your PJQA contribution! Please consult my response in a similar exhibit.

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