Transfer Assignment

Ethereum Smart Contract Programming 101
#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
#32

Thank you for your reply. I can see now that the solution is buggy for no reason (I didn’t know that with assignment a pointer is created and I tried to find the optimal solution).
It’s was an enlightening insight.

1 Like
#33 
function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount, “Not enough available funds.”);
balance[msg.sender]-=amount;
msg.sender.transfer(amount);
return balance[msg.sender];
}

I added the ‘require’ function that’ll compare the current balance of the sender to amount they’re trying to withdraw. If this ‘require’ statement resolves as false then the message “Not enough available funds.” will return and the transaction will revert. If the function is successful, the now updated balance of the sender will be returned.

1 Like
#34 
    function withdraw(uint amount) public returns (uint){
        require(balanceMapp[msg.sender] >= amount);
        msg.sender.transfer(amount);
        balanceMapp[msg.sender] -= amount;

I modified the function to check whether the msg.sender has more funds than he wants to withdraw and also his balance is reduced by the amount withdrawn.

I checked it and it seems to be working although it’s not really clear to me why it’s working.

1 Like
#35

So, I added the following:

  • Require to make sure the withdrawal does not exceed the deposited amount
  • Updated the internal balance after transfer
  • Added assertions to make sure the new balance is correct and not below zero.
function withdraw(uint amount) public returns(uint) {
        require(balance[msg.sender] >= amount);
        uint oldBalance = balance[msg.sender];
        
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        
        assert(balance[msg.sender] == oldBalance - amount);
        assert(balance[msg.sender] >= 0);
        
        return balance[msg.sender];
    }
1 Like
#36

It is always more easy to understand if we comment each line of code, that way we could have a guideline on what that function suppose to do.

Here is an example based on your function:

//withdraw funds from contract, transfer to msg.sender the required amount if is permitted
    function withdraw(uint amount) public returns (uint){
// check that balance of the user account is less or equal to amount requested
        require(balanceMapp[msg.sender] >= amount);
//transfer the amount to msg.sender
        msg.sender.transfer(amount);
//update the mapping value for that account after a successful transfer 
        balanceMapp[msg.sender] -= amount

If you have any more questions, please let us know so we can help you! :slight_smile:

Carlos Z.

1 Like
#37

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

I created a new value called “newBalance” which should act as the balance after withdraw has been made. They specifying what it will do, the new balance should be equal to the function callers amount minus the withdrawn amount. Then it will be saved to the caller of the function and not across multiple addresses. In the end I returned the new balance.

Let me know if I have missed something or got something wrong!

1 Like
#38

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

#39

@filip I submitted the answer previously, just a quick question, besides addresses depositing in to a contract what are the other ways a Contract holds value, so in the example we are just selecting from value button the amount we want to deposit in to an account, but my question is how does Contract gets that value initially?

1 Like
#40

function withdraw(uint amount) public returns (uint){
// only transfer if msg.senders balance is >= amount
require(balance[msg.sender] >= amount, “Non-sufficient Funds”);
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
return balance[msg.sender];
}

1 Like
#41

Hi @ArvidK,

Your solution won’t prevent the caller from withdrawing more than their account balance, because your function is missing two important lines of code:

  • You need a require statement to check that the caller’s balance is sufficient to cover the withdrawal amount. If you don’t include this, then they can withdraw any amount up to the overall contract balance!

  • You are recording the correct new balance and returning it as the output, but you don’t update the mapping for this. Your newBalance is a local variable and so its value is lost once the function has finished executing. In the mapping, balance[msg.sender] will remain as the previous balance, and so even after adding the require statement I’ve mentioned above, the address withdrawing the funds could still keep calling the withdraw function repeatedly for an amount lower than their original balance, even after they’ve withdrawn all of it. In this particular scenario, there isn’t actually any need to store the remaining balance locally before updating the mapping. You can perform this with just one single line of code.

Once you’ve had a go at making the above modifications, 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 unclear, or if you have any questions and we’ll help you :slight_smile: