Transfer Assignment

[jon_m]

Hi @Lehigr2,

You have added all of the additional lines of code needed to solve the problem with the withdraw function However, in terms of risk management and security, you should make an important modification to their order:

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

Checking inputs should be the very first thing we do. Even though the transaction would still revert wherever require is positioned, initiating an external transfer before performing our own internal checks and accounting adjustments is poor risk management and increases the potential for any attack to be successful.

In addition, 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 letting you know for extra information.

Just let us know if you have any questions

[StanleyGM]

Thanks @jon_m for the assistance. It’s greatly appreciated!

[AustinBurks]

Use require() to ensure the person can only withdraw their own balance.

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposite() 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, "Insufficient balance");
        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);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

[jon_m]

Hi @AustinBurks,

The require statement you’ve added is correct but you are missing 2 other important lines of code in your withdraw function:

(1) When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount (from the contract address balance), but their own balance in the mapping (their share of the overall contract balance) is not reduced! I’m sure you can see that this is another very serious bug! How would you correct this? Think about this…

@AustinBurks

Use require() to ensure the person can only withdraw their own balance.

The require() statement ensures that the caller cannot withdraw more than their own balance. This is different. Using msg.sender ensures that the caller can only withdraw from their own balance.

If you can’t work out how to correct this yourself, you’ll find the answer by taking a look at some of the other posts in this topic.

---

(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 as it’s been included in the function header, you also need to include a return statement in the function body.

---

Once you’ve had a go at making the above modifications, 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 you have any questions about how to correct this, or if there is anything that you don’t understand

[benlive]

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

        payable(msg.sender).transfer(amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender];
    }

[BenB]

pragma solidity 0.7.5;

contract ProcessMoney {
    
    struct Account {
        address addr;
        uint balance;
    }
    
    mapping (address => uint) accounts ;
    event DepositDone(uint amount,address depositedTo );
    event WithdrawDone(uint amount,address WithdrawTo );

    function deposit() payable public {
        //accounts[msg.sender] += amount;
        accounts[msg.sender] += msg.value;
        emit DepositDone(msg.value, msg.sender);
    }
    
    function getBalance() public returns(uint) {
        return accounts[msg.sender];
    }
    
    function withdraw(uint amount) public {
        uint previousBalance = accounts[msg.sender];
        require(accounts[msg.sender] >= amount);
        msg.sender.transfer(amount);
        accounts[msg.sender] -= amount;
        emit WithdrawDone(amount,msg.sender);
        assert(accounts[msg.sender]==previousBalance - amount);
    }
    function transfer (address _receiveAddr, uint _amount) public {
        uint previousBalance = accounts[msg.sender];
        require(accounts[msg.sender] >= _amount);
        accounts[msg.sender] -= _amount;
        accounts[_receiveAddr] += _amount;
        assert(accounts[msg.sender]==previousBalance - _amount);
        
    }
}

[Jazmin]

    function withdraw(uint amount) public returns (uint){       //input from user specifying amount to withdraw
        require(amount <= balance[msg.sender],"Balance is less than amount you are trying to withdraw");
        //amount should be deducted from the user balance BEFORE the transfer takes place to avoid certain types of exploits.
        balance[msg.sender] -= amount;
        //msg.sender is a payable type address
        msg.sender.transfer(amount);   // amount in wei
           }

I hope the above is correct! Is there a need to use an assert function here as well?

[jon_m]

Nice solution @benlive

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]

Hi Ben,

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

You are right that  returns(uint)  can be removed from the function header. It isn’t mandatory to include, and the function can still operate effectively without returning a value.

The additional assert statement and withdrawal event (with corresponding emit statement), which you’ve added, are also appropriate and well coded. The only thing I would say, is that the emit statement is probably better positioned after the assert statement, rather than before.

If an input value needs to be checked or validated with a require() statement, then this should normally be the first line of code in the function body to be executed. If require() fails, then the transaction will revert wherever it is placed; but any code placed before require(), and which will already have been executed before require triggers revert, will result in less gas being refunded than if the check had been the first operation to be performed within the function body. 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. You can probably see, therefore, that the earlier in the function require() is placed, the less the cost of gas will be if it throws and the transaction reverts.

