Transfer Assignment

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

[Randallcrum]

pragma solidity 0.7.5;
contract Bank {

    mapping(address => uint) balance;
    
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    event transferred(uint amount, address depositedFrom, address depositedTo); 
    
    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(balance[msg.sender] >= amount, "Insufficient Balance Available");
        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, "Insufficient Balance Available"); //require
        require(msg.sender != recipient, "You may not transfer to the same address.");//require
        uint priorSenderBalance = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
        assert(balance[msg.sender] == priorSenderBalance - amount);//assert
        emit transferred(amount, msg.sender, recipient);

    }

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

}

[jon_m]

Nice assignment solution @ppoluha and, overall, a very well-coded contract

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

The additional assert statement and withdrawal event (with corresponding emit statement) you’ve included are also appropriate and well coded. The emit statement is in the correct position within your withdraw function body, and it logs relevant information when a call to the function has executed successfully.

Now have a look at this post which explains an important security modification that should be made to the order of two of the other statements within your withdraw function body. See if you can work out how to reorder your code for this, bearing in mind you’ve got an additional 2 lines of code for the assert statement.

Just let us know if you have any questions

[jon_m]

An excellent assignment solution @thecryptoargie

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

[jon_m]

Nice assignment solution @Randallcrum and, overall, a very well-coded contract

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.

Your transfer event and corresponding emit statement are also well coded. The emit statement is in the correct position within the transfer function, and it logs relevant information when a call to the transfer function has executed successfully.

Just let us know if you have any questions

[Randallcrum]

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

That makes sense. Make the changes to the balance before committing funds, to prevent possible shenanigans. Nice.

[ppoluha]

Thanks for the rich comment @jon_m! I now understand that the order of the contract’s balance adjustment and the transfer should be in the below order to prevent some kind of attack.

     balances[msg.sender] -= amount;
     msg.sender.transfer(amount);

And I’d be curious to know if it’s good practice to have assert statements like this one, requiring two extra lines of code with extra computing and storage of the previous balance. Shouldn’t I be able to trust the inner mechanics of the EVM?

[Filip_Rucka]

pragma solidity 0.7.5;

contract Epic {

mapping(address => uint) balance;
address owner; 
event depositDone(uint amount, address indexed depositedTo);

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){
        assert(balance[msg.sender] >= amount);
        balance[owner] -= amount;
        //emit LogWithdrawal(msg.sender, amount, balances[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, "Bro don't transfer money to yourself");
    _transfer(msg.sender, recipient, amount);
    
}

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

}

[Dominykas_Pozleviciu]

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

I am just not sure about 2) Correctly adjust the balance of the user after a withdrawal, is this the answer to this task?:

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

[jon_m]

Hi @ppoluha,

@ppoluha

And I’d be curious to know if it’s good practice to have assert statements like this one, requiring two extra lines of code with extra computing and storage of the previous balance. Shouldn’t I be able to trust the inner mechanics of the EVM?

Good questions!

I think this all comes down to risk management, and our own risk assessment of how potentially catastrophic the consequences would be if the code of a particular function in our smart contract fundamentally failed to execute as it should. This obviously depends on the specific operation(s) a particular function is performing, and the overall use case of our smart contract.

Incorporating certain processes and checks in order to prevent highly improbable, or even supposedly impossible, negative events is considered good practice, indeed expected, in all industries where risk management is important (which is pretty much all of them), and of vital importance in ones that involve finance, legal services, health and safety etc.

assert() is not meant to fail and trigger revert because it is checking invariants, to ensure that “impossible” outcomes haven’t happened. I think the fact that, once deployed, smart contracts are essentially immutable, makes the implementation of this kind of security check even more of an absolute necessity within their design.

Obviously there has to be a balance between implementation and operating costs v the probability of a disaster happening; but in terms of just adding some extra code to make the smart contract more robust and secure (even if it’s never actually needed), I think the cost/benefit analysis in many scenarios becomes a no-brainer.

I think another important consideration is the extremely rapid pace of technological change within the blockchain industry. Networks, platforms, protocols, systems and programming language syntax etc. etc… which, today, we may be able to rely upon and trust with a high degree of confidence and certainty, may well, in the future, be susceptible to certain new types of security threat and vulnerabilities, which would be extremely hard to predict today. I think these are the kinds of “impossible” outcomes we want to try to protect our smart contracts from by implementing security features such as assert().

I hope these thoughts go some way towards answering your question, or at least provide some food for thought and further reflection

[jon_m]

Hi @Filip_Rucka,

You’ve got several problems with your withdraw() function that you need to sort out…

Have you tried deploying your contract, and seeing what happens when different addresses deposit funds, and then try to withdraw them?

@Filip_Rucka

balance[owner] -= amount;

Any address can deposit ether and then call withdraw(). But this line will always reduce the balance of the contract owner’s address (who initially deployed the contract), and not the balance of the address making the withdrawal! I’m sure you can see that the owner is going to run into serious problems with this code

