Transfer Assignment

jon_m · March 26, 2022, 11:33pm

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 if you do include it 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.

By the way, I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. 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 anything is unclear, if you have any questions, or if you need any more help with correcting your solution

seybastiens · March 28, 2022, 6:36pm

Ok well understood.
Thanks for the deep explanation.

menarf · March 28, 2022, 7:45pm

Hi @jon_m,

Thanks a lot for your reply.

I guess I didn’t think through the whole task very well. I realized it straight away when I saw Filip’s solution in the following video.

As for Events Assignment, you are right, I missed it completely

I added transfer event to my code and corrected withdrawal function as per your suggestions and Filip’s solution video.

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;

    address owner;

    event depositDone(uint amount, address indexed depositedTo);

    event amountTransferred (uint amount, address sender, address recipient);

    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){
        require(balance[msg.sender] >= amount, "Balance not sufficient"); 
        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 money to yourself");
        
        
        uint previousSenderBalance = balance[msg.sender];

        _transfer(msg.sender, recipient, amount);

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

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

    }

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

    }

}

jon_m · March 29, 2022, 1:10am

Nice modifications @menarf, … your whole contract’s looking great now!

The transfer event and corresponding emit statement you’ve added are both correct and well coded. The emit statement is in the correct position in the transfer() function body and will log appropriate data when the transfer() function is successfully executed

Don’t worry! … As long as you carefully review and check your solution afterwards, in order to understand anything you missed or should be coding differently, you will make good progress. Obviously, there are often several alternative solutions, and we can help you to review and check these kinds of differences when you post your code here in the forum.

Thunarson7999 · March 29, 2022, 2:59pm

See below - add require(balance[msg.sender] >= amount,"Balance not sufficient"); and update balance with balance[msg.sender] -= amount;

pragma solidity 0.7.5;

contract Bank {

    constructor(){

        owner = msg.sender;

    }

    mapping(address => uint) balance;

    address owner;

    modifier onlyOwner {

        require(msg.sender == owner);

        _; //run the function

    }

    event depositDone(uint amount, address depositedTo);

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

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

    }
}

jon_m · April 1, 2022, 5:03am

Great solution @Thunarson7999

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…

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

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

By the way, I think you may have missed out the Events Assignment, because you are missing a Transfer event, and a corresponding emit statement, in your contract. 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

Thunarson7999 · April 4, 2022, 7:36am

Thx @jon_m for your feedback, highly appreciated! Oh yes I forgot a withdraw event.
I’d probably add sth like this in the declaration

event withdrawalCompleted(address withdrawnTo);

and then emit the event in the function body

emit withdrawalCompleted(msg.sender);

JoriDev · April 4, 2022, 2:11pm

Here’s the latest version of my bank contract. I added a withdrawal function and a withdrawal event, along with some corrections after the previous assignment. Just for clarity I’ll post all of the code again.

pragma solidity 0.7.5;

contract Bank {

  mapping(address => uint) balance;

  address owner;

  event DepositDone(uint amount, address depositedTo);

  event WithdrawalDone(address withdrawnTo, uint newBalance)

  event TransferLog(address indexed sender, address indexed recipient, uint transferValue);

  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 withdraw(uint amount) public returns (uint){
     //make sure the balance of the sender is sufficient
    require (balance[msg.sender] >= amount, "the amount is larger than your balance");
    
     balance[msg.sender] -= amount;
     msg.sender.transfer(amount);
     return balance[msg.sender];

     emit WithdrawalDone(msg.sender, balance[msg.sender]);  
  }

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

  function transfer(address recipient, uint amount) public {
    //check balance of msg.sender
    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);

    //event logs and further checks
    assert(balance[msg.sender] == previousSenderBalance - amount);
    emit TransferLog (msg.sender, recipient, amount);
  }

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

mjbam93 · April 6, 2022, 11:56am

Hi,
i tried for a while now, but i cant even compile my code.
Then i looked up Filip’s solution and even his code doesnt work for me.

in line 18 it says:

ypeError: “send” and “transfer” are only available for objects of type “address payable”, not “address”.
–> payable_functions_solution.sol:18:9:
|
18 | msg.sender.transfer(amount);
| ^^^^^^^^^^^^^^^^^^^

is this outdated or so? i dont get what is actually wrong…

mcgrane5 · April 6, 2022, 11:59am

no you need to make the address of type payable. change the lin eof code to

payable(msg.sender).transfer(amount)

mjbam93 · April 6, 2022, 12:11pm

Awesome! Thanks a lot! It worked!

jon_m · April 6, 2022, 1:59pm

Hi @Thunarson7999,

This code is correct, but we would probably want to log the withdrawal amount as another parameter e.g.

event withdrawalCompleted(address withdrawnTo, unit amount);

emit withdrawalCompleted(msg.sender, amount);

By the way, where in the function body would you place the emit statement? Order/position is important, so it would be good to see what you think

jon_m · April 6, 2022, 2:48pm

Hi @JoriDev,

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

