Nice solution @Obi 
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 us know if you have any questions 
Oops, Itâs a typo. âan error in our codeâ should be. Sorry for the confusion.
Jon,
thanks again for more comments. -
I donât quite understand your point: I have adjusted the balance amount BEFORE making the transfer, so that should avoid re-entry attack, no? Isnât that the correct way to do it?
(BTW Iâm completing the bootcamp and am just refreshing as I have a lot happening and some concepts have been slipping my mind hence the refresher)
Another point: you mention the âorange warningâ - yes I get the warning, but for some reason, only that little orange warning appears in remix but without stating the actual text of the warning in the yellowish boxes below the left panel (as Iâm used to seeing normally) Any clues from yur end why these comments are missing?
Screenshot 2021-05-27 at 19.09.23|654x500
thank you.
Hey Ernest,
The require statement youâve added is correct, 
You are also right that returns(uint) can be removed from the function header. It isnât mandatory to include, and the function can still operate effectively without returning a value.
But you are missing one very important line of codeâŠ
If you deploy your contract, make a deposit, and then call your withdraw function (without the additional assert statement), 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.
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.
Your assert statement is incorrect and so itâs always failing whenever require evaluates to true i.e. when the amount requested for withdrawal is less than or equal to the userâs original balance (not their actual balance after having already withdrawn some ether because, as Iâve mentioned above, this isnât being reduced). Maybe this is why your require statement isnât failing and generating the error message you expect. However, even with your current incorrect assert statement, require should still trigger revert first whenever the amount requested for withdrawal is greater than the userâs original balance.
Until you add the additional line of code to adjust the userâs individual balance in the mapping, your assert statement is actually meaningless, because balance[msg.sender] is the same before and after the withdrawal. But even when you have added the additional line code, your assert statement will still be incorrect. By using an assignment operator the assert statement is changing the balance in the mapping as well as checking it. It must only check it. You also need to think carefully about which operand (which side of the comparison operator) to add or subtract the withdrawal amount from.
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.
Let me know if you have any questions about how to correct this, or if there is anything that you donât understand
@tnc,
Thanks for clarifying. I am rather new to programming and I would not have known any better man. Thank you very much. I spend some time reading other posts here to learn from othersâ mistakes and the feedback that is given to others, too. Any who, thanks again!
Hey Andreas,
Yes, thatâs correct. I was actually confirming that what youâve done is correct and providing the reason. Sorry, I probably confused you because you already know the reason why (to prevent re-entrancy). Iâm explaining the reason to everyone taking this new Solidity course, because itâs not covered in the lectures â if Iâd realised you were revising the material I wouldnât have added the explanation to your feedback 
Yes⊠looking at your screenshot, youâve got the Hide Warnings box checked. If you uncheck that you should get the warning messages, and also the line with the issue highlighted in yellow (if you hover over that youâll get the error message directly in the text editor)
Well, it looks like I got it. Jon, I was reading in another of your posts that you cued us to add the additional lines of code to prevent reentry attacks. Could you tell me if I did it right, please? I added it in the the withdraw function.
pragma solidity 0.7.5;
contract Bank{
mapping (address => uint) balance;
address owner;
event depositDone(uint amount, address indexed depositedTo);
modifier onlyOwner{
require(msg.sender == owner);
_; // run the function
}
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];
}
function withdraw(uint amount) public returns (uint) {
msg.sender.transfer(amount);
require(balance[msg.sender] >= amount);
return balance[msg.sender];
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 not sufficient");
require(msg.sender != recipient, "You may not transfer 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;
}
}
Hi Joseph,
Your require statement and return statement are both correct. But the withdraw() function will exit after executing the return statement, and so your last two lines are unreachable. The compiler should have given you a yellow warning for this.
balance[msg.sender] -= amount; âis correct but because it is currently unreachable, the callerâs balance will not be updated in the mapping. 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.
Youâve also included â msg.sender.transfer(amount);â twice! The second one is currently unreachable, and so doesnât affect anything, but the first one should not come before the require statement. Require statements validate inputs or restrict access to the function. Therefore, itâs important that they are placed as near to the beginning of the function body as possible. In addition, if the withdrawal amount isnât checked until after the ether is transferred from the contract address to the callerâs external wallet address, less gas will be refunded if require fails and reverts the transaction, than if the check was performed at the very beginning. 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.
This concerns the order of the statements in the solution (it doesnât require adding any additional lines of code). But you need to resolve the other issues Iâve mentioned first, before the security of your contract against attacks can be checked.
So, basically, youâve got the right code, but you need to remove the duplicated statement and modify the order of the other statements to address the issues Iâve mentioned above.
Let me know if anything is unclear, or if you have any questions.
Re-entrancy attack: ok - I have full empathy for the educational goal of your comments, perfectly fine. Thanks also for the minor correction of terminology re-entry / re-entrancy.
And thanks for the hint with the checkbox - that works now once again
pragma solidity 0.7.5;
contract Bank {
mapping(address => uint) balance;
//Storage - Permanent storage of data (State variables)
//Memory - Temporary storage used in function execution
//Calldata - Save arguments/inputs to our functions
//strings, arrays, mappings, structs
address owner;
event depositDone(uint amount, address indexed depositedTo);
modifier onlyOwner{
require(msg.sender == owner);
_; // Run the function
}
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];
}
function withdraw (uint amount) public 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, "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);
}
function _transfer(address from, address to, uint amount) private{
balance[from] -= amount;
balance [to] += amount;
}
}
An excellent solution @PoeAslan
You have added all of the additional lines of code needed to solve the problem with the withdraw function, and all of the statements in the function body are in the correct order to reduce security risk:
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. We wouldnât expect you to know this at this early stage, though, so Iâm just telling you for extra information, in case you havenât already seen this explanation in some of the other posts in this discussion topic. But itâs great that youâre already getting into good habits in terms of smart contract security 
Let us know if you have any questions
Thanks, but I canÂŽt take credit for this. I tried my best to understand the syntax but nothing worked. I watched the ârequireâ and âassertâ videos several times but was unable to resolve the issue so I looked at the solution and added it. I guess I have to repeat the course a few times from start to finish until I understand the logic.
function withdraw (uint amount) public returns (uint) {
//address payable toSend = 0x17F6AD8Ef982297579C203069C1DbfFE4348c372;
//toSend.transfer(amount);
require(balance[msg.sender] >= amount);
uint previousBalance = balance[msg.sender]
msg.sender.transfer(amount);
assert(balance[msg.sender] = previousBalance - amount);
}
Hello again, Jon. I believe I understand what you are telling me concerning the order of the statements that I add to the function transfer. When the ârequireâ statement is used, as it is a validator/error handling code we need to use it as close to the function body so that it can be read first. Solidity is a statically read language so it is read first and validates specific data.
Also, I added the msg.sender.transfer(amount) statement before the balance[msg.sender] per your feedback to another of the guys here at the academy. I no longer got any error messages except for a ânumber 1â next to the compiler icon but when I refer to the code to see an error message, I see no error messages so I am not sure what that ânumber 1â might refer to. Please let me know if I got it right this time, please big guy! Thanks in advance!
Hi @PoeAslan,
Well thatâs good that you are being realistic about what you still need to work at understanding
Here is a list of some learning and study activities, which you may find useful when repeating the course and the assignments. I think they could help you get more out of the course, and will also make the repetition more varied and interesting.âŠ
Anyway, I hope you find some of these ideas helpful. Just let me know if you have any questions
Hi @vili,
The require statement youâve added is correct but you are missing 2 other important lines of code, and there are couple of other mistakes which prevent your code from compiling:
You are missing a semi colon at the end of one of your statements.
If you format your code before posting it, as well as making it more readable, this will also make it easier to spot these kinds of mistakes.
Follow the instructions in this FAQ: How to post code in the forum
You have used an assignment operator (=) instead of an equality operator (==) in your assert statement.
Once you have corrected the above 2 syntax errors, if you deploy your contract, make a deposit, and then call your withdraw function as it is, your assert statement will always fail. This is because, although the requested withdrawal amount is transferred from the contract address to the callerâs external wallet address, the callerâs balance in the mapping is not reduced.
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.
When youâve corrected your code for this, the invariance your assert statement is checking for will always be true.
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.
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.
Let me know if anything is unclear, or if you have any questions
I have just modified this function as requested:I have added a requirement and an assert function. The require function makes sure that there is enough balance in the address and the assert function to secure.
Iâm not too sure about the syntax of the assert function tho. The idea behind it is to make sure that the new value of the sender is that of the previous value - the transferred amount. Hopefully this is not too far off.
mapping (address => uint) balance;
function withdraw(uint amount) public returns(uint){
require (balance[msg.sender]>=msg.value);
msg.sender.transfer(amount);
return msg.value;
assert(msg.value=msg.value-amount);
}
function withdraw(uint amount) public returns(uint){
//check balance of msg.sender
require(balance[msg.sender] >= amount, "Balance not sufficient");
uint iniitialBalance = balance[msg.sender];
//commit withdrawal and update balance
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
//check balance after withdrawal
assert(balance[msg.sender] == iniitialBalance - amount);
return balance[msg.sender];
}