Transfer Assignment

Joaovianna · January 15, 2021, 9:33pm

function withdraw(uint amount) public onlyOwner returns (uint){
require(balance[msg.sender] >= amount, “Insufficient funds”);
msg.sender.transfer(amount);
return balance[msg.sender];
}

Elekko · January 16, 2021, 12:13am

Just add require check that amount is less or greater to balance in msg.sender

function withdraw(uint amount) public returns(uint) {
    require(amount <= balance[msg.sender], "Not enough ETH.");
    //address payable toSend = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
    //toSend.transfer(amount);
    
    msg.sender.transfer(amount);
}

Nelson1 · January 16, 2021, 6:01am

function Withdraw(uint amount) public returns(uint){
    
    require(Balance[msg.sender] >= amount, "Not enough money to make this transfer!");
    
    //Always substract the money from the account before making the transfer...
    Balance[msg.sender] -= amount;
    
    //...then make the transfer...
    msg.sender.transfer(amount);
    
    return Balance[msg.sender];
    
}

lloyda1 · January 16, 2021, 12:10pm

Hey guys here’s my solution to the Transfer Assignment using solidity!

pragma solidity 0.7.5;
contract Bank {

mapping(address => uint) balance;

address owner;

event depositDone(uint amount, address indexed depositedTo);

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 {
    require(amount <= balance[msg.sender],"Balance not sufficient");
    msg.sender.transfer(amount);
    balance[msg.sender] -= amount;
}


function getBalance() view public 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);
}

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

}

herbz · January 16, 2021, 12:13pm

Here’s my answer:

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

chim4us · January 16, 2021, 12:31pm

pragma solidity 0.7.5;

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

event depositDone(uint amount, address indexed DepositedTo);
event balanceAdded(uint amount, address indexed DepositedTo);

address owner;

modifier onlyOwner{
    require(owner == msg.sender,"Your not the owner");
    _;
}
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 getBalance() public view returns(uint){
    return balance[msg.sender];
}

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

function transfer(address recepiant, uint amount) public{
    require(balance[msg.sender] >= amount);
    require(msg.sender != recepiant);
    
    uint previoussenderbalance = balance[msg.sender];
    _transfer(msg.sender ,recepiant, amount);
    
    assert(balance[msg.sender] == previoussenderbalance - amount);
}

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

}

Mirran · January 17, 2021, 4:49am

// SOLUTION - tested
function withdraw(uint amount) public returns (uint) {
require(balance[msg.sender] >= amount, “Insufficient funds”);
msg.sender.transfer(amount);
balance[msg.sender] -= amount;
return balance[msg.sender];
}

I had a hard time understanding this (pun intended) until I found out about “address(this).balance” and made a function getContractBalance to see for myself. But then I omitted my “balance[msg.sender] -= amount;” and the withdraw worked but the user balance stayed the same while the total contract balance reduced. Why did it happen this way? It feels like the contract balance is separate to the dictionary’s total balance. Is that true?

Mark_Dalton · January 17, 2021, 7:28am

pragma solidity 0.7.5;

contract Bank {

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

event depositDone(uint amount, address depositedTo);
event transferFunds(uint amount, address sentFrom, address sentTo);

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

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 a sufficient balance
    require(balance[msg.sender] >= amount, "Balance is not sufficient");
    
    balance[msg.sender] -= amount; // do prior to transfer to avoid overspending 
    msg.sender.transfer(amount); // safest way to transfer, with error handling
    return balance[msg.sender];
}

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

function transfer(address recipient, uint amount) public {
    // check balance of msg.sender
    require(balance[msg.sender] >= amount, "Balance is not sufficient");
    require(msg.sender != recipient, "Do not send to yourself");

    // Save the senders balance
    uint previousSenderBalance = balance[msg.sender];

    _transfer(msg.sender, recipient, amount);
    
    // Confirm senders balance reduced by amount sent.
    assert(balance[msg.sender] == previousSenderBalance - amount);
    // event logs and further checks
    emit transferFunds(amount, msg.sender, recipient); 
}

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

}

nick_bell2017 · January 17, 2021, 8:42am

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

codinginlondon · January 17, 2021, 4:08pm

Here is a solution to raise an error when the caller attempts to withdraw more than their balance.


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);
        _;
    }
    
    
    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, "balance not sufficient");
        msg.sender.transfer(amount);
    
        balance[msg.sender] -= amount;
        
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }

}

jon_m · January 17, 2021, 4:55pm

Hi @SirGoat,

That’s looking much better The only improvement left to make is the position of the transfer() function. As explained in this post the contract’s internal state should be updated to reduce the balance before performing the transfer of funds from the smart contract address to the external wallet address.

jon_m · January 17, 2021, 5:20pm

Hi @jiadong_han,

You have included all of the additional functionality needed to solve the problem with the withdraw function, and you also have the different operations in the correct order for optimal security:

check inputs (require statement)
effects (update the contract state for reduction in balance)
external interactions (perform the transfer of funds from the smart contract address to the external wallet address).

