Transfer Assignment

jon_m · May 16, 2021, 10:54am

You have added the additional lines of code needed to solve the problem with the withdraw function, and the additional assert statement you’ve included is also appropriate and well coded.

However, the compiler should have given you an orange warning for your function header. This is because it includes returns(uint) , but there is no corresponding return statement in the function body. 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. Do you know what this should be?

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.

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

dustwise · May 18, 2021, 6:33pm

This seems right to me. Thoughts?

pragma solidity 0.7.5;

contract MemoryAndStorage {

    mapping(address => uint) balances;

    function deposit() public payable returns (uint) {
        balances[msg.sender] += msg.value;
        return balances[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint) {
        require(balances[msg.sender] >= amount, "Not enough funds to withdraw!");
        
        uint userPreviousBalance = balances[msg.sender];
        
        msg.sender.transfer(amount);
        balances[msg.sender] -= amount;
        
        assert(balances[msg.sender] == userPreviousBalance - amount);
        
        return balances[msg.sender];
    }

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

}

Maia · May 20, 2021, 2:04am

function withdraw () public returns (uint){
        msg.sender.transfer () {
            require(withdraw > 0);
            require(msg.sender == owner);
        }

jon_m · May 20, 2021, 6:00pm

Nice solution @dustwise

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and the additional assert statement you’ve included is also appropriate and well coded.

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.

One other observation…
I think you may have some interference from the Data Location Assignment code creeping in here: it doesn’t really matter that your contract name is MemoryAndStorage and not Bank, but the uint id parameter in your getBalance function is not needed and should have generated an orange compiler warning indicating that it isn’t used. The users in this contract are identified and referenced by their address (not an ID).

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

yestome · May 21, 2021, 9:42am

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

}

Adrian_Zumba · May 21, 2021, 4:08pm

In the function withdraw() we need a require that I have called checkSenderBalance to check the balance that the person has, who wants to withdraw the money, it is enough then in the private function _withdraw () we update the balance that he must have by subtracting the amount he withdraws and finally we return the updated balance.

Below show the code:

modifier checkSenderBalance(uint amount) {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        _;
    }

function withdraw(uint amount) checkSenderBalance(amount) payable public returns(uint) {
        payable(msg.sender).transfer(amount);
        return _withdraw(amount);
    }

function _withdraw(uint amount) private returns(uint) {
        balance[msg.sender] -= amount;
        return balance[msg.sender];
    }

jon_m · May 22, 2021, 1:23am

Hi @Maia,

Make sure you start with the same starting code from the video: you are missing an all important parameter…the amount the user wants to withdraw!

What does withdraw reference in this require statement? An identifier has to reference something, such as a parameter or a variable.

The instructions were that your require statement needs to check that the amount requested for withdrawal (your missing parameter) is not greater than the exisiting balance. This requires a comparison with the user’s balance stored in the mapping.

Require statements validate inputs or restrict access to the function. Therefore, it’s important that they are placed as near to the beginning of the function body as possible.

Why do you want to restrict withdrawal to the contract owner? What about other addresses that have deposited funds in our Bank contract? I wouldn’t want to deposit any funds if I couldn’t withdraw them! Even if you did want to restrict access to the contract owner, we already have an onlyOwner modifier for that.

You are not transferring anything. You need to add an argument.

You are also missing 2 important lines of code:

As well as the sender’s address receiving the requested withdrawal amount, their balance in the mapping also needs to be reduced by the same amount. If it isn’t, then this is a very serious bug — think about what serious consequences this could lead to.
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.

You have a lot more work to do on this assignment. Take your time to understand what needs to be done. Use some of the learning techniques I suggested in my feedback for the Inheritance Assignment. And make sure you at least try to compile your code and address some of the errors before posting it.

Let me know if you have any concerns or questions about this

jon_m · May 22, 2021, 1:38pm

Hi @yestome,

Sorry, I seem to have reviewed your assignments in the opposite order to what you’ve done them in … Good to see you back here in the forum, by the way

require statement
balance[msg.sender] -= amount;

However, the compiler should have given you an orange warning for your function header. This is because it includes returns(uint) , but you haven’t included a corresponding return statement in your function body. 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. Do you know what this should be?

You have your statements in the withdraw function body in the correct order to reduce security risk:

check inputs (require statement)
effects (update the contract state for reduction in balance)
interactions (perform the transfer of funds from smart contract address to wallet address).

It’s important to modify the contract state:

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

Let us know if you have any questions.

jon_m · May 22, 2021, 3:08pm

Nice assignment solution @Adrian_Zumba

You have included all of the additional functionality needed to solve the problem with the withdraw function.

Your decision to use a modifier for the require statement is a good one, and it is well coded. As you know, using modifiers helps us to avoid code duplication and keep our code concise, so I’m interested to know if you have also used this same checkSenderBalance modifier in your transfer function to avoid having to repeat the same require statement in its function body?

I’ve noticed that you have expilicity converted msg.sender to a payable address using payable(msg.sender) . Are you, therefore, using Solidity v.0.8? If so, well done for finding out that msg.sender is no longer payable by default from v.0.8, and so needs to be explicitly converted whenever it needs to be payable. This isn’t necessary in earlier versions of Solidity, such as the one we are using in the video lectures for this course (v.0.7.5). If you are using a later version than v0.7.5 (and that’s perfectly fine), always include the pragma solidity ... compiler version declaration in the code you post, as it is important for us to know what syntax we are reviewing.

However, in our withdraw() function, it is only the address receiving the ether which needs to be payable — and not the function itself. A function only needs to be marked payable when it needs to receive ether. So, unlike the deposit() function, we don’t want to mark withdraw() as payable in its function header. Marking it as payable doesn’t prevent the code from compiling, or the function from executing, but it is bad practice and poor risk management not to restrict a function to non-payable if it doesn’t need to receive ether, because if someone sends ether by mistake, this amount will still be transferred to the contract.

Your use of a private helper function to deliver a distinct part of the withdraw functionality is certainly logical, and demonstrates that you understand the importance of modularity within your code. The _withdraw() function itself is also very well coded… However, we also want to keep our code readable and, where possible, concise. Do you think that creating a separate helper function to parcel off just one line of code from the main function does that? I think the argument for this is always stronger the more code we are parcelling off.

Finally, in order to reduce security risk, the statements should be in the following order:

check inputs (require statements)
effects (update the contract state)
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 to the external 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. 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.

So, how would you modify your solution to make it more attack-resistant? Does this also affect the appropriateness of the additional _withdraw() helper function?

Let me know if you have any questions. You’re making great progress

tom88norman · May 22, 2021, 3:23pm

Here is my withdraw function, it seems to work well. Let me know if anything could be improved!

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

Here’s a question though, maybe I’ve misunderstood gas. If we were making real transactions on the Ethereum blockchain, there would be gas payable, right? So in fact, would we not have to make sure that balance[msg.sender] is sufficient to cover not only the required transfer/withdrawal amount, but also the associated gas fee?

So the simple line require (balance[msg.sender] >= amount) would actually not be usable in a real smart contract? Maybe I’ve misunderstood something but I’d appreciate any clarification.

jon_m · May 22, 2021, 9:15pm

Nice solution @tom88norman

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are each coded correctly and appropriately.

In order to reduce security risk, the statements should be in the following order:

check inputs (require statements)
effects (update the contract state)
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 to the external address…

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.

In terms of your question about the gas fee, which is a good one and shows you are really thinking about things …

Yes

No… because the gas is paid from the account balance of the caller’s external wallet address, which in Remix is the address selected from the Account dropdown when the function call is made.

When a user deposits ether, the actual ether is transferred from the user’s external wallet address balance to the Bank contract address balance. The adjustment we make to the user’s balance in the mapping balance[msg.sender] is like an internal accounting adjustment, so that we can keep track of their share of the total Bank contract ether balance. The gas fee for the deposit transaction is also deducted from the account balance of the user’s external wallet address, but instead of being transferred to the Bank contract, it will end up being transferred to the miner who successfully mines the block which this transaction ends up being included in. The amount of ether associated with the gas fee never passes through the Bank contract, and so it doesn’t need to be accounted for within the user’s balance in the mapping.

When the user makes a withdrawal, the opposite happens: the requested ether amount is transferred from the Bank contract address balance to the user’s external wallet address balance; but before that, the user’s balance in the mapping balance[msg.sender] is adjusted to reflect the reduction in their share of the total “pooled” ether held in the Bank contract’s address balance. Again, the gas fee for the withdraw transaction is deducted from the account balance of the user’s external wallet address, and not from the Bank contract’s address balance.

No… this require statement is perfectly correct and can be used as it is. Hopefully, you can now see that this is because the gas fees paid by the users for performing transactions with the Bank contract, do not pass through the actual Bank smart contract itself, and so do not need to be accounted for in any of its checks and computations.

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

JonBal · May 24, 2021, 3:43am

pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    address owner;
    
    event depositDone( uint amount, address indexed depositedTo );
    event balanceTransfered( uint amount, address indexed sentTo, address indexed sentFrom );
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _; //run the function
    }
    
    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");
        
        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 {
        //check balance of msg.sender make sure there is money
        require(balance[msg.sender] >= amount, "Insufficient balance");
        require(msg.sender != recipient, "Dont send money to yourself!");
        
        uint previousSenderBalance = balance[msg.sender];
        
          emit balanceTransfered( amount, recipient, msg.sender);
          
         _transfer( msg.sender, recipient, amount );
        
        //Event logs and further checks
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer( address from, address to, uint amount ) private {
        balance[ from ] -= amount;
        balance[ to ] += amount;
    }
}

