Transfer Assignment

Ethereum Smart Contract Programming 101
#745

Nice solution @mohkanaan :ok_hand:

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are also in the correct order to reduce security risk:

  1. check inputs (require statements)
  2. effects (update the contract state)
  3. external interactions

Just as you have done, itā€™s important to modify the contract state for the reduction in the balanceā€¦

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external wallet addressā€¦

msg.sender.transfer(amount);

ā€¦ just in case there is an attack after the transfer, but before the state is modified to reflect this operation. Youā€™ll learn about the type of attack this prevents, and how it does it, in the courses which follow this one. But itā€™s great youā€™re already getting into good habits in terms of smart contract security :+1:

Your transfer event and corresponding emit statement are also both correctly coded, and the emit statement will log appropriate data when the transfer() function is successfully executed.
But in general, an emit statement is probably better placed after an assert statement.

Donā€™t forget to either remove or comment out your addBalance function and event. As it is, the addBalance function allows users to artificially inflate their individual balances in the balance mapping. This would allow some users to withdraw more than their entitlement, whilst others would lose funds.

Just let me know if you have any questions.

#746

Thank you very much for youā€™re answer. Yes the closing bracket was a copy paste error :rofl:

2 Likes
#747

Very good, thank you very much for the explanation.
Iā€™ve altered the code, now itā€™s working as intended, and I have a better understanding of the payable functions. :+1:

pragma solidity 0.8.4;

contract Bank {
    
    mapping(address => uint) balance;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint){
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint){
        
        //require enough balance
        require(balance[msg.sender] >= amount, "Withdrawal balance insufficient");
        
        //reduce balance amount
        balance[msg.sender] -= amount;
        
        //make the transfer
        payable(msg.sender).transfer(amount);
        
        //return balance
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Transfer balance insufficient");
        require(msg.sender != recipient, "No self transfers");
    
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        // event logs and further checks
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}
1 Like
#748

Great! Iā€™m glad you got it working, and now understand the mechanics of ether value transfers much better.

All the code in your contract now looks correct :ok_hand:

#749

Thanks, @jon_m, for the review. I have few questions:

  • does the assert function adds extra cost (gas) to the operation?
  • Can I track using code the ether deposits of each address without keeping track of these deposits as we are doing in the code right now?
1 Like
#750

Hi @mohkanaan,

Yes it does - all Solidity code that is executed in a transaction will add to the total gas consumed by that transaction, and therefore the gas cost. Each low-level operation has an Op Code, and each Op Code consumes a fixed amount of gas. An assert() statement, like any other piece of Solidity code, will equate to a number of specific Op Codes.

As an overview, you may find the following article helpful, depending on how much you already know. It was written 3 years ago, but the fundamentals that it explains should be the same. There is a section on the gas fee schedule, and another on calculating gas costs with Ganache.

https://medium.com/coinmonks/understanding-gas-in-ethereum-53ad816f79ae

I assume you are referring to a frontend interface of a dapp, with a smart contract backendā€¦ Using a library such as web3.js, you can call the smart contractā€™s getter functions from the frontend to retrieve the latest balances; and you can also set up event listeners to capture the data logged by the events emitted by the transactions you want to track e.g. deposits, withdrawals etc. You will learn how to do this in the Ethereum Dapp Programming course.

1 Like
#751 
function withdraw(uint amount) public returns(uint){
        require(balance[msg.sender] >= amount, "not enough funds");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
    }
1 Like
#752 
function withdraw(uint amount) public returns (uint){
    require (balance[msg.sender] >= amount, ā€œNeed More MOOOLAā€);
    msg.sender .transfer(amount)
}
#753

hmmmm i looked at the solution i think i need to spend more time on this

1 Like
#754

Hi @jacobo,

You have added both of essential lines of code needed to solve the problem with the withdraw function :ok_hand:

However, notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as itā€™s been included in the function header, you should also include a return statement at the end of the function body. You should have got a yellow/orange compiler warning for this.

Also, have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

Just let me know if you have any questions :slight_smile:

#755

