Transfer Assignment

Wyse_Sousa · February 10, 2022, 4:49pm

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

Liquidbaker89 · February 10, 2022, 5:02pm

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 · February 11, 2022, 11:12pm

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 · February 12, 2022, 8:40pm

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 · February 13, 2022, 3:48pm

//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 · February 13, 2022, 2:05pm

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 · February 13, 2022, 2:10pm

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 · February 13, 2022, 2:41pm

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 · February 13, 2022, 3:06pm

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 · February 13, 2022, 3:50pm

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 · February 13, 2022, 4:07pm

No problem
Glad the feedback was helpful.

Adrian1 · February 15, 2022, 10:16pm

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

jon_m · February 17, 2022, 4:22am

Nice solution @Adrian1

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.

Some additional comments …

(1)

No … this line of code doesn’t add Ether to the contract address balance.

A function is marked payable when it needs to receive Ether from an external address to add to the contract address balance . Including payable in a function header enables the Ether value sent to the function to be added to the contract address balance, without us having to add any further code for this. In the deposit() function, this happens automatically because we have marked the function as payable . The line …

… then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

(2)

To clarify …

The _transfer() helper function is called with 3 arguments. These 3 values are input into the _transfer() function and assigned to 3 corresponding parameters:

The argument msg.sender (an address) is assigned to the from parameter.
The argument recipient (an address) is assigned to the to parameter.
The argument amount (an unsigned integer) is assigned to the _amount parameter.

msg.sender always references the address that has called the function in which it is used.

(3) You now have the emit statement for the DepositDone event in the correct position in the deposit function

(4) Have a look at the names of all of your functions, and think about what I said in my feedback for your Events Assignment code regarding the importance of consistency with names …

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

CryptoUsic · February 17, 2022, 8:32am

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

jon_m · February 18, 2022, 6:27pm

An excellent solution @CryptoUsic

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

tbem · February 21, 2022, 11:09pm

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.7.5;

contract Bank{

  mapping(address => uint) balance;
  // MAX 3 indexed params per event
  event depositDone(uint256 amount, address indexed depositedTo);
  event withdrawtDone(uint256 amount, address indexed depositedTo);
  event transferDone(address indexed fromAddress, address indexed toAddress, uint256 amount);

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

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

  function withdraw(uint amount) public payable{
    require(balance[msg.sender] >= amount, "You have not enought balance!");
    msg.sender.transfer(amount);
    balance[msg.sender] -= amount;
    emit withdrawtDone(msg.value, msg.sender);
  }

}

jon_m · February 22, 2022, 12:48am

Hi @tbem,

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. Besides, we do already have the getBalance() function which a user can call to consult their own individual updated balance after a withdrawal.

However, we should not mark the withdraw function as payable, because it doesn’t need to receive Ether from an external address in order to update the contract balance. If we make it payable, then, if the caller sends an Ether value to the withdraw function by mistake (as well as calling it with a withdrawal amount) then this Ether would be added to the contract balance and potentially lost to the user: this is because the withdraw function does not make an equivalent accounting adjustment to the mapping for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller makes the same mistake, the function will revert, and the Ether won’t be sent.

A function only needs to be marked payable when it receives ether from an external address to add to the contract address balance. It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function, this happens automatically because we have marked the function as payable. The line balance[msg.sender] += msg.value; then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

In contrast, the withdraw function is deducting ether from the contract balance and sending it to an external address. It does this using the address member transfer (which is like a method called on a payable address)…

<address payable>.transfer(uint256 amount);

Your withdraw event is a nice thing to add. The event declaration is well coded, but from what I’ve just explained about the function type payable, you can probably now see why referencing msg.value in the corresponding emit statement will not log the withdrawal amount: msg.value will only log a value if Ether is also sent to the function and, as I’ve mentioned above, we want to prevent that from happening by leaving the function type as undefined (i.e. non-payable). Instead, the emit statement needs to reference the amount parameter. You correctly deducted amount (and not msg.value) from the individual user’s balance in the mapping, so that it continues to accurately reflect their share of the total amount of Ether held in the contract…

balance[msg.sender] -= amount;

This amount is the same value you want your withdraw event to log for the withdrawal amount, together with the withdrawer’s address (msg.sender).

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.

Can you also add your transfer() function? That way we can see your transfer event’s corresponding emit statement, and your full solution to the Events Assignment from the Events video lecture.

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

RyanYeap · February 27, 2022, 3:21pm

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

I get a TypeError.

from solidity:
TypeError: "send" and "transfer" are only available for objects of type "address payable", not "address".
  --> helloworld.sol:27:9:
   |
27 |         msg.sender.transfer(amount);
   |         ^^^^^^^^^^^^^^^^^^^

I thought msg.sender is already a payable object?

edit: Error only comes up with the latest compiler 0.8.7. I switched to 0.7.5 and it works.
From 0.8.0, using

        payable(msg.sender).transfer(amount);

seems to work.

jon_m · March 1, 2022, 2:56pm

Great solution @RyanYeap

You have added both of the essential 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

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. Besides, we do already have the getBalance() function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.

Yes … well done for resolving this yourself

msg.sender is payable by default in versions of Solidity prior to v0.8, where it doesn’t need to be explicitly converted (which is what you were expecting). However, from v0.8 msg.sender is non-payable by default, and so when it needs to refer to a payable address — such as here, when the transfer method is called on it — it needs to be explicitly converted using payable()

Just let me know if you have any questions

ekr990011 · March 2, 2022, 12:26am

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

I found out that the
balance[msg.sender] -= amount;
was needed by messing around and to my shock being able to take out more. You can have the require all you want but if you never actually update their balance in the balance mapping

I see I am not the only one to have found that out and that luckily I put it in the correct spot to stop a re-entrancy attack as mentioned by jon_m above. Makes sense and really enjoyed the extra reasoning.