Transfer Assignment

[jon_m]

Hi Federico,

Apologies for the delay in giving you some feedback on this assignment.

Your solution is excellent, and I really like how you’ve adapted and been creative with the code, experimenting with your own variations

You’ve solved the problem with the withdraw function (adding your own adaptation — see my comments below), and the following operations are performed in the correct order within the control flow, which reduces 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…

 // if there is sufficient balance to withdraw the whole amount requested
balances[recipient] -= amount;

// if the requested amount is greater than the user's individual balance 
balances[recipient] = 0;

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

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

---

Some additional comments …

(1) Still allowing a withdrawal to go ahead if the amount requested by a user is greater than their individual balance, but reducing the amount actually withdrawn to the total balance the user has available, is an interesting variation. You’ve implemented this very well, and the require() statements that you’ve included instead are appropriate.

However, do you think that this would be appropriate in practice? When someone requests the withdrawal of a specific amount, don’t you think they would rather have the transaction stopped, and be notified that they don’t have sufficient funds, rather than finding out afterwards that the amount they’ve received is less than expected? The transaction can be reverted and the error handled in the front-end user interface in such a way that the user is fully aware of the circumstances, and is given the opportunity to repeat the withdrawal but for a lower amount.

(2) Adding the helper functions _addToBalance() and _removeFromBalance() is a nice implementation of modularity: breaking down a computation and separating out specific chunks of code into their own specialised functions, where they can be reused elsewhere. However, you’ve missed the opportunity to re-use both of these functions in the transfer() function to perform the necessary adjustments to the individual balances of the sender and recipient. Instead of adding another helper function (_transfer), you can call both of the helper functions you already have …

  //_transfer(msg.sender, recipient, amount);
  amount = _removeFromBalance(msg.sender, amount);
  _addToBalance(recipient, amount);

As the _removeFromBalance() function would no longer only be used for withdrawals, you would also need to think of a less specific name for the address parameter than recipient, so that it makes sense for all use cases. However, your if/else logic could still be applied to internal transfers initiated by the transfer() function, allowing them to still go ahead if the transfer amount requested is greater than the caller’s individual balance. Just like with withdrawals, in these cases the amount actually transferred could also be reduced to the total balance available to the caller.

(3) In your deposit() function, you don’t need to assign the value returned from the _addToBalance function call to a local variable, because this value isn’t used and will just be lost anyway when the function finishes executing.

@fedev

uint deposited = _addToBalance(msg.sender, msg.value);

You should have got an orange compiler warning for this, notifying you of an unused local variable.

The _addToBalance() helper function doesn’t need to return any value, and the function call can be made as follows…

function _addToBalance(address recipient, uint amount) private {
    balances[recipient] += amount;
}

function deposit() public payable returns(uint) {
    _addToBalance(msg.sender, msg.value);
    emit depositReceived(msg.sender, msg.value);
    return balances[msg.sender];
}

On the other hand, your _removeFromBalance() helper function does need to return a value, because the amount may need to be changed to the caller’s maximum balance available in the if-statement. Therefore, it’s also correct to assign the return value to the amount parameter in the main function that calls this helper …

@fedev

amount = _removeFromBalance(msg.sender, amount);

Notice in point 2, above, that this line of code could be used in both the withdraw() and the transfer() function.

(4) All 3 event declarations and their corresponding emit statements are well coded, and the emit statements log appropriate data when their respective functions are successfully executed.

Where possible, an emit statement should be placed at the end of a function, after all of the operations associated with the event itself have occurred, but before a return statement if there is one. Generally speaking, it’s probably also better to place an emit statement after an assert statement if there is one.

So, bearing this in mind, your emit statement in the withdraw() function is in the correct position, but you should reconsider where you place the one in your deposit() function.

Do you think it’s necessary to emit 2 events for each deposit and each withdrawal? Personally, I think it’s an unnecessary duplication of data, and that you could safely remove the balanceChanged event and its corresponding emit statements in the helper functions. You already have sufficient data logged by the depositReceived and the withdrawalProcessed events. We know that the amount associated with a depositReceived event will be an increase to the sender’s individual balance, and that associated with a withdrawalProcessed event will be a reduction in the recipient’s individual balance, without needing the Boolean values to be logged in a separate balanceChanged event.

Removing the balanceChanged event and adding a separate event only for the transfer() function, means that you’ll only need one event emitted at the end of this function as well (which is the most appropriate place for an event, anyway). At the same time, the transfer() function will still be able to make use of the two helper functions to adjust the individual user balances accordingly.

