Transfer Assignment

Ethereum Smart Contract Programming 101
#438

function withdraw(amount) public returns (uint) {
msg.sender.transfer(amount);
returns balance[msg.sender];

}
#439

Hello everyone
I just wanna make sure about a small detail that wasn’t clearly stated in the video.
After succesfully using the transfer function, and after querying the same address the balance doesn’t update automatically.
We have to add a line of code and substract the amount from the previous balance or am I missing something?

1 Like
#440

Hi @Meriem, and welcome to the forum! :smiley:

It’s great to see you here, and I hope you’re enjoying the course.

Correct :ok_hand:

That’s all part of the assignment challenge. We want you to start noticing these kinds of issues yourself… for example …

… and to come up with appropriate solutions. Developing this kind of awareness is an important part of learning to become a good programmer… so, you’re making good progress :muscle:

#441

Hi @Jacob_Pettit,

You have included nearly all of the additional lines of code needed to solve the problem with the withdraw function, but you need to have them in a different order:

  1. checks (require statement)
  2. effects (update the contract state for reduction in balance)
  3. external interactions (perform the transfer of funds from the smart contract address to the external wallet address).

In terms of the require statement, this needs to come first, because it is checking that the input is valid. While it is true that if require() is placed later in the function body all previous operations will still be reverted if it throws, this will end up costing more gas because only the remaining gas (after require has thrown) will be refunded, and not the gas consumed for executing require() itself and any code executed previously.

Have a look at this post which explains the importance of the order of the other statements within your withdraw function body.

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 also need to include a return statement in the function body.

Let us know if you have any questions about these additional points :slight_smile:

1 Like
#442

Hi @Andrewg,

You’re missing a lot of code here…

Firstly, you should be getting compiler errors for:

  • the parameter: as well as giving it a name, you also need to define the data type;
  • the return statement: you need returns in the function header, but return (without ‘s’) in the function body. But well done for realising that you need to add a return statement :ok_hand:

You need to add a require statement to check that the caller has sufficient funds to cover their requested withdrawal amount.

Having made these changes, if you now deploy your contract and call the withdraw function, you will notice that the sender’s 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!

Once you’ve added all of this code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

If you find it hard to know where to start with any of these points, have a look at some of the other students’ code and feedback posted here in this discussion topic. And, of course, let us know if you have any questions, or if there is anything that you don’t understand :slight_smile:

#443

Thanks for the assistance.
So this is what I came up with

pragma solidity 0.7.5;

contract bank {

mapping(address => uint) balance;

event withdrawal(address wallet, uint _amount);

function deposit() public payable{
    balance[msg.sender] += msg.value; 
}

function withdraw(uint amount) public{
    require(balance[msg.sender] >= amount, "Insufficient balance !");
    require(amount > 0, "There is no point in payaing gas fees withdrawing 0 !");
    
    uint previousBalance = balance[msg.sender];

    msg.sender.transfer(amount);

    balance[msg.sender] -= amount;

    assert(balance[msg.sender] == previousBalance - amount);
    
    emit withdrawal(msg.sender, amount);
}

function getBalance() public view returns(uint){
    return balance[msg.sender];
}

}

1 Like
#444

An excellent assignment solution @Meriem :muscle:

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 :ok_hand:

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. See if you can work out how to reorder your code for this, bearing in mind you’ve got an additional 2 lines for the assert statement. :slight_smile:

The additional event you’ve added is also good. It emits and logs relevant information when a call to the withdraw function has executed successfully. Just remember that the convention is to start event names and contract names with a capital letter (e.g. event Withdrawal ,   contract Bank ). Your code does still work starting these names with a lower case letter, but it’s standard practice to start contract, struct and event names with a capital letter.

1 Like
#445

pragma solidity 0.7.5;
contract Bank {

mapping(address => uint) balance;

address owner;
event fundsDeposited(uint amount, address indexed depositedTo);

modifier onlyOwner {
    require(msg.sender==owner);
    _; //Run the function
}

modifier enoughBalance (uint amount) {
    require(balance[msg.sender]>= amount, "Not enough funds");
    _; //Run the function
}

constructor(){
    owner=msg.sender;
}

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

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

function transfer   (address recipient, uint amount) enoughBalance(amount) public {
    //require(balance[msg.sender]>= amount, "Not enough funds");
    require(msg.sender!= recipient, "Dont transfer to yourself");
    
    uint previousSenderBalance=balance[msg.sender];
    
    balance[recipient]+=amount;
    balance[msg.sender]-=amount;
    assert (balance[msg.sender]==previousSenderBalanceamount);
}

function getBalance () view public returns(uint){
     return balance[msg.sender];
}

}

3 Likes
#446

An excellent assignment solution @decrypter :muscle:

