Transfer Assignment

Ethereum Smart Contract Programming 101
#12

Excellent solution to this assignment @dcota :muscle:

You can remove the payable keyword from the withdraw function, though, because you only need to make a function payable if it is receiving ether.

You’ve also made a good job of the additional event for the transfer :ok_hand:

Just a couple of other observations:

  • It’s a good idea to indent your code in the function bodies. This makes it clearer and easier to read.

  • Maybe it’s just a copy-and-paste error but you are missing a few lines of code in your transferTo function:
    - 2nd require statement (You can’t transfer tokens to yourself).
    - uint previousSenderBalance = balance[msg.sender];
    - assert statement: you also need to think carefully about the positioning of your emit statement (for your transfer event) in relation to assert()

Keep up the great work! :slight_smile:

#13

Hi @tnc,

Yes, this is the require statement that is needed.

This won’t solve the original problem, because  address(this)  refers to the address of the contract, not the msg.sender. So this alternative require statement would allow withdrawal of any amount up to the entire contract balance — which is the problem we started with.

Your solution is also missing 2 other important things:

  • If you only add the require statement, and then deploy your contract and call your 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! How would you correct this?
  • You need a return statement in the 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.

1 Like
#14

Hi @Paul_Mun,

The require statement you’ve added is correct, but your function is missing 2 other important things:

  • 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?
  • You are missing a return statement in the 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.

1 Like
#15

Hi @cmeronuk,

The require statement and the return statement you’ve added are both correct :ok_hand: But you are missing one other important line 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! The value which is returned from the function (the output) should be the reduced balance, but it will be the same as it was before the withdrawal. I’m sure you can see that this is another very serious bug! How would you correct this?

You have also forgotten to remove onlyOwner . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds.

Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand.

#16

Hi @Gorana,

The require statement and the return statement you’ve added are both correct :ok_hand: But you are missing one other important line 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! The value which is returned from the function (the output) should be the reduced balance, but it will be the same as it was before the withdrawal. I’m sure you can see that this is another very serious bug! How would you correct this?

You have also forgotten to remove onlyOwner . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds. In fact, because you have included the owner variable and the onlyOwner modifier, but removed the constructor, whichever address deploys the contract won’t be set as the owner, which will remain the zero address by default. This means than none of the account holders will be able to withdraw any of their funds. You can actually remove the owner state variable and the onlyOwner modifier, as they aren’t used anymore.

Did you have a go at adding the additional Transfer event?

Yeh, this can happen every now and again, and is easily sorted by just exiting and reloading Remix. This should reset all of the address balances to 100 ETH.

Let us know if you have any questions, or if there is anything that you don’t understand :slight_smile:

#17

Hi @Lamar,

…because your require statement references one of the function parameters (amount). A modifier can take parameters, but these then need to be fixed in the function header for each function that it modifies, and cannot be input by the caller of the function (which is what happens with amount). Filip demonstrated this in the lecture on modifiers using a modifier called costs e.g.

/* The costs parameter (price) is not input by the caller of the function.
   It's fixed for each function in the function header */

modifier costs(uint price) {
    require(msg.value >= price);
    _;
}

function transfer(address recipient, uint amount) public payable costs(100) {...}

The require statement you’ve added to the withdraw function is correct, but your function is missing 2 other important things:

  • 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?
  • You are missing a return statement in the 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, and we’ll help you out :slight_smile:

#18

Hi @RojasMessilia,

The require statement you’ve added is correct :ok_hand: but your function is missing 2 other important things.

When you deploy your contract and call your withdraw function as it is, your assert function will always throw. This should obviously only happen if there is a flaw in your code, which there is. Your assert function itself is coded very well :ok_hand: and it’s actually a good thing that it always throws, because that prevents a missing line of important code from allowing the caller to withdraw more funds than their actual balance. Let’s take an example…

  1. msg.sender has 1 ether balance, and calls the withdraw function with an amount argument of 1 ether.
  2. require does not throw (correct).
  3. previousBal = 1 ether;
  4. 1 ether is deducted from the contract balance and transferred to msg.sender BUT IT ISN’T deducted from balance[msg.sender] (the individual account balance stored in the mapping) which remains 1 ether.
  5. assert throws and reverts the transaction because previousBal does not equal balance[msg.sender] + amount
    1 != 1 + 1

