Transfer Assignment

[jon_m]

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…

@JoriDev

balance[msg.sender] -= amount;

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

@JoriDev

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!

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

Just let me know if you have any questions.

[jon_m]

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 …

@mcgrane5

payable(msg.sender).transfer(amount)

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]

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

You wrote:

@jon_m

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

@jon_m

This is because function execution stops after a return statement, so at the moment your WithdrawalDone event data won’t be emitted or logged.

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]

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]

    // 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]

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…

@pappas_kellyn

balance[msg.sender] -= amount;  //ANSWER

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

@pappas_kellyn

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

---

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

@pappas_kellyn

event transferAdded(uint amount, address indexed transferTo, address indexed transferFrom); //ANSWER

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 …

@pappas_kellyn

require(msg.sender != recipient, "Can not transfer to yourself");

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

Let me know if you have any questions

[pappas_kellyn]

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]

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

[jon_m]

Nice solution @Baz

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…

@Baz

balance[msg.sender] -= amount; // deduct amount from balance

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

@Baz

msg.sender.transfer(amount); // 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

[jon_m]

Hi @gunnarseay,

The require statement you’ve added is correct but your withdraw function is still missing an essential line of code (see further below for an explanation about why).

Your return statement is missing a semicolon at the end but, apart from that, it’s correct. But your code won’t compile unless you correct this.

Once you’ve added the semicolon, compiled your code, and deployed your contract … if you now 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.

Once you’ve added this line of code, have a look at this post to check that whether you’ve placed it in the right position within the function body, in order to reduce security risk.

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

[martina]

Here’s assignment solution:

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

 require(balance[msg.sender] >= amount);

 balance [msg.sender] -= amount;

 msg.sender.transfer(amount);

 return balance[msg.sender];

[stanmation]

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

[Heyns]

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

[nathanls78]