jon_m · May 24, 2021, 4:08pm

Nice solution @JonBal

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are each coded correctly and appropriately.

Your transfer event and corresponding emit statement are also well coded, and the emit statement logs relevant information when a call to the transfer function has executed successfully. However, events should only be emitted once all of the associated operations have been completed i.e. all of the necessary modifications have been made to the contract state, and any external interactions have been executed. This can only be assured if the emit statement is positioned at end of the function body (but before the return statement, if there is one) i.e. after this comment in your transfer function (not before)…

Also, in general, emit statements are probably better positioned after assert statements, rather than before.

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 anything is unclear, or if you have any questions

jahh · May 25, 2021, 2:57pm

contract Bank{

mapping(address => uint)balance;

event depositDone(uint amount, address indexed depositedTo);

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)
            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, "insufficien funds");
    require(msg.sender != recipient, "Don't send money to yourself");
    
    uint preivousSenderBalance = balance[msg.sender];
    _transfer(msg.sender, recipient, amount);
    assert(balance[msg.sender] == preivousSenderBalance - amount);
}
function _transfer(address from, address to, uint amount)private{
    balance[from]-= amount;
    balance[to]+= amount;
}

}

It didn’t work, i looked at the answer but didn’t understand the
balance[msg.sender] -= amount;
msg.sender.transfer(amount);

