Transfer Assignment

jon_m · July 14, 2021, 9:23pm

Nice solution @Ben17

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

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 can be called separately to consult the individual account holder’s updated balance after the withdrawal.

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 · July 14, 2021, 10:08pm

This is a very well-coded and annotated assignment solution @Jackthelad

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

Just one observation …

This is the actual transfer of the ether amount from the smart contract’s address balance. All of the ether deposited by all of the individual account holders is effectively pooled, and held as a single balance in the contract’s account.
This is why we also need to reduce the individual account holder’s balance in the mapping by the same amount. It is an essential accounting adjustment that enables us to track the individual’s share of the total pooled funds and, therefore, the maximum amount they are entitled to withdraw from the contract.

You should find it as part of the larger error message (with the red cross) in the Remix console below your code. It should appear in the penultimate line, as follows…

Reason provided by the contract: "Not enough funds".

Just let me know if anything is unclear, or if you have any further questions. And if you’re still having problems generating your error message, then post your full code and I’ll take a look

Jackthelad · July 18, 2021, 1:58am

 function withdraw(uint amount) public returns (uint){
        /*Checking users share of the contract’s total ether balance is sufficient
        by referencing mapping.*/
        require(balance[msg.sender] >= amount);
        /*If condition is met, reduce the function callers balance 
        (total share of pool) by the transfer amountusing the mapping.*/
        balance[msg.sender] -= amount;
        //Actual transfer occurs from the smart contract’s address 
         
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

Would these annotations be more accurate? Still early days in my understanding, presuming I am making errors pretty often. Really appreciate the feedback though!

GahuckImGoofy · July 15, 2021, 7:16pm

pragma solidity 0.7.5;
contract Bank {

    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address depositedTo);
    
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the funciton
    }

    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){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

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

}

Gos · July 16, 2021, 12:52am

Hi @jon_m,

Thanks for the great feedback!

I fixed the issue, but i don’t remember how. Thanks for asking
This course is really teaching me how to solve problems!

suhan_kim · July 16, 2021, 2:20pm

function withdraw(uint amount) public {
require(amount <= balance[msg.sender]);
msg.sender.transfer(amount);
balance[msg.sender] -= amount;

Andre_Mark · July 16, 2021, 7:10pm

All, below is my attempt below, been struggling for days it seems and the below is as far as I’ve got, but something is missing and my brain isn’t yet connecting the dots, can anyone suggest which lesson to go to; I have looked through the thread and seen how others have worked it out, but something is missing here and I’m not sure what, any advice on how to think this one out would be good,

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

gregD · July 16, 2021, 7:49pm

function withdraw (uint amount) public returns (uint) {

    // Check the caller has enough Eth to withdraw in the balance mapping
    require(balance[msg.sender] >= amount, "Insufficient funds");
    
    // Withdraw the Eth to the address that called the function
    msg.sender.transfer(amount);
    
    // Update the balance of the address in the balance mapping
    balance[msg.sender] -= amount;
    
    // Return the new balance of the address
    return balance[msg.sender];
}

jon_m · July 17, 2021, 6:16pm

Hey Jack,

Your code is very good, and accurate. As a beginner, it’s completely normal for there to be scope for improvements with written descriptions of what the code is doing. They will become more accurate as you gain a deeper understanding, and that happens gradually. This is also why it’s good to try to describe what the code is doing in your own words, because it forces you to really think about it, and this will help both you and others identify gaps in your knowledge.

Your comments have improved, but the concept of “pooled” funds applies to ETH, and not to tokens. The contract initially handled non-ether amounts, but we changed that to ETH when we changed the deposit() function to receive value transfers into the contract’s own account (referenced by msg.value). As an example of what I mean, your first comment would be more accurate as follows:

“Checking users share of [the contract’s total ether balance] is sufficient by referencing mapping.”

Just let me know if you have any further questions

jon_m · July 17, 2021, 6:26pm

An excellent assignment solution and, overall a very well-coded contract @GahuckImGoofy

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

check inputs (require statements)
effects (update the contract state)
external interactions

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

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does 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’re making great progress!

jon_m · July 17, 2021, 6:32pm

Nice solution @suhan_kim

You have added all of the additional lines of code needed to solve the problem with the withdraw function.

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 can be called separately to consult the individual account holder’s updated balance after the withdrawal.

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 · July 17, 2021, 8:40pm

Hi @Andre_Mark,

The require statement and the return statement you’ve added to your withdraw function are both correct

Yes… you are missing one very important line of code…

If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). This is a serious problem, because, even though the require statement places a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal.

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

