Transfer Assignment

[jon_m]

Hi @xRave110,

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.

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.

Just let me know if you have any questions

[jon_m]

Hi Saula,

Great! You now have all of the 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

---

@Saula_Atama

require(balance[msg.sender] >= amount); " FED ripped you off "

The error message, which is the require statement’s 2nd parameter, needs to be placed within the parentheses, after a comma, as follows…

require(balance[msg.sender] >= amount, "FED ripped you off");

[jCA]

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;  // Tells compiler which version of Solidity being used

contract Bank {
    mapping(address => uint) balances;
    address owner;
    
    // events
    event _depositEvent(uint _amount, address indexed _depositedTo);
    event transferredDetails(uint _amount, address indexed _to, address indexed _from);
    event _withdrawEvent(uint _amount, address indexed _to);
    
    modifier onlyOwner {
        require(msg.sender == owner, "Not authorized");
        _; // allows remaining code to run if the above is true
    }
    
    modifier onlyAddressOwnerFunds(uint _amount) {
        bool isSufficient = balances[msg.sender] >= _amount;
        require(isSufficient, "Insufficient Balance");
        _;
    }
    
    constructor() {
        owner = msg.sender;
    }
    
    function deposit() public payable returns(uint) {
        balances[msg.sender] += msg.value;
        emit _depositEvent(msg.value, msg.sender);
        return balances[msg.sender];
    }
    
    function withdraw(uint _amount) public onlyAddressOwnerFunds(_amount) returns (uint) {
        uint previousBalance = balances[msg.sender];
        msg.sender.transfer(_amount);
        balances[msg.sender] -= _amount;
        assert(balances[msg.sender] == previousBalance - _amount);
        emit _withdrawEvent(_amount, msg.sender);
        return balances[msg.sender];
    }
    
    function getBalance() public view returns(uint) {
        return balances[msg.sender];
    }
    
    function transfer(address _recipient, uint _amount) public {
        require(balances[msg.sender] >= _amount, "Balance insufficient");
        require(msg.sender != _recipient, "Cannot transfer to self");
        
        uint previousBalance = balances[msg.sender];
        _transfer(msg.sender, _recipient, _amount);
        
        // assert should always be true unless the code it is testing against is defective
        assert(balances[msg.sender] == previousBalance - _amount);
        
        //event logs and additional validation
        emit transferredDetails(_amount, _recipient, msg.sender);
    }
    
    function _transfer(address _from, address _to, uint _amount) private {
        balances[_from] -= _amount;
        balances[_to] += _amount;
    }
}```

[Konzaih]

 function withdraw(uint amount) public returns (uint) {
        require(balance[msg.sender]>= amount, "Not enough funds, click getBalance to check your Account");
        //msg.sender is an address
        msg.sender.transfer(amount);
        balance[msg.sender]-=amount;
    }

[xRave110]

Hello Jon,

I was just wondering about order of the commends “balance[msg.sender] -= amount;” and “msg.sender.transfer(amount);”. It is now clear thanks to you.

[jon_m]

Hi @Konzaih,

I have given you some feedback on your initial attempt here (posted in the Payable Functions discussion topic).

You’ve now included the essential line of code needed to adjust the user’s individual balance in the mapping, which I’ve explained in point (1) of my feedback. Hopefully, my explanation gives you some useful, additional information about why this line is needed.

You’re still missing a return statement, which I’ve explained in point (2) of my feedback.

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.

Just let me know if you have any questions.

[Naoki_Takahashi]

pragma solidity 0.7.5;

contract bank{
mapping (address=>uint) balance;
address owner; //initializing the state variable for the owner of the transaction
uint deposited;

event depositDone(uint amount, address indexed depositedTo);
event transferedDone(uint transferAmount, address transferTo);
event withdrawDone(uint _amount, address indexed withdrawTo)
// event creates a log record so that we can look through a specific record later on 

modifier onlyOwner{
// when the code will be repeated many times, we can make a modifier
// requires only owner of the address to call the funciton
  require(msg.sender==owner, "You are not the owner!");
  _; // run the function
}

modifier sufficientAmount{ 
//checks if the account balance is smaller than or equal to the withraw amount 
  require (balance[msg.sender]>=amount, "Not enough balance!");
  _;
}

constructor(){
  owner = msg.sender; // setting that the owner is whoever that is making the transaction
}

function deposit() public onlyOwner payable returns (uint){ // modifier comes after "public" and exectues before the funciton
  balance[msg.sender]+=msg.value;
  emit depositDone(msg.value, msg.sender);
  return balance[msg.sender];
}

function withdraw(uint amount) public sufficientAmount returns (uint){
  msg.sender.transfer(amount);
  // transfer fuction withdraws the amount from the contract to the address
  // transfer() has a built in function to revert if something oes wrong
  
  emit withdrawDone(msg.value, msg.sender); 
  // creates a log of withdraw history
  balance[msg.sender]-=amount; 
  //updates the balance of the 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, "Blanace not sufficient");
    //checks if the balance of the sender is greater than the amount that is being sent
  require(msg.sender != recipient, "Dont transfer money to yourself"); 
    //checks that sender is not sending the transactions to themselves
   
  uint previousBalance = balance[msg.sender];
  _transfer(msg.sender, recipient, amount);
  emit transferedDone(amount, recipient);
  assert(balance[msg.sender]== previousBalance - amount); // assert() checks for invariants in the function.
}

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

}

[pieczyslaw]

This looks to be working good. Please correct me if there´s a better way

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

[jon_m]

Nice solution @jCA

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

The additional assert statement and withdrawal event (with corresponding emit statement), which you’ve added, are appropriate and well coded, and you have placed both the assert and the emit statement in the correct positions within the withdraw function body.

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.

The rest of your contract is also well coded. Just one observation…

You have added an onlyAddressOwnerFunds modifier, which you have coded and implemented well in the withdraw function. However, the point of writing a separate modifier, instead of placing the input validation at the beginning of the function body, is to avoid code duplication by re-using it. Adding this modifier is very good idea, because it can be used in both the withdraw() and the transfer() function; but you have missed the opportunity to use it in transfer(), which requires exactly the same input validation as withdraw().

Just let me know if you have any questions

[jon_m]

Hi @Naoki_Takahashi,

Withdraw function

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

You have the right idea with the sufficientAmount modifier, but you need to include an amount parameter in the header, so that you can call the modifier in the withdraw function header with the amount the caller wants to withdraw, as follows:

modifier sufficientAmount(uint amount) { 
    require(balance[msg.sender] >= amount, "Not enough balance!");
    _;
}

function withdraw(uint amount) public sufficientAmount(amount) returns(uint) {  }
/* The value of the function's amount parameter is passed to the modifier
    in order for the modifier to check that the withdrawal amount is not
    greater than the caller's balance */

The compiler will generate a red error for your modifier, and the error message will state that there is an undefined identifier — this is the name amount which needs to reference something (in this case a parameter uint amount, which needs to be passed to the modifier).

Your withdrawDone event declaration is appropriate and well coded, except for the missing semi colon at the end of the statement, which will prevent your contract from compiling. The compiler should have generated a red error for this.
The corresponding emit statement should reference the amount parameter, not msg.value (which only references an ether/wei value received by a payable function, such as the deposit function). The compiler will generate a red error for this too.
Also, 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. You should, therefore, position your emit statement at the end of the function body, just before the return statement.

Once you have made these modifications to your withdraw function, have a look at this post which explains an important security modification that should be made to the order of 2 other statements in your withdraw function body.

---

You have put a lot of effort into your contract code as a whole, and also into the comments you have added to explain the purpose of certain lines of code. Here some additional comments about your code, and corrections you should make:

(1) You have a state variable deposited, which isn’t used anywhere.

(2)

@Naoki_Takahashi

address owner; //initializing the state variable for the owner of the transaction

… the owner of the contract (not transaction).

(3)

@Naoki_Takahashi

constructor(){
  owner = msg.sender; // setting that the owner is whoever that is making the transaction
}

… yes, and this transaction is deploying the contract, so this constructor sets the owner state variable as the address which deploys the contract.

(4)  The idea of this contract is to simulate a bank. So, it’s probably not such a good idea to restrict deposits to the contract owner (which you’ve done by applying the onlyOwner modifier to the deposit function). As anyone can transfer and withdraw funds, it would make more sense if anyone can deposit funds into the contract as well.

(5)  Your transfer event and corresponding emit statement are both correctly coded, and the emit statement will log appropriate data when the transfer() function is successfully executed. But in general, an emit statement is probably better placed after an assert statement.
Where the deposit and withdraw functions both execute transactions that only involve 1 user address (the depositor or the withdrawer), the transfer function executes a transaction that involves 2 user addresses (the sender and the recipient). So, it would be useful to include the sender’s address, as well as the recipient’s address, within the data emitted for the transfer event.

(6)  With reference to your sufficientAmount modifier, the point of writing a separate modifier to validate an input or restrict access to a function — instead of placing this code at the beginning of the function body — is to avoid code duplication by re-using the modifier. Adding your sufficientAmount modifier is a very good idea, because it can be used in both the withdraw() and the transfer() function; but you have missed the opportunity to use it in transfer(), which requires exactly the same input validation as withdraw().

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

[jon_m]

Nice solution @pieczyslaw

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

[runHikeClimb]

@jon_m

needed to transfer the funds out of the contract to the external address. The lines of code you have added are all correct, but only hav

thank you for reviewing this code.

[pieczyslaw]

Perfect I was actually curious of the importance of the order of statements as I saw later in the solution it was different than mine.

Thank you

[Claudio]

pragma solidity 0.7.5;

contract Bank_9{

mapping(address => uint) balance;

event depositDone(uint amount, address indexed depositTo);
event withdrawExecuted(uint amount, address indexed depositTo);
event transferExecuted(uint amount, address indexed depositTo);

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

function getBalance() public view returns (uint){
return balance[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, “Balance is not sufficient to withdraw that amount”);

 uint previousSenderBalance = balance[msg.sender];
 
 balance[msg.sender] -=amount;
 
 msg.sender.transfer(amount);
 
  emit withdrawExecuted(amount, msg.sender);
  
  return balance [msg.sender];
  
 assert(balance[msg.sender] == previousSenderBalance - amount);

}

function transfer (address recipient, uint amount) public{
require(balance[msg.sender] >= amount, “Balance is not sufficient to transfer that amount”);
require(msg.sender != recipient, “You can’t send money to yourself”);

uint previousSenderBalance = balance[msg.sender];
 
 _transfer(msg.sender, recipient, amount);
 
 emit transferExecuted(amount, recipient);
 
 assert(balance[msg.sender] == previousSenderBalance - amount);

}

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

[alexhupe]

Hey this is what I wrote. Let me know what you think:

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

Also, can you tell me why I was getting errors when writing “msg.sender.transfer(amount)”? I ended up using "payable(msg.sender).transfer(amount) and it worked. Please explain this. Thanks.

[TRYLAST]

buddy, I think in this line of code you have issue .The structure of this one I think is wrong
payable(msg.sender).transfer(amount);

Correct one You should have done
msg.sender.transfer(amount).

I am student like you. Correct me if I am also wrong. Appreciate

[Konzaih]

Hello jon_m

Thank you for your detailed explanation.
Now I understand what is missing and why it has to be included!
I’ve adjusted the code accordingly.

Many Thanks!

[thecil]

Hey @alexhupe, @TRYLAST hope you guys are well.

It’s one of the syntax changes since solidity v0.8.0.

https://docs.soliditylang.org/en/latest/080-breaking-changes.html?highlight=payable#how-to-update-your-code

Carlos Z

[Claudio]

Hi.

Re: https://github.com/filipmartinsson/solidity-0.7.5/blob/main/transfer-assignment-solution.sol

Doesn’t need the ‘withdraw function’ the ‘assert check’ at the end of it alike we have it in the ‘transfer function’ - for proper error handling?

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

thank you.

[thecil]

@Claudio

Doesn’t need the ‘withdraw function’ the ‘assert check’ at the end of it alike we have it in the ‘transfer function’ - for proper error handling?

Yes, complete valid, probably filip did your forgot to add it, but off course is a good check point for the function

Carlos Z