Solidity Payable Functions - Discussion

[jon_m]

An excellent solution @Samm

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:

1. check inputs (require statements)
2. effects (update the contract state)
3. external interactions

Just as you have done, it’s important to modify the contract state for the reduction in the balance…

balance[msg.sender] -= amount;

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

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. The type of attack this specific order of operations prevents is a re-entrancy attack, and this is covered in detail in the Smart Contract Security course. But it’s great you’re already getting into good habits in terms of best practice in this area

---

@Samm

thought I would need to create a temp address as payable and assign it to msg.sender before using the transfer function on that variable. I wasn’t quite sure how to define an address variable as payable, but now that I see you can cast msg.sender as payable

You need to bear in mind that the syntax taught in this course is for Solidity v0.7. That’s absolutely fine to use v0.8, but one of the changes in syntax from v0.8 is that msg.sender is no longer a payable address by default, and so when it needs to refer to a payable address, it needs to be explicitly converted. As you have discovered, one of the ways to do this is with  payable(<address>)
You can also do it the way you initially thought of, by assigning msg.sender to a local payable-address variable, as follows:

address payable receiver = payable(msg.sender);
receiver.transfer(amount); 

But as you can see, we would still have to explicitly convert msg.sender in order to be able to assign it to a payable address variable, so in our particular case it ends up being much less concise.

If you are using Solidity v0.8 for your assignments, then include that at the top of your code, so we know what syntax we are reviewing and checking for.

The solidity compiler will always generate an error when your syntax needs to be changed for v0.8, and the error message will tell you why, and often what you should change it to. In fact, msg.sender changing from payable to non-payable by default could well be the only change in syntax from v0.7 to v.0.8 that you will face during this initial course. So it shouldn’t cause much of an issue for you.

In order to receive ether, an address needs to be payable , so this is why in Solidity v0.8 you have to convert msg.sender when using it as the address in…

<address payable>.transfer(uint256 amount)

---

The withdrawal event (with corresponding emit statement), which you’ve added, is also appropriate and well coded. The emit statement will log relevant information when a call to the withdraw function has executed successfully.

---

@Samm

I also found it was easier to change my value type to wei in the contract header so I didn’t have to put so many zeros in the number!

I’m not too sure what you mean by this to be honest. What exactly did you add to the contract header?

contract Bank {  // This is the contract header. Is this actually what you mean?

---

Just let me know if you have any questions

[Jimmyjim]

Hi

Yes, now I see I used wei instead of ether when I transferred, Forgot to change that! Thank you

Regards
JImmy

[Samm]

I mean on the deploy and run transactions page of Remix. I paid 1000 wei and withdrew/transfered 500 wei. It made more sense to me than paying 1 eth and withdrawing 500000000000000000000 wei!

[jon_m]

Ah, I see what you mean. I find that doing that makes it harder to check and follow the movements in the address balances, so I’ve got used to banging out 18 zeros rapidly
But, hey, we all have our own survival methods to make things easier

[KennethJP]

pragma solidity 0.7.5;
contract bank {
    
mapping(address => uint) balance;

    address owner;
    
    event withdrawalDone (uint amount, address indexed withdrawTo);
    event depositDone(uint amount, address indexed depositedTo);
    //Arguments that are indexed are actually indexed by the ethereum nodes... 
    //This means we can use these arguments to search and query for certain events that have happened in the past.
    event transferAdded(uint amount, address indexed depositedTo, address depositedFrom);
    
    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 payable returns (uint){
        require (balance [msg.sender] >= amount, "don't try to steal");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        emit withdrawalDone (msg.value, msg.sender);
        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 transfer money to yourself");
            
            uint previousSenderBalance = balance [msg.sender];
            emit transferAdded (amount, recipient, msg.sender);
            _Transfer(msg.sender, recipient, amount);
            
       
                
            assert (balance [msg.sender] == previousSenderBalance - amount);
    }
        function _Transfer(address from, address to, uint amount) private{
            
                balance [from] -= amount;
                 balance[to] += amount;
                  
        }
        
    }

[Flippy]

At first I had trouble in msg,sender.transfer(amount) not working and had to change it to payable (msg.sender).transfer(amount);

pragma solidity 0.8.4;

    contract BinanceSmartChain {
            mapping(address => uint) balance;
            address owner;
            
            event depositDone(uint amount, address indexed depositedTo);
            
            modifier onlyOwner {
                require(msg.sender == owner,"You are not allowed to withdraw!!!");
                
                _;
            }
            
            constructor (){
                owner = msg.sender;
            }
            function deposit()public payable returns (uint){
                
                // balance [msg.sender] = balance[msg.sender] + _toAdd; or 
                balance[msg.sender] += msg.value;
                emit depositDone(msg.value, msg.sender);
                return balance [msg.sender];
            } 
            function withdraw(uint amount) public onlyOwner returns (uint){
                require(balance[msg.sender] >= amount,"Insufficient balance.");
                payable (msg.sender).transfer(amount);
                balance[msg.sender] -= 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, "Your balance is insufficient");
                require(msg.sender != recipient,"Error transfering to yourself is not allowed");
                uint previousBalance = balance[msg.sender];
                
                _transfer (msg.sender,recipient, amount);
                
                assert (balance[msg.sender] == previousBalance - amount);
            } 
            function _transfer(address from,address to, uint amount) private {
                
                balance [from] -= amount;
                balance[to] += amount;
            }
    }
    

