pragma solidity 0.7.5;
contract Bank {
mapping(address => uint) balance;
address owner;
event depositDone(uint amount, address indexed depositedTo);
event transferred(uint amount, address depositedFrom, address 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, "Insufficient Balance Available");
msg.sender.transfer(amount);
balance[msg.sender] -= 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, "Insufficient Balance Available"); //require
require(msg.sender != recipient, "You may not transfer to the same address.");//require
uint priorSenderBalance = balance[msg.sender];
_transfer(msg.sender, recipient, amount);
assert(balance[msg.sender] == priorSenderBalance - amount);//assert
emit transferred(amount, msg.sender, recipient);
}
function _transfer(address from, address to, uint amount) private {
balance[from] -= amount;
balance[to] += amount;
}
}
1 Like
Nice assignment solution @ppoluha and, overall, a very well-coded contract 
You have added all of the additional lines of code needed to solve the problem with the withdraw function.
The additional assert statement and withdrawal event (with corresponding emit statement) youâve included are also appropriate and well coded. The emit statement is in the correct position within your withdraw function body, and it logs relevant information when a call to the function has executed successfully.
Now have a look at this post which explains an important security modification that should be made to the order of two of the other statements within your withdraw function body. See if you can work out how to reorder your code for this, bearing in mind youâve got an additional 2 lines of code for the assert statement.
Just let us know if you have any questions 
An excellent assignment solution @thecryptoargie
You have included all of the additional functionality needed to solve the problem with the withdraw function, and you have the different lines in the correct order to reduce security risk:
- check inputs (require statement)
- effects (update the contract state for reduction in balance)
- interactions (perform the transfer of funds from smart contract address to wallet address).
Itâs important to modify the contract state:
balances[msg.sender] -= amount;
before actually transferring the funds out of the contract:
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 
1 Like
Nice assignment solution @Randallcrum and, overall, a very well-coded contract
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.
Your transfer event and corresponding emit statement are also well coded. The emit statement is in the correct position within the transfer function, and it logs relevant information when a call to the transfer function has executed successfully.
Just let us know if you have any questions
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, "Insufficient Balance Available");
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance[msg.sender];
}
That makes sense. Make the changes to the balance before committing funds, to prevent possible shenanigans. Nice.
1 Like
Thanks for the rich comment @jon_m! I now understand that the order of the contractâs balance adjustment and the transfer should be in the below order to prevent some kind of attack.
balances[msg.sender] -= amount;
msg.sender.transfer(amount);
And Iâd be curious to know if itâs good practice to have assert statements like this one, requiring two extra lines of code with extra computing and storage of the previous balance. Shouldnât I be able to trust the inner mechanics of the EVM?
1 Like
pragma solidity 0.7.5;
contract Epic {
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){
assert(balance[msg.sender] >= amount);
balance[owner] -= amount;
//emit LogWithdrawal(msg.sender, amount, balances[msg.sender]);
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, "Bro don't transfer money to yourself");
_transfer(msg.sender, recipient, amount);
}
function _transfer(address from, address to, uint amount) private{
balance[from] -= amount;
balance[to] += amount;
}
}
function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, "Withdrawal error");
msg.sender.transfer(amount);
return(balance[msg.sender] -= amount);
}
I am just not sure about 2) Correctly adjust the balance of the user after a withdrawal, is this the answer to this task?:
return(balance[msg.sender] -= amount);
1 Like
Hi @ppoluha,
Good questions!
I think this all comes down to risk management, and our own risk assessment of how potentially catastrophic the consequences would be if the code of a particular function in our smart contract fundamentally failed to execute as it should. This obviously depends on the specific operation(s) a particular function is performing, and the overall use case of our smart contract.
Incorporating certain processes and checks in order to prevent highly improbable, or even supposedly impossible, negative events is considered good practice, indeed expected, in all industries where risk management is important (which is pretty much all of them), and of vital importance in ones that involve finance, legal services, health and safety etc.
assert() is not meant to fail and trigger revert because it is checking invariants, to ensure that âimpossibleâ outcomes havenât happened. I think the fact that, once deployed, smart contracts are essentially immutable, makes the implementation of this kind of security check even more of an absolute necessity within their design.
Obviously there has to be a balance between implementation and operating costsâvâthe probability of a disaster happening; but in terms of just adding some extra code to make the smart contract more robust and secure (even if itâs never actually needed), I think the cost/benefit analysis in many scenarios becomes a no-brainer.
I think another important consideration is the extremely rapid pace of technological change within the blockchain industry. Networks, platforms, protocols, systems and programming language syntax etc. etc⌠which, today, we may be able to rely upon and trust with a high degree of confidence and certainty, may well, in the future, be susceptible to certain new types of security threat and vulnerabilities, which would be extremely hard to predict today. I think these are the kinds of âimpossibleâ outcomes we want to try to protect our smart contracts from by implementing security features such asâassert().
I hope these thoughts go some way towards answering your question, or at least provide some food for thought and further reflection
Hi @Filip_Rucka,
Youâve got several problems with your withdraw() function that you need to sort outâŚ
Have you tried deploying your contract, and seeing what happens when different addresses deposit funds, and then try to withdraw them?
Any address can deposit ether and then call withdraw(). But this line will always reduce the balance of the contract ownerâs address (who initially deployed the contract), and not the balance of the address making the withdrawal! Iâm sure you can see that the owner is going to run into serious problems with this code 
So, we might then think that account holders other than the contract owner will benefit enormously from the bug Iâve just described â by being able to withdraw more than their account balance, because it is never reduced⌠but you should notice that after withdraw() has executed, the ether balance in the callerâs external wallet address does not increase by the withdrawal amount. Thatâs because youâre missing the line of code that was given to you in the video, the one that transfers the ether amount from the contract address to the callerâs external wallet address.
You have used an assert() statement to check the input amount. Instead,â require() âshould be used to validate inputs. We use âassert() âto check invariants, which basically means checking that the code in our function body is executing as it should and producing the expected outcome. assert() is not meant to fail and trigger revert, because we obviously aim to only deploy smart contracts which have been coded correctly, logically, and which operate effectively i.e. with no fundamental flaws in the code.
With the version of Solidity we are using (v.0.7), if âassert()â throws, it causes all of the remaining gas in the âbudgetâ allocated to that transaction (up to the gas limit) to be consumed, and so results in a considerably higher cost. Therefore, it seems logical that we use the more costly assert() to check for conditions which are the least likely to occur (only in âexceptionalâ circustances).
In contrast, if require() throws, this is not considered to be an âexceptionalâ event, because it is testing the validity of the functionâs inputs. We should âexpectâ callers to sometimes attempt to invoke functions with invalid inputs (either by mistake, or on purpose).â require() is designed for such non-exceptional events, because when it throws, only the gas consumed up to that point is spent (including the gas required to execute the require function itself), and the gas remaining in the âbudgetâ for that transaction (up to the gas limit) is refunded to the caller.
If you do include the emit statement â which is currently commented out, but is actually a good idea â what corresponding event definition do you also need to add?
Also check your code carefully in this emit statement: there is a mistake with the mapping name.
If youâre not sure where to start with some of these corrections, have a look at some of the other studentsâ solutions, and the feedback and comments theyâve received, posted here in this discussion topic. And, of course, let us know if anything is unclear, or if you have any questions
This is a very clever solution, Dominykas 
You have added all of the code needed to solve the problem with the withdraw function.
What we expect is for you to first adjust the balance of the user withâŚ
balance[msg.sender] -= amount;
⌠and then to return the new balance on a separate line, after the withdrawal, withâŚ
return balance[msg.sender];
But you have found a clever and concise way to do both within a single statement:
By the way, you only need the parentheses when you are returning more than one value. Here you are only returning one value (the balance) and so you can remove them.
However 
⌠in order to reduce security risk, the statements within the function body should be in the following order:
- check inputs (require statements)
- effects (update the contract state)
- external interactions
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:
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. But it does mean that, even though your solution compiles and works, it isnât very secure because you are performing the transfer of funds (the external interaction) before updating the contract state for the effect of this transfer. Because the return statement has to be last statement, this unfortunately means that you canât update the balance at the same time as returning it 
So how would you modify your solution to make it more attack-resistant?
Just let me know if you have any questions.
1 Like
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount); // within the contract you should have at least that much
msg.sender.transfer(amount);
balance[msg.sender]-=amount;
return balance[msg.sender];
}
1 Like
@jon_m hello! I think for solidity v 0.8.1 I am getting error that address must be address payable. I searched online and found this
https://ethereum.stackexchange.com/questions/94707/msg-sender-not-address-payable-solidity-0-8-2
it works with wei as units for deposit and withdrawal. Care to comment? Thanks!
My code:
pragma solidity 0.8.1;
contract Practice {
string message;
mapping(address => uint) balance;
address public owner; // think this needs to be public...
event balanceAdded(uint amt, address indexed depositedTo);
event depositEvt(uint val, address depositer);
event transferEvt(address transferedTo, uint amt);
modifier onlyOwner {
require(msg.sender == owner);
_; //run function that the modifier is tied to.
}
constructor(string memory _message){
owner = msg.sender; // msg.sender in constructor is contract owner.
message = _message;
}
function deposit() public payable returns (uint) {
balance[msg.sender] += msg.value;
emit depositEvt(msg.value, msg.sender);
return balance[msg.sender];
}
function withdraw(uint amt) public payable returns (uint){
require(balance[msg.sender] >= amt);
payable(owner).transfer(msg.value); // this is key line change for sol v. 8+
// msg.sender.transfer(amt);
balance[msg.sender] -= amt;
return balance[msg.sender];
}
function getBalance() public view returns (uint){
return balance[msg.sender];
}
function transfer(address recipient, uint amt) public {
require(balance[msg.sender] >= amt, "balance not sufficient!");
require(msg.sender != recipient, "don't transfermoney to urself");
uint prevSenderBal = balance[msg.sender];
_transfer(msg.sender, recipient, amt);
emit transferEvt(recipient, amt);
assert(balance[msg.sender] == prevSenderBal - amt); //only throws if something v. off
//event logs further checks
}
function _transfer(address from, address to, uint amt) private {
if (balance[from] >= amt) {
balance[from] -= amt;
balance[to] += amt;
}
}
believe it does need to be payable as per newer versions of solidity. Wonder if this interfears with log writes thoughâŚ
seems address.transfer(msg.value) must be of type payable now. so
payable(owner).transfer(msg.value);
https://ethereum.stackexchange.com/questions/94707/msg-sender-not-address-payable-solidity-0-8-2
this works with units of WEI
function deposit() public payable returns (uint) {
balance[msg.sender] += msg.value;
emit depositEvt(msg.value, msg.sender);
return balance[msg.sender];
}
function withdraw(uint amt) public payable returns (uint){
require(balance[msg.sender] >= amt);
payable(owner).transfer(msg.value);
// msg.sender.transfer(amt);
balance[msg.sender] -= amt;
return balance[msg.sender];
}
let me know your thoughts!
Thanks!
function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, "Withdrawal error"); // check inputs (require statements)
balance[msg.sender] -= amount // effects (update the contract state)
msg.sender.transfer(amount); // external interaction
return balance[msg.sender]; // return statement (last statement)
}
It is cool to know about security risks that my solution could be exposed to. It is written down and noted!
1 Like
okay so maybe it is a solidity error error ⌠I ran the code using 0.8.1 without payable and there is a red error however the contract deploys and runs properly.
Nice solution @ybernaerts
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
Nicely done⌠and with the comments too 
If you find learning about the security risks interesting, then youâll enjoy the Smart Contract Security course, which you can either do after or before the 201 course. Among other things, it teaches you about how some of the most famous attacks on smart contracts were carried out, shows you the actual code that caused the vulnerability and was exploited by the attackers, and then looks at what coding best practices have been developed to prevent these types of attack.
1 Like
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, "Balance not sufficient");
uint previousSenderBalance = balance[msg.sender];
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
assert(balance[msg.sender] == previousSenderBalance - amount);
}
1 Like
Hi @programmedtorun,
Using â<address payable>.transfer(uint256 amount)â â a member of the address data type â to transfer ether from the contract address balance to an external address, has always required a payable address. But in versions prior to v0.8,âmsg.senderâis treated as a payable address by default, and so we donât need to explicitly convert it. So with v0.7, this compilesâŚ
But one of the changes from Solidity v0.8 is that âmsg.senderâ is now treated as a non-payable address by default, which is why we need to explicitly convert âmsg.senderâ when we want to use it to represent a payable address. This is why you are getting a red compiler error with v0.8.1 usingâ msg.sender.transfer(amt);â
No, it doesnât. For example with v0.8 you could add an event:
event Withdrawal(uint amount, address indexed withdrawnBy);
⌠and in the withdraw() function you could reference msg.sender in the corresponding emit statement, without it needing to be a payable address e.g.
emit Withdrawal(amount, msg.sender);
However, it is wrong to use msg.value as the amount argument, as you have done hereâŚ
msg.value âis used to reference a value in wei which a payable function receives as an input from the caller address and adds to the contract balance â as happens in the deposit() function. However, we donât want withdraw() to receive wei, but to send wei. One way we can get a function to do that is to use:
<address payable>.transfer(uint256 amount)
Using this code, the wei amount transferred from the contract address to the function callerâs external wallet address will be the amount requested by the caller and input as the amount parameter. This input amount is then referenced withinâ transfer(uint256 amount)
Also, a function only needs to be marked payable when it needs to receive ether (I said wei earlier because in Solidity the ETH amount is always expressed in units of wei). So, unlike the deposit() function, we donât want to mark withdraw() as payable in its function header. Marking it as payable doesnât prevent the 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.
Notice that in the v.0.7.5 code given to you for the withdraw function, the withdrawal amount is sent to the msg.sender address, and not to the owner address. This difference is important, because any address can deposit funds into our Bank contract, or receive funds via a transfer from another bank account holder, so it is only logical that any of those addresses should be able to call withdraw() and withdraw ether from the contract up to the value of their account balance. At the moment, no one can withdraw any funds from your Bank contract because using msg.value wonât work. But once this is fixed, if you donât also change owner, only the contract owner will be able to withdraw funds; and if an account holder other than the contract owner calls your withdraw function, their recorded balance in the mapping will be reduced by their requested withdrawal amount âŚ
⌠but because of the above issues, not only will they not receive that amount in their external wallet address, but it will be sent to the contract ownerâs instead!
Whatâs more, with your current code, if an address calls withdraw() and sends an ether amount from their wallet by mistake, this amount will be transferred to the contract and then immediately transferred out to the contract ownerâs wallet address!
You can see all of these different issues play out if you test your contract with different types of scenarios.
Now have a look at this post which explains an important security modification that should also be made to the order of the statements within your withdraw function body.
Some additional observations:
This doesnât need to be public. It all depends how accessible you want this variable to be. For example, it needs to have public visibility if you want Solidity to create an automatic getter, so that the owner address can be retrieved from an external service (e.g. from Remix or the frontend of a dapp etc.) In contrast, if owner only needs to be referenced from within the same contract, its visibility can be restricted to private.
Your transfer event and corresponding emit statement are fairly well coded, and the emit statement will log some relevant information when a call to the transfer function has executed successfully. But I think that, as well as the transferred amount and the recipientâs address, the senderâs address is also useful information to log. Emit statements are also usually positioned after any assert statements, rather than before â as indicated by one of your commentsâŚ
There is no need for the if statement and conditional execution in the _transfer() helper function. Firstly, it is better practice to place an input condition in a require() statement. But there is no need for this input verification here, because the same check has already been performed by the 1st require statement in the main transfer() function:
balance[msg.sender] >= amt
// is the same as
balance[from] >= amt
In these two conditional expressions, both msg.sender and from reference the function callerâs bank account balance in the mapping.
See if you can make the necessary modifications for these points. Just let me know if anything is unclear, or if you have any further questions