The gas cost does not affect this calculation/condition, because it is deducted from the caller’s external account balance NOT their internal balance stored and tracked in the balance mapping and accessed with balance[msg.sender].

How would you correct this?

You are also missing a return statement in the function body.

Don’t get your functions mixed up. This should be the withdraw function, not the transfer function. And also be careful with your spelling…

and

Both of these will prevent your code from compiling.

Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand, and we’ll help you out :slight_smile:

1 Like
#19

Ah ok I see what you’re saying! So would these changes be correct?

  1. Adding this as the third line of the withdraw() function…
    balance[msg.sender] - amount;

  2. Adding the return statement below this line as so…
    return balance[msg.sender];

So the final product would end up coming out to…

function withdraw(uint amount) public onlyOwner returns (uint) {
        require(balance[msg.sender] >= amount, "Insufficient Funds: Please enter withdraw amount less than your current balance");
        msg.sender.transfer(amount);
        balance[msg.sender] - amount;
        return balance[msg.sender];

    }

Let me know if this are the appropriate corrections. Thanks!

1 Like
#20

Hey Paul,

Very nearly spot on! :muscle:

Correct — this return statement should be on the last line.

So that the value balance[msg.sender] is actually modified in the storage in the mapping, you need to assign the new value to it. So you need an assignment operator  =
What you have currently is just an expression that calculates a new value, but then doesn’t do anything with it. So…

balance[msg.sender] - amount;
/*
This is correct for the new calculated balance.
You then need to assign it to balance[msg.sender] in the mapping:
*/
balance[msg.sender] = balance[msg.sender] - amount;
/*
There is a more concise way to do this using the
subtraction assignment operator  -=
*/
balance[msg.sender] -= amount;
// This avoids repeating balance[msg.sender]

This will now work. However, it’s actually very important for security to put
balance[msg.sender] -= amount;  before  msg.sender.transfer(amount);
We wouldn’t expect you to know this at this early stage though, so I’m just telling you for extra info. You’ll learn about the type of attack this prevents and how it does it in later courses. In summary, it’s important to modify the contract state before you actually perform the transfer of funds out of the contract, just in case there is an attack after the transfer, but before the state is modified to reflect this operation. But don’t worry about the details for now, but it’s good to be aware of and to start getting into good habits now.

You also need to remove the onlyOwner modifier from the function header. You’d correctly removed it from your first version, but for some reason it’s crept back in :wink: If you restrict this function to onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw their funds.

But, well done! You’re making great progress! :smiley:

#21

Ok great, ahh I total forgot about using the balance[msg.sender] -= amount

Oh ok, that is very interesting that putting that statement above the transfer line could be a protection against an attack. I would definitely not have thought that!

Ya I’m actually not sure why I put that back in there. I think the next assignment had me toggle that onlyOwner part back on. That is a good reinforcement for me though. So the only owner would prevent anyone from else that holds an account to withdraw their funds.

Great, thank you for this explanation. That really helped clarify things for me :+1:

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

Yes, because onlyOwner triggers the require statement in the modifier with that name:

require(msg.sender == owner, "Function can only be called by contract owner");

If you still have this modifier, the constructor and the owner state variable in your contract, then
this restriction would be applied to the withdraw function inputs before any of the other code in its function body executes.

#24

Excellent @inahan :muscle:

You have added all of the additional code that was missing.

The only improvement that can be made is to put
balance[msg.sender] -= amount;  before  msg.sender.transfer(amount);
This is important for security. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra info. You’ll learn about the type of attack this prevents, and how it does it, in later courses. In summary, it’s important to modify the contract state before you actually perform the transfer of funds out of the contract, just in case there is an attack after the transfer, but before the state is modified to reflect this operation. But don’t worry about the details for now, but it’s good to be aware of and to start getting into good habits :slight_smile:

22 Likes
Solidity Payable Functions - Discussion
Inheritance Assignment
Solidity Payable Functions - Discussion
Inheritance Assignment
Inheritance Assignment
Inheritance Assignment
Solidity Payable Functions - Discussion
Solidity Payable Functions - Discussion
Inheritance & External Contracts - Discussion
Solidity Payable Functions - Discussion
#25

