Transfer Assignment

[jon_m]

An excellent assignment solution, and well-coded contract @Raf.PV7

You have included all of the additional functionality needed to solve the problem with the withdraw function.

The additional withdrawal event you’ve added is appropriate and well coded. The emit statement is in the correct position within the withdraw function, and it emits and logs relevant information when a call to the withdraw function has executed successfully.

You also have the other code in your withdraw function in the correct order to reduce security risk:

1. check inputs (require statement)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract:

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 later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. Anyway, don’t worry about the details for now, although it’s good to be aware of, and it’s great that you’re already getting into good habits

Just one other observation…
The point of placing require statements within modifiers is for code reusability. This enables us to avoid code duplication and keeps our code concise. However, you cannot reuse either the withdrawValidations modifier or the transferValidations modifier elsewhere in your code, and it is unlikey you will be able to (if you develop your code further) due to each having a fairly specific use case. By having a separate modifer for both the withdraw and the transfer function, you have actually ended up with more code than you would by removing these 2 modifiers completely, and placing their require statements directly at the beginning of the corresponding function bodies.
However, you can avoid having to repeat the require statement which prevents a user withdrawing more ether than their current balance, by including just this require statement in one additional modifier. You can then include this modifier in both functions (as both require this particular input restriction) and place the other require statement within the only function that uses it. Have a go at making this modification.

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

[jon_m]

An excellent assignment solution @shaikein11

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

The additional withdrawal event you’ve added is appropriate and well coded. The emit statement is in the correct position within the function body, and it emits and logs relevant information when a call to the withdraw function has executed successfully.

The additional assert statements you’ve added to both the withdraw function and the deposit function are also well coded.

The detailed comments you have added to your code show a good understanding of the operations each line of code is performing. Here are a just a couple of comments that could be improved for accuracy:

(1)

// … The payable keyword will transfer the funds from the sender to the Smart Contract.

Not really … the function type payable  enables that particular function to receive ether.

(2)

// Here, we will define the withdraw function, that will receive the amount to withdraw …

Not really … the main job of the withdraw function is to transfer the amount requested for withdrawal (input into the function in wei) from the smart contract address to the caller’s wallet address.

Finally, 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. See if you can work out how to reorder your code for this, bearing in mind you’ve got an additional 2 lines of code for the assert statement

[xela]

I wanted to note that in version 0.8 of solidity you need to convert an address into a payable address like this:

 payable(msg.sender)

pragma solidity ^0.8.4;

