Transfer Assignment

Ethereum Smart Contract Programming 101
#405

Hi @M.K

.transfer() is a method of any payable address therefore is not related to the transfer function declared in your contract.

Cheers,
Dani

1 Like
#406

event withdrawals (uint AmountWithdraw, address WithdrawBy);

function withdraw(uint amount) public returns(uint) {
require(Balance[msg.sender] >= amount, “Not enough money to withdraw this amount!”);
Balance[msg.sender] -= amount;
msg.sender.transfer(amount)
emit withdrawals (amount, msg.sender);
}

1 Like
#407

Here is my answer:

function withdraw(uint amount) public returns (uint){
//check balance of msg.sender
require(balance[msg.sender] >= amount, “Balance not suficient”);
msg.sender.transfer(amount);
//reduce balance of mapping
balance[msg.sender] -= amount;
return balance[msg.sender];
}

1 Like
#408

Hi @bfleming98,

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:

#409

Nice solution @Nelson1 :ok_hand:

You have included all of the additional lines of code needed to solve the problem with the withdraw function, and you have them all in an order that optimises security:

  1. check inputs (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).

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.

The additional event that 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 with a capital letter (e.g. event Withdrawal ), although your code does still work using a lower case letter. What is a problem, though, is writing the mapping name balance starting with a capital letter: Balance. If you have also used an upper case letter in your mapping name, then your code will work, but if you haven’t then it will throw an error. Remember that it’s standard practice to start variable names (including mappings and arrays) with a lower case letter. It’s just struct and event names that we start with a capital letter.

Your posted solution will also not execute, because you’ve missed off a semi colon at the end of one of your lines. You should format your code before posting it. This should also make it easier to spot any errors like the ones I’ve just mentioned.
Follow the instructions in this FAQ: How to post code in the forum

#410

An excellent assignment solution @galgostark :muscle:

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. I think you’ll find it interesting :slight_smile:

Just one final thing. Please format your code before posting it, so it’s clearer and easier to read. Follow the instructions in this FAQ: How to post code in the forum

#411

Change the withdraw function to the following:

function withdraw (uint amount) public returns (uint){

   require(balance[msg.sender] >= amount);
   msg.sender.transfer(amount);
   balance[msg.sender] -= amount;
1 Like
#412 
    function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        msg.sender.transfer(amount);
        uint previousAmount = balance[msg.sender];
        balance[msg.sender] -= amount;
        assert(balance[msg.sender] == previousAmount - amount);
        return balance[msg.sender];

    }
1 Like
#413 
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 started out trying to create code similar to the transfer function by creating a uint previousBalance
and then creating an assert function that would subtract transfer balance from previousBalance.
This got very complicated fast and never worked.  I had to look at Filip's answer to complete this assignment. It makes sense now.
1 Like
#414

Hi @PhilD99,

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:

#415

An excellent assignment solution @Ominira  :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:

#416

Hi @CMar10,

That’s fine to check the solution after you’ve worked hard at solving it yourself first… which you did :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. This order, therefore, reduces security risk. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information. You’ll learn about the type of attack this prevents, and how it does it, in later courses. But don’t worry about the details for now, although it’s good to be aware of and to start getting into good habits :slightly_smiling_face:

Have a look at @Ominira’s solution. This achieves what I think you were trying to do. Just be aware that this solution doesn’t have the lines of code in an order that optimises security (as your solution does). So, see if you can now implement an assert statement, whilst maintaining the order of your other statements.

#417

require (balance[msg.sender] >= amount, “Not enough funds”);

this would prevent them from withdrawing too many funds.

#418

Hi @Mickey_McClimon ,

Your require statement is correct, but can we see your whole withdraw function? You need to add more than just the require statement:

  • If you deploy your contract and call the withdraw function having only added the require statement, 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!

  • 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 also added these, 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, or if there is anything that you don’t understand :slight_smile:

#419 
pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositTo);
    
    event amountTransfered(uint amount, address recipientAddress, address senderAddress);
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; // run the function
    }
    
    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 getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint){
        //makes sure user cannont with draw more than their deposited funds. 
        require(balance[msg.sender] >= amount, "Insufficent funds for withdrawl");
        balance[msg.sender] -= amount; //adjusts balance after withdraw occurs. 
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficent");
        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);
        
        emit amountTransfered(amount, recipient, msg.sender);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}
1 Like
#420
  1. Compare that the amount to withdraw is lesser or equal the account balance, else withdraw will be reverted and will return an error with message “insufficient funds”.
  2. Transfer the amount
  3. Update the users balance
    function withdraw(uint amount) public
    {
        // 1.
        require(amount <= balance[msg.sender], "insufficient funds");
        // 2.
        msg.sender.transfer(amount);
        // 3.
        balance[msg.sender] -= amount;
    }
1 Like
#421

I’m not certain the syntax of this assert is correct, but this might ensure that the correct balance is maintained.

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

Hi @CMar10,

This part of your assert statement isn’t right:

It throws an error in the compiler, because:

  • balance is the name of the mapping, but you can’t reference the mapping without including the key (address) of one of its values, like with balance[msg.sender]
  • You can’t use  msg.sender.transfer(amount) as this doesn’t evaluate to a value.

Instead, you first need to save the initial balance to a local variable, and then reference that in the assert statement. So, you need two lines of code:

uint initialBalance = balance[msg.sender];
/* Once the initial balance has been saved locally, you can now adjust it
by deducting the withdrawal amount. Then you can perform the transfer of
ether from the contract to the caller's external wallet */
assert(initialBalance == balance[msg.sender] + amount);
// or
assert(initialBalance - amount == balance[msg.sender]);

Also, when execution reaches a return statement, the function is exited, and so your assert statement in its current position is unreachable. You will see this indicated in Remix as a compiler warning, once the actual code of your assert statement is valid.

Do you want to have another go? :slight_smile:

#424

Excellent solution @DylanHepworth :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.

This is what you’ve done, but your comment says the opposite:

The balance stored in the mapping (in the contract state) is adjusted before the withdrawal of ether occurs.

We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information. You’ll learn about the type of attack this prevents, and how it does it, in later courses. So, don’t worry about the details for now, although it’s good to be aware of and to start getting into good habits :slightly_smiling_face:

#425 
    function withdraw(uint amount) public onlyOwner {
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
    
Would this be correct as you are taking the balance - the amount before the transfer? is that what you were asking for?
1 Like