Transfer Assignment

[Restford]

function withdraw(uint withdrawAmount) public returns (uint) {
require(balance[msg.sender] >= withdrawAmount, “Insufficient balance!”);
balance[msg.sender] -= withdrawAmount;
msg.sender.transfer(withdrawAmount);
return balance[msg.sender];
}

[mcs.olive]

1) Make sure that the user can not withdraw more than their balance
require(balance[msg.sender] >= _amount, “Balance not sufficient”);
2) Correctly adjust the balance of the user after a withdrawal
balance[msg.sender] -= _amount;

    function withdraw( uint _amount) public returns (uint) {
        require(balance[msg.sender] >= _amount, "Balance not sufficient");
        balance[msg.sender] -= _amount;
        msg.sender.transfer(_amount);
        return balance[msg.sender];
    }

[jon_m]

Nice solution @0xBrito

You have added all of 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 me know if you have any questions

[jon_m]

An excellent solution @Restford

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

• Check inputs (require statements)
• Effects (update the contract state)
• External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= withdrawAmount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(withdrawAmount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

Please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

And don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Just let me know if you have any questions

[jon_m]

An excellent solution @mcs.olive

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

• Check inputs (require statements)
• Effects (update the contract state)
• External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= _amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(_amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

Don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Let me know if you have any questions

[codingluan]

Hi, this is my solution:

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

contract Bank {
    mapping(address => uint) balance;

    event depositDone(address indexed from, uint amount);
    event withdrawDone(address indexed to, uint amount);

    function deposit() public payable {
        balance[msg.sender] += msg.value;

        emit depositDone(msg.sender, msg.value);
    }

    function withdraw(uint _amount) public {
        require(balance[msg.sender] >= _amount, "balance not sufficient");

        _withdraw(msg.sender, _amount);

        emit withdrawDone(msg.sender, _amount);        
    }

    function _withdraw(address payable _to, uint _amount) private {
        balance[_to] -= _amount;
        _to.transfer(_amount);
    }

    function getBalance() public view returns(uint) {
        return balance[msg.sender];
    }
}

[JaimeAS]

 function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount); 
        
    }

[jon_m]

This is an excellent and well thought-out solution @codingluan

Your use of an additional helper function (_withdraw) is particularly interesting. It is appropriate, well coded, and you have done a great job of applying the same coding pattern used for the _transfer() helper function and adjusting it to the requirements of the withdraw() function. You have also ensured that the transfer method is still called on a payable address.

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the control flow, in order to reduce security risk:

1. Check inputs (require statements)
2. Effects (update the contract state)
3. External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[_to] -= _amount;

before actually transferring the funds out of the contract to the external address…

_to.transfer(_amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

You are also right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which a user can call separately to consult their own individual updated balance after a withdrawal.

Your withdraw event and corresponding emit statement are also both well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed.

Just let me know if you have any questions

[jon_m]

Hi @JaimeAS,

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

1. Check inputs (require statements)
2. Effects (update the contract state)
3. External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

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 should also include a return statement at the end of the function body. The compiler should have given you a yellow/orange warning for this.

Just let me know if you have any questions

[InterstellarX]

Hi! This is my solution: what I do is use the balance mapping for msg.sender and ensure that the balance is greater than or equal to the amount of withdraw at initiation. Then I subtract the amount from the balance of msg.sender to ensure that is OK and possible. Then I run the transfer function for the amount requested on msg.sender. It then returns a bool of success (either true or false) and I envision this can be helpful for event logging, where it can return logs/notifications upon withdraws — this is something I did in my function with the boolean success I implemented

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    // mapping(address => uint) public balances;
    address owner;

    // define event
        // indexed = by eth nodes, can use params to search/query for events in the past
    event depositDone(uint amount, address indexed depositedTo);

    event balanceTransfered(uint amount, address indexed depositedTo);

    modifier onlyOwner {
        require(msg.sender == owner);
        _; 
    }

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

    constructor() {
        owner = msg.sender;
    }

    // for anyone to deposit
        // payable - allows function to receive money when people call it
    function deposit() public payable returns (uint) {
        // if you deposit 1 ether, it should be attributed to account
            // don't need to worry about memory or storage, because saved to the blockchain
            // balance mapping keeps track of internal transfers/sending
        balance[msg.sender] += msg.value; // to internally track who deposited money
        // add event 
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender]; 
    }

    // function for folks to withdraw eth to their address
        
    function withdraw(uint amount) public returns (bool success) {
        // msg.sender is an address 
            // you can define an address payable sender and name whatever name you want to
            // address payable toSend = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
        
        // Transfer has revert abilities...if fails, will revert non-gas eth
            // transfer has built in error handling
            // MUST check balance of user to ensure can't overdraw/withdraw other people's money
            // toSend.transfer(amount);
        require(balance[msg.sender] >= amount); // prevents withdraw of others' money
        balance[msg.sender] -= amount; // remove amount from balance before transfer occurs
        msg.sender.transfer(amount); // run the transfer function
        return true;
    }

    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 money to yourself");

        uint previousSenderBalance = balance[msg.sender];
        
       _transfer(msg.sender, recipient, amount);

       // add event 
       emit balanceTransfered(amount, msg.sender);

       assert(balance[msg.sender] == previousSenderBalance - amount);
    }

    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

