function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] > amount);
msg.sender.transfer(amount);
}
function withraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance [msg.sender];
}
1 Like
pragma solidity 0.7.5;
contract HelloWorld {
mapping(address => uint) balance;
address owner;
event depositDone(uint amount, address indexed depositedTo);
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];
}
function withdraw(uint amount) public returns(uint) {
require(balance[msg.sender] >= amount, "You do not have enough funds");
msg.sender.transfer(amount);
return balance[msg.sender];
}
function getBalance() public view returns(uint) {
return balance[msg.sender];
}
function transfers(address recipient, uint amount) public onlyOwner {
// check balance of msg.sender
require(msg.sender != recipient, "You cannot send funds to yourself, idiot!");
uint previousSenderBalance = balance[msg.sender];
_transfers(msg.sender, recipient, amount);
assert(balance[msg.sender] == previousSenderBalance - amount);
// event logs and further checks
}
function _transfers(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
Nice solution @martina 
… but don’t forget the function’s closing curly bracket 
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 
Just let me know if you have any questions 
1 Like
Hi @stanmation,
The require statement you’ve added works in that it prevents a user from withdrawing more than their share of the total contract balance, but it also prevents a user from withdrawing their full individual balance. How would you modify the condition in your require() statement to allow users to withdraw Ether up to and including their individual balances, and to only prevent withdrawal amounts greater than their individual balances?
Your withdraw() function is also 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.
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 anything is unclear, or if you have any questions about any of these points
Hi @nathanls78,
The require statement you’ve added is correct, but your withdraw function is still missing another 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.
Once you’ve modified 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.
A few other observations …
-
Your onlyOwner modifier in the transfers() function header prevents anyone other than the contract owner from making an internal transfer within the contract from their own individual balance to another user’s individual balance. If this is your intention, then that’s fine, but I just wanted to let you know that there isn’t any reason why any user with sufficient funds shouldn’t be allowed to perform this transfer, especially as any address can now deposit Ether in the contract.
-
At the beginning of your transfers() function body, you have a comment which correctly states that the calling address’s individual balance needs to be checked, but you haven’t included a require() statement to perform this check. You need to add this for the same reason that it’s needed in the withdraw() function: to prevent users from transacting with Ether amounts greater than what they are individually entitled to i.e. their own individual balances as tracked and recorded in the mapping.
-
I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. 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
Hi everyone, I have a “strange” question about something that’s happening when testing my withdrawAll() function on Remix. I thought this thread would be a pretty good place to post it.
In words this is what’s happening: when I adjust the msg.sender balance to 0 before making the transfer, I get an out of gas error. If I adjust the balance to any other value, even 1 Wei the function goes through.
I’m going to attach my function below, plus the screenshots from Remix, I think it’s really strange, I was hoping anybody would have an insight on something I’m clearly not seeing here.
function withdrawAll () public returns (bool _success) {
require (balances[msg.sender] > 0, "Insufficient funds");
uint _amountToWithdraw = balances[msg.sender];
balances[msg.sender] = 0;
payable(msg.sender).transfer(_amountToWithdraw);
assert(balances[payable(msg.sender)] == 0);
emit withdrawSuccessful(payable(msg.sender), _amountToWithdraw);
_success = true;
}
This first screenshot shows what happens when I update the balance to 0:
This second screenshot shows what happens when I update the balance to 1 Wei:
(I’ve commented out the assert() method since it will clearly throw with balance at 1 Wei.)
I would really appreciate any insights since this problem has been keeping me busy for a couple of days now, and I have no answer for it, nothing on google…
Thank you all!
1 Like
Hi @Giupi,
This certainly got me thinking and experimenting!!
@mcgrane5 & @thecil : Do you agree with my conclusion, below? Or have I missed something?
Through a process of elimination, I’ve worked out that the line of code that is causing the transaction to revert in Remix (with the error message: out of gas) is the one setting the withdrawer’s individual balance in the mapping to zero …
However, as far as I could see, there was nothing wrong with this line of code. In fact, quite the opposite: you have correctly set the user’s individual balance in the mapping to zero before transferring their full Ether balance out of the contract to their external address.
I thought, maybe, one or two of the other lines of code in this function could be pushing the gas consumption over the gas limit (which can be adjusted in the Gas Limit field below the Account field in the Deploy & Run Transactions panel in Remix). But this isn’t the case, and no matter how much the gas limit is increased, this doesn’t seem to make any difference.
What’s more, I also couldn’t see any reason why setting balances[msg.sender] to 1 instead of zero should make any difference to the gas cost.
Out of interest, I tried changing …
balances[msg.sender] = 0;
// to
delete balances[msg.sender];
…which should produce the same effect. But this also triggers the out of gas error in Remix.
So, I tested your complete function (within a larger Bank contract) using Truffle and Ganache, instead of Remix … and it worked! I used Solidity compiler v.0.8.9. So, this suggests to me that there is most probably a bug in the current version of Remix.
Even the following function triggers the out of gas error in Remix. This is very strange and makes me even more convinced that there is some kind of bug causing this…
function setBalanceToZero() public {
balances[msg.sender] = 0;
}
Once you’ve completed the section on using Truffle in the 201 course, you’ll be able to see that your withdrawAll() function works fine. However, I would suggest you also make the following amendments, which will also save a small amount of gas…
function withdrawAll() public /*returns (bool _success)*/ {
// Do you use this return value? If not, you should remove it.
require(balances[msg.sender] > 0, "Insufficient funds");
uint _amountToWithdraw = balances[msg.sender];
balances[msg.sender] = 0;
payable(msg.sender).transfer(_amountToWithdraw);
assert(balances[/*payable(*/msg.sender/*)*/] == 0);
// msg.sender doesn't need to be payable to look up the balance in the mapping
emit withdrawSuccessful(/*payable(*/msg.sender/*)*/, _amountToWithdraw);
// msg.sender doesn't need to be payable to emit/log this address as event data
// _success = true;
// Do you use this return value? If not, you should remove it.
}
Let us know if you have any questions
1 Like
@jon_m this is weird, setting a users balance maping to 0 like this is standard in a lot of contracts. . so it worked when seeting to 1 and not zero. so this issue happens in sol 0.8.9? i want to check cos i want to try reproduce
2 Likes
ok so i was playing around with remix for the past 20 minutes trying anything and everything. @jon_m yeah i can confirm evrything that youve done above, setting it to any value other than zero works, but setting directly to zero causes a gasLimit error. even tried setting the gas limit stupidly high and the same.
so then i started looking around on google and came across this post from three years ago
https://ethereum.stackexchange.com/questions/11149/problem-setting-uint-value-to-zero
a guy in this post was having a similar issue which was puzzling some of the people trying to help him out. however one guy confitmed that at the time it was an issue with remix and the browser Solidity interacting with geth. he then goes on to show exactly what jon does above by deploying manually and it works. apparently at the time of that post it was an issue with like three versions
so i tried like 5 or six different versions and the same gas error presisted across all the compiler versions i tried. so this must definitely be an issue with remix and connecting to geth or something like that. i think this would be worth opening an issue. i wonder where we could do this though.
this is an insane bug as a withdrawAll function is quite common. i can say with confidence that the eroor is due to the javascript VM that remix uses because when i deployed the test contratc i wrote which users your withdraw all function with injected web3 to kovan it works fine. so its seems like the bug only presists in the local version of remix and their javascript VM, tried chaning the deafult EVM version to london berlin and a few others also and the error on the local deployment still remains. very weird indeed
3 Likes
Thanks for confirming that @mcgrane5 ! … I agree that it’s very weird indeed. But at least we’ve come to the same conclusion.
Yeh … I tried compiler v0.5 and v0.7 and still had the same issue. I also tried using the call method instead of transfer, but it’s not the line of code that sends the Ether which is causing the problem. It’s the assignment of zero to the mapping … so weird!!
Yeh, that’s exactly what I couldn’t understand. For ages, I thought I was just missing something obvious due to brain fog!
Funnily enough, I was just looking for where to do this, and the following issue opened earlier today in the remix-project repository looks like it relates to the same bug …
https://github.com/ethereum/remix-project/issues/2320#issue-1207367110
I haven’t looked at it in detail yet … but this seems like further evidence that we are dealing with a bug, which will hopefully soon be fixed by their developer team 
1 Like
Thank you @jon_m and @mcgrane5 I really appreciate your responses.
I’m not sure how to open an issue with Remix, but I’ll do some research and see what I can find.
In the meantime thanks again. I love Moralis and this forum!
2 Likes
No problem @Giupi … thanks for giving us a good challenge
Have a look at what I’ve posted in the last few minutes … I’ve found an issue opened in the Remix Project’s GitHub repository that looks like it may be caused by the same bug. I haven’t looked in detail yet, but you may want to start there
1 Like
aww thats good @jon_m, it must be something that is only recent especillay if the issue was opened today. and @Giupi no worries my man glad you love the fourms
2 Likes
@jon_m Thanks for sharing these links. As a matter of fact, I had a similar issue with another function that I wrote to basically remove a struct from an array of structs. It seems related to the issue pointed out in the link you shared, I’m guessing since, when using delete on a struct, it would revert all values to zero. Maybe. Not sure, but it’s an interesting learning experience.
2 Likes
hahaha glad to give you guys a nice challenge 
2 Likes
If you look up or reference a value in a mapping or a fixed-size array, and the value has either previously been deleted using delete, or you use a key or index value which hasn’t had a value mapped or assigned to it yet, then you will retrieve the equivalent zero value according to the data type…
-
uintandint…0 -
bool…false -
address…0x000...000(zero address) -
string…""(empty string)
If the value you look up or reference, as above, is a struct instance, its properties will also be equivalent zero values according to each property’s data type.
If you try to retrieve a value from a dynamically-sized array at an index value which hasn’t had a value assigned to it yet, then this results in an error.
I’ll have to check whether it’s possible to use delete to delete a previously assigned value at a specific index within a dynamically-sized array. Unfortunately, I haven’t been able to quickly test this in Remix because of the bug we’ve discovered! 
If this can be done, I’d assume that the index value would subsequently hold an equivalent zero value (or a struct instance with properties holding their equivalent zero values), and that it would be these zero values which would be retrieved when referencing this index value (as with the fixed-size array).
I’ll have a look in a bit more detail to see if this relates to the issue I linked to in the Remix Project repository in GitHub … and I’ll get back to you.
function withdraw(uint amount) public {
require(msg.sender.balance >= amount, "Not enough funds.");
require(balance[msg.sender] >= amount, "Not enough funds.");
uint sourceAccountBalance = msg.sender.balance;
payable(msg.sender).transfer(amount);
uint sourceInternalBalance = balance[msg.sender];
balance[msg.sender] -= amount;
assert(msg.sender.balance - amount == sourceAccountBalance);
assert(balance[msg.sender] + amount == sourceInternalBalance);
}
1 Like
hey @xenmayer nice solution but theres a few things. why do you sue msg.sender.balance and balance[msg.sender]. you dont need both. msg.sender.balance holds the wei denominated ether balance of your wallet address. however balance[msg.sender] holds the wei denominated ether balance of the user in the SMART CONTRACT, so since your funds have been deposited into the smary contarct there is no need to make a check on the users wallet balance theough msg.sender.balance.
this is actually a big risk. you wont have came across this because all remix accounts have 100 ether which takes a long time to burn through. but if you some how managed to get your wallet balance to zero, for example you could deposit 100 ether into the contract with your deposit function. then if you try to withdraw that 100 ether back into you wallet with your implementation of the function above this would flag yur revert at line
require(msg.sender.balance >= amount, "Not enough funds.");
this is because this check isw checking whether you wallet address has funds greater than the amount your trying to withdraw which will always be false in thi scase because yuour wallet address now has 0 balance after we deposuted the full 100 ether. so the only way to get this money back would be to fund your wallet address with an amount of ether eqaul to or greater than the amount your trying to withdraw. so in this case to get back the 100 ether you would need to get another 100 eth into you wallet address. so this is a big securoty risk. below ive optimised yur function
function withdraw(uint amount) public {
require(balance[msg.sender] >= amount, "Not enough funds.");
uint balanceBefore = balance[msg.sender];
balance[msg.sender] -= amount;
payable(msg.sender).transfer(amount);
assert(balanceBefore - amount == balance[msg.sender]);
}
we can get rid of the first require statement. and the diffeence here is we store the previous balance in a temp variable so that we can reduce the balance of msg.sender before the actual transfer. not that its a big risk here, bu this just makes sure our function is safe from being exploitable in a callback exploit (reentrancy attack), but since we stroed their previous balance in the tep variable we can do the assertio check after the transfer is done.
so we can see that the new code is a lot more consise and efficent. not at bad attempt at all keep it up
2 Likes
you could use msg.sender.balance in the deposit function but should use it here
1 Like