[sohamdey_2006]

As mentioned in the payable function video, how come the function will be pay without the " balance[msg.sender] = msg.value; " . It is mentioned in the video that just writing ‘payable’ in the function header the function will make the payment, but how? Without adding the msg.value in the balance[address] how will the value be added? Any explanation and help is appreciated.

[jon_m]

Hi @KennethJP,

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

You have added all of the additional lines of code needed to solve the problem with the withdraw function. However there are also a few issues which need resolving:

(1) We should not mark the withdraw function as payable , because it does not need to receive ether from an external address in order to update the contract balance. If we made it payable , then, if the caller sent 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, because there is no accounting adjustment made to the mapping in the withdraw function 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 made the same mistake, the function would revert, and the ether would not be sent.

(2) At the moment, if you only use the withdraw() function to withdraw an amount (its intended purpose), then your withdrawal event will log 0 for the amount withdrawn. This is because msg.value references an ether value that is sent to a payable function and is automatically added to the contract address balance. The emit statement should reference the amount parameter, instead. When you restrict the function to non-payable, using msg.value will throw a compiler error, anyway.
Apart from this, your withdrawal event and corresponding emit statement are both correct, and the emit statement is placed in the correct position within the function body.

(3) Your transfer event and corresponding emit statement are also correctly coded, and the emit statement will log relevant information about a transfer. However, events should only be emitted after the event itself (here, the transfer) has occurred. Therefore, the emit statement should be placed at the end of your transfer() function, after the assert() statement.

(4) 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.

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

[jon_m]

Hi @Flippy,

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

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

@Flippy

At first I had trouble in msg,sender.transfer(amount) not working and had to change it to payable (msg.sender).transfer(amount);

That’s because you are using Solidity v0.8. That’s absolutely fine, but then you also need to bear in mind that this course material is based on v0.7 syntax. The compiler will always generate an error when your syntax needs to be changed for v0.8, and the error message will tell you why, and often what you should change it to.
In order to receive ether, an address needs to be payable . In versions prior to v0.8 msg.sender is payable by default and so doesn’t need to be explicitly converted. However, from v0.8 msg.sender is non-payable by default, and so when it needs to refer to a payable address, it needs to be explicitly converted. One of the ways to do this is using payable() , which is what you’ve done:

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

Well done for working this out

---

You’ve modified the withdraw function with the onlyOwner modifier, but this means that only the contract owner can withdraw funds up to their own individual balance. But the contract allows multiple addresses to deposit funds (we are simulating a bank with bank account holders) and so it only seems fair that all users should be able to withdraw their funds as well, don’t you think?

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

Hi @sohamdey_2006,

Apologies for the delay in answering your question…

@sohamdey_2006

It is mentioned in the video that just writing ‘payable’ in the function header the function will make the payment, but how?

Any function marked payable will automatically add to the contract address balance any ether/wei value sent to the function when it is called. This happens without us having to add any further code for this. This is what happens in our deposit() function.

@sohamdey_2006

Without adding the msg.value in the balance[address] how will the value be added?

As well as the ether/wei value being added to the contract address balance automatically,
the line   balance[msg.sender] += msg.value;   does then add the same ether/wei value to the individual user’s balance in the mapping, in order to keep a track of their share of the total funds held in the contract.

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

