Transfer Assignment

gokhantaskan · June 8, 2021, 11:45am

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

        balance[msg.sender] -= amount;
        
        return msg.value;
    }

To prevent user withdraw more than he/she has, the amount he/she wants back must be lower than or equal to the amount in balance mapping.

KK_Cheah · June 9, 2021, 10:09am

My super simple version of that is bare minimum to work(doesn’t even show error messages but it reverts) that seems to be working. Any review/constructive criticism is appreciated and welcome!

pragma solidity 0.7.5;

contract transferAssignment{
    mapping(address => uint)balance;
    
    address owner;
    
    event depositCompleted(uint amount, address indexed depositedTo);
    
    modifier onlyOwner{
        require(msg.sender == owner);
        _; // run the function
    }
    
    constructor() {
        owner = msg.sender;
    }
    
    function addBalance() public payable returns(uint){
        balance[msg.sender]+=msg.value;
        emit depositCompleted(msg.value, msg.sender);
        
    }
    
    function showBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function tranfer(uint _toTransfer, address _addressToTransfer) public returns (uint){
        require(balance[msg.sender]>=_toTransfer);
        balance[msg.sender]-=_toTransfer;
        balance[_addressToTransfer]+=_toTransfer;
    }
    
    function withdraw(uint _toWithdraw)public payable returns(uint){
        require(balance[msg.sender]>=_toWithdraw);
        msg.sender.transfer(_toWithdraw);
        balance[msg.sender]-=_toWithdraw;
    }
}

Zaqoy · June 9, 2021, 10:04pm

Hi! Here is my code - I would appreciate your feedback, thanks.

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);
        _; //run the function
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    function deposit() public payable {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
    }
    
    function withdraw(uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        msg.sender.transfer(amount);
        balance[msg.sender] = balance[msg.sender] - 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");
        
        balance[msg.sender] = balance[msg.sender] - amount;
        balance[recipient] = balance[recipient] + amount;
    }
}

RBeato · June 10, 2021, 11:07am

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

Phaxsam · June 11, 2021, 1:58am

@filip below is mine.i don’t know if i get it right but i’ve tried my best…i’ll apprecaite feedback…

pragma solidity 0.7.5;

contract Bank {

//event SKETCH
event balanceAdded(uint amount, address depostedto);

mapping(address => uint) balance;

address owner = msg.sender;

 //(D) Modifier SKETCH
 modifier OnlyOwner {
   require(msg.sender == owner, "this is not the owner");
   _; // it means run the code.
 }

function deposit() public payable  returns(uint){

//event sktech
emit balanceAdded(msg.value, msg.sender);

    balance[msg.sender] += msg.value; 
     return balance[msg.sender];

}

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

}

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

// this function sends a specific amount from the sender to the receiver by reducig the sender’s balance with an amount amd update the reciever’s balance by adding the particular amount reduced from the sender’s wallet to it.

