Transfer Assignment

[CMar10]

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.

[jon_m]

Hi @PhilD99,

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

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

[jon_m]

An excellent assignment solution @Ominira 

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.

[jon_m]

Hi @CMar10,

@CMar10

I had to look at Filip’s answer to complete this assignment. It makes sense now.

That’s fine to check the solution after you’ve worked hard at solving it yourself first… which you did

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

@CMar10

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.

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.

[Mickey_McClimon]

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

this would prevent them from withdrawing too many funds.

[jon_m]

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

[DylanHepworth]

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

[hihaiu]

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

[CMar10]

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

[jon_m]

Hi @CMar10,

This part of your assert statement isn’t right:

@CMar10

balance - msg.sender.transfer(amount)

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?

[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