Look again at the code in your deposit() function and work out which line increases the caller’s individual account balance in the mapping. The additional line of code you need in your withdraw() function will be the same, but instead of increasing the user’s balance, it needs to reduce it instead.

Once you’ve had a go at correcting your code for this, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

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

jon_m · July 17, 2021, 8:46pm

Nice solution @gregD

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

Andre_Mark · July 18, 2021, 7:09pm

Hi,
I have read your response several times and looked at my code, and I think I understand it, but then again not so much - paradox.

I looked at deposit and thought that you were referring to the below line to include,

balance[msg.sender] += amount;

But then I read your post and you mentioned to have,

balance[msg.sender] -= amount; before msg.sender.transfer(amount);

Which is what I thought I done, and now it looks like the below;

function withdraw (uint amount) public onlyOwner returns (uint) {
        require(balance[msg.sender] >= amount, "Insuffcient funds");
        balance[msg.sender] -= amount;
        **balance[msg.sender] += amount;** -  I am assuming this needs to be removed
        msg.sender.transfer(amount);
        return balance[msg.sender];

How else could I look at this challenge?
Andre

WMXgTW6wh · July 19, 2021, 2:20am

Here’s my withdraw function code:

    function withdraw(uint amount) public returns (uint){
        
        //require enough balance
        require(balance[msg.sender] >= amount, "Withdrawal balance insufficient");
        
        //reduce balance amount
        balance[msg.sender] -= amount;
        
        //check balance reduction
        uint previousBalance = balance[msg.sender];
        assert(balance[msg.sender] == previousBalance - amount);
        
        //make the transfer
        payable(msg.sender).transfer(amount);
        
        //return balance
        return balance[msg.sender];
    }

I had to add “payable” in front of (msg.sender) to comply with solidity 0.8.4.

When testing the withdrawal in Remix, I correctly see the function throw an error due to insufficient balance.

However, I’ve tried depositing and then running a withdrawal, but the balance isn’t saved and I still receive an error - is this due to not saving the balance in “storage”?

full code:

pragma solidity 0.8.4;

contract Bank {
    
    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the function
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposit(uint _toAdd) public payable returns (uint){
        balance[msg.sender] += msg.value; //only necessary to track internal balances by address
        emit depositDone(_toAdd, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint){
        
        //require enough balance
        require(balance[msg.sender] >= amount, "Withdrawal balance insufficient");
        
        //reduce balance amount
        balance[msg.sender] -= amount;
        
        //check balance reduction
        uint previousBalance = balance[msg.sender];
        assert(balance[msg.sender] == previousBalance - amount);
        
        //make the transfer
        payable(msg.sender).transfer(amount);
        
        //return balance
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Transfer balance insufficient");
        require(msg.sender != recipient, "No self transfers");
    
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        // event logs and further checks
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

Epicdisaster · July 20, 2021, 2:50pm

function withdraw(uint amount)public {
// error handling
require(balance[msg.sender] >= amount, “You cannot withdraw more money than you have”);

    //Adjust the balance of msg.sender
    balance[msg.sender] -= amount;
    msg.sender.transfer(amount);

jon_m · July 20, 2021, 5:37pm

Hi @Andre_Mark,

Apart from the onlyOwner modifier (which you don’t need to add), your solution is now correct

By adding the onlyOwner modifier, only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think?

Yes, that’s right, but …

What I meant here, without actually telling you the answer was that you needed a line of code which adjusts the user’s balance in the mapping, but reduces it instead of increasing it, because we are withdrawing not depositing.

So you’re right you need to add …

and not…

So, yes, that line needs to be removed

If you already had these two statements in the correct order, then you wouldn’t need to make any further changes. I only asked you to look at this post so you could check the order, and for you to read why this particular order is important for security: because it prevents a specific type of attack.

Apart from the onlyOwner modifier, I think you’ve now done a good job at working out what needed to be added and where

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

jon_m · July 20, 2021, 11:44pm

Hi @WMXgTW6wh,

You have added all of the additional lines of code needed to solve the problem with the withdraw function The problem with it is the two lines of code you’ve added for the assert() statement. You are saving the user’s balance to the local variable after you’ve reduced it for the withdrawal amount, and so you’re not saving the previous balance. This means that, if the require() statement evaluates to true and revert isn’t triggered for an insufficient balance, then assert() will always fail, instead.

This error is caused by your deposit() function being incorrectly coded. You need to go back and re-watch the video where you are shown how to code this function, before the assignment. You have some code left over from the previous addBalance function, which isn’t correct now that we are depositing ether into the contract, and not just an amount which represents some other kind of value of our own creation.

The deposit() function is receiving ether from the caller’s external address, so that it can be added to the contract address balance. To perform this kind of value transfer you need to send an amount in wei to the function which is receiving it; but this isn’t passed as an argument in the usual way. In Remix we do this by entering the value we want to deposit in the Value field in the Deploy & Run Transactions panel. You can use the dropdown on the right to select the uints of value you want the number you’ve entered to represent (ether or wei etc.). The integer passed to the smart contract and processed by the Solidity code will be treated as units of wei, but the dropdown just saves us having to input a whole lot of 0’s. Marking a function payable enables it to receive ether. This then happens automatically without having to include a parameter for this value. In the same way that we can reference the caller’s address with msg.sender (without having to include an address parameter), we can use msg.value to reference a wei value which is sent to a function by the caller (as long as the function has been marked payable).

If you haven’t done this when calling deposit(), no ether will have been added to the contract balance, and msg.value will be zero. When you make a value transfer in the way I’ve explained above, you will see the caller’s address balance (the one showing in the Account field) is reduced by that amount, and the caller’s individual balance in the mapping in the Bank contract increases by the same amount.

In the withdraw() function, we use the pre-defined transfer() function to perform a value transfer from the contract address balance to the caller’s external address.

<address payable>.transfer(uint256 amount)

The amount in wei deducted from the contract balance, and transferred out of the contract, will be the amount requested by the caller of the withdraw() function and input as the amount parameter. This input amount is then passed to transfer() within the function. Because transfer() is called on the caller’s own external address, the amount input by them is transferred to their external address. The amount parameter also needs to be referenced within withdraw() in the line of code which reduces the user’s individual account balance in the mapping (in order to track their own individual share of the total ether held in the contract). This is why the withdraw() function needs an amount parameter, whereas the deposit() function doesn’t. msg.value is only available in payable functions, and references the value received by the contract, not sent from it.

Let me know if anything is unclear about how you need to correct your code, or if you have any further questions.

jon_m · July 21, 2021, 12:12am

Nice solution @Epicdisaster

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

check inputs (require statements)
effects (update the contract state)
external interactions

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

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. You’ll learn about the type of attack this prevents, and how it does 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 can be called separately to consult the individual account holder’s updated balance after the withdrawal.

… I assume the missing closing bracket for the contract was a copy and paste error

mohkanaan · July 21, 2021, 7:08am

Here below the solution to the assignment:

pragma solidity 0.7.5;

contract Bank
{
    
    mapping (address => uint) balance;
    
    address owner;
    
    modifier onlyOwner
    {
        require(msg.sender == owner);
        _;
    }
    
    event balanceAdded(uint amount, address indexed depositedTo);
    
    event balanceTransfered(uint amount, address indexed from, address indexed to);
    
    event depositDone(uint amount, address indexed depositedTo);
    
    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) {
       require(balance[msg.sender] >= amount, "Your balance is not sufficient");
       balance[msg.sender] -= amount;
       msg.sender.transfer(amount);
       return balance[msg.sender];
   }
    
    //To be removed
    function addBalance (uint _toAdd) public onlyOwner returns (uint)
    {
        balance[msg.sender] += _toAdd;
        emit balanceAdded(_toAdd, msg.sender);
        return balance[msg.sender];
    }
    
    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);
        
        emit balanceTransfered(amount, msg.sender, recipient);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private
    {
        balance[from] -= amount;
        balance[to] += amount;
    }
    
    
}```