[jon_m]

Nice solution @InterstellarX

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

1. Check inputs (require statements)
2. Effects (update the contract state)
3. External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This isn’t, as you say, just “to ensure that is OK and possible” …

@InterstellarX

Then I subtract the amount from the balance of msg.sender to ensure that is OK and possible. Then I run the transfer function for the amount requested on msg.sender .

Our require() statement has already checked whether the withdrawal amount can be covered by the user’s individual balance, and it would have thrown an error and triggered revert if it couldn’t.

Instead, this is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one.

---

@InterstellarX

Then I run the transfer function for the amount requested on msg.sender . It then returns a bool of success (either true or false)

The transfer method doesn’t return a Boolean true or false. As you have said in one of your comments within your code…

@InterstellarX

 // Transfer has revert abilities...if fails, will revert non-gas eth
     // transfer has built in error handling

The point here is that if the transfer method itself fails, it will throw an error and trigger revert in the same way that require() does if that fails. When revert is triggered, the function doesn’t finish executing, and any changes made to the contract state — by the code in the function body that has already been executed before revert was triggered — are reversed (i.e. reverted). This is what we mean by “built-in error handling” — the transfer method doesn’t need to return a Boolean, because we don’t have to handle its success or error with any additional code of our own.

Returning true doesn’t prevent your solution from working, because the return statement will only be reached if the withdrawal has been successful; but it’s unnecessary to include. If the transaction is successful, then the transfer method will also have executed successfully.

The send method, on the other hand, doesn’t revert if it fails, and it does return a Boolean true or false, which we have to handle with additional code e.g…

function withdraw(uint amount) public {
    require(balance[msg.sender] >= amount, "Insufficient funds"); 
    balance[msg.sender] -= amount;
    bool success = msg.sender.send(amount);
    require(success, "Withdrawal failed");
}

---

@InterstellarX

It then returns a bool of success (either true or false) and I envision this can be helpful for event logging, where it can return logs/notifications upon withdraws — this is something I did in my function with the boolean success I implemented

If you want to emit an event and log specific data associated with the withdrawal, then you need to do this with an event declaration and an emit statement e.g.

event Withdrawal(uint amount, address indexed withdrawnBy);

function withdraw(uint amount) public {
    require(balance[msg.sender] >= amount, "Insufficient funds"); 
    balance[msg.sender] -= amount;
    msg.sender.transfer(amount);
    emit Withdrawal(amount, msg.sender);
}

---

@InterstellarX

// if you deposit 1 ether, it should be attributed to account
    // don't need to worry about memory or storage, because saved to the blockchain

To clarify …

A function is marked payable when it needs to receive ether from an external address to add to the contract address balance. Including payable in a function header enables the ether value sent to the function to be added to the contract address balance, without us having to add any further code for this. In the deposit() function, this happens automatically because we have marked the function as payable. The line …

balance[msg.sender] += msg.value;

… then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

@InterstellarX

    // balance mapping keeps track of internal transfers/sending

The balance mapping keeps track of each individual user’s share of the total contract balance, effectively the amount of ether each is entitled to withdraw from the contract. Each user’s share (or entitlement) changes with each deposit or withdrawal they make, and also with each internal transfer they are involved in as either the sender or the recipient.

Just let me know if you have any questions

[InterstellarX]

This was super helpful, thanks so much @jon_m!!!

[InterstellarX]

Actually, @jon_m i have a quick question for you…so my transfer function works, but only in the uint: 0 decoded output, but doesn’t seem to work in the actual ETH balance UNTIL I withdraw it, i’m just noticing this now looking at the balance of the accounts. Do you know what might be causing this? I would just think the transfer would be as instantaneous as the deposit, but I have to transfer to the address…and then that address needs to withdraw to their account for their balance to take shape.

Basically, the variable balance works, but when you look at the account address balance, it does not change, only can lose ETH, but not get it back or transfer it to someone else. It seems like I am depositing/transferring imaginary ETH, but the actual address’ balance in remix doesn’t change. I’m just not sure why the balance wouldn’t be updated instantaneously on transfer but you have to withdraw to then have your balance be affected.

Any clarity on this would be greatly appreciated!

[jon_m]

Hey @InterstellarX,

I’m not sure what you mean by…

@InterstellarX

so my transfer function works, but only in the uint: 0 decoded output …

… but I think the following might help you understand what’s happening …

Unlike the deposit() and withdraw() functions, calling the transfer() function doesn’t involve any ether entering or leaving the Bank contract at all. Instead, it performs an internal transfer of funds (effectively, a reallocation of funds) between two individual users of the Bank contract. The net effect to the contract’s ether balance is zero, and the only change is to the individual user balances in the mapping, in order to adjust the amount each of the parties involved in the internal transfer is entitled to withdraw from the contract (their share of the total pooled ether) i.e. the recipient’s entitlement is increased by the same amount the sender’s entitlement is reduced.