contract EtherBank {
    
    mapping(address => uint) balances;
    address owner;
    
    event deposited(address sender, uint amount);
    event withdrawn(address withdrawer, uint amount);
    event transfered(address sender, address receiver, uint amount);

    modifier checkFunds(uint requiredAmount){
        require(balances[msg.sender] >= requiredAmount, "Not enough funds for transaction!");
        _; //runs the function
    }
    
    modifier onlyOwner {
        require(msg.sender == owner, "You need to be the owner!");
        _; //runs the function
    }

    modifier checkValueForZero(uint value){
        require(value > 0, "Value needs to be greater than 0.");
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }

    function deposit() public payable checkValueForZero(msg.value) returns(uint) {
        uint balanceBefore = balances[msg.sender];
        balances[msg.sender] += msg.value;
        emit deposited(msg.sender, msg.value);
        assert(balances[msg.sender] == balanceBefore + msg.value);
        return balances[msg.sender];
    }
    
    function withdraw(uint amount) public payable checkFunds(amount) checkValueForZero(amount) returns(uint){
        uint balanceBefore = balances[msg.sender];
        balances[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        emit withdrawn(msg.sender, amount);
        assert(balances[msg.sender] + amount == balanceBefore);
        return balances[msg.sender];
    }
    
    function getBalance() public view returns(uint){
        return balances[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        uint previousSenderBalance = balances[msg.sender];
        _transfer(msg.sender, recipient, amount);
        emit transfered(msg.sender, recipient, amount);
        assert(balances[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private checkFunds(amount){
        balances[from] -= amount;
        balances[to] += amount;
    }
}

[IntrepidShape]

Thanks for clarifying.

   function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount, "Not enough balance.");
        require(msg.sender == owner, "You can only withdraw into your own account");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        return amount;
        
    }

[jon_m]

Hi @IntrepidShape,

The change you’ve made from…

@IntrepidShape

balance[owner] -= amount;

to

@IntrepidShape

balance[msg.sender] -= amount;

… makes no difference unless you also remove…

@IntrepidShape

require(msg.sender == owner, "You can only withdraw into your own account");

…because this require statement still restricts access to the withdraw function to the contract owner. If you do want to do this, then we already have an onlyOwner modifier (unless you’ve removed it) that can apply this restriction, while keeping the code more concise.

However, as you haven’t said otherwise, I’m still assuming …

@jon_m

that your intention is to allow anyone to withdraw, but only from their own balance.

Is that correct?

Your return statement will still output the amount withdrawn, not the caller’s new balance after the withdrawal. Is that really what you intend?

Did you check this post, which explains an important security modification which needs to be made to the order of two of your statements within the function body? Yours are still in the same order as before…

Let us know if anything is unclear, or if you have any questions, so we can help you

[jon_m]

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

You have included all of the additional functionality needed to solve the problem with the withdraw function.

The additional withdrawal event and corresponding emit statement you’ve added are appropriate and well coded, as are all of your events and their emit statements. The only thing I would say, is that the emit statements are probably better positioned after the assert statements, rather than before.

The additional assert statements you’ve added to both the withdraw function and the deposit function are also well coded.

---

You have the other code in your withdraw function in the correct order to reduce security risk:

1. check inputs (require statements)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

balances[msg.sender] -= amount;

before actually transferring the funds out of the contract:

payable(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 later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. But it’s great that you’re already getting into good habits in terms of smart contract security

@xela

I wanted to note that in version 0.8 of solidity you need to convert an address into a payable address like this:

 payable(msg.sender)

… when you need to use msg.sender as a payable address, then, yes. That’s because from v0.8 msg.sender is by default a non-payable address, whereas in prior versions it is payable by default. Apart from msg.sender,  payable()   has been used to convert non-payable addresses to payable ones since Solidity v0.6.

---

Your additional modifiers with parameters, each used to provide the same require statement in more than one function, are skillfully implemented, appropriate, and show a good understanding of how modifiers help us avoid code duplication, and keep our code concise
Just a couple of observations, though…

1. checkFunds would be better placed in the “main” transfer function, instead of the helper _transfer() function. If an input value needs to be checked or validated, this should happen at its first “point of entry” into the smart contract. In addition, if the transfer amount isn’t checked until it is passed to the helper transfer function, less gas is likely to be refunded if require fails and reverts the transaction, than if the check was performed at the beginning of the “main” transfer function. 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.

2. Is there a reason why you have used checkValueforZero to check that amounts deposited into or withdrawn from the contract are > 0, but not amounts transferred between users within the contract?

3. You’re missing the require statement in transfer() which checks that an address isn’t transferring funds to itself.

---

deposit() has to be a payable function, because it needs to receive ether. However, you don’t need to mark withdraw() as payable, because it only needs to send ether (not receive it).

---

Let us know if anything is unclear, or if you have any questions about the points I’ve raised.

You are making great progress!

[xela]

Hey @jon_m and thanks once again for the detailed code review,

I was already wondering if the order of updating the internal state and interactions does matter, so thanks for making this clear

about your obersvations:

1. I was not aware that there is gas refunded if some require statement fails (you probably should add this information to the course, or have i just missed that? ). So from now on I will take care of using the modifiers at first point of entry.
2. Yeah I probably forgot to use that modifier there again.
3. I think I have implemented this actually later on but didn’t had it in the version that I uploaded here.

Thanks for your kind words
xela

[juanmcba]

pragma solidity 0.7.5;

contract transferFunction {
    
    mapping(address => uint) balance;
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    event balanceTransferred(address from, address to, uint 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) {
        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 send money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        emit balanceTransferred(msg.sender, recipient, amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

[kopino4]

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

[StanleyGM]

withdraw function

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

[Lehigr2]

function withdraw(uint amount) public returns (uint){
        msg.sender.transfer(amount);
        require(balance[msg.sender] >= amount, "That's too much money. Check your balance");
        balance[msg.sender] -= amount;
        return balance[msg.sender];

[jon_m]

You’re very welcome @xela — I’m glad you are finding the feedback and comments helpful

@xela

1. I was not aware that there is gas refunded if some require statement fails (you probably should add this information to the course, or have i just missed that? ).

I think it’s mentioned so very briefly in Filip’s blackboard lectures on require() and assert(). But to be honest, it may just explain that the remaining gas isn’t refunded if assert() fails, and not mention about require() and the gas refund. I know that it definitely does come up in the Error Handling quiz though. But you’re right, it isn’t highlighted, or explained in any detail. I’ll certainly mention what you say, but the main outcome for this 101 course is that students know how to implement require() and assert(). The intricacies of gas consumption can be a bit overwhelming at first, so it may be that a full explanation of these additional points has been left to the more advanced Solidity courses. As you are clearly making such good progress, I felt you would benefit from some extra info

By the way, in this particular context, the term refund refers to the gas limit set for the transaction. As far as I am aware, none of the gas is refunded which has been consumed by the operations executed up to and including the require() which triggers revert. But the remaining, unspent gas up to the gas limit is refunded to the caller. If a transaction is successful, then the remaining gas (after all associated operations have been executed) is also refunded. But by placing our require() statements as early in the control flow as is practical and secure, we reduce the caller’s gas cost of a failed transaction (due to invalid inputs or restricted access) as much as possible.

However, if assert() triggers revert, the remaining gas is not refunded. So, depending what the gas limit is set at, this can result in a considerably higher gas cost!

@xela

3. I think I have implemented this actually later on but didn’t had it in the version that I uploaded here.

[jon_m]

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

You have included all of the additional functionality needed to solve the problem with the withdraw function, and you have the different lines in the correct order to reduce security risk:

1. check inputs (require statement)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

balances[msg.sender] -= amount;

before actually transferring the funds out of the contract:

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 later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just telling you for extra information, in case you haven’t already seen this explanation in some of the other posts in this discussion topic. But it’s great that you’re already getting into good habits in terms of smart contract security

Your transfer event and corresponding emit statement are also well coded. The emit statement is in the correct position within the transfer function, and it emits and logs relevant information when a call to the transfer function has executed successfully.

You are making great progress!

[jon_m]

Nice solution @kopino4

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

[jon_m]

Nice solution @StanleyGM

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

[jon_m]

Hi @Lehigr2,

You have added all of the additional lines of code needed to solve the problem with the withdraw function However, in terms of risk management and security, you should make an important modification to their order:

1. check inputs (require statement)
2. effects (update the contract state for reduction in balance)
3. interactions (perform the transfer of funds from smart contract address to wallet address).

Checking inputs should be the very first thing we do. Even though the transaction would still revert wherever require is positioned, initiating an external transfer before performing our own internal checks and accounting adjustments is poor risk management and increases the potential for any attack to be successful.

In addition, it’s important to modify the contract state:

balances[msg.sender] -= amount;

before actually transferring the funds out of the contract:

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 later courses. We wouldn’t expect you to know this at this early stage, though, so I’m just letting you know for extra information.

Just let us know if you have any questions

[StanleyGM]

Thanks @jon_m for the assistance. It’s greatly appreciated!

[AustinBurks]

Use require() to ensure the person can only withdraw their own balance.

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposite() 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, "Insufficient balance");
        msg.sender.transfer(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, "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]

Hi @AustinBurks,

The require statement you’ve added is correct but you are missing 2 other important lines of code in your withdraw function:

(1) When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount (from the contract address balance), but their own balance in the mapping (their share of the overall contract balance) is not reduced! I’m sure you can see that this is another very serious bug! How would you correct this? Think about this…

@AustinBurks

Use require() to ensure the person can only withdraw their own balance.

The require() statement ensures that the caller cannot withdraw more than their own balance. This is different. Using msg.sender ensures that the caller can only withdraw from their own balance.

If you can’t work out how to correct this yourself, you’ll find the answer by taking a look at some of the other posts in this topic.

---

(2) 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 also need to include a return statement in the function body.

---

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

Let us know if you have any questions about how to correct this, or if there is anything that you don’t understand

[benlive]

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

        payable(msg.sender).transfer(amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender];
    }