Thank you so much for your time!
Its not easy with us starters :slight_smile:
I changed it to:
pragma solidity 0.7.5;
import “./Ownable.sol”;
import “./Destroyable”;

contract Bank is Ownable, Destroyable {

mapping(address => uint) balance;

event depositDone(address indexed depositedTo, uint ammount);

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

function transfer(address recipient, uint amount) public {
require(balances[msg.sender] >= amount, “Not enough balance!”);
require (msg.sender != recipient, “dont send money to yourself”);
uint previousSenderBalance = balance [msg.sender];
_transfer(msg.sender, recipient, amount);
}

function _transfer(address sender, address recipient, uint amount) private {
balances[sender] -= amount;
balances[recipient] += amount;
}

function withdraw(uint amount) public returns(uint) {
require(balances[msg.sender] >= amount, “Not enough balance!”);
require (msg.sender != recipient, “dont send money to yourself”);
balances[msg.sender] -= amount;
msg.sender.transfer(amount);
return balances[msg.sender];
}

function getbalance() public view returns(uint) {
return balances[msg.sender];
}
}
and now Im gettin’ another error saying “not found browser/Destroyable”

1 Like
#26

Thank you for the help!

So if I code; modifier balanceOf(uint amount){
require(balance[msg.sender] >= amount);
_;
}
Is this correct then?

function withdraw(uint amount) public onlyOwner returns(uint){
require(balance[msg.sender] >= amount);
//address payable test = ;
//test.transfer(amount);
msg.sender.transfer(amount);//transfer reverts and gives an error if its something wrong
balance[msg.sender] -= amount;
return balance[msg.sender];

1 Like
#27 
function withdraw(uint amount) public returns (uint){

require(balance[msg.sender] >= amount, “Insufficient funds”);
//msg.sender.transfer(amount);----old location
balance[msg.sender]-=amount;
msg.sender.transfer(amount);//new location
return balance[msg.sender];

}
1 Like
#28

Hi @Gorana,

You’ve got some additional code from a later assignment mixed in here. Either remove the imports of Ownable.sol and Destroyable.sol, and also  is Ownable, Destroyable  from the contract header, or make sure you have the relevant Ownable.sol and Destroyable.sol files in Remix so that Bank can do what it’s being told to do and import and inherit from them. If you are executing Bank with the imports and inheritance, then before clicking “Deploy”, check the dropdown in the “CONTRACT” field just above and you should have all 3 contracts listed there. You need to make sure that Bank is selected from the dropdown and showing in the field when the dropdown is collapsed.

This is now all correct apart from the 2nd require statement. This is for the transfer function, and seems to have crept in here somehow :wink: It will give you a compiling error, because recipient isn’t a parameter in this function, or a defined variable.

You have also used the mapping name balance inconsistently throughout the contract. Sometimes you have balances instead. This will also give you compiling errors.

If you correct those things, then it should work a dream! :grin:

#29

Hey @Lamar,

No, you can’t use a modifier here, because the require statement for our withdraw function references the function parameter amount. The function needs to accept different values for this input parameter (depending how much the different users want to withdraw). Your modifier balanceOf would only be of any use if amount was fixed for each function it was used in — it could be a different amount in different functions, but fixed for each one. To code this, you would need to put the fixed amount in parentheses after the modifier name in your function header e.g. if the fixed amount for the withdraw function was 100, we would have

function withdraw(uint amount) public balanceOf(100) returns(uint) { ... }

Notice that we would have to use the name of the modifier we want to use in the function header (not onlyOwner — that’s a different modifier and not correct here because we don’t want to restrict withdrawals to just the contract owner).

As you can probably see, this is now a completely useless function, because we want the restriction to change depending on the requested withdrawal amount. So, your withdraw function is correct if you remove onlyOwner from the function header, and add the missing closing curly bracket at the end of the function. In this case, it is correct to have the require statement within the function body on the first line (as you have) and not in a modifier.

Have a read through my previous post again, where I give an example of where a modifier with a require statement and a parameter is appropriate.

Also, have a look at this post where I explain another important security modification that should be made to the order of the statements within your withdraw function body.

Let us know if anything is still unclear, or if you have any further questions.

#30

Excellent @ArtCenter1 :muscle:

You have added all of the additional code that was missing.

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.

#31

Thank you! :heart_eyes:

1 Like