Transfer Assignment

Davelopa · December 20, 2021, 1:20pm

Thanks for the feedback @jon_m!

This kind of information is very useful and is good to have the feedback right after learning and trying out new things!

Cheers

DanDD · December 20, 2021, 10:28pm

Modified the withdraw routine:


    function withdraw(uint amount) public returns(uint){
        require(balance[msg.sender] >= amount, "You don't have enough to withdraw");
        msg.sender.transfer(amount);
        balance[msg.sender] -= amount;
    }

jon_m · December 21, 2021, 12:41am

Hi @DanDD,

You have added both of the essential 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 as it’s been included in the function header, you should also include a return statement at the end of the function body. The compiler should have given you a yellow/orange warning for this.

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.

Let me know if you have any questions.

Kunanon_Jarat · December 21, 2021, 10:07am

pragma solidity 0.7.5;

contract Bank {
    
    mapping(address => uint) balance;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    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[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, "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;
    }
    
}

jon_m · December 22, 2021, 3:09am

An excellent solution @Kunanon_Jarat

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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

Keep up the great coding!

jon_m · December 22, 2021, 3:15am

By the way, I think you’ve missed out the Events Assignment, because you are missing a Transfer event in your code, and a corresponding emit statement in the transfer() function.

0xBrito · December 22, 2021, 11:09pm

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

Restford · December 23, 2021, 10:06pm

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

mcs.olive · December 23, 2021, 10:54pm

1) Make sure that the user can not withdraw more than their balance
require(balance[msg.sender] >= _amount, “Balance not sufficient”);
2) Correctly adjust the balance of the user after a withdrawal
balance[msg.sender] -= _amount;

    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 · December 23, 2021, 11:31pm

Nice solution @0xBrito

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 · December 23, 2021, 11:58pm

