Transfer Assignment

[jon_m]

Excellent solution @DylanHepworth

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:

@DylanHepworth

balance[msg.sender] -= amount; //adjusts balance after withdraw occurs. 
msg.sender.transfer(amount);

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

[Mickey_McClimon]

    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?

[jon_m]

Yeh that’s great @Mickey_McClimon

As you have a return statement, returning a uint value, you need to include returns(uint) in the function header (which was there originally). The compiler should have displayed an error (in red) for that, helping you to see the problem.

[Mickey_McClimon]

I did add it back after I posted this haha of course I missed it!

doing some other debugging in JavaScript so sorry about that!

[jon_m]

Nice solution @hihaiu

You have added 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.

Just let us know if you have any questions

[PhilD99]

Thanks jon_m, appreciate your help!

[Bhushan_Pawar]

function withdraw (uint amount) public payable returns(uint)
{
require (balance[msg.sender] >= amount,“insufficient Funds!”);
msg.sender.transfer(amount);
balance[msg.sender] -= amount;

}

[jon_m]

Hi @Bhushan_Pawar,

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

The withdraw function doesn’t need to be marked payable, because it is sending ether. Functions and addresses only need to be payable in order to be able to receive ether.

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

[Jacob_Pettit]

pragma solidity 0.7.5;

// trasnfer contract

contract HelloWorld {
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    modifier onlyOwner {    
        require(msg.sender == owner);
        _;
    }
        
    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 withdraw(uint amount) public returns(uint) {
        msg.sender.transfer(amount);
        require(balance[msg.sender] >= amount);        // solution
        balance[msg.sender] -= amount;                 // solution
    }
    
    function getBalance() public view returns(uint) {
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {  
        require(balance[msg.sender] >= amount, "Balance not sufficient"); 
        require(msg.sender != recipient, "Don't transfer to youself");       
        uint previousSenderBalance = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
       assert(balance[msg.sender] == previousSenderBalance - amount); 
        
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

[Bhushan_Pawar]

Thanks Jon! Updating in my memory

[Andrewg]

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

}

[Meriem]

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?

[jon_m]

Hi @Meriem, and welcome to the forum!

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

@Meriem

We have to add a line of code and substract the amount from the previous balance

Correct

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

@Meriem

… the balance doesn’t update automatically.

… 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

[jon_m]

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

[jon_m]

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

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

[Meriem]

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

}

[jon_m]

An excellent assignment solution @Meriem

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

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.

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.

[decrypter]

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

}

[jon_m]

An excellent assignment solution @decrypter

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

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:

@decrypter

assert (balance[msg.sender]==previousSenderBalanceamount);

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

[Michael_Gerst]

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