Transfer Assignment

[benj]

Edit: still on a train, still can’t compile so committing to this untested!

    function withdrawBalance(uint amount) public returns (uint) {
        require(balance[msg.sender] >= amount,"The balance is not high enough to withdraw this amount.");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
        return balance[msg.sender];
    }

Edit again - got this wrong, tried to pass in a variable to a modifier. Forget modifier variables need fixing in the function header, which won’t work here.

[Nathan_Burkiewicz]

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    address owner;
    
    event transferCompleted(uint amount, address transferredFrom, address transferredTo);
    event depositMade(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 depositMade(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public returns (uint){
        require(balance[msg.sender] >= amount, "Withdrawal amount exceeds current balance");
        balance[msg.sender] -= amount;
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Insufficient balance");
        require(msg.sender != recipient, "Transfers cannot be made to your own account");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);

        emit transferCompleted(amount, msg.sender, recipient);

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

[Julian97]

pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    address owner;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    event transferDetails(uint amount, address indexed senderAddress, address indexed recipientAddress);
    
    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);
        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, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        emit transferDetails(amount, msg.sender, recipient);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance [to] += amount; 
    }
    
}

[jon_m]

Nice solution @AngSin17

Apologies for the delay in giving you some feedback on your solution to this assignment!

You have added all of the necessary code needed to solve the problem with the withdraw function. And your additional checkWithdrawLimit modifier with amount parameter is well coded, and will allow the same require statement to be applied to the transfer function as well as the withdraw function, thereby helping to avoid code duplication and to keep your code concise.

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

[jon_m]

Hi @jtb,

Apologies for the delay in giving you some feedback on your solution to this assignment!

The require statement and the return statement you’ve added are both correct
But you are missing one very important line of code…

If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal.

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.

---

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?

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 me know if you have any questions about how to make the necessary modifications to your code, or if there is anything that you don’t understand

[jon_m]

Hi @KennethJP,

Here is a link to my review of your solution to this assignment. You’ve posted the exact same code in the Solidity Payable Functions discussion topic, and I saw that first

[jon_m]

Hi @MattPerry,

The require statement and the return statement you’ve added to your withdraw function are both correct
But you are missing one very important line of code…

If you deploy your contract, make a deposit, and then call your withdraw function, you will notice that the caller’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced (call getBalance to check this). I’m sure you can see that this is a very serious bug because, even though there is a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal.

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.

Once you’ve had a go at correcting your code for this, have a look at this post which explains the importance of 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,

Here is a link to my review of your solution to this assignment. You’ve posted the exact same code in the Solidity Payable Functions discussion topic, and I saw that first

[MattPerry]

Thanks again Jon,

I appreciate all the extra info. I understand why this would a bad bug to have. I will look at my code again and give it my best to fix it.

[jon_m]

Hi @Gomez1333,

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

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

However, 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. You should have got a yellow/orange compiler warning for this.

Also, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Just let me know if you have any questions

[Gos]

Not sure what is going wrong but i think this is it, when i deploy contract is does not update the balance after withdraw.

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

[jon_m]

This is a perfect solution @Gos

@Gos

when i deploy contract is does not update the balance after withdraw.

Are you maybe only inputting a value of, say 5 for the amount argument, instead of 5000000000000000000 ? Solidity handles values in units of wei, not ether. In Remix, when sending a value to a contract, we have the dropdown to allow us to input the value in units of ether, and it will be converted to wei for us. Unfortunately, we don’t have that option when calling a function with a uint value we want to be processed as an amount in ether — we have to input our ether amount in uints of wei, with all the zeros!

If that is the issue, then the balance will be being updated, but only for a very small amount, and so you may not have noticed it if you are looking for a change in units of ether.

If that isn’t the issue, then post your full contract code, and I’ll take a look.

---

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are also 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…

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

[jon_m]

Hi @Ruzz,

Apologies for the delay in answering your question…

@Ruzz

I did just notice that I had the:

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

around a different way to Filip. Does that matter at all?

The order of the statements does matter, yes, although we wouldn’t expect you to know about this yet. But seeing as you’ve asked…

In order to reduce security risk, we should apply the following order to the statements we place within the function body:

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 to the external wallet 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 noticing these kinds of details and wanting to find out why they are as they are

Just let me know if if you have any further questions

[jon_m]

Hi @cgironda,

Just be careful not to mix up your assignment (=) and equality (==) operators…

@cgironda

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

This makes a huge difference, because your code (although it does still compile) does not reduce the user’s balance in the mapping by the withdrawal amount. I’m sure you can see why this is a very serious bug: even though the require statement places a limit on each separate withdrawal, this limit is never reduced to reflect each withdrawal, meaning that the user can repeatedly make withdrawals of amounts up to their initial balance, until the contract balance (which includes all user account balances) is reduced to zero!