/*function transfer(address recipient, uint amount) public payable {
require(balance[msg.sender] >= amount, “balance not sufficient”);
require(msg.sender != recipient, " this is your wallet you can’t send to your self");

// assert code implementation

uint previousSenderBalance = balance[msg.sender];
assert(balance[msg.sender] == previousSenderBalance - amount);

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

}
//or
/*_transfer(msg.sender, recipient, uint amount);

}

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

TolgaKmbl · June 12, 2021, 4:34am

Why do we need a return in withdraw function ? Is it necessary ?

function withdraw(uint amount) public {
// Checking the balance of msg.sender is sufficient.
require(balance[msg.sender] >= amount);

// Adjust the balance
balance[msg.sender] -= amount;
// Transfer to the wallet
msg.sender.transfer(amount);

}

// To view our current balance
function getbalance() view public returns(uint) {
    return balance[msg.sender];
}

crypto-djent76 · June 11, 2021, 6:41pm

1) Make sure that the user can not withdraw more than their balance

In order to make sure that the user cannot withdraw more than their balance, a require method/function is used to revert the transaction, and we can add the error message in the code for the user to identify what went wrong.
I will add the whole code afterwards, but this is the snippet of code I used for this particular query:

require(balance[msg.sender] >= amount, "Not enough funds for this transaction");

This snippet of code is making sure, by using the require method, that the balance of the sender is greater or equal to the amount to send. If it is not, the transaction fails and the error message is activated.

2) Correctly adjust the balance of the user after a withdrawal

To correctly adjust the balance of the user after a withdrawal, I implemented this line of code:

balance[msg.sender] -= amount;

This takes away the amount that the user requested from their original balance as long as they had enough funds to do so.

Code
So this was all the code I had in its entirety, and again, I hope it makes sense as I am still learning.

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);
        _; //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) {
        //check that amount to withdraw is not more than what they already have
        require(balance[msg.sender] >= amount, "Not enough funds for this transaction");
        //the amount is deducted if require is false
        balance[msg.sender] -= amount;
        //the transfer of the amount can now proceed
        msg.sender.transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint) {
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public view {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
    }

}

jon_m · June 12, 2021, 1:55am

An excellent solution and, overall, a very well-coded contract @ibn5x

Apologies for the delay in giving you some feedback.

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:

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 wallet address…

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

Your transfer event and corresponding emit statement are both well coded, and the emit statement will log relevant information when the function has executed successfully. I would just add, though, that in general, emit statements are probably better placed after assert statements, rather than before.

Your function to get the total contract balance (in additional to individual user balances) is also very good. However, you do have a redundant state variable contractValue which isn’t used, and so should be removed. I guess you must have forgotten to remove it when you realised you could just use account(this).balance

Let me know if you have any questions

jon_m · June 12, 2021, 3:03am

Welcome to the forum @gokhantaskan! … I hope you’re enjoying the course

require() statement

balance[msg.sender] -= amount;

In our withdraw() function, it is only the address receiving the ether which needs to be payable — not the function itself. A function only needs to be marked payable when it needs to receive (not send) 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 your 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.

When you remove the payable keyword, you will notice that the compiler flags an error for the return statement. This is because msg.value can only be used when the function is payable. This makes sense, because msg.value will only have a value if ether is sent to the function, which isn’t the case with the withdraw() function. We ideally want to return the user’s new balance after the withdrawal. How would you change the code in the return statement for this?

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

And don’t forget to also do the first 2 assignments before this one: Data Location, and Events.

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

jon_m · June 12, 2021, 3:44am

Hi @KK_Cheah,

require() statement

balance[msg.sender] -= _toWithdraw;

Notice that both the withdraw and the addBalance function headers also include returns(uint) . This is not mandatory to include, and the functions can still operate effectively without returning values. But as it’s been included in the function headers, you should also include return statements in the function bodies. You should have got yellow/orange compiler warnings for this. Can you add the appropriate return statements to both functions?

In our withdraw() function, it is only the address receiving the ether which needs to be payable — not the function itself. A function only needs to be marked payable when it needs to receive (not send) 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 your 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.

When you’ve made the above changes, have a look at this post which explains the importance of the order of the statements within your withdraw function body.

Your transfer() function is missing one of the require() statements and two lines of code for the assert statement. Did you remove these deliberately? You’ve also added returns(uint) to the function header but, as with the deposit and withdraw functions, you’ve not included a corresponding return statement. This isn’t necessary to include, so you can either remove it, or add an appropriate return statement.

Just one other thing…
The convention is to always start contract names with a capital letter i.e. TransferAssignment not transferAssignment

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

jon_m · June 12, 2021, 4:09am

An excellent assignment solution and, in general, a very well-coded contract @Zaqoy

You’ve added the additional lines of code needed to solve the problem with the withdraw function.

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.

Now 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

jon_m · June 12, 2021, 4:09am

Hi @RBeato,

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

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.

Now 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

jon_m · June 12, 2021, 6:29am

Hi @Phaxsam,

You’ve added all of the additional lines of code needed to solve the problem with the withdraw function However, it is only the address receiving the ether which needs to be payable — not the withdraw function itself. A function only needs to be marked payable when it needs to receive (not send) ether. So, unlike the deposit function, we don’t want to mark the withdraw function, or the transfer function, as payable in their function headers. Marking these two functions as payable doesn’t prevent your code from compiling, or the functions 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 address, but won’t be added to the individual account balance of the user who transferred it, which means the user may experience difficulties in having the amount refunded.

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

There are also some additional issues with other parts of your contract, some of which I have already drawn your attention to in other feedback I’ve given you about other code you’ve posted …

(1)

Solidity Error Handling

Solidity Error Handling

address owner = msg.sender;

This is also something I wanted to mention about the code in your previous post. You should be assigning msg.sender to the owner state variable via a constructor:
address owner;

constructor() {
    owner = msg.sender;
}
address owner = msg.sender; does still work, but I don’t think it’s good practice. Although, I would need to find out the reason why…

(2)

In terms of your emit statement for your balancedAdded event…

(3)

Standard practice is to start modifier names with lower case letters i.e. onlyOwner

(4)

This shouldn’t be commented out, so remove the /* at the beginning of this line of code

(5)
Your assert statement in the transfer function should be the last line of code to be executed (after the sender’s balance in the mapping has been reduced) otherwise it will always fail and trigger revert.

(6)

An argument’s data type is declared with its corresponding parameter in the function header, and not in the function call itself. Only an argument’s value is placed within the function call i.e. _transfer(msg.sender, recipient, amount);

Let me know if you have any questions

KK_Cheah · June 12, 2021, 6:44am

Thank you so much for the code review @jon_m. Insane dedication right there and it would be shameful if you gave so much advice and I didn’t attempt to code this assignment better. Below is my attempt at refactoring what you’ve instructed me to do as well some extra bits and pieces to improve the user experience. Hopefully I did better this round. Thank you so much once again

pragma solidity 0.7.5;

contract TransferAssignment{
    mapping(address => uint)balance;
    
    address owner;
    
    event depositCompleted(uint amount, address indexed depositedTo);
    event transferCompleted(address indexed sentBy, uint amount, address indexed receivedBy);
    event withdrawalCompleted(address indexed withdrawTo, uint amount);
    
    modifier onlyOwner{
        require(msg.sender == owner);
        _; // run the function
    }
    
    constructor() {
        owner = msg.sender;
    }
    
    function deposit() public payable{
        balance[msg.sender]+=msg.value;
        emit depositCompleted(msg.value, msg.sender);
        
    }
    
    function showBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(uint _toTransfer, address _addressToTransfer) public {
        
        require(msg.sender != _addressToTransfer, "Sending Ether back to oneself is prohibited, please change receiving address");
        require(balance[msg.sender]>=_toTransfer, "Insufficient balance to perform transfer");
        
        
        uint beforeTransferSenderBalance = balance[msg.sender];
       
        balance[msg.sender]-=_toTransfer;
        balance[_addressToTransfer]+=_toTransfer;
        
        uint newSenderBalance = balance[msg.sender];
        
        assert(beforeTransferSenderBalance == newSenderBalance + _toTransfer);
        emit transferCompleted(msg.sender, _toTransfer, _addressToTransfer);
        
    }
    
    function withdraw(uint _toWithdraw) public {
        require(balance[msg.sender]>=_toWithdraw, "Insufficient balance to perform withdrawal");
        
        uint preWithdrawalBalance = balance[msg.sender];
        
        balance[msg.sender] -= _toWithdraw;
        msg.sender.transfer(_toWithdraw);
        
        uint postWithdrawalBalance = balance[msg.sender];
        
        assert(preWithdrawalBalance == postWithdrawalBalance + _toWithdraw);
        emit withdrawalCompleted(msg.sender, _toWithdraw);
    }
}

andrewwy · June 12, 2021, 4:14pm

Thanks, @jon_m for your review and comments. I will take a note and learn more about the security issues later in the course.

ibn5x · June 12, 2021, 11:03pm

Hello @jon_m ,

Thank you so much for the encouragement and thoughtful explanations. I did indeed forget about that state variable after the discovery of account(this).balance. I will absolutely put into practice assert statements before emit statements. Before you pointed it out it was not clear too me, but after putting it into practice I understand why.

Thanks again, your time is much appreciated,

Mikal

jon_m · June 14, 2021, 10:39pm

An excellent solution @TolgaKmbl

Apologies for the delay in giving you some feedback on this.

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:

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

It isn’t mandatory to return the updated balance, and the withdraw() function can still operate effectively without returning a value. It is useful information to return, but we do already have the getBalance function which can be called separately to consult the updated balance after the withdrawal. It all really depends on your particular use case. If you had left returns(uint) in the function header (as in the code given to you) it would be strange not to include a corresponding return statement in the function body — in fact you would get a compiler warning about this. However, it’s equally valid to remove returns(uint) and not include a return statement, 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 later courses. But it’s great that you’re already getting into good habits in terms of smart contract security

Let me know if you have any further questions

jon_m · June 14, 2021, 11:25pm

An excellent assignment solution @crypto-djent76

Apologies for the delay in giving you some feedback on this.

You have added all of the additional lines of code needed to solve the problem with the withdraw function, and they are in the correct order to reduce security risk:

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 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 later courses. But it’s great that you’re already getting into good habits in terms of smart contract security

Your analysis is also very good. Just a couple of comments…

crypto-djent76:

2) Correctly adjust the balance of the user after a withdrawal

To correctly adjust the balance of the user after a withdrawal, I implemented this line of code:
balance[msg.sender] -= amount;

For the reasons I’ve explained above, we need to modify the contract state for reduction in the user’s balance (like an internal accounting adjustment) before the actual withdrawal takes place i.e. before the ether is transferred from the contract address to the caller’s external wallet address. This is what you’ve done in your code, so I’m sure this was just a slip while writing the explanation.

This comment should be … The amount is deducted if require is true
(If require evaluates to false, then the transaction will revert, and the amount isn’t deducted).

Also, your transfer function shouldn’t have view only access, because it needs to modify the contract state. The compiler may have prompted you to add view because you have a lot of code missing from this function…

Let me know if you have any questions

jon_m · June 15, 2021, 12:05am

Hi @KK_Cheah,

You’ve addressed all of the issues I mentioned by making appropriate modifications. Your whole contract is now looking really good

Just one final observation about your additional local variables to store the user’s new balance after either the withdraw or transfer has taken place, but before the assert statement. While these are correct, they also make your code longer than is necessary. Instead of assigning the new user’s balance (balance[msg.sender) to an additional local variable, and then referencing that local variable in the assert statement, it is more straightforward just to include balance[msg.sender] directly within the assert statement.

Let me know if you have any questions

Well done… you’re making great progress!

mike_sutanto · June 15, 2021, 5:43am

To make sure that the address doesn’t withdraw more than their balance we can apply

require(balance[msg.sender]>=amount, "Not enough funds stored");

To adjust the balance after withdrawing we could use:

balance[msg.sender] -= amount;
The complete code of the withdraw function would be

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