function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
}
function transfer(address recipient, uint amount) public {
require(balance[msg.sender] >= amount, “Balance not enough”);
require(msg.sender != recipient, “Not for yourself”);
}
This will be my lines added
Hi @c1cag1b3,
You have added both of the essential lines of code needed to solve the problem with the withdraw function 
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. The compiler should have given you an orange warning for this.
The other issues you have with your solution concern the order of the statements within your withdraw() function body…
-
If an input value needs to be checked or validated with a require() statement, then this should normally be the first line of code in the function body to be executed. If require() fails, then the transaction will revert wherever it is placed; but any code placed before require(), and which will already have been executed before require() triggers revert, will result in less gas being refunded than if the check had been the first operation to be performed within the function body. This is because only the gas cost associated with the unexecuted operations after require() is saved; any gas already consumed executing code for the operations before and including require() is not refunded. You can probably see, therefore, that the earlier in the function
require()is placed, the less the cost of gas will be if it throws and the transaction reverts. -
Have a look at this post which explains an important security consideration in terms of the order of the other two statements within your withdraw() function body.
See if you can reorder your code for these points, and also make the necessary modification in terms of returning, or not returning, the user’s new balance after the withdrawal.
Just one final comment …
You have a duplication of the assert() statement in your transfer() function. I’m sure this is unintentional, but because the 1st assert statement is in the wrong position it will throw an error and trigger revert whenever the function is called. If you remove it, then your transfer() function will work again as it should.
Let me know if you have any questions 
Hi @frandolz,
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 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 have given you an 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.
Also, 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, in order to provide you with any help you might need.
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
This function the owner who deploy contract can withdraw from his address but not allow to withdraw from other people and amount of money to withdraw need to be less than total balance.
address owner;
event withdrawSuccess (uint money);
modifier onlyOwner {
//check the person who can add balance is only owner do
require(msg.sender == owner, "only the owner can add money/withdraw");
_; //run the function
}
Here is the assignment
function withdraw(uint amount) public onlyOwner returns (uint){
//deposit 1 ether and withdraw 10 wei
//check that balance of owner is more than amount of withdraw
require(balance[msg.sender] >= amount, "Your balance is insufficient!!");
balance[msg.sender] = balance[msg.sender] - amount;
msg.sender.transfer(amount); //withdraw from sender in wei
emit withdrawSuccess(amount);
return balance[msg.sender];
}
1 Like
pragma solidity 0.7.5;
contract Bank {
mapping(address => uint) balance;
event depositDone(address indexed from, uint amount);
event withdrawDone(address indexed to, uint amount);
function deposit() public payable {
balance[msg.sender] += msg.value;
emit depositDone(msg.sender, msg.value);
}
function withdraw(uint _amount) public {
require(balance[msg.sender] >= _amount, "balance not sufficient");
_withdraw(msg.sender, _amount);
emit withdrawDone(msg.sender, _amount);
}
function _withdraw(address payable _to, uint _amount) private {
balance[_to] -= _amount;
_to.transfer(_amount);
}
function getBalance() public view returns(uint) {
return balance[msg.sender];
}
}
1 Like
Hi @thanasornsawan,
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 
- Check inputs (require statements)
- Effects (update the contract state)
- External Interactions (e.g.
transfermethod)
Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…
before actually transferring the funds out of the contract to the external address…
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, the share of the total contract balance they are entitled to withdraw) 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 
Some additional comments …
(1) Have you also restricted access to the deposit() function and the transfer() function to the contract owner with the onlyOwner modifier? If you have, then also adding the onlyOwner modifier to the withdraw() function header makes sense, as there will be no other users with individual balances to withdraw. However, this would be a very limited Bank contract if there can only ever be one user, the contract owner, and would also mean we wouldn’t need the balance mapping to store multiple user balances.
Instead, the aim of our contract is to simulate a bank with bank account holders, with multiple addresses being able to deposit funds and make internal transfers between themselves. This means that we don’t want to restrict access to any of our functions to the contract owner, including the withdraw() function, because while the contract is still deployed it only seems fair that all users should be able to withdraw their funds 
(2) Your withdraw event and corresponding emit statement are both coded correctly. The emit statement is in the correct position in the withdraw() function body and will log the withdrawal amount when the withdraw() function is successfully executed. Is the reason why the event is only logging the withdrawal amount, and not the withdrawer’s address, because only the contract owner can make withdrawals? If, for the reasons I’ve mentioned above, we allow multiple users to withdraw funds, you will want your withdraw event to also log the withdrawer’s address, in the same way that our deposit event logs both the amount deposited and the depositor’s address.
(3)
This line of code withdraws the wei amount from the contract’s ether balance and transfers it to the caller’s (msg.sender's) external address balance (showing in the Account field in the Deploy & Run Transactions panel in Remix). It’s important to understand that the caller’s (msg.sender's) individual balance stored in the mapping records and tracks their individual share of the total contract address balance (effectively, their entitlement to withdraw ether from the contract balance). The individual user’s balance in the mapping is adjusted to reflect changes in their share of the total contract balance, but isn’t actually where the ether is transferred out of the contract from.
Also, don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
Let me know if anything is unclear, or if you have any questions about any of these points
1 Like
This is an excellent solution @r0bz078
However, I couldn’t help noticing that it’s exactly the same (down to all of the identifiers used) as this student’s solution. Is this where you gained your inspiration? It’s a very good idea, and I very much encourage checking your own solutions and getting additional help and ideas from other students’ solutions, and the feedback they’ve received, posted here in the forum. However, you’ll benefit more from the course if you post your own solution, perhaps enhanced, or corrected, based on other solutions you’ve seen, even you think someone else’s is better. That way, you’ll receive valuable feedback about your own code.
I apologise, if it really is just a coincidence that both of these excellent and creative solutions are exactly the same, and you’ve both come up with the same ideas independently! Do let me know if you have.
Anyway, here’s a link to the feedback I gave the other student, because it also applies to the solution you’ve posted.
Also, don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
Just let me know if you have any questions
function withdraw(uint amount) public payable returns (uint){
require(amount <= balance[msg.sender]);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance[msg.sender];
}
1 Like
@jon_m thank you for your comment
- I just know that .transfer(amount) help prevent re-entrancy attack from your comment.I will study more in the “smart contract security” course as you mention.
- I didn’t restrict access to the deposit() function and the transfer() function to be only owner.
In the first time, I understand that anyone can transfer or deposit to our address but if withdraw should be only owner.That’s why I put onlyOwner at withdraw() function.
I just want to make function that look like normal wallet that have only one address per one owner.Because now I use address A deploy contract but I also can deposit to any accounts and withdraw from any accounts if that account has balance more than zero.
Thank you for help correct me that I understand wrong to put onlyOwner modifier on the withdraw function.
- About withdraw event, I totally understand wrong after read your commend and try check debug “address from” and “address to” again in console.
When usemsg.sender.transfer(amount);,it is withdraw from any address that we call it and transfer amount to the balance of contract.
for example: In remix I use address ‘0x5B38Da6a701c568545dCfcB03FcB875f56beddC4’ to deploy contract and get the contract address ‘0x7EF2e0048f5bAeDe046f6BF797943daF4ED8CB47’ and then I try withdraw from address ‘0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2’.It should be refund back to the address ‘0x5B38…dC4’ but actually when withdraw from any address it will always refund money to the contract address ‘0x7EF…B47’.
I understand concept of msg.sender wrong.That’s why I put only amount because we know msg.sender is this person but really thank you for your comment.I will put more detail about withdraw from which address in the log too.
event withdrawSuccess (address indexed withdrawFrom, uint money);
function withdraw(uint amount) public returns (uint){
//deposit 1 ether and withdraw 10 wei
require(balance[msg.sender] >= amount, "Your balance is insufficient!!");
balance[msg.sender] = balance[msg.sender] - amount;
msg.sender.transfer(amount); //withdraw from sender in wei
//msg.sender can be anyone that we call to withdraw money.
//the amount of money will always return to the balnce of contact address
emit withdrawSuccess(msg.sender, amount);
return balance[msg.sender];
}
1 Like
Hi @thanasornsawan,
Ok … so, your modified withdraw() function without the onlyOwner modifier is now much better This is because any address can now call the withdraw() function and be the msg.sender, and withdraw any ether from the contract to which they are entitled. The ether an address is entitled to withdraw will be their individual balance recorded in the mapping (mapped to their address), which they will have if they have either (i) called the deposit() function and transferred ether into the contract, or (ii) been the recipient of an internal transfer made by another address calling the transfer() function.
Your withdraw event declaration, and corresponding emit statement, have also been correctly updated and will now log the withdrawer’s address as well as the withdrawal amount
Below are a few corrections to some of your comments. It may be that you do understand correctly, but have just used the wrong words in a few places. Apologies, if this is the case, but I thought it best to just make sure …
This line of code deducts the wei amount FROM the contract address’s ether balance and transfers and adds it TO the msg.sender's external address balance (the address showing in the Account field in the Deploy & Run Transactions panel in Remix).
The withdrawal amount, which is deducted from the contract address’s ether balance, is also deducted from msg.sender's individual balance stored in the mapping. This is to correctly adjust their individual share of the total contract address balance for the amount they’ve withdrawn.
As you know, the individual balances stored in the mapping can be retrieved by calling getBalance(). You can retrieve the total amount of ether stored in the contract (the contract address balance) by adding the following function to your contract …
function getContractBalance() public view returns(uint) {
return address(this).balance;
}
After the withdraw() function is called, if you call getContractBalance() you will see that the contract ether balance has reduced by the withdrawal amount. The address which called the withdraw() function will have the same withdrawal amount added to their external address balance (showing in the Account field in the Deploy & Run Transactions panel in Remix).
As a result of what I’ve just explained, have a look at the following changes (in capital letters) that I’ve made to a couple of the comments you’ve added to your code …
Now, in terms of your example …
… here, I understand that address 0xAb8…cb2 already has a positive individual balance in the mapping, and is now calling the withdraw() function to withdraw an amount less than or equal to their individual balance in the mapping.
No … it shouldn’t be transferred to the address that deployed the contract (0x5B3…dC4) or to the smart contract address (0x7EF…B47). It should be transferred out of the smart contract (deducted from 0x7EF…B47) and added to the external account balance of 0xAb8…cb2, the address that called the withdraw() function (the msg.sender).
I think you might find the following two posts helpful …
- This post lays out step-by-step instructions of how to properly check that all of your functions are working correctly, and it details what you should see happening and where, if it is.
- This post contains some useful information about what specific operations each of the different functions (deposit, withdraw and transfer) are performing, and how the different balances are affected (or not affected) by each.
Anyway, I hope this is helpful. Just let me know if anything is unclear, or if you have any questions
Hi @firequo,
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 …
- Check inputs (require statements)
- Effects (update the contract state)
- External Interactions (e.g.
transfermethod)
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, the share of the total contract balance they are entitled to withdraw) 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
However, we should not mark the withdraw function as payable, because it doesn’t need to receive ether from an external address in order to update the contract balance. If we make it payable, then, if the caller sends an ether value to the withdraw function by mistake (as well as calling it with a withdrawal amount) then this ether would be added to the contract balance and potentially lost to the user: this is because the withdraw function does not make an equivalent accounting adjustment to the mapping for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller makes the same mistake, the function will revert, and the ether won’t be sent.
A function only needs to be marked payable when it receives ether from an external address to add to the contract address balance. It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function, this happens automatically because we have marked the function as payable. The line balance[msg.sender] += msg.value; then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.
In contrast, the withdraw function is deducting ether from the contract balance and sending it to an external address. It does this using the address member transfer (which is like a method called on a payable address)…
<address payable>.transfer(uint amount);
And don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.
Let me know if you have any questions
Hi Jon!
Thank you so much for your assistance!
Here are the changes I made:
function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, "Balance not sufficient");
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, "Balance not sufficient");
require(msg.sender != recipient, "Dont transfer money to yourself");
uint previousSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
emit balanceTransfer(msg.sender, recipient, amount);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
Hope I got everything correct
1 Like
Yes … that’s all correct now
Hey! This is my solution.
If the msg sender is trying to withdraw more than what he has in the balance, he will still be able to withdraw 100% of what the bank owes him. It raises just if he has an empty balance.
pragma solidity 0.7.5;
contract SimpleBank {
mapping(address => uint) balances;
address owner;
constructor(){
owner = msg.sender;
}
modifier onlyOwner {
require(msg.sender == owner, "Owner only");
_;
}
/***
* Events
*/
event balanceChanged(
address indexed caller, // can be queried later
address indexed target, // can be queried later
uint amount,
bool increment // true == add, false == remove
);
event depositReceived(
address indexed sender,
uint amount
);
event withdrawalProcessed(
address indexed recipient,
uint amount
);
/**
* Getters
*/
function getSenderBalance() public view
returns(uint){
return balances[msg.sender];
}
function getBalance(address addr) public view
returns(uint){
return balances[addr];
}
/**
* Single balance modifiers
*/
function _removeFromBalance(address recipient, uint amount) private
returns(uint){
if (amount > balances[recipient]){
uint amt = balances[recipient];
balances[recipient] = 0;
emit balanceChanged(msg.sender, recipient, amt, false);
return amt;
} else {
balances[recipient] -= amount;
emit balanceChanged(msg.sender, recipient, amount, false);
return amount;
}
}
function _addToBalance(address recipient, uint amount) private
returns(uint){
balances[recipient] += amount;
emit balanceChanged(msg.sender, recipient, amount, true);
return amount;
}
function deposit() public payable
returns(uint){
emit depositReceived(msg.sender, msg.value);
uint deposited = _addToBalance(msg.sender, msg.value);
return balances[msg.sender];
}
function withdraw(uint amount) public
returns(uint){
require(amount > 0, "Amount too small");
uint senderPreBalance = balances[msg.sender];
require(senderPreBalance > 0, "Balance empty");
amount = _removeFromBalance(msg.sender, amount);
msg.sender.transfer(amount);
emit withdrawalProcessed(msg.sender, amount);
return balances[msg.sender];
}
/**
* From - to
*/
function _transfer(address from, address to, uint amount) private
{
balances[from] -= amount;
emit balanceChanged(msg.sender, from, amount, false);
balances[to] += amount;
emit balanceChanged(msg.sender, to, amount, true);
}
function transfer(address recipient, uint amount) public
{
require(amount > 0, "No 0 amount transfer");
require(msg.sender != recipient, "No self transfer");
require(balances[msg.sender] >= amount, "Insufficient balance");
uint preSenderBalance = balances[msg.sender];
uint preRecipientBalance = balances[recipient];
_transfer(msg.sender, recipient, amount);
assert(balances[msg.sender] == preSenderBalance - amount);
assert(balances[recipient] == preRecipientBalance + amount);
}
}
1 Like
event withdrawComplete(uint amount, address indexed withdrawnFrom);
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, "Balance not sufficient");
uint previousSenderBalance = balance[msg.sender];
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
emit withdrawComplete(amount, msg.sender);
return balance[msg.sender];
}
1 Like
Seems to work through testing, anyone have any suggestions on where to insert emit for event logging THANKS
function withdraw(uint amount) public returns (uint){
require(amount >= balance[msg.sender]);
msg.sender.transfer(amount);
uint previousBalance = balance[msg.sender];
assert(balance[msg.sender] == previousBalance - amount);
}
Nice solution @JayM700
You have added all of the additional lines of code needed to solve the problem with the withdraw function.
Your withdraw event and corresponding emit statement are also both well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed.
The additional two lines you’ve included for the assert() statement are also well coded.
Now have a look at this post which explains an important security modification that should be made to the order of two of the statements within your withdraw function body.
Just let me know if you have any questions
Hi @casadcode,
The require statement and the return statement you’ve added to your withdraw function are both correct, and the additional two lines you’ve included for the assert() statement are also well coded
But you are missing one very important line of code …
If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal. This means that a user can withdraw more than their own individual balance (the share of the total contract balance to which they are entitled), and effectively steal other users’ funds.
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 (effectively, each user’s entitlement to withdraw ether from the contract). 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.
Once you’ve had a go at correcting your code for this, have a look at this post which explains the importance of the order of the statements within your withdraw function body.
After completing this assignment, have a really good think about why your testing didn’t pick up on the missing line of important code discussed above. Think about how you can make your testing more thorough, so that it identifies this kind of issue
An emit statement should be placed at the end of a function, after the operations associated with the event itself (which it is logging data for) has occurred, but before a return statement if there is one. Generally speaking, it’s probably also better to place an emit statement after an assert statement if there is one.
Don’t forget to do the Events Assignment and post your solution here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course, and it will help you to practise coding event declarations, emit statements, and where to position them.
And finally, a friendly reminder not to post screen shots, because we can’t copy and paste your code in order to execute and test it, if we need to. Instead, follow the instructions in this FAQ: How to post code in the forum
Let me know if you have any questions about any of these points
Hi @Cosmin_Fechet,
I think you need to slow down a bit, and make sure you test your code to see if it actually works and does what it should
There are lots of issues with this solution. I’ll point you in the right direction, and then see if you can correct it yourself
(1) The condition in your require() statement does the opposite of what it should. We want to make sure a user can’t withdraw more than their individual balance, not force them to only withdraw more than their share!
(2) Once you’ve corrected the require() statement, you will still find that every time you call the withdraw() function, assert() fails and triggers revert. This is because the condition in your assert() statement can never be true. Can you see why?
Once, you can see the problem, comment out these last two lines, and continue with point 3. We’ll come back and correct these two lines of code associated with assert() at the end, because you need to make another important modification first.
(3) Your withdraw function is missing an essential line of code …
If you now deploy your contract (with just the two statements you should currently have in your withdraw function body), make a deposit, and then call your withdraw() function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal. This means that a user can withdraw more than their own individual balance (the share of the total contract balance to which they are entitled), and effectively steal other users’ funds.
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 (effectively, each user’s entitlement to withdraw ether from the contract). 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.
(4) 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 have given you an orange warning about this.
(5) Now let’s return to the two lines of code I asked you to comment out. Your local variable previousBalance was saving the user’s balance after the transfer, not before. This didn’t make any difference because you hadn’t included the essential line of code (mentioned above) needed to reduce this balance for the withdrawal amount. However, after adding this essential line of code (in point 3) you’ll now need to reposition this local variable declaration and assignment, so that the assert() statement checks an invariance that should always be true, instead of a condition that can never be true 
(6) Once you’ve made all of the above changes and tested your withdraw() function to ensure it is working as it should, have a look at this post which explains the importance of the order of two of the statements that you should now have in your withdraw() function body. Make sure you have these statements in the right order to guard against re-entrancy attacks and keep your smart contract secure.
Let me know if anything is unclear, if you have any questions, or if you need any help in correcting your withdraw() function