(5) If you need a getter to return a user’s individual balance to anyone, then you no longer need the getter which only returns the caller’s own balance, because the other getter can now be used for both. Remember that the more code a contract contains, the higher the gas cost will be to deploy it. So, we can’t afford to be wasteful by including redundant code.

I hope you find these comments helpful. Let me know if you have any questions, and please do let me know if you disagree with any of my suggestions.

You’re making great progress with Solidity, Federico Keep it up!

[fedev]

Hi @jon_m what a in-depth review, thank you so much, I really appreciate!
Regarding your comments:

@jon_m

(1) Still allowing a withdrawal to go ahead if the amount requested by a user is greater than…

Yes you’re right, it might be confusing and has definitely its drawbacks (smart contracts which depends upon on that must absolutely take this behaviour in consideration). But this can protect greedy and lazy users (as I am) from bad frontends. Have you ever had to replay (and sadly… rePAY) a transaction just because the UI of the dapp wasn’t rounding properly? That sucks and does really hurt when fees are as high as they’re now on ethereum

@jon_m

(2) Adding the helper functions _addToBalance() and _removeFromBalance() is a nice implementation of modularity

No excuses, you’re 100% right. The private helpers should have been used in transfer too.

@jon_m

(3) In your deposit() function, you don’t need to assign the value returned from

Yeah I saw it, but as I said I’m kind of lazy and I just let that… “just in case I need it for some reason…”
Obviously not for-prod behaviour

@jon_m

(4) … Where possible, an emit statement should be placed at the end of a function

You’re right, I had better understood this (and coded accordingly) in the MultiSig final project in which I’ve always put assertions in the helpers and emitted signals at the end of the public function.
I still have to get used to signals, as I still think them more as log functions, when I should threat them as costly signals for other processes. And this require clear vision/planning of what I would expect to integrate/need from the SC itself. It’s a good exercise.

@jon_m

(5) If you need a getter to return a user’s individual balance to anyone, then you no longer need the getter which only returns the caller’s own balance, because the other getter can now be used for both. Remember that the more code a contract contains, the higher the gas cost will be to deploy it. So, we can’t afford to be wasteful by including redundant code.

Yes I definetly have to get used to think about gas!

Your comments are really helpful and I would be super happy if you could review the Multi Sig final project with the same spirit: I’m trying to learn as much as I can!

Have a nice day

[jon_m]

Hey Federico,

Glad you found the comments helpful. What’s good is that, I can see from your reply, it got you thinking and reflecting further on things that you’d already come across yourself. Personally, I’ve always found this reflection, re-working and re-factoring stage really profitable, especially if you’ve already invested so much effort into the original solution, as you obviously do.

I will gladly take a look at your Multisig project and provide you with some feedback. I will try to do that sometime over the next few days

---

Point 1

@fedev

But this can protect greedy and lazy users (as I am) from bad frontends. Have you ever had to replay (and sadly… rePAY) a transaction just because the UI of the dapp wasn’t rounding properly? That sucks and does really hurt when fees are as high as they’re now on ethereum

Good point However, when you build a dapp, I do think the best solution for this would be to handle any rounding issues in the front-end before the actual withdraw() or transfer() function is called. The user’s current balance could be retrieved first by calling the getter (no gas cost for calling a getter), and then performing or alerting the user to any roundings or necessary adjustments to the actual amount they have requested to withdraw or transfer. Once these issues have been addressed (based on current information retrieved from the smart contract), the user can then have an option to accept or reject the adjusted/rounded amount (generated in the front-end). If they accept, then only at this point would the function in the smart contract, which sends the transaction to the network, automatically be called. The user experience would be that of performing just one single operation/transaction, but with a few additional steps in the UI if the original amount they request needs rounding or adjusting. The smart contract function would still contain the necessary require statements as further validation checks, but overall this approach should address the important issue you’ve raised, whilst also keeping gas costs down.

[fedev]

Hey! You’re definitely right. Also while attending the 201 course and coming across the EIP20 proposal I’ve found that both txs with amount > balance should throw AND txs with amount == 0 should not throw and emit standard txs.
I will obviously stick to the standards even if the second clause looks a bit strange/waste of gas but maybe there was a reason at that time to ask complaiance to not throw on transfer 0 amounts.

[diehlca17]

This problem can be easily fixed by setting a requirement that the balance is larger than the amount we are withdrawing. It returns the balance after the transfer.

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

[jon_m]

An excellent solution @diehlca17

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…

balance[msg.sender] -= amount;

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

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

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 you have any questions

[Oprea]