An excellent solution @Restford

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= withdrawAmount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(withdrawAmount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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

Please, format your code before posting it. This will make it clearer and easier to read. It will also make it easier to copy and paste if we need to deploy and test it.
Follow the instructions in this FAQ: How to post code in the forum

And don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Just let me know if you have any questions

jon_m · December 24, 2021, 2:16am

An excellent solution @mcs.olive

You have added all of the lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= _amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(_amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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

Don’t forget to post your solution to the Events Assignment here. This is the assignment from the Events video lecture, which is near the end of the Additional Solidity Concepts section of the course.

Let me know if you have any questions

codingluan · December 30, 2021, 3:30am

Hi, this is my solution:

// SPDX-License-Identifier: MIT

pragma solidity 0.7.5;

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

    event depositDone(address indexed from, uint amount);
    event withdrawDone(address indexed to, uint amount);

    function deposit() public payable {
        balance[msg.sender] += msg.value;

        emit depositDone(msg.sender, msg.value);
    }

    function withdraw(uint _amount) public {
        require(balance[msg.sender] >= _amount, "balance not sufficient");

        _withdraw(msg.sender, _amount);

        emit withdrawDone(msg.sender, _amount);        
    }

    function _withdraw(address payable _to, uint _amount) private {
        balance[_to] -= _amount;
        _to.transfer(_amount);
    }

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

JaimeAS · December 30, 2021, 11:26am

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

jon_m · December 31, 2021, 12:31pm

This is an excellent and well thought-out solution @codingluan

Your use of an additional helper function (_withdraw) is particularly interesting. It is appropriate, well coded, and you have done a great job of applying the same coding pattern used for the _transfer() helper function and adjusting it to the requirements of the withdraw() function. You have also ensured that the transfer method is still called on a payable address.

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the control flow, in order to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[_to] -= _amount;

before actually transferring the funds out of the contract to the external address…

_to.transfer(_amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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 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 a user can call separately to consult their own individual updated balance after a withdrawal.

Your withdraw event and corresponding emit statement are also both well coded. The emit statement is in the correct position in the withdraw() function body and will log appropriate data when the withdraw() function is successfully executed.

Just let me know if you have any questions

jon_m · December 31, 2021, 12:42pm

Hi @JaimeAS,

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent 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

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 at the end of the function body. The compiler should have given you a yellow/orange warning for this.

Just let me know if you have any questions

InterstellarX · January 1, 2022, 3:14pm

Hi! This is my solution: what I do is use the balance mapping for msg.sender and ensure that the balance is greater than or equal to the amount of withdraw at initiation. Then I subtract the amount from the balance of msg.sender to ensure that is OK and possible. Then I run the transfer function for the amount requested on msg.sender. It then returns a bool of success (either true or false) and I envision this can be helpful for event logging, where it can return logs/notifications upon withdraws — this is something I did in my function with the boolean success I implemented

pragma solidity 0.7.5;

contract Bank {

    mapping(address => uint) balance;
    // mapping(address => uint) public balances;
    address owner;

    // define event
        // indexed = by eth nodes, can use params to search/query for events in the past
    event depositDone(uint amount, address indexed depositedTo);

    event balanceTransfered(uint amount, address indexed depositedTo);

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

    // modifier costs(uint price) {
    //     require(msg.value >= price);
    //     _;
    // }

    constructor() {
        owner = msg.sender;
    }

    // for anyone to deposit
        // payable - allows function to receive money when people call it
    function deposit() public payable returns (uint) {
        // if you deposit 1 ether, it should be attributed to account
            // don't need to worry about memory or storage, because saved to the blockchain
            // balance mapping keeps track of internal transfers/sending
        balance[msg.sender] += msg.value; // to internally track who deposited money
        // add event 
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender]; 
    }

    // function for folks to withdraw eth to their address
        
    function withdraw(uint amount) public returns (bool success) {
        // msg.sender is an address 
            // you can define an address payable sender and name whatever name you want to
            // address payable toSend = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
        
        // Transfer has revert abilities...if fails, will revert non-gas eth
            // transfer has built in error handling
            // MUST check balance of user to ensure can't overdraw/withdraw other people's money
            // toSend.transfer(amount);
        require(balance[msg.sender] >= amount); // prevents withdraw of others' money
        balance[msg.sender] -= amount; // remove amount from balance before transfer occurs
        msg.sender.transfer(amount); // run the transfer function
        return true;
    }

    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);

       // add event 
       emit balanceTransfered(amount, msg.sender);

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

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

jon_m · January 1, 2022, 7:38pm

Nice solution @InterstellarX

You have added both of the essential lines of code needed to solve the problem with the withdraw function, and your statements are also in the correct order within the function body to reduce security risk:

Check inputs (require statements)
Effects (update the contract state)
External Interactions (e.g. transfer method)

Just as you have done, it’s important to modify the contract state for the reduction in the individual user’s balance…

balance[msg.sender] -= amount;

before actually transferring the funds out of the contract to the external address…

msg.sender.transfer(amount);

This isn’t, as you say, just “to ensure that is OK and possible” …

Our require() statement has already checked whether the withdrawal amount can be covered by the user’s individual balance, and it would have thrown an error and triggered revert if it couldn’t.

Instead, this is to prevent what is known as a re-entrancy attack from occuring after the transfer, but before the user’s individual balance (effectively, the share of the total contract balance they are entitled to withdraw) is reduced to reflect this operation. You’ll learn more about this type of attack, and how the above order of operations helps to prevent it, in the courses which follow this one.

The transfer method doesn’t return a Boolean true or false. As you have said in one of your comments within your code…

The point here is that if the transfer method itself fails, it will throw an error and trigger revert in the same way that require() does if that fails. When revert is triggered, the function doesn’t finish executing, and any changes made to the contract state — by the code in the function body that has already been executed before revert was triggered — are reversed (i.e. reverted). This is what we mean by “built-in error handling” — the transfer method doesn’t need to return a Boolean, because we don’t have to handle its success or error with any additional code of our own.

Returning true doesn’t prevent your solution from working, because the return statement will only be reached if the withdrawal has been successful; but it’s unnecessary to include. If the transaction is successful, then the transfer method will also have executed successfully.

The send method, on the other hand, doesn’t revert if it fails, and it does return a Boolean true or false, which we have to handle with additional code e.g…

function withdraw(uint amount) public {
    require(balance[msg.sender] >= amount, "Insufficient funds"); 
    balance[msg.sender] -= amount;
    bool success = msg.sender.send(amount);
    require(success, "Withdrawal failed");
}

If you want to emit an event and log specific data associated with the withdrawal, then you need to do this with an event declaration and an emit statement e.g.

event Withdrawal(uint amount, address indexed withdrawnBy);

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

To clarify …

A function is marked payable when it needs to receive ether from an external address to add to the contract address balance. Including payable in a function header enables the ether value sent to the function to be added to the contract address balance, without us having to add any further code for this. In the deposit() function, this happens automatically because we have marked the function as payable. The line …

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

… then adds the same value to the individual user’s balance in the mapping, in order to keep track of their share of the total funds held in the contract.

The balance mapping keeps track of each individual user’s share of the total contract balance, effectively the amount of ether each is entitled to withdraw from the contract. Each user’s share (or entitlement) changes with each deposit or withdrawal they make, and also with each internal transfer they are involved in as either the sender or the recipient.

Just let me know if you have any questions

InterstellarX · January 2, 2022, 1:51am

This was super helpful, thanks so much @jon_m!!!

InterstellarX · January 2, 2022, 2:11am

Actually, @jon_m i have a quick question for you…so my transfer function works, but only in the uint: 0 decoded output, but doesn’t seem to work in the actual ETH balance UNTIL I withdraw it, i’m just noticing this now looking at the balance of the accounts. Do you know what might be causing this? I would just think the transfer would be as instantaneous as the deposit, but I have to transfer to the address…and then that address needs to withdraw to their account for their balance to take shape.

Basically, the variable balance works, but when you look at the account address balance, it does not change, only can lose ETH, but not get it back or transfer it to someone else. It seems like I am depositing/transferring imaginary ETH, but the actual address’ balance in remix doesn’t change. I’m just not sure why the balance wouldn’t be updated instantaneously on transfer but you have to withdraw to then have your balance be affected.

Any clarity on this would be greatly appreciated!