Finally, have a look at this post which explains an important security modification that should be made to the order of two other statements within your withdraw function body. See if you can reorder your code for this, as well as for the other points I have explained above.

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

[jon_m]

Hi @Jazmin,

You have included nearly 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
Your comments are very good and accurate, so you clearly understand what the code is doing, and have also been reading the additional information here in this discussion topic about why the order of the statements is important

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 should have given you an orange warning with a message explaining what needed your attention. What code should you add for this?

@Jazmin

Is there a need to use an assert function here as well?

It wasn’t necessary for this assignment, but you certainly could add one. Your decision would most likely be based on how critical you considered the potential risk and consequences to be of the function code failing to execute as it should.

You could have also added an event and emit statement to log key information about the withdrawal. In practice this would depend on the use cases for such information about the transaction (e.g. in the frontend to display to users etc.). But don’t worry, we weren’t expecting you to add this either. We were just looking for the essentials. Most of the assignments leave scope for adding addtional code, because we want to encourage you to experiment. So, if in doubt, or if you’re feeling inspired, feel free to add any additional code you think is appropriate. Include some comments, with questions if necessary, so we can see what lines you are thinking along.

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

[m_Robbie]

There are no errors, but I know the code is too verbose. I’m also not always sure which address/contract object is making the call. Looked up best practices on why it’s best to explicitly return a value on a withdrawal function.

I’m assuming the transfer() function we were suggested to use within our withdrawal() block means that we overloaded it by defining it with different parameters than is ordinary with address objects…

Here’s my block:

        function withdraw(uint amount, address recipient) public returns (uint){
            //check balance mapping and error handling
            require(balance[msg.sender] >= amount, "Must have adequate funds.");    //check balance of msg.sender
            require(msg.sender != recipient, "Cannot send funds to yourself.");
            msg.sender.transfer(amount);    //overloaded transfer() function for use by address obj
            balance[msg.sender] -= amount;
            return balance[msg.sender];
        }

[Jazmin]

Wow thank you for all the feedback, it’s very helpful!

[1984]

