Transfer Assignment

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

[JosephGarza]

@jon_m

Hello again, Jon. I trust you are doing well today and enjoying yourself, wherever it is that you may be found today.

I redid the Inheritance / Destroyable task. I noticed that you prompted me to add a third contract but it was not quite clear why you would like me to add a third contract when the activity is to deploy an inherited contract that inherits a destroyable function from the base contract with the ether being sent to the owner of the contract, being that the only one that can actually call on the function is the owner. Please let me know if I got it right?

Again, thank you for your kind support, kind man.

pragma solidity 0.7.5;

// SPDX-License-Identifier: UNLICENSED

contract Destroyable{
    
    address  payable owner; 
   
    constructor() {
        owner = msg.sender; 
        
    }


    modifier onlyOwner{
        
        require(msg.sender == owner); 
        _; 
    }


  
  function destroy_contract() public  {
     selfdestruct(owner); 
  }
    
    
    
}

pragma solidity 0.7.5;

// SPDX-License-Identifier: UNLICENSED


import "./Destroyable.sol"; 

contract Bank is Destroyable{
   

    mapping (address => uint) balance; 
   
    event depositDone(uint amount, address 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); 
        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, "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; 
    }
    
}  

Jon, I’d like to mention, I attempted to use all the files that you asked me to, which were the inherited file and the two base-files (which in this case were “DESTROYABLE” and “OWNABLE”). I was unable to code the inherited file adequately because I kept getting an error message that said that had to do something with “an already defined value” which if I’m not mistaken drew my attention to the “onlyOwner” constructor that I was adding to the “DESTROYABLE” file. Once, you get me the green light on this task, I’d like to try the other one. I would like to remind you that I have never programmed before and I was able to obtain financial support so that I can dedicate the bulk of my time to learning to program on the blockchain. So, I welcome your challenges but I also like to feel like I am getting the hang of it as I go. And, that occurs when I am able to internalize and to use the code. Please forgive me if I decided to do it this way initially.

Again, thank you very much, Jon.

[JosephGarza]

@jon_m

Jon, hello again, thank you for your sincere suggestions and again, thank you for sharing your knowledge on the topic at hand. I will be taking your advice in due time. Can’t wait to finish this course here and find a suitable to recall and recycle what I have learned in this course. I agree with you, the course at “Eat the blocks” is very very helpful and I highly recommend it to everyone.

BTW, you are a fantastic teacher. Congratulations, kind Sir.

Looking forward to hearing from you soon!

[jon_m]

Hey Atakan,

Yes, your withdrawSuccessful event declaration is correct and well-coded, and the emit statement in the withdraw function will successfully log the relevant event data whenever the function executes successfully

@Deini-Atakan

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.

Excellent! You will learn a lot by reading some of the other posts in each of the discussion topics. There are a lot of useful comments and explanations posted here. It’s well worth spending your time browsing and reading, either to get some help with an assignment, or to check your completed solution to see if you’ve missed anything. It will also enable you to discover alternative coding solutions or answers. After finishing a section of the course, it’s also a great way to review and reflect on what you’ve learnt

[jon_m]

Hi @Klem,

The require statement you added is correct and I can see from your comments…

@Klem

However I see in the forum comments that there may be some things I’m missing ?

@Klem

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

… that you have already checked the solution and some of the other posts in this discussion topic, seen what needs to be added, and tried to understand why. That’s a great way of approaching these assignments, and often just as much work needs to go into the review stage after you’ve coded as much as you can yourself.

Here is a summary of what needs to be added to your code, and the reasons why. This is so you can check that you have fully understood everything. If you haven’t already modified your code for these additional points, I encourage you to that, as that will help with the learning process. Another good learning technique to use is to 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.

---

Your solution is missing 2 other important lines of code…

(1) 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 wallet 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!
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:

balance[msg.sender] -= amount;

(2) 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:

return balance[msg.sender];

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.

---

@Klem

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 !

Have you already completed the Javascript Programming for Blockchain Developers course? If you haven’t, then I would strongly recommend you do that before moving on with Solidity. The JavaScript course is specifically designed for beginners who aren’t used to reading code, and it gives you time to build up this skill slowly before launching into this Solidity course which moves at a quicker pace, because we assume that you are already comfortable with what is covered in the JavaScript course. Even though there are obviously important differences, Solidity does have a lot in common with JavaScript, in terms of both syntax and concepts.

Just let me know if anything is unclear, or if you have any questions

[jon_m]

Nice solution @Alex9230

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

By the way, your transfer() function is missing a lot of code… but I’m guessing that’s just because you only copied and pasted half of it…

Let me know if you have any questions

[Ernest_SG]

Hey man, yeah I noticed my mistake on the next video, you’re totally right, at first I thought that we didn’t need to update the balance because all transactions are updated in the blockchain but then after reading your feedback I get that it is imperative to keep track of balance internally for accounting. I’ve modified the code, and also fixed the order of statements, I also thought at first that one should first do the transfer and then update de balance mapping, but after reading the post I get why.

function withdraw(uint amount) public onlyOwner{
     
    require(balance[msg.sender] >= amount, "You haven't staked that amount of ETH");
    
    uint balanceBeforeWithdraw = balance[msg.sender];

        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        
        
    assert(balance[msg.sender] == (balanceBeforeWithdraw -= amount));
 }

[taodai]

Idk why but in pragma solidity 0.8.0 msg.sender isn’t a payable address, do i have to declare an address and convert msg.sender to a payable address?

pragma solidity 0.8.0;

contract MaticBank{

mapping(address => uint) balance;

function addBalance() public payable returns(uint){
    balance[msg.sender] += msg.value;
    return balance[msg.sender];
}

function getBalance() public view returns(uint){
    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];
}

}

[taodai]

It says “send” and “transfer” are only available for objects of type “address payable”.