As I said in the previous video. I want you to modify the withdraw function to eliminate the problem where one depositor could withdraw all the funds of the bank.
Use the proper error handling tools to achieve this and post your solution here.
12 Likes
function withdraw(uint amount) public onlyOwner returns (uint) {
//error handling addition
require (balance[msg.sender] >= amount);
msg.sender.transfer(amount);
}Here is my code for the assignment.
The key is using the “requirement()” to check balances.
pragma solidity 0.7.5;
contract EtherBank {
mapping(address => uint) balances;
address owner;
event depositDone(address indexed depositedTo, uint ammount);
event transactionDone(address indexed fromSender, address indexed toRecipient, uint amount);modifier onlyOwner {
require(msg.sender == owner);
_; //run function
}constructor() {
owner = msg.sender;
}function deposit() public payable returns(uint) {
balances[msg.sender] += msg.value;
emit depositDone(msg.sender, msg.value);
return balances[msg.sender];
}function transferTo(address toRecipient, uint amount) public {
//check balance of msg.sender done by modifier sufficientFunds
require(balances[msg.sender] >= amount, “Not enough balance!”);//Perform transfer _transfer(msg.sender, toRecipient, amount); //event logs and further checkes emit transactionDone(msg.sender, toRecipient, amount);}
function _transfer(address _from, address _recipient, uint amount) private {
balances[_from] -= amount;
balances[_recipient] += amount;
}function withdraw(uint amount) public payable returns(uint) { //check balance of msg.sender is sufficient. require(balances[msg.sender] >= amount, "Not enough balance!"); //Adjust balance balances[msg.sender] -= amount; //msg.sender is an address and address has a method to transfer. msg.sender.transfer(amount); return balances[msg.sender]; } function getbalance() view public returns(uint) { return balances[msg.sender]; }}
4 Likes
For the code of the video:
require (balance[msg.sender] >= amount, “Not enough funds”);
But as it is referred in the video, balance mapping is used by us to track the balances internally and so it could be an error in hour code. For security reasons I 'd search if there is a way to get the account balance. I google it and I found address(this).balance. So I believe that would be more reliable:
require(address(this).balance >= amount, “Not enough funds”)
function transfer (uint amount) public returns (uint){
require(balance[msg.sender]>=amount, “Insufficient balance for transaction”);
uint previousBal = balance[msg.sender];
msg.sender.transer (amount);
assert (prevousBal == balance[msg.sender]+amount);/* in reality gas should be considered*/
}
1 Like
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, "Insufficient Funds: Please enter withdraw amount less than your current balance");
msg.sender.transfer(amount);
}
function withdraw(uint amount) public onlyOwner returns (uint){
require(balance[msg.sender] >= amount, “Insufficient funds”);
msg.sender.transfer(amount);
return balance[msg.sender];
}
pragma solidity 0.7.5;
contract Bank {
mapping(address => uint) balance;
address owner;
modifier onlyOwner {
require(msg.sender == owner);
_;
}
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 onlyOwner returns (uint){
require(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, "Don't transfer money 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;
}
}
So how come, on every address I have balance of 0 Ether? There’s no way to check if this works…
Correction,I added a new address and there comes a 100ETH for me, yayyyy 
Sorry for asking and not trying alone…
Why can’t I use the require as a modifier, any help guys?
the code works but I want to use it as a modifier so it can be reused.
function withdraw(uint amount) public returns(uint){
require(balance[msg.sender] >= amount);
//address payable test = ;
//test.transfer(amount);
msg.sender.transfer(amount);//transfer reverts and gives an error if its something wrong
}
Hi @ChrisPap,
The require statement you’ve added is correct, but you are missing 2 important things:
-
When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s 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! How would you correct this?
-
You are missing a return statement in the function body.
You have also forgotten to remove onlyOwner. If you leave this function as onlyOwner, then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand.
1 Like
Excellent solution to this assignment @dcota 
You can remove the payable keyword from the withdraw function, though, because you only need to make a function payable if it is receiving ether.
You’ve also made a good job of the additional event for the transfer 
Just a couple of other observations:
-
It’s a good idea to indent your code in the function bodies. This makes it clearer and easier to read.
-
Maybe it’s just a copy-and-paste error but you are missing a few lines of code in your transferTo function:
- 2nd require statement (You can’t transfer tokens to yourself).
-uint previousSenderBalance = balance[msg.sender];
- assert statement: you also need to think carefully about the positioning of your emit statement (for your transfer event) in relation to assert()
Keep up the great work!
Hi @tnc,
Yes, this is the require statement that is needed.
This won’t solve the original problem, because  address(this)  refers to the address of the contract, not the msg.sender. So this alternative require statement would allow withdrawal of any amount up to the entire contract balance — which is the problem we started with.
Your solution is also missing 2 other important things:
- If you only add the require statement, and then deploy your contract and call your withdraw function, you will notice that the sender’s 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! How would you correct this?
- You need a return statement in the function body.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand.
1 Like
Hi @Paul_Mun,
The require statement you’ve added is correct, but your function is missing 2 other important things:
- When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s 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! How would you correct this?
- You are missing a return statement in the function body.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand.
1 Like
Hi @cmeronuk,
The require statement and the return statement you’ve added are both correct But you are missing one other important line of code:
- When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! The value which is returned from the function (the output) should be the reduced balance, but it will be the same as it was before the withdrawal. I’m sure you can see that this is another very serious bug! How would you correct this?
You have also forgotten to remove onlyOwner . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand.
Hi @Gorana,
The require statement and the return statement you’ve added are both correct But you are missing one other important line of code. When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! The value which is returned from the function (the output) should be the reduced balance, but it will be the same as it was before the withdrawal. I’m sure you can see that this is another very serious bug! How would you correct this?
You have also forgotten to remove onlyOwner . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds. In fact, because you have included the owner variable and the onlyOwner modifier, but removed the constructor, whichever address deploys the contract won’t be set as the owner, which will remain the zero address by default. This means than none of the account holders will be able to withdraw any of their funds. You can actually remove the owner state variable and the onlyOwner modifier, as they aren’t used anymore.
Did you have a go at adding the additional Transfer event?
Yeh, this can happen every now and again, and is easily sorted by just exiting and reloading Remix. This should reset all of the address balances to 100 ETH.
Let us know if you have any questions, or if there is anything that you don’t understand
Hi @Lamar,
…because your require statement references one of the function parameters (amount). A modifier can take parameters, but these then need to be fixed in the function header for each function that it modifies, and cannot be input by the caller of the function (which is what happens with amount). Filip demonstrated this in the lecture on modifiers using a modifier called costs e.g.
/* The costs parameter (price) is not input by the caller of the function.
It's fixed for each function in the function header */
modifier costs(uint price) {
require(msg.value >= price);
_;
}
function transfer(address recipient, uint amount) public payable costs(100) {...}
The require statement you’ve added to the withdraw function is correct, but your function is missing 2 other important things:
- When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s 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! How would you correct this?
- You are missing a return statement in the function body.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand, and we’ll help you out
Hi @RojasMessilia,
The require statement you’ve added is correct but your function is missing 2 other important things.
When you deploy your contract and call your withdraw function as it is, your assert function will always throw. This should obviously only happen if there is a flaw in your code, which there is. Your assert function itself is coded very well and it’s actually a good thing that it always throws, because that prevents a missing line of important code from allowing the caller to withdraw more funds than their actual balance. Let’s take an example…
- msg.sender has 1 ether balance, and calls the withdraw function with an amount argument of 1 ether.
- require does not throw (correct).
- previousBal = 1 ether;
- 1 ether is deducted from the contract balance and transferred to msg.sender BUT IT ISN’T deducted from
balance[msg.sender](the individual account balance stored in the mapping) which remains 1 ether. - assert throws and reverts the transaction because previousBal does not equal
balance[msg.sender] + amount
1 != 1 + 1
The gas cost does not affect this calculation/condition, because it is deducted from the caller’s external account balance NOT their internal balance stored and tracked in the balance mapping and accessed with balance[msg.sender].
How would you correct this?
You are also missing a return statement in the function body.
Don’t get your functions mixed up. This should be the withdraw function, not the transfer function. And also be careful with your spelling…
and
Both of these will prevent your code from compiling.
Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand, and we’ll help you out
1 Like
Ah ok I see what you’re saying! So would these changes be correct?
-
Adding this as the third line of the withdraw() function…
balance[msg.sender] - amount; -
Adding the return statement below this line as so…
return balance[msg.sender];
So the final product would end up coming out to…
function withdraw(uint amount) public onlyOwner returns (uint) {
require(balance[msg.sender] >= amount, "Insufficient Funds: Please enter withdraw amount less than your current balance");
msg.sender.transfer(amount);
balance[msg.sender] - amount;
return balance[msg.sender];
}
Let me know if this are the appropriate corrections. Thanks!
1 Like
Hey Paul,
Very nearly spot on!
Correct — this return statement should be on the last line.
So that the value balance[msg.sender] is actually modified in the storage in the mapping, you need to assign the new value to it. So you need an assignment operator  =
What you have currently is just an expression that calculates a new value, but then doesn’t do anything with it. So…
balance[msg.sender] - amount;
/*
This is correct for the new calculated balance.
You then need to assign it to balance[msg.sender] in the mapping:
*/
balance[msg.sender] = balance[msg.sender] - amount;
/*
There is a more concise way to do this using the
subtraction assignment operator -=
*/
balance[msg.sender] -= amount;
// This avoids repeating balance[msg.sender]
This will now work. However, it’s actually very important for security to put
balance[msg.sender] -= amount;  before  msg.sender.transfer(amount);
We wouldn’t expect you to know this at this early stage though, so I’m just telling you for extra info. You’ll learn about the type of attack this prevents and how it does it in later courses. In summary, it’s important to modify the contract state before you actually perform the transfer of funds out of the contract, just in case there is an attack after the transfer, but before the state is modified to reflect this operation. But don’t worry about the details for now, but it’s good to be aware of and to start getting into good habits now.
You also need to remove the onlyOwner modifier from the function header. You’d correctly removed it from your first version, but for some reason it’s crept back in 
If you restrict this function to onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw their funds.
But, well done! You’re making great progress! 
Ok great, ahh I total forgot about using the balance[msg.sender] -= amount
Oh ok, that is very interesting that putting that statement above the transfer line could be a protection against an attack. I would definitely not have thought that!
Ya I’m actually not sure why I put that back in there. I think the next assignment had me toggle that onlyOwner part back on. That is a good reinforcement for me though. So the only owner would prevent anyone from else that holds an account to withdraw their funds.
Great, thank you for this explanation. That really helped clarify things for me 
1 Like