What may be confusing you is that all ether deposited into the contract is held altogether as one single contract address balance. This contract balance is different to each individual user’s balance stored in the mapping. However, at any one time, they should all add up to the contract balance. These individual balances are like accounting entries which enable the contract to keep track of each individual user’s share of the total pooled funds held in the contract balance.

When we make a function payable , Solidity automatically makes msg.value available to us as a way to reference any amount of ether that is sent to the function when it is called. Effectively, msg.value is a “pre-defined parameter”, which already comes “built-in” to functions we write and mark as payable . In a similar way to msg.sender (which already comes “built-in” to every function we write) we don’t have to explicitly declare msg.value as a parameter within the parentheses in the function header like we do with other values we want to pass to a function.

I hope this clarifies things for you. But just let me know if anything is still unclear, or if you have any further questions

[Jacky_Li]

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

[jon_m]

Nice solution @Jacky_Li

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 function body.

Your additional emit statement looks well-coded, but can we also see the corresponding event declaration, which I’m assuming you’ve added at the top of the contract with the other event declarations.

Just let me know if you have any questions

[Konzaih]

I’ve tried to solve the withdrawal problem with a require function.
When testing I was not able to withdraw more than the balance, but I would appreciate someone having a look to confirm it does exactly what we want

 function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender]>= amount);
        //msg.sender is an address
        msg.sender.transfer(amount);
        
    }

[Claudio]

hi. how can one check the totoal amount of ETH in the Bank contract using Remix? I only managed to get the balance by address.

[jon_m]

Hi Claudio,

@Claudio

how can one check the totoal amount of ETH in the Bank contract using Remix?

address(this).balance

You can implement a getter to retrieve the contract’s account balance:

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

This is a very useful thing to implement, because it helps a lot with testing.

Just let me know if you have any further questions

[jon_m]

Hi @Konzaih,

The require statement you’ve added is correct, but your withdraw function is missing 2 other lines of code…

(1) 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 wallet address receives the requested withdrawal amount, but their balance in the mapping is not reduced (you can call getBalance to check this). This means that, while they cannot withdraw more than their balance for their 1st withdrawal, 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). This would obviously be a very serious bug.

msg.sender.transfer(amount) transfers ether from the contract address balance to the caller’s external wallet 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.

(2) Notice that the withdraw function header also includes returns(uint) . This is not mandatory to include, and the function can still operate effectively without returning a value. But if you include it in the function header, you should also include a return statement in the function body. If you don’t, the compiler will give you a yellow/orange warning for this.

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

[Claudio]

thank you! very helpful

[DavidV]

Hi Everyone, I’m a bit confused as to why the deposit function is payable but not the witdraw function. Is the contract not sending Ether back to msg.sender?

[thecil]

Hey @DavidV, hope you are well.

@DavidV

I’m a bit confused as to why the deposit function is payable but not the witdraw function. Is the contract not sending Ether back to msg.sender?

The payable keyword is used on any function that must receive funds in ETH, while in the withdraw function there is no need for it because is not receiving, instead it will sent ETH out of the contract.

Carlos Z

[DavidV]

Hi Carlos, thank you for that. I’m still not quite clear though… sorry…

For clarity I’m looking at the bank contract provided by Filip :

pragma solidity 0.8.0;
pragma abicoder v2;
import "./Ownable.sol";
import "./SafeMath.sol";

contract Bank is Ownable {
    
    using SafeMath for uint256;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] = balance[msg.sender].add(msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] = balance[msg.sender].sub(msg.value);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
      function contractBalance() public view returns (uint){
        return address(this).balance;
    }
    
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
    
}

I assume for the ETH to move from the contract to the msg.sender address this method

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

is responsible for moving the ETH and the internal balance is handled by the balance mapping

mapping(address => uint) balance;

Correct?

I have a problem though:

Here is the withdrawal function from the bank contract that Filips provided, however without making it payable I get an error :

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

This is the error.

TypeError: “msg.value” and “callvalue()” can only be used in payable public functions. Make the function “payable” or use an internal function to avoid this error.
–> Dawie_Laptop/Remix Exercises/BANK 2.0/Bank_Fillip.sol:24:55:
|
24 | balance[msg.sender] = balance[msg.sender].sub(msg.value);
| ^^^^^^^^^

If you have some insight for me I’d much appreciate it. Obviously, this is a fundamental part of Solidity that I want to fully understand.