pragma solidity 0.7.5;

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) {
        require(balance[msg.sender] >= amount, "You do not have enough funds");
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

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

    function transfers(address recipient, uint amount) public onlyOwner {
        // check balance of msg.sender
        require(msg.sender != recipient, "You cannot send funds to yourself, idiot!");
        uint previousSenderBalance = balance[msg.sender];

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

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

}

[jon_m]

Nice solution @martina

… but don’t forget the function’s closing curly bracket

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…

@martina

balance [msg.sender] -= amount;

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

@martina

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

Just let me know if you have any questions

[jon_m]

Hi @stanmation,

The require statement you’ve added works in that it prevents a user from withdrawing more than their share of the total contract balance, but it also prevents a user from withdrawing their full individual balance. How would you modify the condition in your require() statement to allow users to withdraw Ether up to and including their individual balances, and to only prevent withdrawal amounts greater than their individual balances?

Your withdraw() function is also 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.

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.

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

[jon_m]

Hi @nathanls78,

The require statement you’ve added is correct, but your withdraw function is still missing another 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.

Once you’ve modified 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.

---

A few other observations …

• Your onlyOwner modifier in the transfers() function header prevents anyone other than the contract owner from making an internal transfer within the contract from their own individual balance to another user’s individual balance. If this is your intention, then that’s fine, but I just wanted to let you know that there isn’t any reason why any user with sufficient funds shouldn’t be allowed to perform this transfer, especially as any address can now deposit Ether in the contract.

• At the beginning of your transfers() function body, you have a comment which correctly states that the calling address’s individual balance needs to be checked, but you haven’t included a require() statement to perform this check. You need to add this for the same reason that it’s needed in the withdraw() function: to prevent users from transacting with Ether amounts greater than what they are individually entitled to i.e. their own individual balances as tracked and recorded in the mapping.

• 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, or if you have any questions about any of these points

[Giupi]

Hi everyone, I have a “strange” question about something that’s happening when testing my withdrawAll() function on Remix. I thought this thread would be a pretty good place to post it.

In words this is what’s happening: when I adjust the msg.sender balance to 0 before making the transfer, I get an out of gas error. If I adjust the balance to any other value, even 1 Wei the function goes through.

I’m going to attach my function below, plus the screenshots from Remix, I think it’s really strange, I was hoping anybody would have an insight on something I’m clearly not seeing here.

function withdrawAll () public returns (bool _success) {
        require (balances[msg.sender] > 0, "Insufficient funds");
        uint _amountToWithdraw = balances[msg.sender];
        balances[msg.sender] = 0;
        payable(msg.sender).transfer(_amountToWithdraw);
        assert(balances[payable(msg.sender)] == 0);
        emit withdrawSuccessful(payable(msg.sender), _amountToWithdraw);
        _success = true;
    }

This first screenshot shows what happens when I update the balance to 0:

This second screenshot shows what happens when I update the balance to 1 Wei:
(I’ve commented out the assert() method since it will clearly throw with balance at 1 Wei.)

I would really appreciate any insights since this problem has been keeping me busy for a couple of days now, and I have no answer for it, nothing on google…
Thank you all!

[jon_m]

Hi @Giupi,

This certainly got me thinking and experimenting!!

@mcgrane5 & @thecil :  Do you agree with my conclusion, below? Or have I missed something?

Through a process of elimination, I’ve worked out that the line of code that is causing the transaction to revert in Remix (with the error message: out of gas) is the one setting the withdrawer’s individual balance in the mapping to zero …

@Giupi

balances[msg.sender] = 0;

However, as far as I could see, there was nothing wrong with this line of code. In fact, quite the opposite: you have correctly set the user’s individual balance in the mapping to zero before transferring their full Ether balance out of the contract to their external address.

I thought, maybe, one or two of the other lines of code in this function could be pushing the gas consumption over the gas limit (which can be adjusted in the Gas Limit field below the Account field in the Deploy & Run Transactions panel in Remix). But this isn’t the case, and no matter how much the gas limit is increased, this doesn’t seem to make any difference.

What’s more, I also couldn’t see any reason why setting balances[msg.sender] to 1 instead of zero should make any difference to the gas cost.

Out of interest, I tried changing …

balances[msg.sender] = 0;
// to
delete balances[msg.sender];

…which should produce the same effect. But this also triggers the out of gas error in Remix.

So, I tested your complete function (within a larger Bank contract) using Truffle and Ganache, instead of Remix … and it worked! I used Solidity compiler v.0.8.9. So, this suggests to me that there is most probably a bug in the current version of Remix.

Even the following function triggers the out of gas error in Remix. This is very strange and makes me even more convinced that there is some kind of bug causing this…

function setBalanceToZero() public {
    balances[msg.sender] = 0;
}

Once you’ve completed the section on using Truffle in the 201 course, you’ll be able to see that your withdrawAll() function works fine. However, I would suggest you also make the following amendments, which will also save a small amount of gas…

function withdrawAll() public /*returns (bool _success)*/ {
// Do you use this return value? If not, you should remove it.
    require(balances[msg.sender] > 0, "Insufficient funds");
    uint _amountToWithdraw = balances[msg.sender];
    balances[msg.sender] = 0;
    payable(msg.sender).transfer(_amountToWithdraw);
    assert(balances[/*payable(*/msg.sender/*)*/] == 0);
// msg.sender doesn't need to be payable to look up the balance in the mapping 
    emit withdrawSuccessful(/*payable(*/msg.sender/*)*/, _amountToWithdraw);
// msg.sender doesn't need to be payable to emit/log this address as event data
    // _success = true;
// Do you use this return value? If not, you should remove it.
}

Let us know if you have any questions

[mcgrane5]

@jon_m this is weird, setting a users balance maping to 0 like this is standard in a lot of contracts. . so it worked when seeting to 1 and not zero. so this issue happens in sol 0.8.9? i want to check cos i want to try reproduce