However, there is one serious problem with how you have coded the withdraw function. There is no restriction on who can call this function, yet the only balance that can be withdrawn from is the owner’s balance (balance[owner]). Whoever the msg.sender is can withdraw funds from the contract to their own address, without needing to have deposited any funds in the first place, because the require statement compares the withdrawal amount to the owner’s balance and not to the sender’s balance.

Effectively, anyone can steal the owner’s funds held within the contract and recorded in one of the balances in the mapping!

The idea of this withdraw function is for anybody who has deposited funds in the contract to be able to withdraw these funds, but only their own funds and not more than the total amount they have deposited.

How would you correct this?

jon_m · January 17, 2021, 5:36pm

Hi @okcrypto,

The require statement you’ve added is correct but you are missing 2 other important lines of code. Have a look at this post which explains the issues and what you need to add to correct this.

Let us know if you have any questions, or if there is anything that you don’t understand

jon_m · January 17, 2021, 6:58pm

Hi @Ignacio,

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 operations within your withdraw function body.

Your Transfer event is well coded, but you haven’t included a corresponding emit statement in the transfer function. 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.

You’ve put a lot of effort into your comments, so I just want to mention a couple of inaccuracies that you will probably want to correct:

The role of a constructor is to assign specific values to specific variables on deployment. It allows our contract to be initialised with a predetermined state. In our Bank contract, the fact that the condition in our onlyOwner modifier is set by the constructor on deployemnet, is only a consequence of the specific code we have placed in our constructor, and isn’t something that all constructors do by definition.

This line is still needed in this function. The fact that the function is payable means that it is able to receive a payment in ether (msg.value) from the caller of the function (msg.sender). This amount in ether will automatically be added to the balance of the contract’s address (without us needing to code this explicitly), but we do still need to add this line of code in order to record which address deposited this amount, so that we know how much of the total funds held in the contract they are entitled to withdraw.

I’m not really sure what you are intending to do with this modifier, but it won’t have anything to do with the gas cost of running the function. What it will do is ensure that the caller’s balance held in the contract is not less than the amount stated as the modifier’s parameter in the function header:

But this is an unnecessary additional constraint in your transfer function, because you are already checking in the 1st require statement in the function body whether the sender has sufficient balance to transfer the amount requested. Gas is paid from the account balance associated with the address which calls the function, and not from any balance allocated to that address and held within the contract. If the caller doesn’t have enough ether to pay the gas cost, then the transaction will revert anyway, so there is no need to add any code for this.

I hope you find these comments helpful. Just let us know if anything is unclear, or if you have any questions.

You’re making great progress! Keep it up

jon_m · January 17, 2021, 7:26pm

Hi @Joaovianna,

The require and return statements you’ve added are correct but you are missing one other important line of code:

When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! I’m sure you can see that this is another very serious bug! How would you correct this? If you can’t work this out yourself, you’ll find the answer by taking a look at some of the other posts in this topic.

You have also forgotten to remove onlyOwner . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. The idea of this function is for all account holders to be able to withdraw funds.

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 us know if you have any questions about how to correct this, or if there is anything that you don’t understand

jon_m · January 17, 2021, 7:11pm

Hi @Elekko ,

The require statement you’ve added is correct but you are missing 2 other important lines of code. Have a look at this post which explains the issues and what you need to add to correct this.

Let us know if you have any questions, or if there is anything that you don’t understand

jon_m · January 17, 2021, 7:25pm

Hi @nick_bell2017,

The require statement you’ve added is correct but you are missing another important line of code:

When you deploy your contract and call your withdraw function as it is, you will notice that the sender’s address receives the requested withdrawal amount, but their balance in the mapping is not reduced! I’m sure you can see that this is another very serious bug! How would you correct this? If you can’t work this out yourself, you’ll find the answer by taking a look at some of the other posts in this topic.

The incomplete withdraw function given to you for the assignment also includes returns(uint) in the function header. This is not mandatory to include and the function can still operate effectively without returning a value. Was it your intention to remove it? If you add the returns(uint) to the function header, then you would also need to include a return statement in the function body.

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 us know if you have any questions about how to correct this, or if there is anything that you don’t understand.

ZigaP · January 17, 2021, 8:10pm

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 onlyOwner returns (uint) {
        require(balance[msg.sender] >= _toWithdraw);
        
        msg.sender.transfer(_toWithdraw);
        balance[msg.sender] -= _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 17, 2021, 10:03pm

Hi @codinginlondon,

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 operations within your withdraw function body.

jon_m · January 17, 2021, 10:15pm

Hi @ZigaP,

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 operations within your withdraw function body.

You have also forgotten to remove onlyOwner from the function header . If you leave this function as onlyOwner , then only the contract owner can withdraw their own personal funds. But the idea of this function is for all account holders to be able to withdraw funds.

ZigaP:

  function deposit() public payable returns(uint){
        //balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }

The line of code you have commented out in the deposit function is still needed. The fact that the function is payable means that it is able to receive a payment in ether (msg.value) from the caller of the function (msg.sender). This amount in ether will automatically be added to the balance of the contract’s address (without us needing to code this explicitly), but we do still need to add this line of code in order to record which address deposited this amount, so that we know how much of the total funds held in the contract they are entitled to withdraw.

Let us know if you have any questions, or if there is anything that you don’t understand.