function withdraw (uint amount) public {
require(amount <= balance[msg.sender], “solde insufisant”);
balance[msg.sender]-=amount;
msg.sender.transfer(amount);

[1984]

Also just to make sure that I understood well, we use also a function transfer that we created ourselves in order to transfer From To an Amount. It has nothing to do we this transfer() function. Right?

[ppoluha]

pragma solidity 0.7.5;

contract TransferAssignment {
    
    mapping(address => uint) balances;
    address owner;
    
    event depositDone(address indexed depositedFrom, uint amount);
    event withdrawalDone(address indexed withdrawnTo, uint amount);

    constructor() {
        owner = msg.sender;
    }
    
    function deposit() public payable returns (uint) {
        balances[msg.sender] += msg.value;
        emit depositDone(msg.sender, msg.value);
        return balances[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint) {
        require(balances[msg.sender] >= amount, "Not enough funds for withdrawal!");
        uint originalAmount = balances[msg.sender];
        msg.sender.transfer(amount);
        balances[msg.sender] -= amount;
        assert(balances[msg.sender] + amount == originalAmount);
        emit withdrawalDone(msg.sender, amount);
        return balances[msg.sender];
    }
    
    function getBalance() public view returns (uint) {
        return balances[msg.sender];
    }
}

[jon_m]

Hi @m_Robbie,

You have added all of the additional lines of code needed to solve the problem with the withdraw function
Your code compiles, but you’ve also added some extra code which is superfluous, and which makes it extremely likely that the function will be called with input parameters that cause the transaction to revert. I’ve explained why, below…

@m_Robbie

I’m also not always sure which address/contract object is making the call.

This is whichever address we select from the dropdown in the Account field (in the Deploy & Run Transactions panel). We are simulating banking transactions, and this withdrawal function is for account holders, who already have an account balance of ETH (resulting from either their own deposits or transfers from other account holders), to withdraw ETH from their bank account to their external wallet address.

I think where you may be getting confused is the fact that when ETH is deposited (by any bank account holder), it is transferred to the contract address. Therefore, the ETH balance of the Bank contract address is effectively a “pool” of all the ETH deposited by individual account holders (less any withdrawals).

• Deposits from external wallet addresses, decrease the ETH balance of the caller’s wallet address, and increase the ETH balance of the contract’s address by the same amount.
• Withdrawals to external wallet addresses have the exact opposite effect.
• Transfers do not involve any ETH entering or leaving the “pool” of ETH held by the contract address. They simply perform internal +/- accounting adjustments to the individual balances recorded for each account holder. These individual balances reflect each account holder’s share of the total contract balance (the pooled funds).

So you need to clearly distinguish between (i) actual movements of ETH between external wallet addresses and the contract address, and (ii) internal accounting adjustments which don’t actually involve movements of ETH, but adjust the apportionment of the total contract balance between individual bank account holders.

You should now be able to see that the caller (msg.sender) of the withdraw() function is also the address receiving the ETH (the recipient). The transaction that is executed moves ETH from the contract address to the caller’s wallet address. The “inbuilt” transfer() function is what actually performs this operation, transferring an amount of wei (the argument passed in) from the contract balance to the address the function is called on (appended to).

Hopefully you can now see that your recipient parameter and 2nd require statement should be removed — which will also make your code less verbose
With your current code, if an account holder wants to successfully withdraw funds, then they have to input a recipient address that is different from their own, otherwise your 2nd require statement will fail. This is counterintuitive, and just doesn’t make any sense.

You are right to include…

@m_Robbie

balance[msg.sender] -= amount;

… because this is the accounting adjustment which ensures the caller’s share of the total contract balance is reduced by the same amount as the reduction in the total pooled funds.

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.

@m_Robbie

Looked up best practices on why it’s best to explicitly return a value on a withdrawal function.

The function can still operate effectively without returning a value. It’s not mandatory to include. But as  returns(uint)  was included in the function header, you are right to also include the return statement in your function body.

@m_Robbie

I’m assuming the transfer() function we were suggested to use within our withdrawal() block means that we overloaded it by defining it with different parameters than is ordinary with address objects…

I don’t really understand what you mean, here. But hopefully, my other explanations go some way towards clarifying this as well

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

[m_Robbie]

Wow, thank you- that was extremely useful! I’ll be using that as a guide until all is engrained. Your careful explanation was very much appreciated, Jon!

[jon_m]

Hi @1984,

@1984

Also just to make sure that I understood well, we use also a function transfer that we created ourselves in order to transfer From To an Amount. It has nothing to do we this transfer() function. Right?

Correct… the transfer() function we are using in withdraw() is “predefined” for us within the Solidity language. It is a member of the address data type, but can only be used with payable addresses. Think of it as being like a method in JavaScript.
It’s predefined syntax is:

<payable address>.transfer(uint256 amount);

It transfers an amount in wei from the contract address to <payable address>.

The other transfer() function we wrote ourselves does not transfer any actual wei between addresses. When it is executed, the contract address’s wei/ether balance doesn’t change. It just makes accounting adjustments to 2 user account balances to reflect an internal transfer of funds between 2 account holders. Each user account balance records the share of the total Bank contract balance which is attributable to each separate user.

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

[jon_m]

An excellent assignment solution @1984

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

You are also right that returns(uint) can be removed from the function header. It isn’t mandatory to include, and the function can still operate effectively without returning a value.

Just one further observation…
Your posted solution won’t actually compile, because you’ve missed off the contract’s closing curly bracket. This kind of thing will be easier to spot if you format your code before posting it, and will also make your code more readable.
Follow the instructions in this FAQ: How to post code in the forum

Let us know if you have any questions

[thecryptoargie]

  function withdraw(uint amount) public returns (uint){
        require(balances[msg.sender] >=amount, "Not enough balance!");
        balances[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balances[msg.sender];
    }