All of your statements are in the correct order within the withdraw() function body, except for the emit statement. Once you’ve resolved the compiler error by adding a semi colon to the end of your WithdrawalDone event you will get an orange compiler warning for the emit statement. The warning message tells you that this line of code is unreachable. This is because function execution stops after a return statement, so at the moment your WithdrawalDone event data won’t be emitted or logged. You can test this for yourself by calling the withdraw() function and then having a look at logs in the transaction receipt (the one with the green tick) in the Remix terminal at the bottom of the screen.

You are right that an emit statement should only be executed after the event itself has occurred. However, it needs to be placed before a return statement if there is one. And generally speaking, it’s probably better to place an emit statement after an assert statement if there is one, which is what you’ve done with your TransferLog emit statement in the transfer() function body.

Apart from its position, your WithdrawalDone event declaration and corresponding emit statement are well coded. But don’t you think it would be important to log the amount withdrawn as well as the user’s updated balance?

Your other statements are in the correct order within the withdraw() function body, in order 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…

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

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!

Your other modifications related to the previous Events Assignmnet are also well done

Just let me know if you have any questions.

jon_m · April 6, 2022, 3:15pm

Hi @mjbam93,

You need to bear in mind that this course material is based on Solidity v0.7 syntax. It’s perfectly fine to use Solidity v0.8 and, in fact, it’s better to use this latest version. But if you do, you will need to make some occasional adjustments to your code for the one v0.8 syntax change which affects the code for this 101 course …

In order to receive Ether, an address needs to be payable . Prior to Solidity v0.8, msg.sender is payable by default and so doesn’t need to be explicitly converted. However, from v0.8, msg.sender is non-payable by default, and so when it needs to refer to a payable address, it has to be explicitly converted. One of the ways to do this is using payable() in the way that @mcgrane5 has already shown you …

You should find that the next time you get a compiler error due to this syntax change, you now know how to fix it. But just let either of us know if you have any more questions

JoriDev · April 8, 2022, 12:08pm

Well I had a question on your reply, but after reading it once again the answer was already in it.

You wrote:

And I was like okay, but why is the order of emit before return so important?

Then there was the light bulb moment, as you were telling earlier:

I’ll definitely keep this in mind while writing future contracts to prevent parts of my function executions from being unreachable. Thanks for clearing up this accidental error!

pappas_kellyn · April 11, 2022, 12:41am

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;
address owner;

event depositAdded(address indexed depositedTo, uint amount);
event transactionDone(address indexed depositedFrom, address indexed toRecipient, uint amount);

constructor() {
    owner = msg.sender;
    }

function deposit() public payable returns(uint) {
    balance[msg.sender] += msg.value;
    emit depositAdded(msg.sender, msg.value);
    return balance[msg.sender];
}

function transferTo(address toRecipient, uint amount) public {

    require(balance[msg.sender] >= amount, "Insufficient funds!");
    require(msg.sender != recipient, "Can not transfer to yourself");

    uint previousSenderBalance = balance[msg.sender];
    _transfer(msg.sender, toRecipient, amount);
    assert(balance[msg.sender] == previousSenderBalance - amount);

    emit transactionDone(msg.sender, toRecipient, amount);
}

function _transfer(address _from, address _recipient, uint amount) private {
    balance[_from] -= amount;
    balance[_recipient] += amount;

}

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

    require(balance[msg.sender] >= amount, "Insufficient funds!"); //ANSWER
    balance[msg.sender] -= amount;  //ANSWER
    msg.sender.transfer(amount);
    return balance[msg.sender];

}

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

}

Baz · April 11, 2022, 6:08am

    // withdraw function
    function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount, "Insufficient funds");
        balance[msg.sender] -= amount; // deduct amount from balance
        msg.sender.transfer(amount); // transfer amount
        return balance[msg.sender];
    }

jon_m · April 11, 2022, 6:40pm

Nice solution @pappas_kellyn

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…

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

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

A couple of other comments …

(1) Your transfer event and corresponding emit statement are both coded correctly. The emit statement is in the correct position in the transferTo() function body and will log appropriate data when the function is successfully executed

However, calling any of the deposit(), withdraw() or transferTo() functions results in a transaction, so instead of transactionDone, I think a better/clearer name for your transfer event would be either transferDone, or the name you gave it in your Events Assignment code (transferAdded) …

I think it would also be a good idea to rename your transfer event’s first address parameter from depositedFrom, to something like sentFrom, fromSender or transferFrom (this last name being what you called this parameter in your Events Assignment code). Can you see why I’m suggesting this? What do you think?

(2) In your transferTo() function body, you’ve forgotten to change one of the recipient parameter references to toRecipient …

Unless you correct this, your contract won’t compile.

Let me know if you have any questions

pappas_kellyn · April 11, 2022, 8:51pm

Yes, I should probably stick with clear, standard names for functions and events, so I don’t confuse myself! Also, I added that last require statement in remix before copying it there to paste it here in the forum, but I should get in the habit of saving, compiling and deploying everything, so I can at least catch the error messages that pop up. Thanks for the feedback!

gunnarseay · April 11, 2022, 9:00pm

Solution for our withdrawal function:

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