Transfer Assignment

Paul_Mun · December 21, 2020, 3:27pm

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

Adding this as the third line of the withdraw() function…
balance[msg.sender] - amount;
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!

jon_m · December 21, 2020, 6:15pm

Hey Paul,

Very nearly spot on!

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 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!

Paul_Mun · December 21, 2020, 7:27pm

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

inahan · December 21, 2020, 8:46pm

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

jon_m · December 21, 2020, 9:16pm

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.

jon_m · December 21, 2020, 9:25pm

Excellent @inahan

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

Gorana · December 21, 2020, 11:23pm

Thank you so much for your time!
Its not easy with us starters
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”

Lamar · December 22, 2020, 1:03am

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];

ArtCenter1 · December 23, 2020, 12:22pm

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];

}

jon_m · December 23, 2020, 5:58am

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 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!

jon_m · December 23, 2020, 6:52am

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.

jon_m · December 23, 2020, 7:18am

Excellent @ArtCenter1

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.

ArtCenter1 · December 23, 2020, 12:20pm

Thank you!

tnc · December 23, 2020, 6:14pm

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.

Bobcat · December 24, 2020, 1:09am

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.

Andro · December 24, 2020, 3:37am

    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.

seer · December 24, 2020, 2:50pm

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];
    }

thecil · December 24, 2020, 3:57pm

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!

Carlos Z.

ArvidK · December 25, 2020, 4:07pm

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!

kHarsh · December 26, 2020, 12:41am

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];
}