Transfer Assignment

ZigaP · January 18, 2021, 8:34am

Hey Jon. Thanks for the feedback!

I have read the post and understand the concept of why the order is important.
I intentionally kept the onlyOwner because I thought the msg.sender (= the person actually trying to call the withdraw function) is any fund-owner not only the smart contract owner. Thanks for clearing it up!
The comment was unintentionally left from trying something out.

I am pasting the updated code below.

(Edited again, according to the next comment)

pragma solidity 0.7.5;

contract mappings {
    
    mapping(address => uint) balance;
    address owner;
    
    modifier onlyOwner {
        require(msg.sender == owner);
        _;
    }
    
    constructor(){
        owner = msg.sender;
    }
    
    event depositDone(uint howMuch, address depositedTo);
    
    function deposit() public payable returns(uint){
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns(uint){
        return balance[msg.sender];
    }

    function withdraw(uint _toWithdraw) public returns (uint) {
        require(balance[msg.sender] >= _toWithdraw);

        balance[msg.sender] -= _toWithdraw;
        msg.sender.transfer(_toWithdraw);
        
        return balance[msg.sender];
    }

    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Insufficient ammount");
        require(msg.sender != recipient);
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
        
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
}

jon_m · January 18, 2021, 12:35am

msg.sender is anyone who calls the withdraw function, and so can be any “fund-owner” (not only the smart contract owner). The problem with making the function onlyOwner, is that only the contract owner will successfully meet the condition in the onlyOwner modifier, and the function will revert if msg.sender is any other address.

Great — but you still have these two lines in the wrong order:

The balance should be reduced in the mapping before the funds are transferred from the contract to the external address.

jiadong_han · January 18, 2021, 11:40am

Hi, thanks a lot for your point of veiw. I will use the following modifier:

modifier OnlyOwner {
require (msg.sender == owner );
_;
}
and write the modifier on my withdraw function.
function withdraw(uint _tominus) public OnlyOwner returns(uint){

 uint oldSenderBalance = balance[owner];
 require (oldSenderBalance >= _tominus, "Balance is not sufficient" );
 balance[owner] -= _tominus;
 msg.sender.transfer( _tominus);
 assert(balance[owner] == oldSenderBalance - _tominus  );
 return balance[owner];

}

nick_bell2017 · January 18, 2021, 11:53am

Thanks @jon_m! I will take a look this afternoon and get back to you.

SirGoat · January 18, 2021, 1:20pm

thankyou!
yes, I can see how a double payment may be executed if the attack were to occur prior to the balance state being updated but after the transfer of funds.

function withdraw(uint amount) public returns (uint){
// check msg.sender balance
require(balance[msg.sender]>=amount, “Balance insufficient!”);
//adjust contract balance
uint newBalance = balance[msg.sender] -= amount;
//transfers are denominated in Wei
msg.sender.transfer(amount);
balance[msg.sender]= newBalance;
return (newBalance);
}

nick_bell2017 · January 20, 2021, 3:25am

Hi again @jon_m. Thanks for the feedback. I think I have now fixed there errors

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

Conor_Neary · January 18, 2021, 9:27pm

Updated to include return statement and change order per other posts

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

Carlosvh96 · January 19, 2021, 6:34am

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;
address owner;

modifier OnlyOwner{
require(msg.sender == owner);
_;
}

event depositDone(uint amount, address depositedTo);

event transferListener(address depositedFrom, address depositedTo, uint amount);

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, “you cant withdraw more than you have”);
msg.sender.transfer(amount);
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);
require(msg.sender != recipient, “Don’t send funds to yourself”);

uint previousSenderBalance = balance[msg.sender];

_transfer(msg.sender, recipient, amount);

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

}

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

Ignacio · January 19, 2021, 11:01pm

Hi Jon,

Your comments are very much appreciated! I have made some changes, if you could confirm that now it makes more sense would be great. I have corrected the inaccuracies you pointed out and added more events to display in ‘logs’


