hats-finance / ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4

Smart Contracts for Ether Fi dapp
1 stars 1 forks source link

The `LiquidityPool::requestWithdraw()` function is vulnerable to unchecked transfers, as it lacks verification of the return value in the external transferFrom call involving eETH. #43

Open hats-bug-reporter[bot] opened 11 months ago

hats-bug-reporter[bot] commented 11 months ago

Github username: @dappconsulting Twitter username: 0xSCSamurai Submission hash (on-chain): 0xaafb0409fd4bb808a559e0d6b4883f51d98cdaff1b5f04eb22c66ba772362072 Severity: high

Description: Description\

The LiquidityPool::requestWithdraw() function is vulnerable to unchecked transfers, as it lacks verification of the return value in the external transferFrom call involving eETH.

If the transfer silently fails for whatever reason, i.e. if no eETH is transferred from the msg.sender account, the function call won't revert, instead it will just return false, but the user will now get a free NFT that represents his withdraw request.

Result: user withdraws free tokens/funds, and protocol internal accounting is messed up.

Attack Scenario\

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional) : Recommendation attached.

Files:

seongyun-ko commented 11 months ago

Hi! Can you come up with any VALID scenario when eETH's transferFrom fails silently? Could you provide any test code to validate your concern?

I don't think eETH's transferFrom can fail silently.

dappconsulting commented 11 months ago

Hi. Thanks for your comment, I appreciate it. I see that I overlooked something. It's NOT possible to send zero eETH, I somehow missed that.

So right now without fuzz testing it I dont know how it can fail silently.

I think the standard recommendation is to either have the require check for the boolean value, or use safeTransferfrom, even if we're 100% sure it cannot/won't silently fail... So this now is probably my best argument.

Thanks for pointing it out.