Hi @Chazzman22,

The require statement youā€™ve added is correct, but your withdraw function is missing 2 other lines of codeā€¦

  • If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the callerā€™s external wallet address receives the requested withdrawal amount, but their balance in the mapping is not reduced! Iā€™m sure you can see that this is another very serious bug!
    msg.sender.transfer(amount)ā€‚ transfers ether from the contract address balance to the callerā€™s external wallet address, but it doesnā€™t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each userā€™s share of the contractā€™s total ether balance. So, just as we increase a userā€™s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

  • Notice that the withdraw function header also includesā€‚returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as itā€™s been included in the function header, you should also include a return statement in the function body. The compiler will give you a yellow/orange warning for this ā€” once youā€™ve added the semi colon at the end of the second statement, which will give you an error first :wink:

Once youā€™ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Let me know if anything is unclear, or if you have any questions about these points :slight_smile:

1 Like
#756

Simply by requiring the address to have deposited more or equal the amount wanted to be withdrawn, the user cannot withdraw more Ether than he deposited. (reused the balance mapping)

    function withdraw(uint amount) public returns (uint){
 
        require(balance[msg.sender] >= amount, "User does not have sufficient Ether to withdraw");
        msg.sender.transfer(amount);
        
    }
#757

Hi @Nuno_Silva,

The require statement youā€™ve added is correct, but your withdraw function is missing 2 other lines of codeā€¦

(1) If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the callerā€™s external wallet address receives the requested withdrawal amount, but their balance in the mapping is not reduced (you can call getBalance to check this). This means that they can make repeat withdrawals ā€” each one no greater than their initial balance, which isnā€™t being reduced ā€” up to the total amount of ether held in the contract (the sum of all of the individual account holdersā€™ balances). So this is a very serious bug, and what you say hereā€¦

ā€¦isnā€™t true.

msg.sender.transfer(amount) transfers ether from the contract address balance to the callerā€™s external wallet address, but it doesnā€™t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each userā€™s share of the contractā€™s total ether balance. So, just as we increase a userā€™s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

(2) Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But as itā€™s been included in the function header, you should also include a return statement in the function body. The compiler will give you a yellow/orange warning for this.

Once youā€™ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Let me know if anything is unclear, or if you have any questions about these points :slight_smile:

#758

Thank you for the clarifications!

I didnā€™t thought about that problem and I didnā€™t test that possibility of multiple withdrawingsā€¦ :sweat_smile:

After taking a look at the solution, Iā€™ve changed the code to:

    function withdraw(uint amount) public onlyOwner {
        require(balance[msg.sender] >= amount, "User does not have sufficient Ether to withdraw");
        balance[msg.sender] -= amount;
        //Use the balance[msg.sender] -= amount before msg.sender.transfer(amount) to prevent further bugs.
        msg.sender.transfer(amount);
    }

Also deleted the return to make the code cleaner, as you sugested! :slight_smile:

1 Like
#759

Hello Jon! I noticed it after I had posted it :pensive:, thank you for the explanation too. Iā€™m about to check out the section on the security modification like you stated as well! Iā€™ll check back in with any questions, thanks again I appreciate the corrections!!

1 Like
#760

I modified the transfer function as follows:

    function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "balance not sufficient");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
    }
1 Like
#761 
  function withdraw(uint amount) public returns (uint){
        //transfer function has a built error handling
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender];
        // make sure it returns correct balance

}
#762 
function withdraw(uint amount) public returns(uint) {
	require(balance[msg.sender] >= amount);
	msg.sender.transfer(amount);
	return balance[msg.sender];
}
#763

Solution:

    function withdraw(uint amount) public {
        require(balance[msg.sender] >= amount, "You do not have enough funds");
        msg.sender.transfer(amount); 
        balance[msg.sender] -= amount;
    }
1 Like
#764

Hi @Nuno_Silva,

Apart from the onlyOwner modifier (which you donā€™t need to add), your solution is now correct :ok_hand:

By adding the onlyOwner modifier, only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, donā€™t you think? :sweat_smile: