Transfer Assignment

[jon_m]

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:

1. check inputs (require statements)
2. effects (update the contract state)
3. 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 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

[PoeAslan]

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.

[vili]

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);
}

[JosephGarza]

@jon_m

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!

[jon_m]

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.…

• Have a look at other students’ posts in the relevant forum discussion topic, with their attempts, solutions, and the help, comments and feedback posted as replies. There are a lot of comments and explanations posted here. It’s well worth spending your time browsing and reading, not only to get help with the assignments, but also after finishing a section or particular assignment as a way to review what you’ve learnt, and to discover alternative coding solutions or answers.
• When you look at the assignment solutions, if they are different to your code, you should spend some time analysing and thinking about them. This is a really important stage in the learning process. You can learn a lot by working out what the solution code does and how it does it, and also how to go about “bridging the gap” from what you managed to do (or not) on your own. However, it’s important to remember that there are usually several different alternative approaches and solutions to these assignments, so if you’ve coded yours differently, but it still works, then it may well be valid. If you’re not sure, then post it here in the forum, and we’ll review it for you.
• Another good learning technique to use (after having already done the tasks described above) is to then try the assignment again from scratch without looking back at the solution (like a memory test). You can also try to do this after having left it for a certain period of time (say, the next day, then after a few days, then a week etc. etc.) This builds up longer-term retention. This is a good approach to take when reviewing a course you’ve already done, or earlier parts of the course you’re currently doing. Instead of just looking back at your solutions to the assignments, test yourself to see how many of them you can redo from scratch — only checking your previous code and notes, and rewatching the videos, if you need to remind yourself about certain things.
• Another important point to remember is that it can take some time before you fully understand all of the code in an assignment, so another good strategy is to come back and revisit an assignment, which you didn’t fully understand, after you’ve progressed a bit more through the course. You may well find that it’s much easier then.
• Don’t forget to do some research yourself on the Internet. Here is a link to a playlist from the YouTube channel Eat The Blocks. I’ve seen some of this guy’s videos myself and I think they are particularly helpful for learning Solidity. The videos are quite short and the explanations clearly explained and well demonstrated with clear examples. You’ll find that several of the videos in this playlist correspond to specific sections of this course, and so serve as an ideal starting point for your own further research.
  https://www.youtube.com/playlist?list=PLbbtODcOYIoE0D6fschNU4rqtGFRpk3ea
• Finally … play around with the code, experiment, and try to come up with your own examples, no matter how basic, short or simple. Even by just making a few small changes and adaptations to the code presented in the course, any amount of personalisation that you are able to add to your code will help it to mean more to you, and will therefore help you to internalise it better.

Anyway, I hope you find some of these ideas helpful. Just let me know if you have any questions

[jon_m]

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

[Daniel_Haydn]

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);
}

[Kippie]

    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];
    }

[jon_m]

Hi Joseph,

Your require statement and return statement are both now correctly positioned. All of the lines of code in your function body will now execute and perform all of the operations necessary to solve the initial problem with the withdraw function.

@JosephGarza

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.

More or less correct… We need to place require statements as near to the beginning of the function body as is practically possible because they validate INPUTS or restrict ACCESS.
assert() statements also handle errors, but as they are checking invariances (expected OUTPUTS), these are usually placed towards the end of the function body.

The other reason for placing require statements as near to the beginning of the function body as possible, is to keep the gas cost as low as possible in the event that require fails.

@jon_m

… 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.

---

@JosephGarza

Solidity is a statically read language so it is read first and validates specific data.

Solidity is a statically-typed programming language, but this doesn’t have anything to do with the top-to-bottom control flow that occurs during the execution of the code in a function body.

---

@JosephGarza

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…

Your code will now compile and execute successfully, but to reduce security risk, it is 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. I think you must have misread my feedback to another student regarding this, but if you do think I’ve said the opposite somewhere, then please let me know, so I can check.

@JosephGarza

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.

If the compiler icon is indicating an error (in red) or a warning (in orange) but is not highlighting the specific line in your code which is causing this, then you need to go to the Solidity Compiler panel by clicking on the icon with the number and scrolling down to the bottom of the panel where you will also find any error or warning messages (including any that you can’t access directly in the text editor). If you only have 1 orange warning, then I suspect it will be the ubiquitous:

Warning: SPDX license identifier not provided in source file.
Before publishing, consider adding a comment containing
"SPDX-License-Identifier: <SPDX-License>" to each file ...

This isn’t important because you aren’t publishing your code. However, I always include…

// SPDX-License-Identifier: UNLICENSED

… at the top of each of my files, just to get rid of this constant warning, because I find it annoying

By the way, please don’t post screen shots of your code, because then we can’t copy it and run it ourselves, if necessary. We also can’t quote parts of it, which is also very useful when providing feedback. In this particular post of yours, it doesn’t really matter, but when other students see screen shots, they think it’s generally OK to post them too, and then it will just snowball. Thanks, for your understanding

Let me know if you have any further questions about this assignment

[jon_m]

Hi @Daniel_Haydn,

There are quite a few problems with your code, so I’ll explain what they are, and then I suggest you have another go

---