Hello! This is my solution:
` function withdraw(uint amount) public returns(uint){

require(balance[msg.sender] >= amount, "Not enough balance");
balance[msg.sender] -= amount;
msg.sender.transfer(amount);
emit withdrawedBalance(amount, msg.sender);
return balance[msg.sender];

}`

[0xsundown]

Hello! This is what I came up with for my solution.

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

[jon_m]

An excellent solution @Oprea

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…

@Oprea

balance[msg.sender] -= amount;

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

@Oprea

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 emit statement also looks good, and it’s in the correct position within the withdraw() function body. However, to be sure the code is correct, we need to see the corresponding event declaration you’ve added. Can you post that as well so we can see the complete picture?

Also, 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.

Just let me know if you have any questions

[jon_m]

Nice solution @0xsundown

You have added all of 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.

Once you’ve read the linked post, my following comments (about two of your inline comments) should also make sense …

@0xsundown

//Withdraw amount from balance
msg.sender.transfer(amount);

This line of code withdraws the amount from the contract address balance and transfers it to the external address which the transfer method is called on (in this case, msg.sender, the address calling the withdraw function). The contract’s Ether balance is reduced by the withdrawal amount, and the external address’s ether balance is increased by the same amount.

@0xsundown

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

This line of code reduces the individual user’s balance in the mapping to account for the fact that it’s this particular user who is withdrawing this amount from the contract.
msg.sender.transfer(amount)   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, effectively, the amount each is entitled to withdraw from the contract.

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

[simfin94]

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;

    address owner;

    event depositDone(uint amount, address indexed depositedTo);

    event transferDone(uint amount, address toAddress, address fromAddress);

    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(amount <= balance[msg.sender]);
        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);

        emit transferDone(amount, recipient, msg.sender);
    }

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


}

[jon_m]

Hi @simfin94,

The require statement you’ve added is correct, but your withdraw function is missing an essential line of code …

If you deploy your contract, make a deposit, and then call your withdraw function as it is, you will notice that the caller’s external address receives the requested withdrawal amount, but their balance in the mapping is not reduced! This means that they can make repeat withdrawals — each one no greater than their initial balance, which isn’t being reduced — up to the total amount of ether held in the contract (the sum of all of the individual account holders’ balances). So, I’m sure you can see why this is a very serious bug!

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external address, but it doesn’t adjust the individual user balances in the mapping. These balances perform an internal accounting role, and record each user’s share of the contract’s total ether balance. So, just as we increase a user’s individual balance when they deposit ether in the contract, we need to do the opposite whenever they make a withdrawal.

Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But 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.

Your transfer event and corresponding emit statement (from the previous Events Assignment) are both correct and well coded. The emit statement is in the correct position in the transfer() function body and will log appropriate data when the transfer() function is successfully executed

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

[LELUK911]

This is my code.
I use the modifier to control the balance and stop function during execution to avoid re-entry attacks and I updated the receipt before sending Eth from the contract to the user.

// SPDX-License-Identifier: Leluk911
pragma solidity 0.8.7;
contract Bank_Eth {
    // deposit receipt Eth
    mapping(address=>uint) public receipt;
    // variable modifier 
    bool paused=false;
    // balance eth bank
    uint public balance_Bank = address(this).balance;
    // event register transaction
    event Deposit_bank (address indexed,uint indexed);
    //event register withdrawal
    event withdrawal_bank(address indexed,uint indexed);
    
    // stop contract during execution
    modifier stay_paused{
        require (!paused, "contract in exet");
        paused = true;
        _;
        paused = false;
    }
    // control receipt user after withdrawal
    modifier control_receipt(uint _amount){
        require (receipt[msg.sender]>=_amount, "you deposit is less you require");
        _;
    }

    // function deposit in contract
    function depositEth()payable public stay_paused {
        receipt[msg.sender] += msg.value;
        emit Deposit_bank(msg.sender,msg.value);
    }

    // function withdrawal
    function withdrawalEth(uint _amount)public control_receipt(_amount) stay_paused {
        receipt[msg.sender] -= _amount;
        (bool success,) = msg.sender.call{value:_amount}("");
        require(success,"transaction faill");
        emit withdrawal_bank(msg.sender,_amount);
    }



}

[imran82]

pragma solidity 0.7.5;

contract Bank{

mapping(address => uint)balance;

// This function will deposit money to the contract

function deposit() public payable returns(uint){

    balance[msg.sender]+=msg.value;

    return balance[msg.sender];

   

    }

//this function will withdraw the amount from the contract to the address calling this function    

function withdraw(uint amount)  public  {

    require(amount <= balance[msg.sender],"Balance is not sufficient");

    msg.sender.transfer(amount);

    balance[msg.sender]-=amount;

   

}

//this function will get the balance of address executing this function

function getyourBalance() public view returns(uint){

    return balance[msg.sender];

}

//this function will get you the total balance of ether in your contract

function CheckBalanceofContract() public view returns(uint){

    return address(this).balance;

}

}