contract banking{
    
    mapping(address => uint) balance;   // maps an address to balance object, e.g. in python: balance["address"] = uint
    address owner;
    
    
    // modifier: restricts access to whatever meets conditions
    modifier onlyOwner {
        require(msg.sender == owner);
        _;   // underscore (;) means to run the function
    }
    
    
    // create an event to be able to query data 
    event balanceAdded(uint amount, address depositedTo);
    event balanceTransfer(uint amount, address depositedFrom, address depositedTo);
    event balanceWithdrawal(uint amount, address withdrawnFrom);
    
    //constructor: allows to pre-assign specific values to variables
    constructor() {
        owner = msg.sender;
    }

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

    // add balance to an address and update
    function deposit_balance() public payable returns(uint){
        balance[msg.sender] += msg.value; // payable functions act on the net, however this line is for us to keep track of the balances
        emit balanceAdded(msg.value, msg.sender); // emit the event check the logs in the output
        return balance[msg.sender];
    }
    // withdraw function
    function withdraw_balance(uint amountx) public returns(uint){
        require(balance[msg.sender] >= amountx);
        msg.sender.transfer(amountx);
        balance[msg.sender] -= amountx;
        emit balanceWithdrawal(amountx, msg.sender);
        return balance[msg.sender];
    }
    
    
    // this function is PRIVATE!! can only be called from this contract,
    // substracts amount from sender and adds it to receipent
    function transfer_funds(address from, address to, uint amountx) private{
        balance[from] -= amountx;
        balance[to] += amountx;
        emit balanceTransfer(amountx, from, to);
    }
    
    // 
    function transfer(address receipent, uint amount) public {
        require(balance[msg.sender] >= amount, "balance isn't enough");   // verify that the sender has enough money
        require(receipent != msg.sender, "can't transfer to yourself");         // verify that the sender doesn't auto-transfer
        
        uint previous_balance = balance[msg.sender];
        
        // call the ohter function called: transfer_funds
        transfer_funds(msg.sender, receipent, amount);
        
        // assert: checks that conditions are met
        assert(balance[msg.sender]  == previous_balance - amount);
    }
}

jon_m · January 20, 2021, 2:47am

Hi @jiadong_han,

Your withdraw function is now secure, but only the owner can withdraw their funds. As I mentioned, the idea of this withdraw function is for anybody who has deposited funds in the contract to be able to withdraw their own funds.

The deposit function allows anybody to deposit funds (their balance being recorded in the mapping) but they are unable to withdraw them. By using msg.sender instead of owner within the withdraw function, you would allow any address to withdraw their funds, but only their own funds.

jon_m · January 20, 2021, 3:23am

Hi @nick_bell2017,

Yes, that’s looking much better now
I hope you don’t mind, but I’ve edited the full contract you posted to just leave your final withdraw function. Some of the other code relates to later assignments, so I don’t want to risk confusing students, or show them parts of the solutions to assignments they haven’t done yet

jon_m · January 20, 2021, 3:55am

Hi Carlosvh96,

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

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 seeing as the original assignment code includes this, you may want to consider what additional line of code you would need to include in the function body if your solution did return a value.

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

Your Transfer event is well coded, and successfully emits the event upon transfer. However, it would be better to include it at the end of the main transfer function, rather than in the helper function _transfer(). The only additional piece of data that I think would also be useful to include in this event is the new balance of msg.sender after the transfer.

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

jon_m · January 20, 2021, 4:39am

Hi @Ignacio,

It’s looking better, but there are still a few things you can improve:

Your statements still aren’t in the best order for security.

You’ve included the emit statement, but this would be better placed at the end of the main transfer function instead of in the helper function transfer_funds.

I think you misunderstood me here. I meant that the new sender’s balance (after making a transfer) would be a useful piece of additional data to emit in your Transfer event. I didn’t mean create an additional Withdrawal event, although what you have added for this is good.

You’re putting a lot of effort into this, which is great to see

Carlosvh96 · January 20, 2021, 5:37am

Hi John thanks you for the feedback really appreciated, as it is about something importnat and that i completely ignored xD, so yes from now on im going to try to think more about how the code will behave in the real world

pragma solidity 0.7.5;

contract Bank {

mapping(address => uint) balance;
address owner;

modifier OnlyOwner{
require(msg.sender == owner);
_;
}

event depositDone(uint amount, address depositedTo);

event transferListener(address depositedFrom, address depositedTo, uint amount);

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, “you cant withdraw more than you have”);
balance[msg.sender]-= amount;
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 {
require(balance[msg.sender] >= amount);
require(msg.sender != recipient, “Don’t send funds to yourself”);

uint previousSenderBalance = balance[msg.sender];

_transfer(msg.sender, recipient, amount);

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

}

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