jahh · May 25, 2021, 3:01pm

Why is there a
balance[msg.sender] -= amount; ?
and what does ( msg.sender.transfer(amount); ) mean?

jahh · May 25, 2021, 3:02pm

thanks in advance for the response

jon_m · May 26, 2021, 12:36am

Hi @jahh,

The require statement and the return statement you have added are both correct, but don’t forget the semi-colon at the end of the require statement.

Without these two statements, the withdraw() function doesn’t actually make a withdrawal. Instead all it does is check that the caller has sufficient balance to cover the withdrawal amount, and then it just returns the existing balance, effectively doing the same as the getBalance function. Your code generates an orange compiler warning, indicating that the withdraw() function can be restricted to view because, like getBalance(), it only reads the contract state, but doesn’t modify it.

The deposit() function transfers ether:

From the caller’s external wallet address (you will see the caller’s ether balance, displayed in the Account field, decrease by the deposit amount)
This is achieved by calling deposit() with an ether amount (input in the Value field)
To the contract address (the contract’s ether balance will increase by the deposit amount)
This is achieved by making the deposit() function payable

To be able to check the contract’s current ether balance, you can add the following getter:

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

The withdraw() function does the exact opposite, and transfers ether:

From the contract address (the contract’s ether balance will decrease by the withdrawal amount)
This is achieved with <address payable>.transfer(uint256 amount)
This is like a method which is called on a payable address
The amount withdrawn from the contract is the amount argument’s value
To the caller’s external wallet address (you will see the caller’s ether balance, displayed in the Account field, increase by the withdrawal amount)
This is achieved by calling .transfer(uint256 amount) on the caller’s address.
As the caller will always be msg.sender, the code used is:
msg.sender.transfer(amount);

As multiple users can deposit ether into the contract, the contract’s ether balance represents the sum of all of the ether that is currently held in the contract by all users. In order to track each user’s share of this total, we also need to perform some internal accounting, as follows:

Each user has a current balance stored in a mapping, which represents their share of the contract’s total ether balance at any given time. Each user’s balance is mapped to their external wallet address.

When a deposit is made, the user’s balance in the mapping is increased by the same amount:
balance[msg.sender] += amount;
Note that the actual ether is added to the contract’s balance. The increase in the user’s balance in the mapping is purely an accounting adjustment.

When a withdrawal is made, the user’s balance in the mapping is decreased by the same amount:
balance[msg.sender] -= amount;
Note that the actual ether is deducted from the contract’s balance. The reduction in the user’s balance in the mapping is purely an accounting adjustment.

When the transfer() function is called, no ether is transferred into or out of the contract, and so the contract’s ether balance remains unchanged. Instead, what is happening is a change in the share two users have of the contract’s total ether balance.
The caller’s balance in the mapping is decreased by the amount they are transferring:
balance[msg.sender] -= amount;
And the recipient’s balance in the mapping is increased by the same amount :
balance[recipient] += amount;

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

tom88norman · May 26, 2021, 1:57am

Ah ok, that’s really helpful, thanks a lot for taking the time to write that out!

Obi · May 26, 2021, 4:48pm

Ernest_SG · May 26, 2021, 6:42pm

Here’s my solution for this assignment:

function withdraw(uint amount) public{
     
    require(balance[msg.sender] >= amount, "You haven't staked that amount of ETH");     
        msg.sender.transfer(amount);
 }

It works just fine like this, but to make sure that is correct I added the assert() error handling function at the end, but once I try to withdraw more ETH than my balance it reverts but doesn’t show the message I wrote on the require() function.

function withdraw(uint amount) public{
     
    require(balance[msg.sender] >= amount, "You haven't staked that amount of ETH");
    uint balanceBeforeWithdraw = balance[msg.sender];
     
        msg.sender.transfer(amount);
     
    assert(balance[msg.sender] == (balanceBeforeWithdraw += amount));
 }