A user’s external address is used as the key to that user’s balance in the mapping, but the actual ether associated with, and tracked by, these individual user balances is the contract address balance. The individual user balances recorded in the mapping are, effectively, just accounting entries.

You will only see a change in the ether balance of a user’s external address (displayed in the Account field near the top of the Deploy & Run Transactions panel) when (i) ether is transferred from a user’s external address to the contract address by calling the deposit() function, and when (ii) ether is transferred from the contract address to a user’s external address when the transfer method is called on this address (msg.sender) from within the withdraw function.

So, this is why you …

@InterstellarX

… have to transfer to the address…and then that address needs to withdraw to their account for their balance to take shape.

… and why you are experiencing the following …

@InterstellarX

the actual address’ balance in remix doesn’t change. I’m just not sure why the balance wouldn’t be updated instantaneously on transfer but you have to withdraw to then have your balance be affected.

When the deposit() function is called, the user’s external address balance will decrease by the amount of ether sent from this address to the contract address. The contract address balance will increase by the same amount. If you add the following getter to your contract, you can call it to check the contract’s total ether balance at any particular point in time …

function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

When you call the getBalance() function, you are viewing a user’s share of (or entitlement to) the total contract balance returned by the getContractBalance() function.

When the withdraw() function is called, the contract address balance will decrease by the amount of ether sent from this address to the user’s external address balance. The user’s external address balance will increase by the same amount.

I hope this gives you the extra clarity you need

You may also find this post helpful. It lays out step-by-step instructions of how to properly check that all of your functions are working correctly, and it details what you should see happening and where, if it is.

Just let me know if anything is still unclear, or if you have any further questions

[InterstellarX]

Thanks so much, @jon_m! This was EXACTLY what I was curious on. The “internal transfer of funds” or relocation. I really appreciate the clarity on this. I thought something was wrong with my solution’s contract the way i wrote it, but turns out it was functioning as normal

[jon_m]

Hey Brian,

That’s great! … I’m glad that’s cleared things up for you

[c1cag1b3]

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;

address owner;

event depositDone(uint indexed amount, address indexed depositedTo);

event balanceTransfer(address sender, address recipient, uint indexed amount);

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, "Balance not sufficient");

    balance[msg.sender] -= amount;

}

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, "Dont transfer money to yourself");

    uint previousSenderBalance = balance[msg.sender];

    assert(balance[msg.sender] == previousSenderBalance - amount);

    _transfer(msg.sender, recipient, amount);

   

    assert(balance[msg.sender] == previousSenderBalance - amount);

    emit balanceTransfer(msg.sender, recipient, amount);

    }  

    function _transfer(address from, address to, uint amount) private {

    balance[from] -= amount;

    balance[to] += amount;

    }

}

[frandolz]

function withdraw(uint amount) public returns (uint){
require(balance[msg.sender] >= amount);
msg.sender.transfer(amount);
}
function transfer(address recipient, uint amount) public {
require(balance[msg.sender] >= amount, “Balance not enough”);
require(msg.sender != recipient, “Not for yourself”);
}
This will be my lines added

[jon_m]

Hi @c1cag1b3,

You have added both of the essential lines of code needed to solve the problem with the withdraw function

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 should also include a return statement at the end of the function body. The compiler should have given you an orange warning for this.

The other issues you have with your solution concern the order of the statements within your withdraw() function body…

• If an input value needs to be checked or validated with a require() statement, then this should normally be the first line of code in the function body to be executed. If require() fails, then the transaction will revert wherever it is placed; but any code placed before require(), and which will already have been executed before require() triggers revert, will result in less gas being refunded than if the check had been the first operation to be performed within the function body. This is because only the gas cost associated with the unexecuted operations after require() is saved; any gas already consumed executing code for the operations before and including require() is not refunded. You can probably see, therefore, that the earlier in the function require() is placed, the less the cost of gas will be if it throws and the transaction reverts.

• Have a look at this post which explains an important security consideration in terms of the order of the other two statements within your withdraw() function body.

See if you can reorder your code for these points, and also make the necessary modification in terms of returning, or not returning, the user’s new balance after the withdrawal.

---

Just one final comment …

You have a duplication of the assert() statement in your transfer() function. I’m sure this is unintentional, but because the 1st assert statement is in the wrong position it will throw an error and trigger revert whenever the function is called. If you remove it, then your transfer() function will work again as it should.

Let me know if you have any questions

[jon_m]

Hi @frandolz,

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

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 should also include a return statement in the function body. The compiler will have given you an orange warning about this.

Once you’ve made the above modifications to your code, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Also, please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it, in order to provide you with any help you might need.
Follow the instructions in this FAQ: How to post code in the forum

Let me know if anything is unclear, or if you have any questions about any of these points