[DeMarco_Saunders]

Here’s my answer.

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

[Techman120]

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

        require(balance[msg.sender] >= amount, "Balance not sufficient");

        msg.sender.transfer(amount);

        balance[msg.sender] -= amount;

    }

[Arch.xyz]

pragma solidity 0.7.5;

contract Bank{

    address owner;

    mapping(address => uint) balance;

    event depositDone(address depositedTo, uint amount);

    event transferred (address indexed from, address indexed to, uint amount);

    event withdrawal(address indexed To, uint amount);

    modifier onlyOwner {

        require(msg.sender == owner);

        _;

    }

   

    constructor(){

        owner = msg.sender;

    }

    function deposit() public payable returns (uint){

        balance[msg.sender] = balance[msg.sender] + msg.value;

        emit depositDone(msg.sender, msg.value);

        return balance[msg.sender];

    }

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

        require(balance[msg.sender] >= _amount, "You dont have enough eth");

        msg.sender.transfer(_amount);

        balance[msg.sender] -= _amount;

        emit withdrawal(msg.sender, _amount);

        return(balance[msg.sender]);

     

    }

    function getBalance() public view returns(uint){

        return balance[msg.sender];

    }

    function transfer(address resipiant, uint amount) public{

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

        require(msg.sender != resipiant, "ERROR");

        _transfer(msg.sender, resipiant, amount);

        emit transferred(msg.sender, resipiant, amount);

    }

    function _transfer(address from, address to, uint amount) private {

        balance[from] -= amount;

        balance[to] += amount;

    }

}

[jon_m]

Hi @LELUK911,

This is an excellent solution which not only solves the problem with the withdraw() function, but does it by implementing some advanced smart-contract coding techniques

Correctly coded and well implemented modifier with a require() statement to prevent users from withdrawing more than their individual balance.

Correct implementation of the call method, as an alternative to using the transfer method, in order to transfer the withdrawal amount from the contract address balance to the withdrawer’s own external address balance.

You are right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from both the withdrawalEth() and the depositEth() function headers.
However, do you not think the getBalance() function is a more straightforward way for a user to retrieve their current balance, instead of the automatically created getter for the public mapping? The getBalance() function uses msg.sender to avoid having to input an address argument.

Your withdraw event and corresponding emit statement are coded well enough to enable appropriate data to be logged when the withdrawalEth() function is successfully executed. The emit statement is also in the correct position in the function body.
However, you haven’t named the parameters in either of the event declarations (for both the depositEth and the withdrawalEth functions). Naming the event data isn’t mandatory, and both data values will still be emitted and logged for both events; but it will make the data clearer and easier to identify, and especially in situations where you have more than one parameter with the same data type.

@LELUK911

I updated the receipt before sending Eth from the contract to the user.

Yes … your withdrawalEth() function code ensures that each operation is executed in the correct order to reduce security risk:

• Check inputs (require statements)
• Effects (update the contract state)
• External Interactions (e.g. transfer or call method)

Modifying 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 helps to prevent re-entrancy attacks from occuring. In fact, in terms of protection against re-entrancy attacks, your stay_paused modifier reinforces your contract’s security even further

---

@LELUK911

uint public balance_Bank = address(this).balance;

The automatic getter created for this public state variable will not retrieve the current contract balance, because it is assigned to the state variable on deployment (when the contract balance is zero) and then never updated. So, the balance_Bank getter will always return zero. Instead, you should add a getter to your contract which returns the contract balance at the time the getter is called e.g.

function getContractBalance() public view returns(uint) {
    return address(this).balance;
}

---

By the way, don’t forget to do the Events Assignment and post your solution here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course, and it provides useful practice at coding events.

Just let me know if you have any questions about anything

[jon_m]

Hi @imran82,

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

You are also right that it’s not necessary to return the updated balance, and so  returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which a user can call separately to consult their own individual updated balance after a withdrawal.

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.

By the way, don’t forget to do the Events Assignment and post your solution here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course, and it provides useful practice at coding events.

Just let me know if you have any questions

[jon_m]

Hi @DeMarco_Saunders,

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…

balance[msg.sender] -= amount;

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

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

It’s just a shame you omitted the function body’s closing curly bracket

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 you have any questions.