Spartak · January 20, 2021, 2:19pm

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

jon_m · January 20, 2021, 5:43pm

Hey @SirGoat,

Your solution works but you can simplify it by updating the mapping directly without using the local variable (newBalance), as follows:

   ...

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

Obviously, you would then need to return balance[msg.sender] instead of newBalance .

Even if you do keep the local variable, you can still remove the line:
balance[msg.sender] = newBalance;
Even though you have initially assigned the new balance to a local variable, the fact that you have assigned balance[msg.sender] -= amount , instead of just balance[msg.sender] - amount , seems to still update the mapping from this line of code (if it didn’t update the mapping until the line balance[msg.sender] = newBalance then you wouldn’t have removed the vulnerability, because this comes after the external transfer in your code).

SirGoat · January 20, 2021, 5:59pm

thank you… that makes sense… I had not considered that I was using a local variable…
thank you for the clarification.

Ignacio · January 20, 2021, 9:46pm

I understand, thanks for the comments. Here is my final code. I understood that my function withdraw_balance is not the one that we are focusing for now, but I have left it anyways. The main correction occurs in the function called transfer as you pointed out. Thanks Jon!

pragma solidity 0.7.5;

contract banking{
    
    mapping(address => uint) balance;   // maps an address to balance object, e.g. in python: balance["address"] = uint
    address owner;
    
    
    // modifier: restricts access to whatever meets conditions
    modifier onlyOwner {
        require(msg.sender == owner);
        _;   // underscore (;) means to run the function
    }
    
    
    // create an event to be able to query data 
    event balanceAdded(uint amount, address depositedTo);
    event balanceTransfer(uint amount, address depositedFrom, address depositedTo, uint currentBalance);
    event balanceWithdrawal(uint amount, address withdrawnFrom);
    
    //constructor: allows to pre-assign specific values to variables
    constructor() {
        owner = msg.sender;
    }

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

    // add balance to an address and update
    function deposit_balance() public payable returns(uint){
        balance[msg.sender] += msg.value; // payable functions act on the net, however this line is for us to keep track of the balances
        emit balanceAdded(msg.value, msg.sender); // emit the event check the logs in the output
        return balance[msg.sender];
    }
    // withdraw function
    function withdraw_balance(uint amountx) public returns(uint){
        require(balance[msg.sender] >= amountx);
        balance[msg.sender] -= amountx;  // this goes before applying ***.transfer for security reasons
        msg.sender.transfer(amountx);
        emit balanceWithdrawal(amountx, msg.sender);
        return balance[msg.sender];
    }
    
    
    // this function is PRIVATE!! can only be called from this contract,
    // substracts amount from sender and adds it to receipent
    function transfer_funds(address from, address to, uint amountx) private{
        balance[from] -= amountx;
        balance[to] += amountx;
    }
    
    // 
    function transfer(address receipent, uint amount) public {
        require(balance[msg.sender] >= amount, "balance isn't enough");   // verify that the sender has enough money
        require(receipent != msg.sender, "can't transfer to yourself");         // verify that the sender doesn't auto-transfer
        
        uint previous_balance = balance[msg.sender];
        
        // call the ohter function called: transfer_funds
        transfer_funds(msg.sender, receipent, amount);
        
        // assert: checks that conditions are met
        assert(balance[msg.sender]  == previous_balance - amount);
        
        // emit logs of the transfer_funds
        emit balanceTransfer(amount, msg.sender, receipent, balance[msg.sender]);
    }
}

matottjm · January 23, 2021, 12:47pm

   function withdraw(uint _amount) public returns (uint) {
       
       // check balance of sender to enusre they have the funds to withdraw
       require(balance[msg.sender] >= _amount, "Overdraft error");
       
       // msg.sender is an address
       msg.sender.transfer(_amount);
       
       uint previousSenderBalance = balance[msg.sender];
       
       // update the balance of the sender 
       balance[msg.sender] -= _amount;
       
       assert(balance[msg.sender] == previousSenderBalance - _amount);
       
       emit withdrawlComplete(_amount, msg.sender);
       
       return balance[msg.sender];
   }

dxdoan · January 26, 2021, 6:05pm

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