Transfer Assignment

[jon_m]

Hi @IntrepidShape,

1st require statement 

2nd require statement

@IntrepidShape

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

Your error message for this require statement suggests that your intention is to allow anyone to withdraw, but only from their own balance.
However, assuming your contract also has a variable:  address owner;   which, on deployment of the contract, is set using a constructor to the deployer’s address…

constructor() {
    owner = msg.sender;
}

… this require statement, instead, restricts withdrawal to the contract owner. If you do want to do this, then we already have an onlyOwner modifier than can apply this restriction, while keeping the code more concise. But the restriction that I think you intended to apply would be achieved with …

balance[msg.sender] -= amount;

instead of

@IntrepidShape

balance[owner] -= amount;

msg.sender.transfer(amount);  transfers the withdrawal amount from the total funds in the contract (i.e from the single contract address) to the caller’s wallet address.
Additionally…  balance[msg.sender] -= amount;   will ensure that the accounting reduction is made to the caller’s balance (their individual share of the total funds held in the contract).

Return statement   But do you think it would be more useful to output the caller’s balance after the withdrawal, rather than the amount withdrawn? You could even return both values.

Finally, 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 anything is unclear, or if you have any questions

Also, if I’ve misinterpreted what you have intended to achieve with your solution, then do let me know, and post your complete contract code, so I can see your withdraw() function in context.

[jon_m]

Excellent solution @nutrina

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:

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

[jon_m]

An excellent assignment solution @Mike_C

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

The additional event you’ve added is also 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.

@Mike_C

I’m not actually sure if I need to return the balance at the end of the withdraw function. Can somebody confirm this?

You are right. It is not mandatory to return the updated balance, and the withdraw() function can still operate effectively without returning a value. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the updated balance after the withdrawal. However, as the withdraw function header (in the code given to you) includes returns(uint), you are right to include a return statement in the function body, otherwise you would get a compiler error — unless of course you decide to just remove  returns(unit)  which would be equally valid.

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.

[shaikein11]

I made the following code:

//Here I will define events in order to have transaction logs
event depositAdded(uint amount, address indexed sender);
event withdrawCompleted(uint amount, address indexed receiver);
//Then, I will define a payable deposit function to receive funds from users. It will return the user's balance. The payable keyword will transfer the funds from the sender to the Smart Contract.
function deposit() public payable returns (uint)
{
        //Here we use an auxiliary variable to store the previous balance
        uint previousBalance = balance[msg.sender];
        //Then, we update the sender address balance, to consider the deposited amount
        balance[msg.sender] += msg.value;
        //Later, we will use assert to verify that the balance was updated 
        assert(balance[msg.sender] == previousBalance + msg.value);
        //Here we trigger an event to save the transaction in the log
        emit depositAdded(msg.value, msg.sender);
        //Finally, we return the current balance for the request sender
        return balance[msg.sender];
}

//Here, we will define the withdraw function, that will receive the amount to withdraw, and will return the user's balance
function withdraw(uint amount) public returns (uint)
{
        //First, we check if the user has enough funds to withdraw the required amount, else we reject the transaction
        require(balance[msg.sender] >= amount, "Insufficient funds");
        //Here we use an auxiliary variable to store the previous balance
        uint previousBalance = balance[msg.sender];
        //Then, we use the transfer function to send money from the Smart Contract to the request sender
        msg.sender.transfer(amount);
        //Now, we will update the balance for the address, to consider the withdrawn amount
        balance[msg.sender] -= amount;
        //Later, we will use assert to verify that the balance was updated 
        assert(balance[msg.sender] == previousBalance - amount);
        //Here we trigger an event to save the transaction in the log
        emit withdrawCompleted(amount, msg.sender);
        //Finally, we return the current balance for the request sender
        return balance[msg.sender];
}

[Raf.PV7]

My code:

pragma solidity 0.7.5;

contract Bank{
    
    mapping(address => uint) balance;
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    event withdrawalDone(uint amount, address indexed depositedTo);
    
    event transferSended(address indexed from, address indexed to, uint amount);
    
    constructor(){
        owner = msg.sender;
    }
    
    modifier onlyOwner{
        require(msg.sender == owner);
        _;
    }
    
    modifier transferValidations(uint amount, address recipient){
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        _;
    }
    
    modifier withdrawValidations(uint amount){
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        _;
    }
    
    
    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 withdrawValidations(amount) returns (uint){
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        emit withdrawalDone(amount, msg.sender);
        return balance[msg.sender];
        
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public transferValidations(amount, recipient) {
        uint previousSenderBalance = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
        assert(balance[msg.sender] == previousSenderBalance - amount);
        emit transferSended(msg.sender, recipient, amount);
  
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] +=  amount;
    }
   
}

[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