Using  msg.value  will have generated red compiler errors.
msg.value is used to reference an ether value which is sent to a function by the caller. To receive ether a function must be marked payable, and so msg.value can only be used (and can only reference an ether value) in a payable function. This is the case with the deposit() function, which receives ether from an external wallet address, but not with the withdraw() function, which transfers ether from the contract address to an external wallet address.

In the deposit() function msg.value references the ether amount deposited into the contract. Instead, in the withdraw() function, the ether amount being withdrawn from the contract always needs to be referenced by the amount parameter. Your require statement needs to be corrected for this.

In your require statement, you correctly reference the caller’s balance in the mapping with:  balance[msg.sender]  You need to use this same reference instead of msg.value in other parts of your code, where you are referencing the caller’s balance either before or after the withdrawal. The difference in the before and after balances, should be the withdrawal amount. However, this won’t be case, unless you add another line of code…

msg.sender.transfer(amount)   transfers ether from the contract address balance to the caller’s external wallet address, but it doesn’t adjust their individual balance 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.

---

A function is exited after executing a return statement, and so your assert statement is currently unreachable. To fix this, it needs to be placed before the return statement.

@Daniel_Haydn

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.

This explanation of what the code in your assert statement should do is correct, but your code doesn’t do this. After adding an additional line of code to adjust the user’s balance in the mapping for the withdrawal amount (as explained above), you then need to reference both the before and after balances in your assert statement. Have a look at how this is done in the transfer() function, because you need to use the same method here. You’ll need a local variable to save the previous balance, temporarily, before it is adjusted for the withdrawal amount.

---

Finally, have a think about an appropriate value to return, and how to reference that.

Let me know if anything is unclear, or if you have any questions about how to make these changes. If you get stuck you can also take a look at some of the other students’ attempts and solutions, and the comments and feedback they’ve received, posted here in this discussion thread. I think you’d find that helpful

[jon_m]

Nice solution @Kippie

You have added all of the additional lines of code needed to solve the problem with the withdraw function. Your additional assert statement is also appropriate and well coded.

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 me know if you have any questions

[PoeAslan]

Thank you! I will go through the material you linked to.

[Deini-Atakan]

function withdraw(uint _amount)public returns(uint){
        require(balance[msg.sender] >= _amount, "Not enough funds");
        balance[msg.sender] -= _amount;
        msg.sender.transfer(_amount);
        emit withdrawSuccessful(_amount,msg.sender);
        return balance[msg.sender];
    }

Is this correct?

[vili]

Hi Jon, thank you for the comments. I noticed the syntax errors afterwards

[jon_m]

An excellent solution @Deini-Atakan

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:

1. check inputs (require statements)
2. effects (update the contract state)
3. 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 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

In terms of the additional emit statement you’ve added, this looks good and seems well-coded, but I can’t comment on whether it’s correct or not, unless I also see the corresponding event declaration, which I assume you must have added towards the top of your contract. Could you post that too?
However, I can confirm that your emit statement is in the correct position within your withdraw function body.

Let me know if you have any questions

[jon_m]

Hi @vili,

@vili

I noticed the syntax errors afterwards

Yeh, that often happens … you can edit your code after posting it. Just click on the pencil icon at the bottom of the post, next to Reply. You can edit it as many times as you want

Are you going to have a go at modifying your solution for the other important points I mentioned?

[Deini-Atakan]

Hi @jon_m, here is the declaration of the event.

    event withdrawSuccessful(uint amount, address indexed _to);

I hope it is also right. I checked some of the posts right before posting my answer yesterday and saw the security issue that you wrote. I tweaked the code after that, I am a new beginner but I hope that I will become a good blockchain developer.

[Klem]

Hello and I wish you a good day !

I did this simple thing that seemingly works in remix as far as I can tell:

function withdraw(uint amount) public returns (uint){
        
        require(balance[msg.sender] >= amount, "balance not sufficient");
        
        msg.sender.transfer(amount);
    }

However I see in the forum comments that there may be some things I’m missing ? I guess I’ll see in the solutions ? I can’t read code that well yet so it’s a little difficult to know what’s going on but I can see the progress ! Very exciting !

Thank you and I wish you good fortune !

(edit after viewing the solution: oh, I see, yes, makes sense, seemed as if there was something missing indeed)

[Alex9230]

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 is not sufficient");
    require(msg.sender != recipient, "Don't transfer money to yourself");
        }

[JosephGarza]

@jon_m

Again, thanks in advance for sharing your knowledge in the field. I reviewed the difference between statically-typed and top-bottom control flow programming languages- thank you for your correction. Solidity is a statically-typed, top-botton control flow language and so I understand that we should place the “require” statement as close to the function body as possible so that if the validator does not meet the requirements, and the contract is reverted to its original state, the least amount of gas will be lost VS running the other strings before the “require statement” and losing the gas used to execute those strings without leading to successful execution of the function itself. I also understand the geist of a re-entry attack and it is clear that it’s very important to update the balance of the contract before actually transferring the funds.

function withdraw(uint amount) public returns (uint) {
        
        require(balance[msg.sender] >= amount); 
        balance[msg.sender] -= amount; 
        msg.sender.transfer(amount); 
        return balance[msg.sender]; 
       
    }

I also added the piece of code that you do too as I find it quite annoying as well.

// SPDX-License-Identifier: UNLICENSED

Again, thank you in advance, kind Sir.