Transfer Assignment

You’re very welcome Marcus! :slightly_smiling_face:

Your modified return statement is now much more useful :ok_hand:

That’s actually a pretty understandable excuse! :sweat_smile: … yeh, that definitely makes learning to program smart contracts doubly challenging. Just remember, doing a little but often will be more productive than trying to do too much but infrequently.

That’s good that you’ve done the JavaScript course, and so you’ll definitely be OK to continue with this course. When you’re a beginner, lots of repetition and practice is important and not trying to cover too much too quickly. Going back over the JavaScript course will definitely be helpful. Experimenting with the code from lectures and assignments yourself can also be a very productive learning method. You can try coding as much yourself from memory as possible, and then start reworking, adapting and personalising the same syntax and patterns so that you end up with your own variations on the same theme. When you’re going back over the code, and experimenting with it, really try to actively think about what it’s doing. Also, have a good read of some of the other posts in these forum discussions (for additional information, clarification, confirmation and inspiration), and then ask questions if anything is still unclear.

require(balance [msg.sender] >= amount, “not enough funds”);
msg.sender.transfer (amount);

Hi @zlr1984,

The require statement you’ve added is correct, but your withdraw function is missing an essential line 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 address receives the requested withdrawal amount, but their balance in the mapping is not reduced! 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, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount)   transfers ether from the contract address balance to the caller’s external 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 original withdraw function header for this assignment contains  returns(uint) . Did you remove it from your solution? It’s not mandatory to include, and the function can still operate effectively without returning a value. But if you’ve included it in the function header, you should also include a return statement in the function body. The compiler would give you a yellow/orange warning about 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.

Please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

And post your full withdraw function, so we can see whether it is returning a value or not.

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

Thanks. I will check that.

1 Like