So, we might then think that account holders other than the contract owner will benefit enormously from the bug I’ve just described — by being able to withdraw more than their account balance, because it is never reduced… but you should notice that after withdraw() has executed, the ether balance in the caller’s external wallet address does not increase by the withdrawal amount. That’s because you’re missing the line of code that was given to you in the video, the one that transfers the ether amount from the contract address to the caller’s external wallet address.

---

You have used an assert() statement to check the input amount. Instead,  require()  should be used to validate inputs. We use  assert()  to check invariants, which basically means checking that the code in our function body is executing as it should and producing the expected outcome. assert() is not meant to fail and trigger revert, because we obviously aim to only deploy smart contracts which have been coded correctly, logically, and which operate effectively i.e. with no fundamental flaws in the code.

With the version of Solidity we are using (v.0.7), if  assert()  throws, it causes all of the remaining gas in the “budget” allocated to that transaction (up to the gas limit) to be consumed, and so results in a considerably higher cost. Therefore, it seems logical that we use the more costly assert() to check for conditions which are the least likely to occur (only in “exceptional” circustances).

In contrast, if require() throws, this is not considered to be an “exceptional” event, because it is testing the validity of the function’s inputs. We should “expect” callers to sometimes attempt to invoke functions with invalid inputs (either by mistake, or on purpose).  require() is designed for such non-exceptional events, because when it throws, only the gas consumed up to that point is spent (including the gas required to execute the require function itself), and the gas remaining in the “budget” for that transaction (up to the gas limit) is refunded to the caller.

---

If you do include the emit statement — which is currently commented out, but is actually a good idea — what corresponding event definition do you also need to add?
Also check your code carefully in this emit statement: there is a mistake with the mapping name.

If you’re not sure where to start with some of these corrections, have a look at some of the other students’ solutions, and the feedback and comments they’ve received, posted here in this discussion topic. And, of course, let us know if anything is unclear, or if you have any questions

[jon_m]

This is a very clever solution, Dominykas

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

@Dominykas_Pozleviciu

I am just not sure about 2) Correctly adjust the balance of the user after a withdrawal

What we expect is for you to first adjust the balance of the user with…

balance[msg.sender] -= amount;

… and then to return the new balance on a separate line, after the withdrawal, with…

return balance[msg.sender];

But you have found a clever and concise way to do both within a single statement:

@Dominykas_Pozleviciu

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

By the way, you only need the parentheses when you are returning more than one value. Here you are only returning one value (the balance) and so you can remove them.

However … in order to reduce security risk, the statements within the function body should be in the following order:

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

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:

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. But it does mean that, even though your solution compiles and works, it isn’t very secure because you are performing the transfer of funds (the external interaction) before updating the contract state for the effect of this transfer. Because the return statement has to be last statement, this unfortunately means that you can’t update the balance at the same time as returning it

So how would you modify your solution to make it more attack-resistant?

Just let me know if you have any questions.

[ybernaerts]

function withdraw(uint amount) public returns (uint) {
    require(balance[msg.sender] >= amount); // within the contract you should have at least that much
    msg.sender.transfer(amount);
    balance[msg.sender]-=amount;
    return balance[msg.sender];
}

[programmedtorun]

@jon_m hello! I think for solidity v 0.8.1 I am getting error that address must be address payable. I searched online and found this

https://ethereum.stackexchange.com/questions/94707/msg-sender-not-address-payable-solidity-0-8-2

it works with wei as units for deposit and withdrawal. Care to comment? Thanks!

My code:

pragma solidity 0.8.1;

contract Practice {
    string message;
    mapping(address => uint) balance;
    address public owner; // think this needs to be public...
    event balanceAdded(uint amt, address indexed depositedTo);
    event depositEvt(uint val, address depositer);
    event transferEvt(address transferedTo, uint amt);
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run function that the modifier is tied to. 
    }
    
    constructor(string memory _message){
        owner = msg.sender; // msg.sender in constructor is contract owner. 
        message = _message;
    }
    
    function deposit() public payable returns (uint) {
        balance[msg.sender] += msg.value;
        emit depositEvt(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amt) public payable returns (uint){
        require(balance[msg.sender] >= amt);
        payable(owner).transfer(msg.value); // this is key line change for sol v. 8+
        // msg.sender.transfer(amt);
        balance[msg.sender] -= amt;
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    function transfer(address recipient, uint amt) public {
        require(balance[msg.sender] >= amt, "balance not sufficient!");
        require(msg.sender != recipient, "don't transfermoney to urself");
        uint prevSenderBal = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amt);
        emit transferEvt(recipient, amt);
        
        assert(balance[msg.sender] == prevSenderBal - amt); //only throws if something v. off
        //event logs further checks
    }
    function _transfer(address from, address to, uint amt) private {
        if (balance[from] >= amt) {
            balance[from] -= amt;
            balance[to] += amt;
        }
    }