Transfer Assignment

[jon_m]

Hi @Techman120,

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

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 at the end of the function body. The compiler should have given you an orange warning for this.

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.

Let me know if you have any questions.

[jon_m]

Nice solution @Arch.xyz

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

The withdrawal and transfer events you’ve also added, and their corresponding emit statements, are correct and well coded. Both emit statements are correctly positioned within their respective function bodies and will log appropriate data when these functions are successfully executed

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.

---

A couple of other minor observations …

@Arch.xyz

return(balance[msg.sender]);

If you are only returning one value, you don’t need to enclose it within parentheses in the return statement. You only need the parentheses if you are returning more than one value within the same statement, each separated by a comma.

@Arch.xyz

event withdrawal(address indexed To, uint amount);

Keep your use of upper v lower case consistent for the first letter of names/identifiers. The convention is to start the names of structs, events, contracts and interfaces with a capital letter, and the names of variables (including arrays and mappings), functions, modifiers, parameters, arguments and return values with a lower case letter. Keeping to this convention, or at least being consistent across all of the smart contracts in the same project — so that, for example, you don’t do what you’ve done in the event declaration above and start one parameter name with a capital letter and another with a lower case letter — makes your code more readable and developer-friendly.

Just let me know if you have any questions

[LELUK911]

hi @jon_m
, thank for your response.
I will study the arguments better that you have me see for correct my error in next code…

[Techman120]

Thanks for the feedback!

function withdraw(uint amount) public {

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

        balance[msg.sender] -= amount;

        msg.sender.transfer(amount);

    }

[Wyse_Sousa]

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

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

        balance[msg.sender] -= amount;

        return balance[msg.sender];

[Arch.xyz]

thank you for the reply and ive taken note, remix did a good job in keeping me in check when i made similar mistake on the project.

[Arch.xyz]

Corrected

    }

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

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

        balance[msg.sender] -= _amount;
        
        msg.sender.transfer(_amount);

        emit withdrawal(msg.sender, _amount);

        return(balance[msg.sender]);

[jon_m]

Hi @Wyse_Sousa,

You have added the additional lines of code needed to solve the problem with the withdraw function, but you’ve left out the following statement which was in the code you were given to start with …

msg.sender.transfer(amount);

If you deploy your contract, make a deposit, call your withdraw function as it is, and then call getBalance() with the same withdrawer’s address, you will notice that the caller’s individual balance in the mapping is reduced by the requested withdrawal amount, but their external address does not receive the Ether. If the function is coded correctly, you should see the withdrawer’s external address balance (showing in the Account field near the top of the Deploy & Run Transactions panel in Remix) increase by this amount.

Instead, each time your withdraw() function is called, the caller is effectively losing ownership of the amount of Ether they have requested to withdraw. Essentially, the smart contract has been coded to steal users’ funds, so I’m sure you can see why this is a very serious bug!

The individual user balances in the mapping perform an internal accounting role, and record each user’s share of the contract’s total Ether balance (effectively, each user’s entitlement to withdraw Ether from the contract). However, reducing a user’s individual balance in the mapping for a specific amount doesn’t actually transfer an equivalent amount of Ether from the contract address balance to the user’s external address balance: that’s what the transfer method does …

<payable address>.transfer(uint amount);

Once you’ve included the missing transfer method, have a look at this post to make sure the order of your statements within the withdraw() function body ensures your smart contract is protected against re-entrancy attacks.

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

[Wyse_Sousa]

Once again Thank you for the explanation!
I saw the errors in the video and understood the mistakes.

[Liquidbaker89]

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

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

balance[msg.sender] -= amount;

msg.sender.transfer(amount);

return balance[msg.sender];

}

[Giupi]

Hi everyone, I thought that it’d be pretty straightforward to have a require statement, to make sure that the balance is higher or equal than the balance to withdraw, then adjust the balance accordingly.

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

[MichaelAudire]

