Transfer Assignment

jon_m · October 24, 2021, 3:16am

I’ll try to explain things in a simpler way …

Correct

This line of code only reduces the total contract balance (the total amount of ether held in the contract by all individual users). The caller is the address calling the withdraw function, in order to withdraw the ether amount. In Remix, you can see the caller’s external account balance in the Account field at the top of the Deploy & Run Transactions panel. Think of it as their wallet address. You will see their wallet’s ether balance increase by the withdrawal amount.

We also have to reduce this user’s balance in the mapping by the same amount. Think of the balances in the mapping as recording each user’s individual share of the total funds held in the contract. Your require statement checks the user’s balance in the mapping to see if they have enough to cover the requested amount. If we don’t reduce the user’s balance in the mapping each time they withdraw ether from the contract, then the require statement will no longer prevent the user from withdrawing too much.

// Look at this line in the deposit() function ...
balance[msg.sender] += msg.value;

This declares the data type of the value returned by the function as an unsigned integer (uint). But in order to actually return (output) a value, we need a return statement at the end of the function. The withdraw function can return the user’s reduced balance after the withdrawal, just like the deposit function returns the user’s increased balance after a deposit.

I hope that’s clearer, and easier to understand. If you’re still not sure what code to add to your function, then have a look at some of the other students’ solutions in this discussion topic.

Just let me know if you have any more questions

jon_m · October 24, 2021, 3:22am

Nice solution @Lennart_Lucas

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

Now have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

Just let me know if you have any questions

josejalapeno · October 24, 2021, 1:31pm

Hi Jon, this '>= is the thing I am not understanding, in english this would be my balance as I am the msg.sender should be more than or equal to amount. meaning I should be able to withdraw more than or equal to the amount I deposited. Pls clarify it with me.

jon_m · October 24, 2021, 2:35pm

Hi Jose,

Correct…

Your (the caller’s, msg.sender's) balance is referenced by balance[msg.sender]
This code finds the msg.sender address in the balance mapping (the address is the key) and then references the value that is mapped to this key (the balance).

This is where you are confused. It’s the opposite …

  balance[msg.sender]           >=               amount
/* amount deposited      IS GREATER THAN     amount requested
                           OR EQUAL TO         to withdraw                */

If this is true, then msg.sender will be able to withdraw the amount requested, because their ether balance in the Bank contract is greater than this amount.

But if it is false, then this means their ether balance in the Bank contract is less than the amount requested; so, there is “Not Enough Funds”, require() will fail, and the transaction will revert.

Does that make sense now?

jon_m · October 24, 2021, 2:45pm

… and one further point, which is important for understanding why you need to add an extra line of code…

For the require() statement to work as it should (i.e. check whether msg.sender has enough funds to cover the withdrawal amount) then balance[msg.sender] must always reflect the user’s current balance (their share of the ether deposited in the contract). The user’s balance in the mapping (balance[msg.sender]) will only be accurate if it is always increased when ether is deposited in the contract…

… and decreased when ether is withdrawn from the contract: this is the extra line of code you need to add to your withdraw function.

Markus_Bielaszka · October 28, 2021, 12:39am

function withdraw(uint256 _amount) external {
  require(balance[msg.sender] >= _amount, "You do not have enough funds to withdraw");
        
  balance[msg.sender] -= _amount;
  payable(msg.sender).transfer(_amount);
}

PaulS96 · October 27, 2021, 10:50am

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

jon_m · October 27, 2021, 4:19pm

An excellent solution @PaulS96

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

check inputs (require statements)
effects (update the contract state)
external interactions

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

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

… 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 the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

Keep up the great coding!

MightyYak · October 29, 2021, 6:35am

Here is my go:

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

Quick question, does the order of the payable line and internal balance line matter much in production?

jon_m · October 29, 2021, 2:45pm

Hi @MightyYak,

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

You are also right that it’s not necessary to return the updated balance, and so returns(uint) can be removed from the function header. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the individual account holder’s updated balance after the withdrawal.

Yes, it does… This is good question, and a very important one in terms of smart contract security. Best practice is for the order of the statements within the function body to follow the Checks-Effects-Interactions Pattern:

check inputs (require statements)
effects (update the contract state)
external interactions

It’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

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

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

… to guard against re-entrancy attacks. In this type of attack a malicious fallback function (in a receiving contract) would be able to make successful re-entrant calls to the withdraw() function, thereby allowing it to execute multiple withdrawals before its individual account balance is modified in the contract state to accurately reflect these operations (which would otherwise enable the initial require statement to fail and trigger revert).

In fact, the latest advice is not to use the address members transfer() or send(). The fixed stipend of 2300 gas, which they forward to a calling contract’s fallback function, used to be enough gas for the fallback function to execute successfully, but not enough gas to allow a re-entrant call (thereby reducing the risk of re-entrancy attacks). But since the Istanbul hard fork changed the gas charges for certain operations, this fixed gas stipend is no longer guaranteed to be sufficient.

Instead of transfer() we can use the address member call() to perform the transfer of ether from the contract address balance to the external address …

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");

Using call() would normally pose a greater risk of re-entrancy attacks than using transfer() or send(), because it will forward all of the remaining gas (up to the gas limit set by the caller) to the fallback function. However, implementing the Checks-Effects-Interactions Pattern will guard against this, as I’ve described above.

You will find more background information about these issues in the following articles:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21

You’ll learn more about re-entrancy and other types of attacks, and some of coding techniques and smart-contract best practices we can implement to guard against them and reduce security risk, in the courses which follow this one: Ethereum Smart Contract Programming 201, and the Ethereum Smart Contract Security course. But it’s great that you’re already starting to consider these types of issues

Just let me know if you have any further questions.

belinox · October 31, 2021, 1:42pm

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

jon_m · October 31, 2021, 9:42pm

An excellent solution @belinox

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

check inputs (require statements)
effects (update the contract state)
external interactions

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

balance[msg.sender] -= amount;

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

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, their entitlement) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

