adding a requirement
require(msg.sender == owner)
require (balance[msg.sender] >= amount
Thank you for taking time out @jon_m , as always, itâs very much appreciated.
So that I can gather my thoughts on what you have suggested, the line of code for:
balance[msg.sender] -= amount;
is purely for the contract state to modify the reduction in the balance and it doesnât actually reduce the balance until
msg.sender.transfer(amount);
is True.
Is this correct?
Also, I did forget to add lines of code from the function which I only saw after checking out the solution. The compiler did ask me to change the the transfer function to view only access before I checked the solution, so apologies for that.
Still learning, but gaining knowledge from the feedback, so thank you again
1 Like
Hey Denis,
Itâs to modify the individual userâs balance, which is stored in the mapping and part of the contract state at any point in time.
This line of code is what actually transfers the ether out of the smart contract to the userâs external wallet address. This reduces the smart contractâs address balance which is also part of the contract state, but corresponds to the total âpooledâ funds for all users who have ether held in the Bank contract. This is not the balance which is stored in the mapping. Each individual userâs share of these âpooledâ funds changes every time they are involved in a transaction. If we donât also keep track of each userâs specific share of the total contract balance (âpooledâ funds), we will have no way of knowing how much they are entitled to withdraw.
When the individual userâs balance in the mapping is adjusted, this is purely an internal adjustment, and so does not expose the contract to the risk of any kind of malicious attack from an external source. Thatâs why itâs important to adjust this first before the external interaction occurs (the transfer of the actual ether between two contract account balances e.g. Bank and the userâs wallet address). The internal accounting adjustment that we make first doesnât actually involve the transfer of ether, but it plays a critical role in ensuring that individual users cannot withdraw more than their entitled share of the total âpooledâ funds. If âmsg.sender.transfer(amount)â is executed first, then an attacker could potentially find a way to call the withdraw function multiple times in rapid succession, before their individual balance is reduced enough to cause the require statement to fail and prevent this from happening.
require(balance[msg.sender] >= amount);
Yes⌠if the userâs individual balance in the mappingâISâ>=âthe requested withdrawal amount, then the condition in the require statement will evaluate to true. This allows the next line of code to execute, which reduces the userâs individual balance in the mapping by the requested withdrawal amount.
balance[msg.sender] -= amount;
In contrast ⌠if the userâs individual balance in the mappingâIS NOTâ>=âthe requested withdrawal amount, then the condition in the require statement will evaluate to false. This causes execution of the function to revert, and so the next line of code isnât reached and the userâs individual balance in the mapping isnât reduced by the requested withdrawal amount (which it canât be anyway because it isnât big enough).
I hope that makes things clearer. Itâs normal for these concepts to take a while to fully grasp, especially if you like to think deeply and analyse things fully. It may mean you move more slowly through the material, but in the long run you will achieve more depth to your understanding and a more solid foundation on which to build and progress to more advanced aspects 
Donât worry â these things happen. It does actually show that you are taking note, and working hard at understanding what the compiler warnings and error messages mean. This is a very important skill and technique to develop, and even though this time it caused you to change something that didnât need changing, in the long run the benefits will far outweigh anything negative.
Just let me know if anything is still unclear, or if you have any further questions. 
1 Like
pragma solidity 0.8.1;
contract HelloWorld {
mapping(address => uint) balance;
address owner;
event depositDone(uint amount, address depositedTo);
constructor(){
owner == msg.sender;
}
modifier onlyOwner {
require(msg.sender == owner);
_;
}
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 onlyOwner returns (uint) {
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
}
function getBalance() public view returns(uint){
return balance[msg.sender];
}
function transfer(address recipient, uint amount) public {
require(balance[msg.sender] >= amount, âBalance is not sufficientâ);
require(msg.sender != recipient, âTransfer failedâ);
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
âŚItâs normal for these concepts to take a while to fully grasp, especially if you like to think deeply and analyse things fully. It may mean you move more slowly through the material, but in the long run you will achieve more depth to your understanding and a more solid foundation on which to build and progress to more advanced aspects
You hit the nail on the head with this @jon_m . Rarely do I find it easy to pick things up and grasp them fully, so I like to take my time, analyse, learn, re-analyse and then try to figure it out.
Not always the easiest, and sometimes even doing this it doesn't "fit" as one would think. However, persevarence, dedication and asking questions (even ones that might seem basic), seem to help especially for a subject that I'm truly interested in. The only other moment I've had this would be when learning to drum, or learning a new language. Literally years to accomplish a standard I think is decent, but as with everything, there's always more to learn. I will keep rereading your info, and hopefully I'll get a lightbulb moment :upside_down_face:
Thanks as always :+1:
1 Like
Hey Denis,
From my own experience, the light bulb moment for many of these concepts usually comes later on, after âparkingâ the issue and moving on to other things. It depends on each individual, but some concepts only start to make sense when other pieces of the jigsaw have also fallen into place. It is good to review, re-read, experiment, and turn things over in your mind, but with some things we will nearly always be left with that unsettling feeling that âsomething is still missingâ. And that could be that you, personally, need to learn about another concept first, or need to experience the concept youâre struggling with in the context of a different exercise or project. If you find yourself going round in circles too much, then make a note of it and try to move on.
Whatâs also interesting is that sometimes the opposite happens. A concept that you think you have understood perfectly well, can suddenly confuse you again when you see or use it in a different context. So often, knowing enough to be able to move on productively should be enough, because moving on provides you with more practice, more examples, and exposes you to more coding situations, which may be what you need to understand something in a more satisfactory and convincing way.
1 Like
An excellent solution @mike_sutanto
You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:
- check inputs (require statements)
- effects (update the contract state)
- 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 later courses. But itâs great youâre already getting into good habits in terms of smart contract security 
Let me know if you have any questions
Hi @Ryan_Moore,
This is the correct require statement (but donât forget the semi colon at the end of the statement, otherwise it wonât compile).
We already have the onlyOwner modifier, which enables this require statement to be applied to any function it modifies. However, adding this require statement to the withdraw function means that only the contract owner can withdraw funds up to their own individual balance. But the Bank 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 will be able to withdraw their funds as well, donât you think? 
Can we see your complete code? ⌠To solve the problem with the withdraw function you need to add more lines than just the require statement.
If youâre not sure what else needs adding, have a look at some of the other studentsâ solutions, and the feedback and comments theyâve received, here in this discussion topic. Just let me know if anything is unclear, or if you have any questions
Hi @BShehaj,
The require statement youâve added is correct 
but you are missing 2 other important lines of code in your withdraw function. There are also a couple of other issues with your solution:
-
If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the requested withdrawal amount is transferred from the contract address to the callerâs external wallet address, but the callerâs balance in the mapping is not reduced.
<address payable>.transfer(uint256 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 into the contract, we need to do the opposite whenever they make a withdrawal.
So, this is also a very serious bug â have a think about what serious consequences it could lead to if not corrected. -
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. You should have got a yellow/orange compiler warning for this. -
Youâve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner can withdraw funds. 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?
-
You are using Solidity v0.8. Thatâs absolutely fine, but then you also need to bear in mind that this course material is based on v0.7 syntax. The compiler will always generate an error when your syntax needs to be changed for v0.8, and the error message will tell you why, and often what you should change it to. As well as the warning, I mentioned above, the compiler will have highlighted an error (in red) with this line of codeâŚ
ââmsg.sender.transfer(amount);
In order to receive ether, an address needs to bepayable. In versions prior to v0.8msg.senderis payable by default and so doesnât need to be explicitly converted. However, from v0.8msg.senderis non-payable by default, and so when it needs to refer to a payable address, it needs to be explicitly converted. One of the ways to do this is usingâpayable()âe.g.
ââpayable(msg.sender).transfer(amount);
If you make this modification, you will notice that the compiler error disappears.
Once youâve had a go at making the above modifications, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
Also, please format all of your code before posting it, not just part of it. This will make it more readable and easier to review.
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
Again @jon_m all of this is absolutely true. Unfortunately, I seem to get to the point of struggling before I ask for assistance or move on to another concept, and I try too hard (sometimes burning myself out or stressing myself out) before I either quit, walk away or throw my toys out the pram 
I really do hope that I can maintain composure and learn plenty with these blockchain development courses / programming courses as I really would love to do this as a career in the future.
At the moment, it feels so far away, but I keep practising everyday (even for 10-20 minutes) just to digest and understand what I have learned from the videos.
Thank you again Jon for your assistance. Very much appreciated 
1 Like
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, "Not sufficient!!!");
balance[msg.sender] -= amount;
payable(msg.sender).transfer(amount);
return balance[msg.sender];
}
This is the improved function with all the fixed errors and style. Thank you for the help!!
1 Like
%function withdraw (uint amount) public returns(uint){
// check for stored amount
require(Balance[msg.sender] >= amount , 'Withdrawal is more than a deposit');
// Storing old balance for assert statement
uint oldBlance = Balance[msg.sender];
// calling transfer function
msg.sender.transfer(amount);
// On successful transfer, changing balance
Balance[msg.sender] -= amount;
// Check for invariant
assert(Balance[msg.sender] == oldBlance - amount);
// returning balance of the caller
return Balance[msg.sender];
}
1 Like
Nice solution @tanu_g
You have added all of the additional lines of code needed to solve the problem with the withdraw function. Your additional assert statement is also appropriate and well coded.
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
function withdraw(uint amount) public payable returns(uint) {
require(balance[msg.sender] >= amount, "Insufficient balance!");
uint balanceBeforeWithdraw = balance[msg.sender];
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
assert(balance[msg.sender] == balanceBeforeWithdraw - amount);
return balance[msg.sender];
}
1 Like
pragma solidity 0.7.5;
contract Bank_TransferFunctionExample{
mapping(address => uint) balance;
address owner;
event depositDone (uint amount, address depositedTo);
event balanceTransfered (uint amount, address transferedTo);
modifier onlyOwner{
require(msg.sender == owner);
_;
}
constructor(){
owner = msg.sender;
}
function deposit() public payable returns (uint) {
balance[msg.sender] += msg.value;
emit depositDone(msg.value, msg.sender);
return balance[msg.sender];
}
//New code here; new withdraw function
//Adding onlyOwner to function header, a require() condition, user balance update after withdrawl, and a return statement to update balance
//This prevents someone from withdrawing someone elses deposits
function withdraw(uint amount) public onlyOwner returns (uint) {
require(balance[msg.sender] >= amount);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
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, "Insufficent Balance");
require(msg.sender != recipient, "You cannot send funds to yourself");
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
emit balanceTransfered(amount, to);
}
}
Looks like onlyOwner was not needed but Iâll leave it as is since it works and I didnât catch it until after.
1 Like
Here is my solution.
Iâm curious about something. Iâm not sure I understand the difference between msg.value and the amount of token one would like to deposit/withdraw if it wasnât ETH.
In my solution below I call this xToken.
Is there a way for msg.value to be another sort of Token?
Would the withdraw/deposit functions takes args instead of reusing msg.value? @filip mixed both options in the previous video so Iâm a little bit confused.
Thanks for your insights!
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.7.0 <0.9.0;
/**
* @title Storage
* @dev Store & retrieve value in a variable
*/
contract Storage {
mapping (address => uint) xTokenBalances;
event moneyDeposited(uint);
event moneyWithdrawn(uint);
function deposit () public payable returns (uint){
xTokenBalances[msg.sender]+=msg.value;
emit moneyDeposited(msg.value);
return xTokenBalances[msg.sender];
}
function withdraw () public payable returns (uint){
require(xTokenBalances[msg.sender]>=msg.value,"Not enough X Token available");
payable(msg.sender).transfer(msg.value);
xTokenBalances[msg.sender]-=msg.value;
emit moneyWithdrawn(msg.value);
return xTokenBalances[msg.sender];
}
}
Nice solution @Woland
You have added all of the additional lines of code needed to solve the problem with the withdraw function. Your additional assert statement is also appropriate and well coded.
However, it is only the address receiving the ether which needs to be payable â not the withdraw function itself. A function only needs to be marked payable when it needs to receive (not send) ether. So, unlike the deposit function, we donât want to mark the withdraw function as payable in its function header. Marking it as payable doesnât prevent your code from compiling, or the function from executing, but it is bad practice and poor risk management not to restrict a function to non-payable if it doesnât need to receive ether, because if someone sends ether by mistake, this amount will still be transferred to the contract address, but wonât be added to the individual account balance of the user who transferred it, which means the user may experience difficulties in having the amount refunded.
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.
Let me know if anything is unclear, or if you have any questions
1 Like
Hi @Dustin_Gearhart,
You have added all of the additional lines of code needed to solve the problem with the withdraw function and they are in the correct order to reduce security risk:
- check inputs (require statements)
- effects (update the contract state)
- 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 later courses. But itâs great youâre already getting into good habits in terms of smart contract security
If you think it works with onlyOwner then you havenât been thorough enough with your testing 
The onlyOwner modifier restricts access to the withdraw function to the contract owner, meaning that 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 will be able to withdraw their funds as well
You wouldnât have noticed this if you only tested the withdraw function using the contract owner address to make withdrawals. This is an important lesson to learn regarding how important it is to test your completed solution under a variety of different scenarios to see if it performs as expected in all of them.
Just a couple of other minor points regarding your comments in your code above the withdraw functionâŚ
For the reasons Iâve explained above, the userâs individual balance in the mapping should be updated before the actual withdrawal of funds occurs. Your code does perform these steps in the correct order, but your comment doesnât reflect what happens in your code.
The return statement doesnât update the userâs balance (or the contract balance). It simply returns the userâs reduced balance after the withdrawal.
Let me know if anything is unclear, or if you have any questions
1 Like
That was incredibly informative! Thank you vey much for taking the time. The deeper understanding of whatâs happening beneath the surface is definitely still fuzzy but points of clarification like that really help shape my understanding.
1 Like