My withdraw function solution so that an account may not withdraw funds that is not theirs.

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    address owner;
    

    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    modifier insureAccFunds(uint _amount) {
        require(balance[msg.sender] >= _amount, "Insufficient funds.");
        _;
    }

    event fundsAdded(uint amount, address depoAddy);
    event fundsTransferred(address indexed fromAccount, address indexed toAccount, uint _amount);

    function addBalance() public payable returns(uint){
        balance[msg.sender] += msg.value;
        emit fundsAdded(msg.value, msg.sender);
        return balance[msg.sender];
    }

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

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


    function transfer(address recipient, uint amount) public onlyOwner insureAccFunds(amount) {
        require(msg.sender != recipient, "(recipient != msg.sender)");
        uint balanceBeforeTransfer = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
        assert(balance[msg.sender] == balanceBeforeTransfer - amount);
        emit fundsTransferred(msg.sender, recipient, amount);

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


}

[Abel]

//I used a modifier to check if the msg.sender has enough balance to withdraw, and after the transfer I updated the balance[msg.sender]

modifier checkBalance(uint amount){ 
         require(balance[msg.sender] >= amount, "insufficient funds"); 
         _;
             }
 function withdraw(uint amount) public checkBalance(amount) returns(uint){
        balance[msg.sender]-=amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }

[jon_m]

Nice solution @Liquidbaker89

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

[jon_m]

Nice solution @Giupi

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.

Just let me know if you have any questions

[jon_m]

Nice solution @MichaelAudire

The code you’ve added has successfully solved the problem with the withdraw function. And your use of an additional modifier (insureAccFunds), to avoid having to duplicate the same require() statement in both the transfer() and the withdraw() functions, is very nicely done

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 one other observation…
Your onlyOwner modifier in the transfer() 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.

Just let me know if you have any questions

[jon_m]

Nice solution @Abel

The code you’ve added has successfully solved the problem with the withdraw function, and your checkBalance modifier is well coded and nicely implements the require() statement. The benefit of using this modifier is to avoid having to duplicate the same require() statement in both the transfer() and the withdraw() functions. Have you also added this modifier to your transfer() function header, and removed the first require() statement from the function body?

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.

Please, fully format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum
Your posted solution should end up looking like this …

modifier checkBalance(uint amount) {
    require(balance[msg.sender] >= amount, "insufficient funds");
    _;
}

function withdraw(uint amount) public checkBalance(amount) returns(uint) {
    balance[msg.sender] -= amount;    // order of statements changed
    msg.sender.transfer(amount);      // (see comment and link above)
    return balance[msg.sender];
}

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.

Just let me know if you have any questions

[Abel]

Thanks Jon for your input, very valuable, I didn’t even think about the order of the statements and now I can see the importance to do it correctly.

Sorry about the formatting issue, I thought I did it correctly, I just edited it and I will be more careful next time

[jon_m]

No problem
Glad the feedback was helpful.

[Adrian1]

pragma solidity 0.7.5;
contract Bank{

  mapping(address => uint)Balance;
  address owner;
  modifier OnlyOwner{
        require(owner == msg.sender);
        _; //run the function
  }
  
  event DepositDone(uint Amount, address DepositedTo);
  event TransactionDetails(address sender, address receiver, uint amount);

  constructor(){
     owner = msg.sender;
     
  }

    function Deposit() public payable returns(uint){

        Balance[msg.sender] = Balance[msg.sender] + msg.value; //Adds ether to the contract
        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); //transfers the Ether in the smart contract to the selected address

        Balance[msg.sender] = Balance[msg.sender] - amount;

        return Balance[msg.sender];

        
    }

    function transfer(address recipient, uint amount) public{

        require(Balance[msg.sender] >= amount, "Balance not enough");
        require(msg.sender != recipient, "You can't send funds to yourself!");

        uint BalanceBefore = Balance[msg.sender];

        _transfer(msg.sender, recipient, amount);

        assert( Balance[msg.sender] == BalanceBefore - amount);

         emit TransactionDetails(msg.sender , recipient, amount);

        //calls the transfer function, and takes the parameters of msg.sender(owner of an address), recipient and an amount
    }

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

        Balance[from] -= _amount;
        Balance[to] += _amount;

        //Subtracts balance from the sender and adds it to the recipient

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