Keep up the great coding!

antonfriedmann · November 4, 2021, 2:06pm

Hello y’all!

Here is my solution for the withdraw function, added require statement and a line to update the balance in the balance mapping

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

jon_m · November 5, 2021, 3:37am

Nice solution @antonfriedmann

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

Now have a look at this post which explains an important security modification that should be made to the order of the statements within your withdraw function body.

Just let me know if you have any questions

B_S · November 5, 2021, 4:57am

    function withdrawal(uint _amountinwei) public returns(uint) {
        require(balance[msg.sender]>=_amountinwei, "don't take out more than your share!");
        msg.sender.transfer (_amountinwei);
        balance[msg.sender] -= _amountinwei;
        return balance[msg.sender];
    }

after reading @jon_m’s response above, i realized the 3rd and 4th lines should be swapped; thanks!

ballack13r · November 7, 2021, 2:45pm

function withdraw (uint amount) public returns (uint){
    require(balance[msg.sender]>=amount, "Balance not sufficient!"); //cannot withdraw more than your balance
    balance[msg.sender] -= amount; // remaining balance after withdrawal
    payable(msg.sender).transfer(amount);
    emit withdrawEvent(amount,msg.sender);
}

jon_m · November 7, 2021, 5:23pm

Hi @ballack13r,

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order to reduce security risk:

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

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

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, their entitlement) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one. But it’s great you’re already getting into good habits in terms of smart contract security

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 at the end of the function body. The compiler should have given you a yellow/orange warning for this.

This comment applies more to the return statement you need to add.

This line is reducing the user’s individual balance before withdrawal by the withdrawal amount.

As you’ve explicitly converted msg.sender to a payable address type using payable() , I assume you’re using Solidity v0.8. Is that correct? That’s perfectly fine — in fact, better — but if you’re using a different version to the videos (v0.7.5), then let us know what it is (if you’re not posting your full code with the pragma statement).

Just let me know if you have any questions

ballack13r · November 7, 2021, 6:57pm

Hello Jonathan!
Thank you for all your answers and your thorough verification of my answers. It helps me a lot and it’s giving motivation to continue and to improve myself.

I managed to solve the task myself, but as a lot of my colleagues, I wrote the code in an order that can be easily hacked. I took a peak on the forum, I saw other responses, also your advice to change the order, so I finally did that. But I forgot to change the comment:

// remaining balance after withdrawal