Apart from this, your solution is excellent.

Just let me know if you have any questions

[jon_m]

Hi @Cero,

Just be careful with your assignment operators…

@Cero

//reducing our balance in contract for amount
balance[msg.sender]=-amount;

In this line of code you need…

• the subtraction assignment operator, which has the minus sign before the equals sign ( -= ). This will deduct the value on the right (the withdrawal amount) from the user’s current balance stored in the mapping.

and not…

• The “standard” assignment operator (=) followed by the unary minus operator ( - ). This negates the value on the right of the assignment operator (the withdrawal amount) and then assigns the resulting value as the user’s balance in the mapping (replacing their current balance). As the mapping is defined as unsigned integers mapped to addresses (address => uint) , it cannot store negative balances. So, if you input a positive withdrawal amount (as expected), the unary minus operator will convert this to a negative amount, and when this is assigned to the mapping it will be expressed as an extremely large positive balance — the largest integer which can be stored with 256 bits (a binary number with 256 1’s) less the withdrawal amount. This happens as a result of something called underflow.

I’m sure you can see why this is a very serious bug, because the limit on withdrawals is so high, that it effectively allows the user to withdraw all of the funds held in the contract (which includes all user account balances).

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

[jon_m]

Hi @benj,

You’ve done a great job at explaining these concepts to @Flippy

@benj

though the terminology might not be 100% as I’m coming from Python programming.

Your terminology is actually very good. The term used in the Solidity documentation for attributes or properties is members, but it is clear what you mean by attributes, and that’s the main thing.

Just a couple of observations …

@benj

// dictionaryName[dataToLookup]
users[msg.sender]

This will look up the address of the active user in the mapping to find the ID and balance associated with that one record in the dictionary.

This is correct if our mapping is defined with an address data type as the key…

mapping(address => User) users;

… and not with an unsigned integer data type …

@benj

mapping(uint => User) users;

From your explanation, I think this is actually how you meant to define your mapping, but I just wanted to clarify this.

With the mapping defined as User instances (created from the struct) mapped to addresses (and not unsigned integers), users[msg.sender] will reference a User instance, and not an address, and so instead of…

@benj

address activeUser = users[msg.sender]
uint activeUserBalance = activeUser.balance

… the local activeUser variable should be defined with the custom data type User (and not as an address)…

User activeUser = users[msg.sender];
uint activeUserBalance = activeUser.balance;

I think it’s important to make clear that, when we are referencing a specific “entry” in a mapping, the value within the square brackets is the key the entry is mapped to, and the data type of the key (the data type declared before the arrow in the mapping definition) is usually not the same as the data type of the value that is mapped to it (the data type declared after the arrow in the mapping definition).

---

Your explanation breaking down   msg.sender.transfer(amount);   is really quite excellent

@benj

Here the method transfer() is being applied to the attribute sender from the object (or data type) msg. The method requires additional data as without that input it can’t be used. This is fed in as the variable ‘amount’.

Strictly speaking, msg is not a data type, but instead we can consider msg.sender as representing one value with an address data type (the address calling the function). Address data types in Solidity have many available members, some of which are what you have described as methods — which I think is a really good term to use to describe how these particular members operate.

Here is a link to the full list of Members of Address Types in the documentation:

https://docs.soliditylang.org/en/latest/units-and-global-variables.html#members-of-address-types

Just let me know if either of you have any further questions

[jon_m]

Hi @FerfiC,

Apologies for the delay in giving you feedback on your solution to this assignment, and in answering your question.

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

@FerfiC

Couldnt find the WHY payable(msg.sender).transfer(amount) doesn’t work without payable()

In order to receive ether, an address needs to be payable . From Solidity 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() . If you are using v0.8, then this is why the compiler was generating an error when you had…

msg.sender.transfer(amount);
// instead of
payable(msg.sender).transfer(amount);

You need to bear in mind that this course material is based on v0.7 syntax. In versions prior to v0.8 msg.sender is payable by default and so doesn’t need to be explicitly converted. That’s why the course video uses  msg.sender.transfer(amount);  without the compiler throwing an error.

---

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 the importance of the order of the statements within your withdraw function body.

Just let me know if you have any further questions

[jon_m]

Hi @TuomoP,

Apologies for the delay in giving you feedback on your solution to this assignment.

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

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

@TuomoP

pragma 0.8.4

I’m sure this was just a slip, but if you don’t include solidity , and the semi colon at the end of the statement, the compiler will throw an error:

pragma solidity 0.8.4;

Also, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Just let me know if you have any questions

[jon_m]

Well, it’s definitely right now @benj

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

[jon_m]

An excellent assignment solutuion @Nathan_Burkiewicz 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, and they are also 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…

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

You’re making great progress!