function withdraw(uint amount) public returns(uint){
require(balance[msg.sender] >= amount, “Not Enough Funds”);
msg.sender.transfer(amount);

Hi @josejalapeno

The require statement you’ve added is correct, but your withdraw function is missing an essential line 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 address receives the requested withdrawal amount, but their balance in the mapping is not reduced! 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, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external 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 contains returns(uint). It’s not mandatory to include, and the function can still operate effectively without returning a value. But if you’ve included it in the function header, you should also include a return statement in the function body. The compiler will have given you a yellow/orange warning about 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.

And don’t forget to format your code before posting it. This will make it clearer, easier to read, and will also help you to spot any syntax errors before posting — for example, you’ve missed a closing curly bracket at the end of your withdraw function. Formatting your code also makes it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

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

I saw your answer, its really difficult until right now for me to fully understand everything, I still don’t have any ideas what kind of code should I be putting.

Add a require and reduce balance:

function withdraw(uint amount) public returns (uint) {
   require(balance[msg.sender] >= amount, "Not enough funds!");
   msg.sender.transfer(amount);
   balance[msg.sender] -= amount;
   return balance[msg.sender];
}
1 Like

Hi @josejalapeno,

I’ll try to explain things in a simpler way …

Correct :white_check_mark:

This line of code only reduces the total contract balance (the total amount of ether held in the contract by all individual users). The caller is the address calling the withdraw function, in order to withdraw the ether amount. In Remix, you can see the caller’s external account balance in the Account field at the top of the Deploy & Run Transactions panel. Think of it as their wallet address. You will see their wallet’s ether balance increase by the withdrawal amount.

We also have to reduce this user’s balance in the mapping by the same amount. Think of the balances in the mapping as recording each user’s individual share of the total funds held in the contract. Your require statement checks the user’s balance in the mapping to see if they have enough to cover the requested amount. If we don’t reduce the user’s balance in the mapping each time they withdraw ether from the contract, then the require statement will no longer prevent the user from withdrawing too much.

// Look at this line in the deposit() function ...
balance[msg.sender] += msg.value;

This declares the data type of the value returned by the function as an unsigned integer (uint). But in order to actually return (output) a value, we need a return statement at the end of the function. The withdraw function can return the user’s reduced balance after the withdrawal, just like the deposit function returns the user’s increased balance after a deposit.

I hope that’s clearer, and easier to understand. If you’re still not sure what code to add to your function, then have a look at some of the other students’ solutions in this discussion topic.

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

Nice solution @Lennart_Lucas :ok_hand:

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

Now 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:

1 Like

Hi Jon, this '>= is the thing I am not understanding, in english this would be my balance as I am the msg.sender should be more than or equal to amount. meaning I should be able to withdraw more than or equal to the amount I deposited. Pls clarify it with me.

1 Like

Hi Jose,

Correct…

Your (the caller’s, msg.sender's) balance is referenced by   balance[msg.sender]
This code finds the msg.sender address in the balance mapping (the address is the key) and then references the value that is mapped to this key (the balance).

This is where you are confused. It’s the opposite …

  balance[msg.sender]           >=               amount
/* amount deposited      IS GREATER THAN     amount requested
                           OR EQUAL TO         to withdraw                */

If this is true, then msg.sender will be able to withdraw the amount requested, because their ether balance in the Bank contract is greater than this amount.

But if it is false, then this means their ether balance in the Bank contract is less than the amount requested; so, there is “Not Enough Funds”, require() will fail, and the transaction will revert.

Does that make sense now?

… and one further point, which is important for understanding why you need to add an extra line of code…

For the require() statement to work as it should (i.e. check whether msg.sender has enough funds to cover the withdrawal amount) then balance[msg.sender]  must always reflect the user’s current balance (their share of the ether deposited in the contract). The user’s balance in the mapping (balance[msg.sender]) will only be accurate if it is always increased when ether is deposited in the contract…

… and decreased when ether is withdrawn from the contract: this is the extra line of code you need to add to your withdraw function.

function withdraw(uint256 _amount) external {
  require(balance[msg.sender] >= _amount, "You do not have enough funds to withdraw");
        
  balance[msg.sender] -= _amount;
  payable(msg.sender).transfer(_amount);
}
1 Like
    function withdraw(uint amount) public returns(uint){
        require(balance[msg.sender] >= amount, "Error! Not enough ETH!");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
1 Like

An excellent solution @PaulS96 :muscle:

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body 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 individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external 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:

Keep up the great coding!

1 Like

Here is my go:

function withdraw(uint amount) public {
    require(balance[msg.sender] >= amount, "insufficient funds");
    payable(msg.sender).transfer(amount);
    balance[msg.sender] -= amount;
}

Quick question, does the order of the payable line and internal balance line matter much in production?

1 Like

Hi @MightyYak,

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

You are also right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.

Yes, it does… This is good question, and a very important one in terms of smart contract security. Best practice is for the order of the statements within the function body to follow the Checks-Effects-Interactions Pattern:

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

It’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

payable(msg.sender).transfer(amount);

… to guard against re-entrancy attacks. In this type of attack a malicious fallback function (in a receiving contract) would be able to make successful re-entrant calls to the withdraw() function, thereby allowing it to execute multiple withdrawals before its individual account balance is modified in the contract state to accurately reflect these operations (which would otherwise enable the initial require statement to fail and trigger revert).

In fact, the latest advice is not to use the address members transfer() or send(). The fixed stipend of 2300 gas, which they forward to a calling contract’s fallback function, used to be enough gas for the fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks). But since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient.

Instead of transfer() we can use the address member call() to perform the transfer of ether from the contract address balance to the external address …

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");

Using call() would normally pose a greater risk of re-entrancy attacks than using transfer() or send(), because it will forward all of the remaining gas (up to the gas limit set by the caller) to the fallback function. However, implementing the Checks-Effects-Interactions Pattern will guard against this, as I’ve described above.

You will find more background information about these issues in the following articles:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21

You’ll learn more about re-entrancy and other types of attacks, and some of coding techniques and smart-contract best practices we can implement to guard against them and reduce security risk, in the courses which follow this one: Ethereum Smart Contract Programming 201, and the Ethereum Smart Contract Security course. But it’s great that you’re already starting to consider these types of issues :+1:

Just let me know if you have any further questions.

    function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
1 Like

An excellent solution @belinox :muscle:

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body 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 individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, their entitlement) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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:

Keep up the great coding!

2 Likes