Transfer Assignment

mjbam93 · April 6, 2022, 12:11pm

Awesome! Thanks a lot! It worked!

jon_m · April 6, 2022, 1:59pm

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

jon_m · April 11, 2022, 9:59pm

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…

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

jon_m · April 11, 2022, 10:25pm

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 · April 13, 2022, 9:02am

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 · April 13, 2022, 10:18am

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

Heyns · April 13, 2022, 12:11pm

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 · April 13, 2022, 3:58pm

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 · April 15, 2022, 2:48pm

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…

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

Just let me know if you have any questions

jon_m · April 15, 2022, 3:09pm

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 · April 15, 2022, 3:56pm

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 · April 17, 2022, 9:20pm

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!