You have added all of the additional code needed to solve the problem with the withdraw function. Your additional modifier  enoughBalance , to provide the same require statement in both withdraw() and transfer() functions, is nicely done, appropriate, and shows a good understanding of how modifiers help us avoid code duplication, and keep our code concise :ok_hand:

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.

Also, I’m sure it’s just a copy-and-paste slip, but you are missing an arithmetic operator in the second operand in your assert statement condition:

You may find these types of errors are less likely to occur if you leave whitespace either side of your operators, as follows:

assert (balance[msg.sender] == previousSenderBalance - amount);

This is more standard practice, and also makes your code clearer, more readable, and developer-friendly :slight_smile:

1 Like
#447 
 function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount);
        msg.sender.transfer(amount);
    }
#448 
    function withdraw(uint amount) public returns(uint) {
        require(balance[msg.sender] >= amount);
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
    }
1 Like
#449

Thanks for your feedback. Of course the Assert was a copy past error (it was correct in Remix).

Regarding the order of statements. I agree that the reverse order looks cleaner. However isn’t the function itself atomic? If this is not always the case even the different order will not guarantee correct functionality. The problem might be less severe but still there. In other words, I would expect Solidity to offer a foolproof way to guarantee that both statements or none are executed under any and all circumstances, no?

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

Hi @Michael_Gerst,

The require statement you’ve added is correct :ok_hand: but you are missing 2 other important lines of code:

  • When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s 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! How would you correct this? If you can’t work this out yourself, you’ll find the answer by taking a look at some of the other posts in this topic.
  • 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 also need to include a return statement in the function body.

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 us know if you have any questions about how to correct this, or if there is anything that you don’t understand :slight_smile:

#452

Hi @santoretech,

Require statement  :white_check_mark:
balance[msg.sender] -= amount;  :white_check_mark:

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 also need to include a return statement in the function body.

Then have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Let us know if you have any questions about these additional points :slight_smile:

1 Like
#453

Excellent solution @Bryan :muscle:

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 the security risk:

  1. check inputs (require statement)
  2. effects (update the contract state for reduction in balance)
  3. interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

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, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. Anyway, don’t worry about the details for now, although it’s good to be aware of, and it’s great that you’re already getting into good habits :slightly_smiling_face:

1 Like
#454

Hi @decrypter,

Great question!

Yes, Solidity functions are atomic. But this is not the same as being 100% secure. We still need to adopt best practices to reduce the risk presented by potential vulnerabilities. Here is an interesting article that opens with the fact that Solidity is atomic, but then goes on to discuss why we still need to address potential vulnerabilities in our smart contract code. Right at the very end it mentions the withdrawal pattern (order of statements) that I have highlighted, and the example given is essentially the same as the solution to this assignment in terms of optimal security.

https://blog.b9lab.com/the-solidity-withdrawal-pattern-1602cb32f1a5

This withdrawal pattern reduces the risk of re-entrancy attacks. If you are interested in learning about this type of issue in more detail, I would highly recommend that you take our Smart Contract Security course at some point. In that course we look at examples of different types of attacks (including re-entrancy) and the smart contract best practices we can adopt as developers to reduce the risk of these types of attack occuring.

To whet your appetite, here is another article that specifically addresses re-entrancy attacks, what they are, and how to protect Solidity smart contracts from them occuring.
https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21

I hope that goes some way towards answering your question :slight_smile:

Inheritance Assignment
#455 
    function withdraw(uint amount) public returns(uint) {
        require(amount <= balance[msg.sender], "Insufficient balance");
        uint initialBalance = balance[msg.sender];
        
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        
        assert(balance[msg.sender] == initialBalance - amount);

        return balance[msg.sender];
    }
1 Like
#456

I added a require function to the withdraw function which checked that balance[msg.sender] >= to the amount being withdrawn.

Also added balance[msg.sender] -= amount; to update the balance of the person calling the function.

Also, for the fun of it I added an event to log the withdraw, similar to the event we have for the deposit.

Edit: I’m not actually sure if I need to return the balance at the end of the withdraw function. Can somebody confirm this?

pragma solidity 0.7.5; 

contract Bank {
    
    mapping(address => uint) balance;
   address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    event withdrawDone(uint amount, address indexed withdrawnTo);
  
    modifier onlyOwner {
      require(msg.sender == owner, "Invalid call address");
      _; 
      
    }
    
    constructor(){
        owner = msg.sender;
    }
                            //"payable" allows the function to receive money when people call it.  Only for receiving.        
    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 too low");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        emit withdrawDone(amount, 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 too low");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        //event logs and further checks
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}
2 Likes
#457 
   function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount, "Not enough balance.");
        require(msg.sender == owner, "You can only withdraw into your own account");
        msg.sender.transfer(amount);
        balance[owner] -= amount;
        return amount;
        
    }
1 Like