Yes, I am using Solidity v.0.8.7. I also saw in the comments that someone used payable() so I adopted the solution (otherwise I would have had an error).
Here is my code (I chose the lazy solution, to remove the return completely from the withdraw function):

pragma solidity 0.8.7;

contract SC{
 
mapping(address => uint) balance;

address owner;

constructor(){
    owner = msg.sender;
}

modifier onlyOwner{
    require(msg.sender==owner);
    _; //run the function
}

event depositDone(uint amount, address indexed depositedTo);
event amountTransferred(address transferredTo, address transferredFrom, uint amount);
event withdrawEvent(uint amount, address withdrawTo);

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 {
    require(balance[msg.sender]>=amount, "Balance not sufficient!"); //cannot withdraw more than your balance
    balance[msg.sender] -= amount; // reduce the balance before withdrawal
    payable(msg.sender).transfer(amount); // withdraw amount
    emit withdrawEvent(amount,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];
        _transfer(msg.sender,recipient, amount);
        
        assert (balance[msg.sender] == previousSenderBalance - amount);
        emit amountTransferred (recipient, msg.sender, amount);
    }
    
    function _transfer (address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

michael356 · November 7, 2021, 7:00pm

Hi:
1.Make sure that the user can not withdraw more than their balance**----this works in Remix with error handle code require in the function withdraw.

Correctly adjust the balance of the user after a withdrawal**--------this works half correctly in Remix. After a withdrawl operation, the recipient’s amount will adjust correctly, but if I click gel balance with the result of balance[msg.sender] will stay the same.

pragma solidity 0.7.5;

contract Bank{
// declare mapping

mapping (address => uint) balance;

address owner;

modifier onlyowner{
    require(msg.sender == owner);
    _; // run the functions.
}

// modifier costs(uint price){
//     require (msg.value > price);
//     _; // run the functions
// }

constructor(){
    owner = msg.sender;
}

event depositDone (uint amount,address indexed depositedTo);

// function addBalance (uint _addBalance) onlyowner() public returns(uint){
      
//     balance[msg.sender] += _addBalance;
//     return balance[msg.sender];
    
// }



function deposit() public payable returns(uint){
    // track internal the smart contract own to a address.
    balance[msg.sender] += msg.value;
    emit depositDone(msg.value,msg.sender);
    return balance[msg.sender];
}
    
function withdraw(uint amount) public returns(uint){
    // msg.sender is a address
    // address payable toSend = 0xCA35b7d915458EF540aDe6068dFe2F44E8fa733c;
    // toSend.transfer(amount); is the same asbelow
    msg.sender.transfer(amount);
    return balance[msg.sender];
}

function transfer(address recipient,uint amount) public returns(uint){
    require (balance[msg.sender] >= amount,"Balance not sufficient.");

    // check the balance of msg.sender
    require (msg.sender != recipient,"Don't transfer token to yourself.");
    
    uint previousBalance = balance[msg.sender];
    
    _transfer(msg.sender,recipient,amount);
    
    // event logs and further checks
    assert(balance[msg.sender] == previousBalance - amount);
    
    return balance[msg.sender];
    
} 

    // a private function can be reused in other public function
function _transfer(address from, address to ,uint amount) private{
    
    balance[from] -= amount;
    balance[to] += amount;
}
// from:0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
// to:0x8a7644191756a1122A63C0CA7238A8f15706f825
// amount 3 ether

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

}

jon_m · November 7, 2021, 10:57pm

Hi @ballack13r,

Your full contract code is looking excellent

Your additional withdraw event declaration, and corresponding emit statement, are also well coded, and appropriate.

That’s some good research! After you’ve done the assignment yourself, it’s really good to look at some of the other students’ solutions posted here in the forum, and the comments and feedback they’ve received: that way, you can check any points you were unsure about, maybe learn some extra things, and it can also be interesting to see what some of the alternative (but equally valid) solutions are. It’s fine to adjust what you’ve posted for anything extra you learn: if you want your original code to be reviewed, you can always add a comment about anything you added afterwards.

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 using payable(). The transfer() method takes a payable address. Here is the syntax …

<payable address>.transfer(uint amount)

I’m glad you’re finding the feedback helpful and motivating